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

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