From 26a7cf2bbea656837583f9a1a0f9390db63d6cc3 Mon Sep 17 00:00:00 2001 From: Kui-Feng Lee Date: Wed, 13 Mar 2024 14:41:38 -0700 Subject: selftests/bpf: Ensure libbpf skip all-zeros fields of struct_ops maps. A new version of a type may have additional fields that do not exist in older versions. Previously, libbpf would reject struct_ops maps with a new version containing extra fields when running on a machine with an old kernel. However, we have updated libbpf to ignore these fields if their values are all zeros or null in order to provide backward compatibility. Signed-off-by: Kui-Feng Lee Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/bpf/20240313214139.685112-3-thinker.li@gmail.com --- .../bpf/prog_tests/test_struct_ops_module.c | 47 ++++++++++++++++++++++ 1 file changed, 47 insertions(+) (limited to 'tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c') diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c index ee5372c7f2c7..098776d00ab4 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c @@ -93,9 +93,56 @@ cleanup: struct_ops_module__destroy(skel); } +static void test_struct_ops_not_zeroed(void) +{ + struct struct_ops_module *skel; + int err; + + /* zeroed is 0, and zeroed_op is null */ + skel = struct_ops_module__open(); + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open")) + return; + + err = struct_ops_module__load(skel); + ASSERT_OK(err, "struct_ops_module_load"); + + struct_ops_module__destroy(skel); + + /* zeroed is not 0 */ + skel = struct_ops_module__open(); + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_not_zeroed")) + return; + + /* libbpf should reject the testmod_zeroed since struct + * bpf_testmod_ops in the kernel has no "zeroed" field and the + * value of "zeroed" is non-zero. + */ + skel->struct_ops.testmod_zeroed->zeroed = 0xdeadbeef; + err = struct_ops_module__load(skel); + ASSERT_ERR(err, "struct_ops_module_load_not_zeroed"); + + struct_ops_module__destroy(skel); + + /* zeroed_op is not null */ + skel = struct_ops_module__open(); + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_not_zeroed_op")) + return; + + /* libbpf should reject the testmod_zeroed since the value of its + * "zeroed_op" is not null. + */ + skel->struct_ops.testmod_zeroed->zeroed_op = skel->progs.test_3; + err = struct_ops_module__load(skel); + ASSERT_ERR(err, "struct_ops_module_load_not_zeroed_op"); + + struct_ops_module__destroy(skel); +} + void serial_test_struct_ops_module(void) { if (test__start_subtest("test_struct_ops_load")) test_struct_ops_load(); + if (test__start_subtest("test_struct_ops_not_zeroed")) + test_struct_ops_not_zeroed(); } -- cgit v1.2.3-70-g09d2 From ba0cbe2bb4ab8aa266e48c6399bebf6e1217828a Mon Sep 17 00:00:00 2001 From: Kui-Feng Lee Date: Thu, 4 Apr 2024 16:23:42 -0700 Subject: selftests/bpf: Make sure libbpf doesn't enforce the signature of a func pointer. The verifier in the kernel ensures that the struct_ops operators behave correctly by checking that they access parameters and context appropriately. The verifier will approve a program as long as it correctly accesses the context/parameters, regardless of its function signature. In contrast, libbpf should not verify the signature of function pointers and functions to enable flexibility in loading various implementations of an operator even if the signature of the function pointer does not match those in the implementations or the kernel. With this flexibility, user space applications can adapt to different kernel versions by loading a specific implementation of an operator based on feature detection. This is a follow-up of the commit c911fc61a7ce ("libbpf: Skip zeroed or null fields if not found in the kernel type.") Signed-off-by: Kui-Feng Lee Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/bpf/20240404232342.991414-1-thinker.li@gmail.com --- .../bpf/prog_tests/test_struct_ops_module.c | 24 ++++++++++++++++++++++ .../selftests/bpf/progs/struct_ops_module.c | 13 ++++++++++++ 2 files changed, 37 insertions(+) (limited to 'tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c') diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c index 098776d00ab4..7cf2b9ddd3e1 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c @@ -138,11 +138,35 @@ static void test_struct_ops_not_zeroed(void) struct_ops_module__destroy(skel); } +/* The signature of an implementation might not match the signature of the + * function pointer prototype defined in the BPF program. This mismatch + * should be allowed as long as the behavior of the operator program + * adheres to the signature in the kernel. Libbpf should not enforce the + * signature; rather, let the kernel verifier handle the enforcement. + */ +static void test_struct_ops_incompatible(void) +{ + struct struct_ops_module *skel; + struct bpf_link *link; + + skel = struct_ops_module__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open_and_load")) + return; + + link = bpf_map__attach_struct_ops(skel->maps.testmod_incompatible); + if (ASSERT_OK_PTR(link, "attach_struct_ops")) + bpf_link__destroy(link); + + struct_ops_module__destroy(skel); +} + void serial_test_struct_ops_module(void) { if (test__start_subtest("test_struct_ops_load")) test_struct_ops_load(); if (test__start_subtest("test_struct_ops_not_zeroed")) test_struct_ops_not_zeroed(); + if (test__start_subtest("test_struct_ops_incompatible")) + test_struct_ops_incompatible(); } diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c index 86e1e50c5531..63b065dae002 100644 --- a/tools/testing/selftests/bpf/progs/struct_ops_module.c +++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c @@ -68,3 +68,16 @@ struct bpf_testmod_ops___zeroed testmod_zeroed = { .test_1 = (void *)test_1, .test_2 = (void *)test_2_v2, }; + +struct bpf_testmod_ops___incompatible { + int (*test_1)(void); + void (*test_2)(int *a); + int data; +}; + +SEC(".struct_ops.link") +struct bpf_testmod_ops___incompatible testmod_incompatible = { + .test_1 = (void *)test_1, + .test_2 = (void *)test_2, + .data = 3, +}; -- cgit v1.2.3-70-g09d2 From 1bba3b3d373dbafae891e7cb06b8c82c8d62aba1 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Sat, 27 Apr 2024 20:09:54 -0700 Subject: selftests/bpf: validate nulled-out struct_ops program is handled properly Add a selftests validating that it's possible to have some struct_ops callback set declaratively, then disable it (by setting to NULL) programmatically. Libbpf should detect that such program should not be loaded. Otherwise, it will unnecessarily fail the loading when the host kernel does not have the type information. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20240428030954.3918764-2-andrii@kernel.org Signed-off-by: Martin KaFai Lau --- .../selftests/bpf/prog_tests/test_struct_ops_module.c | 18 ++++++++++++++++-- tools/testing/selftests/bpf/progs/struct_ops_module.c | 7 +++++++ 2 files changed, 23 insertions(+), 2 deletions(-) (limited to 'tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c') diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c index 7cf2b9ddd3e1..bd39586abd5a 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c @@ -66,6 +66,7 @@ static void test_struct_ops_load(void) * auto-loading, or it will fail to load. */ bpf_program__set_autoload(skel->progs.test_2, false); + bpf_map__set_autocreate(skel->maps.testmod_zeroed, false); err = struct_ops_module__load(skel); if (!ASSERT_OK(err, "struct_ops_module_load")) @@ -103,6 +104,10 @@ static void test_struct_ops_not_zeroed(void) if (!ASSERT_OK_PTR(skel, "struct_ops_module_open")) return; + skel->struct_ops.testmod_zeroed->zeroed = 0; + /* zeroed_op prog should be not loaded automatically now */ + skel->struct_ops.testmod_zeroed->zeroed_op = NULL; + err = struct_ops_module__load(skel); ASSERT_OK(err, "struct_ops_module_load"); @@ -118,6 +123,7 @@ static void test_struct_ops_not_zeroed(void) * value of "zeroed" is non-zero. */ skel->struct_ops.testmod_zeroed->zeroed = 0xdeadbeef; + skel->struct_ops.testmod_zeroed->zeroed_op = NULL; err = struct_ops_module__load(skel); ASSERT_ERR(err, "struct_ops_module_load_not_zeroed"); @@ -148,15 +154,23 @@ static void test_struct_ops_incompatible(void) { struct struct_ops_module *skel; struct bpf_link *link; + int err; - skel = struct_ops_module__open_and_load(); - if (!ASSERT_OK_PTR(skel, "open_and_load")) + skel = struct_ops_module__open(); + if (!ASSERT_OK_PTR(skel, "struct_ops_module_open")) return; + bpf_map__set_autocreate(skel->maps.testmod_zeroed, false); + + err = struct_ops_module__load(skel); + if (!ASSERT_OK(err, "skel_load")) + goto cleanup; + link = bpf_map__attach_struct_ops(skel->maps.testmod_incompatible); if (ASSERT_OK_PTR(link, "attach_struct_ops")) bpf_link__destroy(link); +cleanup: struct_ops_module__destroy(skel); } diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c index 63b065dae002..40109be2b3ae 100644 --- a/tools/testing/selftests/bpf/progs/struct_ops_module.c +++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c @@ -63,10 +63,17 @@ struct bpf_testmod_ops___zeroed { int zeroed; }; +SEC("?struct_ops/test_3") +int BPF_PROG(zeroed_op) +{ + return 1; +} + SEC(".struct_ops.link") struct bpf_testmod_ops___zeroed testmod_zeroed = { .test_1 = (void *)test_1, .test_2 = (void *)test_2_v2, + .zeroed_op = (void *)zeroed_op, }; struct bpf_testmod_ops___incompatible { -- cgit v1.2.3-70-g09d2 From 9d66d60e968d85742569d025a2fb509cb57333bb Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Mon, 6 May 2024 17:13:31 -0700 Subject: selftests/bpf: add another struct_ops callback use case test Add a test which tests the case that was just fixed. Kernel has full type information about callback, but user explicitly nulls out the reference to declaratively set BPF program reference. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20240507001335.1445325-4-andrii@kernel.org Signed-off-by: Martin KaFai Lau --- .../bpf/prog_tests/test_struct_ops_module.c | 27 ++++++++++++++++++++++ .../selftests/bpf/progs/struct_ops_nulled_out_cb.c | 22 ++++++++++++++++++ 2 files changed, 49 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c (limited to 'tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c') diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c index bd39586abd5a..f3c61ebad323 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c @@ -4,6 +4,7 @@ #include #include "struct_ops_module.skel.h" +#include "struct_ops_nulled_out_cb.skel.h" static void check_map_info(struct bpf_map_info *info) { @@ -174,6 +175,30 @@ cleanup: struct_ops_module__destroy(skel); } +/* validate that it's ok to "turn off" callback that kernel supports */ +static void test_struct_ops_nulled_out_cb(void) +{ + struct struct_ops_nulled_out_cb *skel; + int err; + + skel = struct_ops_nulled_out_cb__open(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + return; + + /* kernel knows about test_1, but we still null it out */ + skel->struct_ops.ops->test_1 = NULL; + + err = struct_ops_nulled_out_cb__load(skel); + if (!ASSERT_OK(err, "skel_load")) + goto cleanup; + + ASSERT_FALSE(bpf_program__autoload(skel->progs.test_1_turn_off), "prog_autoload"); + ASSERT_LT(bpf_program__fd(skel->progs.test_1_turn_off), 0, "prog_fd"); + +cleanup: + struct_ops_nulled_out_cb__destroy(skel); +} + void serial_test_struct_ops_module(void) { if (test__start_subtest("test_struct_ops_load")) @@ -182,5 +207,7 @@ void serial_test_struct_ops_module(void) test_struct_ops_not_zeroed(); if (test__start_subtest("test_struct_ops_incompatible")) test_struct_ops_incompatible(); + if (test__start_subtest("test_struct_ops_null_out_cb")) + test_struct_ops_nulled_out_cb(); } diff --git a/tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c b/tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c new file mode 100644 index 000000000000..fa2021388485 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/struct_ops_nulled_out_cb.c @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ +#include +#include +#include "../bpf_testmod/bpf_testmod.h" + +char _license[] SEC("license") = "GPL"; + +int rand; +int arr[1]; + +SEC("struct_ops/test_1") +int BPF_PROG(test_1_turn_off) +{ + return arr[rand]; /* potentially way out of range access */ +} + +SEC(".struct_ops.link") +struct bpf_testmod_ops ops = { + .test_1 = (void *)test_1_turn_off, +}; + -- cgit v1.2.3-70-g09d2 From 41df0733ea414a49094258adab4d600db0420731 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Mon, 6 May 2024 17:13:34 -0700 Subject: selftests/bpf: validate struct_ops early failure detection logic Add a simple test that validates that libbpf will reject isolated struct_ops program early with helpful warning message. Also validate that explicit use of such BPF program through BPF skeleton after BPF object is open won't trigger any warnings. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20240507001335.1445325-7-andrii@kernel.org Signed-off-by: Martin KaFai Lau --- .../bpf/prog_tests/test_struct_ops_module.c | 45 ++++++++++++++++++++++ .../selftests/bpf/progs/struct_ops_forgotten_cb.c | 19 +++++++++ 2 files changed, 64 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c (limited to 'tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c') diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c index f3c61ebad323..3785b648c8ad 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c @@ -5,6 +5,7 @@ #include "struct_ops_module.skel.h" #include "struct_ops_nulled_out_cb.skel.h" +#include "struct_ops_forgotten_cb.skel.h" static void check_map_info(struct bpf_map_info *info) { @@ -199,6 +200,48 @@ cleanup: struct_ops_nulled_out_cb__destroy(skel); } +/* validate that libbpf generates reasonable error message if struct_ops is + * not referenced in any struct_ops map + */ +static void test_struct_ops_forgotten_cb(void) +{ + struct struct_ops_forgotten_cb *skel; + char *log; + int err; + + skel = struct_ops_forgotten_cb__open(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + return; + + start_libbpf_log_capture(); + + err = struct_ops_forgotten_cb__load(skel); + if (!ASSERT_ERR(err, "skel_load")) + goto cleanup; + + log = stop_libbpf_log_capture(); + ASSERT_HAS_SUBSTR(log, + "prog 'test_1_forgotten': SEC(\"struct_ops\") program isn't referenced anywhere, did you forget to use it?", + "libbpf_log"); + free(log); + + struct_ops_forgotten_cb__destroy(skel); + + /* now let's programmatically use it, we should be fine now */ + skel = struct_ops_forgotten_cb__open(); + if (!ASSERT_OK_PTR(skel, "skel_open")) + return; + + skel->struct_ops.ops->test_1 = skel->progs.test_1_forgotten; /* not anymore */ + + err = struct_ops_forgotten_cb__load(skel); + if (!ASSERT_OK(err, "skel_load")) + goto cleanup; + +cleanup: + struct_ops_forgotten_cb__destroy(skel); +} + void serial_test_struct_ops_module(void) { if (test__start_subtest("test_struct_ops_load")) @@ -209,5 +252,7 @@ void serial_test_struct_ops_module(void) test_struct_ops_incompatible(); if (test__start_subtest("test_struct_ops_null_out_cb")) test_struct_ops_nulled_out_cb(); + if (test__start_subtest("struct_ops_forgotten_cb")) + test_struct_ops_forgotten_cb(); } diff --git a/tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c b/tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c new file mode 100644 index 000000000000..3c822103bd40 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/struct_ops_forgotten_cb.c @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */ +#include +#include +#include "../bpf_testmod/bpf_testmod.h" + +char _license[] SEC("license") = "GPL"; + +SEC("struct_ops/test_1") +int BPF_PROG(test_1_forgotten) +{ + return 0; +} + +SEC(".struct_ops.link") +struct bpf_testmod_ops ops = { + /* we forgot to reference test_1_forgotten above, oops */ +}; + -- cgit v1.2.3-70-g09d2 From 7b9959b8cdbc40b31b4c66bb900ec8d5e5b305bd Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Mon, 6 May 2024 17:13:35 -0700 Subject: selftests/bpf: shorten subtest names for struct_ops_module test Drive-by clean up, we shouldn't use meaningless "test_" prefix for subtest names. Signed-off-by: Andrii Nakryiko Link: https://lore.kernel.org/r/20240507001335.1445325-8-andrii@kernel.org Signed-off-by: Martin KaFai Lau --- tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c') diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c index 3785b648c8ad..29e183a80f49 100644 --- a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c +++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_module.c @@ -244,13 +244,13 @@ cleanup: void serial_test_struct_ops_module(void) { - if (test__start_subtest("test_struct_ops_load")) + if (test__start_subtest("struct_ops_load")) test_struct_ops_load(); - if (test__start_subtest("test_struct_ops_not_zeroed")) + if (test__start_subtest("struct_ops_not_zeroed")) test_struct_ops_not_zeroed(); - if (test__start_subtest("test_struct_ops_incompatible")) + if (test__start_subtest("struct_ops_incompatible")) test_struct_ops_incompatible(); - if (test__start_subtest("test_struct_ops_null_out_cb")) + if (test__start_subtest("struct_ops_null_out_cb")) test_struct_ops_nulled_out_cb(); if (test__start_subtest("struct_ops_forgotten_cb")) test_struct_ops_forgotten_cb(); -- cgit v1.2.3-70-g09d2