bpf: enforce that struct_ops programs be GPL-only
Subsystems (6)
PATCH 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().
--- kernel/bpf/verifier.c | 5 +++++ 1 file changed, 5 insertions(+)
kernel/bpf/verifier.c +5 -0
@@ -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) {
Signature · 1 line
2.31.0
PATCH v2 2/2 bpf/selftests: test that kernel rejects a TCP CC with an invalid license
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.
--- .../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
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +44 -0
@@ -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 (); }
tools/testing/selftests/bpf/progs/bpf_nogpltcp.c new file +19 -0
@@ -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" , +};
Signature · 1 line
2.31.0
Re: [PATCH bpf v2 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
Quoted 28 lines · Toke Høiland-Jørgensen
This adds a selftest to check that the verifier rejects a TCP CC
struct_opswith a non-GPL license.v2:
- Use a minimalstruct_opsBPF 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 <[email protected]>--- .../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.ctools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +1
-0@@ -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))
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 232
Quoted 5 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +2
-0@@ -227,10 +228,53 @@ static void test_dctcp(void)bpf_dctcp__destroy (dctcp_skel); } +static char *err_str =NULL ; +static bool found = false;
Nit. These two inits are not needed.
Quoted 15 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +15
-0+ +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;
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 257
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +10
-0+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.
Quoted 13 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +13
-0+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); +}
Re: [PATCH bpf v2 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
In reply to Toke Høiland-JørgensenQuoted 21 lines · Toke Høiland-Jørgensen
This adds a selftest to check that the verifier rejects a TCP CC
struct_opswith a non-GPL license.v2:
- Use a minimalstruct_opsBPF 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 <[email protected]>--- .../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.ctools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +0
-0
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c line 9
Quoted 4 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +1
-0@@ -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?
Quoted 31 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +24
-0#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 ; +} +
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 264
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +10
-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()
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 265
Quoted 1 line
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +1
-0+bpf_nogpltcp__destroy (skel);
you should destroy unconditionally
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 267
Quoted 2 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +2
-0+ +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
Quoted 41 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +6
-0+ +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 (); }tools/testing/selftests/bpf/progs/bpf_nogpltcp.c new file +19
-1@@ -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
Re: [PATCH bpf v2 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
Quoted 17 lines · Andrii Nakryiko
Quoted 16 lines · Toke Høiland-Jørgensen
This adds a selftest to check that the verifier rejects a TCP CC
struct_opswith a non-GPL license.v2:
- Use a minimalstruct_opsBPF 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 <[email protected]>--- .../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
Quoted 12 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c line 9
Quoted 9 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +1
-0@@ -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 46 lines
Quoted 31 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +24
-0#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 ; +} +tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 264
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +10
-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()tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 265
Quoted 1 line
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +1
-0+bpf_nogpltcp__destroy (skel);
Quoted 8 lines
you should destroy unconditionally
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 267
Quoted 2 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +2
-0+ +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
Re: [PATCH bpf v2 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
-- Andrii
Quoted 33 lines · Toke Høiland-Jørgensen
Quoted 17 lines · Andrii Nakryiko
Quoted 16 lines · Toke Høiland-Jørgensen
This adds a selftest to check that the verifier rejects a TCP CC
struct_opswith a non-GPL license.v2:
- Use a minimalstruct_opsBPF 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 <[email protected]>--- .../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.cQuoted 12 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c line 9
Quoted 9 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +1
-0@@ -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?
Quoted 1 line
Haha, yeah, good point - my eyes also just lump it into a blob...
thanks
Quoted 52 lines
Quoted 52 lines
Quoted 32 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +24
-0#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 ; +} +tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 264
Quoted 10 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +10
-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()tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c static void test_dctcp(void) · line 265
Quoted 1 line
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +1
-0+bpf_nogpltcp__destroy (skel);you should destroy unconditionally
Quoted 2 lines
tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +2
-0+ +CHECK (!found,"errmsg check" ,"expected string '%s'" , err_str);
Quoted 7 lines
Quoted 3 lines
ASSERT_EQ(found, true, "expected_err_msg");I can never be sure which way
CHECK()is checkingAh, thanks! I always get confused about
CHECK()as well! Maybe it should be renamed toASSERT()? But that would require flipping all theif()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.
Quoted 3 lines
-Toke
Re: [PATCH bpf v2 2/2] bpf/selftests: test that kernel rejects a TCP CC with an invalid license
Quoted 7 lines · Andrii Nakryiko
Quoted 3 lines
Ah, thanks! I always get confused about
CHECK()as well! Maybe it should be renamed toASSERT()? But that would require flipping all theif()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).
Quoted 3 lines
Ideally we complete the set of
ASSERT_XXX()macros and convert as much as possible to that. We can also have just genericASSERT()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