On 3/12/24 16:10, Andrii Nakryiko wrote:
On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@xxxxxxxxx> wrote:
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 <thinker.li@xxxxxxxxx>
---
.../bpf/prog_tests/test_struct_ops_module.c | 35 +++++++++++++++++++
.../selftests/bpf/progs/struct_ops_module.c | 13 +++++++
2 files changed, 48 insertions(+)
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..e0d9ff75121b 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,44 @@ static void test_struct_ops_load(void)
struct_ops_module__destroy(skel);
}
+static void test_struct_ops_not_zeroed(void)
+{
+ struct struct_ops_module *skel;
+ int err;
+
+ skel = struct_ops_module__open();
+ if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+ return;
+
+ bpf_program__set_autoload(skel->progs.test_3, false);
maybe mark test_3 program in progs/struct_ops_module.c as default
not-autoload (SEC("?struct_ops/test_3")), so you don't have to set
this to false everywhere?
Sure! I forgot we have this new feature.
+
+ err = struct_ops_module__load(skel);
+ struct_ops_module__destroy(skel);
+
+ if (!ASSERT_OK(err, "struct_ops_module_load"))
+ return;
+
+ skel = struct_ops_module__open();
+ if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_not_zeroed"))
+ return;
+
+ bpf_program__set_autoload(skel->progs.test_3, false);
+
+ /* libbpf should reject the testmod_zeroed since the value of its
+ * "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);
+}
+
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();
}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c b/tools/testing/selftests/bpf/progs/struct_ops_module.c
index 026cabfa7f1f..d7d7606f639c 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -54,3 +54,16 @@ struct bpf_testmod_ops___v2 testmod_2 = {
.test_1 = (void *)test_1,
.test_2 = (void *)test_2_v2,
};
+
+struct bpf_testmod_ops___zeroed {
+ int (*test_1)(void);
+ void (*test_2)(int a, int b);
+ int (*test_maybe_null)(int dummy, struct task_struct *task);
+ int zeroed;
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___zeroed testmod_zeroed = {
+ .test_1 = (void *)test_1,
+ .test_2 = (void *)test_2_v2,
+};
We should also test the case where we have local ops definition with
incompatible callback function signature (e.g., extra argument or
something). Test then would update the program pointer (through
skeleton's shadow type) to a compatible implementation.
Can you please add a test like that? Because the goal is to have a
single struct_ops definition, if possible, and adjust it at runtime to
make it compatible with the old kernel, let's have a small demo of
this working.
Do you want to check signatures at libbpf? Or you just want a small
demo?
For extra arguments, IIRC, the verifier should reject the program if the
program did access these arguments since it accesses memory beyond the
last byte of the context. Doing extra checking at libbpf can provide
better error messages if that is what you want.
If a program never accesses an extra argument, it should be allowed.(?)
WDYT?
--
2.34.1
