On Fri, Mar 15, 2024 at 4:44 PM Kui-Feng Lee <sinquersw@xxxxxxxxx> wrote:
>
>
>
> On 3/14/24 13:59, Andrii Nakryiko wrote:
> > 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,
> > +};
>
> It is an interesting combination. The newer versions usually have more
> arguments although it is not always true. But, you used the old version
> of a type intentionally. Most people would do opposite, right?
>
> How about to use a version with more arguments than what the kernel
> expected, but assign a function pointer with fewer arguments? For example,
It doesn't matter. I wanted to check that libbpf doesn't enforce type
signatures. Whether it's more or fewer arguments doesn't really
matter. In practice users will need to supply the correct BPF program
that would be verified by the kernel, and that's what I cared about:
whether libbpf will allow users to achieve that.
>
> SEC("struct_ops/test_2_arg3v")
> void BPF_PROG(test_2_arg3v, int a, int b, int c)
> {
> ......
> }
>
> struct bpf_test_ops___new_fn_proto {
> void (*test_2)(int a, int b, int c);
> };
>
> SEC(".struct_ops.link")
> struct bpf_testmod_ops___new_fn_proto testmod_fn_proto = {
> .test_2 = (void *)test_2_arg3v
> };
>
> Basically, we don't check signatures of function pointers so far.
> We have the ability to *decrease* the number of arguments.
>
> >
> >
> > 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
> >>
