Re: [PATCH bpf-next v2 0/3] Ignore additional fields in the struct_ops maps in an updated version.

[Kui-Feng Lee <sinquersw@gmail.com>]

On 3/18/24 11:34, Andrii Nakryiko wrote:
> On Fri, Mar 15, 2024 at 4:44 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>>
>>
>>
>> On 3/14/24 13:59, Andrii Nakryiko wrote:
>>> On Wed, Mar 13, 2024 at 2:41 PM Kui-Feng Lee <thinker.li@gmail.com> 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@gmail.com/
>>>>
>>>> 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.

Now I got it!

> 
>>
>> 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
>>>>