| Message ID | 20210325211122.98620-1-toke@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [bpf,v2,1/2] bpf: enforce that struct_ops programs be GPL-only | expand |
On Thu, Mar 25, 2021 at 2:11 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > > 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 <toke@redhat.com> > --- > .../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? > > #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() > + 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 > + > + 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 >
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > On Thu, Mar 25, 2021 at 2:11 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: >> >> 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 <toke@redhat.com> >> --- >> .../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... >> >> #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() > >> + 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 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 <toke@redhat.com> wrote: > > Andrii Nakryiko <andrii.nakryiko@gmail.com> writes: > > > On Thu, Mar 25, 2021 at 2:11 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote: > >> > >> 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 <toke@redhat.com> > >> --- > >> .../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 > > >> > >> #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() > > > >> + 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 > > 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 <andrii.nakryiko@gmail.com> writes: >> 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
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) {
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 <kafai@fb.com> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- kernel/bpf/verifier.c | 5 +++++ 1 file changed, 5 insertions(+)