[PATCH bpf v2 1/2] bpf: enforce that struct_ops programs be GPL-only

With the introduction of the struct_ops program type, it became possible to
implement kernel functionality in BPF, making it viable to use BPF in place
of a regular kernel module for these particular operations.

Thus far, the only user of this mechanism is for implementing TCP
congestion control algorithms. These are clearly marked as GPL-only when
implemented as modules (as seen by the use of EXPORT_SYMBOL_GPL for
tcp_register_congestion_control()), so it seems like an oversight that this
was not carried over to BPF implementations. Since this is the only user
of the struct_ops mechanism, just enforcing GPL-only for the struct_ops
program type seems like the simplest way to fix this.

v2: Move check to the top of check_struct_ops_btf_id().

Fixes: 0baf26b0fcd7 ("bpf: tcp: Support tcp_congestion_ops in bpf")
Acked-by: Martin KaFai Lau <redacted>
Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 kernel/bpf/verifier.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 44e4ec1640f1..3a738724a380 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -12158,6 +12158,11 @@ static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	u32 btf_id, member_idx;
 	const char *mname;
 
+	if (!prog->gpl_compatible) {
+		verbose(env, "struct ops programs must have a GPL compatible license\n");
+		return -EINVAL;
+	}
+
 	btf_id = prog->aux->attach_btf_id;
 	st_ops = bpf_struct_ops_find(btf_id);
 	if (!st_ops) {

-- 
2.31.0

This adds a selftest to check that the verifier rejects a TCP CC struct_ops
with a non-GPL license.

v2:
- Use a minimal struct_ops BPF program instead of rewriting bpf_dctcp's
  license in memory.
- Check for the verifier reject message instead of just the return code.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 .../selftests/bpf/prog_tests/bpf_tcp_ca.c     | 44 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_nogpltcp.c        | 19 ++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_nogpltcp.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index 37c5494a0381..a09c716528e1 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c

@@ -6,6 +6,7 @@
 #include <test_progs.h>
 #include "bpf_dctcp.skel.h"
 #include "bpf_cubic.skel.h"
+#include "bpf_nogpltcp.skel.h"
 
 #define min(a, b) ((a) < (b) ? (a) : (b))
 

@@ -227,10 +228,53 @@ static void test_dctcp(void)
 	bpf_dctcp__destroy(dctcp_skel);
 }
 
+static char *err_str = NULL;
+static bool found = false;
+
+static int libbpf_debug_print(enum libbpf_print_level level,
+			      const char *format, va_list args)
+{
+	char *log_buf;
+
+	if (level != LIBBPF_WARN ||
+	    strcmp(format, "libbpf: \n%s\n")) {
+		vprintf(format, args);
+		return 0;
+	}
+
+	log_buf = va_arg(args, char *);
+	if (!log_buf)
+		goto out;
+	if (err_str && strstr(log_buf, err_str) != NULL)
+		found = true;
+out:
+	printf(format, log_buf);
+	return 0;
+}
+
+static void test_invalid_license(void)
+{
+	libbpf_print_fn_t old_print_fn = NULL;
+	struct bpf_nogpltcp *skel;
+
+	err_str = "struct ops programs must have a GPL compatible license";
+	old_print_fn = libbpf_set_print(libbpf_debug_print);
+
+	skel = bpf_nogpltcp__open_and_load();
+	if (CHECK(skel, "bpf_nogplgtcp__open_and_load()", "didn't fail\n"))
+		bpf_nogpltcp__destroy(skel);
+
+	CHECK(!found, "errmsg check", "expected string '%s'", err_str);
+
+	libbpf_set_print(old_print_fn);
+}
+
 void test_bpf_tcp_ca(void)
 {
 	if (test__start_subtest("dctcp"))
 		test_dctcp();
 	if (test__start_subtest("cubic"))
 		test_cubic();
+	if (test__start_subtest("invalid_license"))
+		test_invalid_license();
 }

diff --git a/tools/testing/selftests/bpf/progs/bpf_nogpltcp.c b/tools/testing/selftests/bpf/progs/bpf_nogpltcp.c
new file mode 100644
index 000000000000..2ecd833dcd41
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/bpf_nogpltcp.c

@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <linux/types.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_tcp_helpers.h"
+
+char _license[] SEC("license") = "X";
+
+void BPF_STRUCT_OPS(nogpltcp_init, struct sock *sk)
+{
+}
+
+SEC(".struct_ops")
+struct tcp_congestion_ops bpf_nogpltcp = {
+	.init           = (void *)nogpltcp_init,
+	.name           = "bpf_nogpltcp",
+};

-- 
2.31.0

On Thu, Mar 25, 2021 at 10:11:22PM +0100, Toke Høiland-Jørgensen wrote:

Nit. These two inits are not needed.

+
+static int libbpf_debug_print(enum libbpf_print_level level,
+			      const char *format, va_list args)
+{
+	char *log_buf;
+
+	if (level != LIBBPF_WARN ||
+	    strcmp(format, "libbpf: \n%s\n")) {
+		vprintf(format, args);
+		return 0;
+	}
+
+	log_buf = va_arg(args, char *);
+	if (!log_buf)
+		goto out;
+	if (err_str && strstr(log_buf, err_str) != NULL)
+		found = true;
+out:
+	printf(format, log_buf);
+	return 0;
+}
+
+static void test_invalid_license(void)
+{
+	libbpf_print_fn_t old_print_fn = NULL;

Nit. Same here.  Not need to init NULL.

Others lgtm.

Acked-by: Martin KaFai Lau <redacted>

+	struct bpf_nogpltcp *skel;
+
+	err_str = "struct ops programs must have a GPL compatible license";
+	old_print_fn = libbpf_set_print(libbpf_debug_print);
+
+	skel = bpf_nogpltcp__open_and_load();
+	if (CHECK(skel, "bpf_nogplgtcp__open_and_load()", "didn't fail\n"))
+		bpf_nogpltcp__destroy(skel);
+
+	CHECK(!found, "errmsg check", "expected string '%s'", err_str);
+
+	libbpf_set_print(old_print_fn);
+}

On Thu, Mar 25, 2021 at 2:11 PM Toke Høiland-Jørgensen [off-list ref] wrote:

total nit, but my eyes can't read "nogpltcp"... wouldn't
"bpf_tcp_nogpl" be a bit easier?

ASSERT_OK_PTR()

+               bpf_nogpltcp__destroy(skel);

you should destroy unconditionally

+
+       CHECK(!found, "errmsg check", "expected string '%s'", err_str);

ASSERT_EQ(found, true, "expected_err_msg");

I can never be sure which way CHECK() is checking

Andrii Nakryiko [off-list ref] writes:

On Thu, Mar 25, 2021 at 2:11 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

This adds a selftest to check that the verifier rejects a TCP CC struct_ops
with a non-GPL license.

v2:
- Use a minimal struct_ops BPF program instead of rewriting bpf_dctcp's
  license in memory.
- Check for the verifier reject message instead of just the return code.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 .../selftests/bpf/prog_tests/bpf_tcp_ca.c     | 44 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_nogpltcp.c        | 19 ++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_nogpltcp.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index 37c5494a0381..a09c716528e1 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c

@@ -6,6 +6,7 @@
 #include <test_progs.h>
 #include "bpf_dctcp.skel.h"
 #include "bpf_cubic.skel.h"
+#include "bpf_nogpltcp.skel.h"

total nit, but my eyes can't read "nogpltcp"... wouldn't
"bpf_tcp_nogpl" be a bit easier?

Haha, yeah, good point - my eyes also just lump it into a blob...

quoted

 #define min(a, b) ((a) < (b) ? (a) : (b))

@@ -227,10 +228,53 @@ static void test_dctcp(void)
        bpf_dctcp__destroy(dctcp_skel);
 }

+static char *err_str = NULL;
+static bool found = false;
+
+static int libbpf_debug_print(enum libbpf_print_level level,
+                             const char *format, va_list args)
+{
+       char *log_buf;
+
+       if (level != LIBBPF_WARN ||
+           strcmp(format, "libbpf: \n%s\n")) {
+               vprintf(format, args);
+               return 0;
+       }
+
+       log_buf = va_arg(args, char *);
+       if (!log_buf)
+               goto out;
+       if (err_str && strstr(log_buf, err_str) != NULL)
+               found = true;
+out:
+       printf(format, log_buf);
+       return 0;
+}
+
+static void test_invalid_license(void)
+{
+       libbpf_print_fn_t old_print_fn = NULL;
+       struct bpf_nogpltcp *skel;
+
+       err_str = "struct ops programs must have a GPL compatible license";
+       old_print_fn = libbpf_set_print(libbpf_debug_print);
+
+       skel = bpf_nogpltcp__open_and_load();
+       if (CHECK(skel, "bpf_nogplgtcp__open_and_load()", "didn't fail\n"))

ASSERT_OK_PTR()

quoted

+               bpf_nogpltcp__destroy(skel);

you should destroy unconditionally

quoted

+
+       CHECK(!found, "errmsg check", "expected string '%s'", err_str);

ASSERT_EQ(found, true, "expected_err_msg");

I can never be sure which way CHECK() is checking

Ah, thanks! I always get confused about CHECK() as well! Maybe it should
be renamed to ASSERT()? But that would require flipping all the if()
statements around them as well :/

-Toke

-- Andrii

On Fri, Mar 26, 2021 at 2:43 AM Toke Høiland-Jørgensen [off-list ref] wrote:

Andrii Nakryiko [off-list ref] writes:

quoted

On Thu, Mar 25, 2021 at 2:11 PM Toke Høiland-Jørgensen [off-list ref] wrote:

quoted

This adds a selftest to check that the verifier rejects a TCP CC struct_ops
with a non-GPL license.

v2:
- Use a minimal struct_ops BPF program instead of rewriting bpf_dctcp's
  license in memory.
- Check for the verifier reject message instead of just the return code.

Signed-off-by: Toke Høiland-Jørgensen <redacted>
---
 .../selftests/bpf/prog_tests/bpf_tcp_ca.c     | 44 +++++++++++++++++++
 .../selftests/bpf/progs/bpf_nogpltcp.c        | 19 ++++++++
 2 files changed, 63 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/bpf_nogpltcp.c

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
index 37c5494a0381..a09c716528e1 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c

@@ -6,6 +6,7 @@
 #include <test_progs.h>
 #include "bpf_dctcp.skel.h"
 #include "bpf_cubic.skel.h"
+#include "bpf_nogpltcp.skel.h"

total nit, but my eyes can't read "nogpltcp"... wouldn't
"bpf_tcp_nogpl" be a bit easier?

Haha, yeah, good point - my eyes also just lump it into a blob...

thanks

quoted

quoted

 #define min(a, b) ((a) < (b) ? (a) : (b))

@@ -227,10 +228,53 @@ static void test_dctcp(void)
        bpf_dctcp__destroy(dctcp_skel);
 }

+static char *err_str = NULL;
+static bool found = false;
+
+static int libbpf_debug_print(enum libbpf_print_level level,
+                             const char *format, va_list args)
+{
+       char *log_buf;
+
+       if (level != LIBBPF_WARN ||
+           strcmp(format, "libbpf: \n%s\n")) {
+               vprintf(format, args);
+               return 0;
+       }
+
+       log_buf = va_arg(args, char *);
+       if (!log_buf)
+               goto out;
+       if (err_str && strstr(log_buf, err_str) != NULL)
+               found = true;
+out:
+       printf(format, log_buf);
+       return 0;
+}
+
+static void test_invalid_license(void)
+{
+       libbpf_print_fn_t old_print_fn = NULL;
+       struct bpf_nogpltcp *skel;
+
+       err_str = "struct ops programs must have a GPL compatible license";
+       old_print_fn = libbpf_set_print(libbpf_debug_print);
+
+       skel = bpf_nogpltcp__open_and_load();
+       if (CHECK(skel, "bpf_nogplgtcp__open_and_load()", "didn't fail\n"))

ASSERT_OK_PTR()

quoted

+               bpf_nogpltcp__destroy(skel);

you should destroy unconditionally

quoted

+
+       CHECK(!found, "errmsg check", "expected string '%s'", err_str);

ASSERT_EQ(found, true, "expected_err_msg");

I can never be sure which way CHECK() is checking

Ah, thanks! I always get confused about CHECK() as well! Maybe it should
be renamed to ASSERT()? But that would require flipping all the if()
statements around them as well :/

Exactly, it's the opposite of assert (ASSERT_NOT %-), that
CHECK(!found) is "assert not not found", right?) and it throws me off
every. single. time. Ideally we complete the set of ASSERT_XXX()
macros and convert as much as possible to that. We can also have just
generic ASSERT() for all other complicated cases.

-Toke

Andrii Nakryiko [off-list ref] writes:

quoted

Ah, thanks! I always get confused about CHECK() as well! Maybe it should
be renamed to ASSERT()? But that would require flipping all the if()
statements around them as well :/

Exactly, it's the opposite of assert (ASSERT_NOT %-), that
CHECK(!found) is "assert not not found", right?) and it throws me off
every. single. time.

Yup, me too, I have to basically infer the right meaning from the
surrounding if statements (i.e., whether it triggers an error path or
not).

Ideally we complete the set of ASSERT_XXX() macros and convert as much
as possible to that. We can also have just generic ASSERT() for all
other complicated cases.

Totally on board with that! I'll try to remember to fix any selftests I
fiddle with (and not introduce any new uses of CHECK() of course).

-Toke