On Wed, Mar 13, 2024 at 2:41 PM Kui-Feng Lee <thinker.li@xxxxxxxxx> wrote:
>
> According to an offline discussion, it would be beneficial to
> implement a backward-compatible method for struct_ops types with
> additional fields that are not present in older kernels.
>
> This patchset accepts additional fields of a struct_ops map with all
> zero values even if these fields are not in the corresponding type in
> the kernel. This provides a way to be backward compatible. User space
> programs can use the same map on a machine running an old kernel by
> clearing fields that do not exist in the kernel.
>
> For example, in a test case, it adds an additional field "zeroed" that
> doesn't exist in struct bpf_testmod_ops of the kernel.
>
> 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,
> };
>
> Here, it doesn't assign a value to "zeroed" of testmod_zeroed, and by
> default the value of this field will be zero. So, the map will be
> accepted by libbpf, but libbpf will skip the "zeroed" field. However,
> if the "zeroed" field is assigned to any value other than "0", libbpf
> will reject to load this map.
>
> ---
> Changes from v1:
>
> - Fix the issue about function pointer fields.
>
> - Change a warning message, and add an info message for skipping
> fields.
>
> - Add a small demo of additional arguments that are not in the
> function pointer prototype in the kernel.
>
> v1: https://lore.kernel.org/all/20240312183245.341141-1-thinker.li@xxxxxxxxx/
>
> Kui-Feng Lee (3):
> libbpf: Skip zeroed or null fields if not found in the kernel type.
> selftests/bpf: Ensure libbpf skip all-zeros fields of struct_ops maps.
> selftests/bpf: Accept extra arguments if they are not used.
I applied the first two patches and dropped the third one, as I don't
think it's actually testing any new condition. What I actually had in
mind is more along the following lines:
$ git diff
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..9585504ce6b5 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
@@ -103,6 +103,8 @@ static void test_struct_ops_not_zeroed(void)
if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
return;
+ skel->struct_ops.testmod_fn_proto->test_2 = skel->progs.test_2;
+
err = struct_ops_module__load(skel);
ASSERT_OK(err, "struct_ops_module_load");
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_module.c
b/tools/testing/selftests/bpf/progs/struct_ops_module.c
index 86e1e50c5531..d3e0f941c16c 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -68,3 +68,13 @@ struct bpf_testmod_ops___zeroed testmod_zeroed = {
.test_1 = (void *)test_1,
.test_2 = (void *)test_2_v2,
};
+
+struct bpf_testmod_ops___diff_fn_proto {
+ /* differs from expected void (*test_2)(int a, int b) */
+ void (*test_2)(int a);
+};
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops___zeroed testmod_fn_proto = {
+ .test_2 = (void *)test_2_v2,
+};
see how bpf_testmod_ops___diff_fn_proto defines test_2 callback with
an incompatible signature, but at runtime we are switching the program
to the one that the kernel actually expects. This is the scenario
(incompatible struct ops type definition) that I wanted to test and
make sure it works.
I quickly checked that it does work because libbpf doesn't enforce any
type signature (which is both good and bad, but it is what it is). It
would still be nice to have a selftest added with an incompatible
struct_ops type which is "fixed up" by setting thhe correct program
instance. Consider for a follow up.
>
> tools/lib/bpf/libbpf.c | 24 +++-
> .../bpf/prog_tests/test_struct_ops_module.c | 103 ++++++++++++++++++
> .../bpf/progs/struct_ops_extra_arg.c | 49 +++++++++
> .../selftests/bpf/progs/struct_ops_module.c | 16 ++-
> 4 files changed, 186 insertions(+), 6 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_extra_arg.c
>
> --
> 2.34.1
>
