[PATCH bpf-next 0/2] Ignore additional fields in the struct_ops maps in an updated version.

[PATCH bpf-next 0/2] Ignore additional fields in the struct_ops maps in an updated version.

[Kui-Feng Lee]

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.

Kui-Feng Lee (2):
  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.

 tools/lib/bpf/libbpf.c                        | 30 ++++++++++++----
 .../bpf/prog_tests/test_struct_ops_module.c   | 35 +++++++++++++++++++
 .../selftests/bpf/progs/struct_ops_module.c   | 13 +++++++
 3 files changed, 71 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH bpf-next 1/2] libbpf: Skip zeroed or null fields if not found in the kernel type.

[Kui-Feng Lee]

Accept additional fields of a struct_ops type 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.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 tools/lib/bpf/libbpf.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index efab29b8935b..715879796046 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1131,11 +1131,33 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 		__u32 kern_member_idx;
 		const char *mname;
 
+		mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
 		mname = btf__name_by_offset(btf, member->name_off);
+		moff = member->offset / 8;
+		mdata = data + moff;
+		msize = btf__resolve_size(btf, mtype_id);
+		if (msize < 0) {
+			pr_warn("struct_ops init_kern %s: fails to resolve the size of member %s\n",
+				map->name, mname);
+			return msize;
+		}
+
 		kern_member = find_member_by_name(kern_btf, kern_type, mname);
 		if (!kern_member) {
 			pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
 				map->name, mname);
+
+			/* Skip all zeros or null fields if they are not
+			 * presented in the kernel BTF.
+			 */
+			if (btf_is_ptr(mtype)) {
+				if (!st_ops->progs[i])
+					continue;
+			} else {
+				if (libbpf_is_mem_zeroed(mdata, msize))
+					continue;
+			}
+
 			return -ENOTSUP;
 		}
 
@@ -1147,13 +1169,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 			return -ENOTSUP;
 		}
 
-		moff = member->offset / 8;
 		kern_moff = kern_member->offset / 8;
-
-		mdata = data + moff;
 		kern_mdata = kern_data + kern_moff;
-
-		mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
 		kern_mtype = skip_mods_and_typedefs(kern_btf, kern_member->type,
 						    &kern_mtype_id);
 		if (BTF_INFO_KIND(mtype->info) !=
@@ -1230,9 +1247,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 			continue;
 		}
 
-		msize = btf__resolve_size(btf, mtype_id);
 		kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
-		if (msize < 0 || kern_msize < 0 || msize != kern_msize) {
+		if (kern_msize < 0 || msize != kern_msize) {
 			pr_warn("struct_ops init_kern %s: Error in size of member %s: %zd != %zd(kernel)\n",
 				map->name, mname, (ssize_t)msize,
 				(ssize_t)kern_msize);
-- 
2.34.1

Re: [PATCH bpf-next 1/2] libbpf: Skip zeroed or null fields if not found in the kernel type.

[Andrii Nakryiko]

On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>
> Accept additional fields of a struct_ops type 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.
>
> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
> ---
>  tools/lib/bpf/libbpf.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index efab29b8935b..715879796046 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1131,11 +1131,33 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>                 __u32 kern_member_idx;
>                 const char *mname;
>
> +               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);

you don't need to move this up here, btf__resolve_size() can skip
modifiers and typedefs just fine, so let's keep this one below and
just pass member->type to btf__resolve_size()

>                 mname = btf__name_by_offset(btf, member->name_off);
> +               moff = member->offset / 8;
> +               mdata = data + moff;
> +               msize = btf__resolve_size(btf, mtype_id);
> +               if (msize < 0) {
> +                       pr_warn("struct_ops init_kern %s: fails to resolve the size of member %s\n",

s/fails/failed/

> +                               map->name, mname);
> +                       return msize;
> +               }
> +
>                 kern_member = find_member_by_name(kern_btf, kern_type, mname);
>                 if (!kern_member) {
>                         pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
>                                 map->name, mname);
> +
> +                       /* Skip all zeros or null fields if they are not
> +                        * presented in the kernel BTF.
> +                        */
> +                       if (btf_is_ptr(mtype)) {
> +                               if (!st_ops->progs[i])
> +                                       continue;

so, this is both unnecessary to check for NULL (libbpf_is_mem_zeroed
will do it just fine for pointers as well), but it's also wrong
because user could have set this program pointer through skeleton's
shadow type, while here you won't yet see. So let's drop this
btf_is_ptr() special case, and just do generic libbpf_is_mem_zeroed()
check

> +                       } else {
> +                               if (libbpf_is_mem_zeroed(mdata, msize))
> +                                       continue;

I think it's worth emitting informational message here, something like

pr_info("struct_ops %s: member %s not found in kernel, skipping it as
it's set to zero\n", ...")

?

and move that pr_warn("Cannot find member %s in kernel BTF") after
this check, so we don't have scary-looking error-level message

> +                       }
> +
>                         return -ENOTSUP;
>                 }
>
> @@ -1147,13 +1169,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>                         return -ENOTSUP;
>                 }
>
> -               moff = member->offset / 8;
>                 kern_moff = kern_member->offset / 8;
> -
> -               mdata = data + moff;
>                 kern_mdata = kern_data + kern_moff;
> -
> -               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
>                 kern_mtype = skip_mods_and_typedefs(kern_btf, kern_member->type,
>                                                     &kern_mtype_id);
>                 if (BTF_INFO_KIND(mtype->info) !=
> @@ -1230,9 +1247,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>                         continue;
>                 }
>
> -               msize = btf__resolve_size(btf, mtype_id);
>                 kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
> -               if (msize < 0 || kern_msize < 0 || msize != kern_msize) {
> +               if (kern_msize < 0 || msize != kern_msize) {
>                         pr_warn("struct_ops init_kern %s: Error in size of member %s: %zd != %zd(kernel)\n",
>                                 map->name, mname, (ssize_t)msize,
>                                 (ssize_t)kern_msize);
> --
> 2.34.1
>

Re: [PATCH bpf-next 1/2] libbpf: Skip zeroed or null fields if not found in the kernel type.

[Kui-Feng Lee]

On 3/12/24 16:01, Andrii Nakryiko wrote:
> On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> wrote:
>>
>> Accept additional fields of a struct_ops type 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.
>>
>> Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
>> ---
>>   tools/lib/bpf/libbpf.c | 30 +++++++++++++++++++++++-------
>>   1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
>> index efab29b8935b..715879796046 100644
>> --- a/tools/lib/bpf/libbpf.c
>> +++ b/tools/lib/bpf/libbpf.c
>> @@ -1131,11 +1131,33 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                  __u32 kern_member_idx;
>>                  const char *mname;
>>
>> +               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
> 
> you don't need to move this up here, btf__resolve_size() can skip
> modifiers and typedefs just fine, so let's keep this one below and
> just pass member->type to btf__resolve_size()

Ok!

> 
>>                  mname = btf__name_by_offset(btf, member->name_off);
>> +               moff = member->offset / 8;
>> +               mdata = data + moff;
>> +               msize = btf__resolve_size(btf, mtype_id);
>> +               if (msize < 0) {
>> +                       pr_warn("struct_ops init_kern %s: fails to resolve the size of member %s\n",
> 
> s/fails/failed/
> 
>> +                               map->name, mname);
>> +                       return msize;
>> +               }
>> +
>>                  kern_member = find_member_by_name(kern_btf, kern_type, mname);
>>                  if (!kern_member) {
>>                          pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
>>                                  map->name, mname);
>> +
>> +                       /* Skip all zeros or null fields if they are not
>> +                        * presented in the kernel BTF.
>> +                        */
>> +                       if (btf_is_ptr(mtype)) {
>> +                               if (!st_ops->progs[i])
>> +                                       continue;
> 
> so, this is both unnecessary to check for NULL (libbpf_is_mem_zeroed
> will do it just fine for pointers as well), but it's also wrong
> because user could have set this program pointer through skeleton's
> shadow type, while here you won't yet see. So let's drop this
> btf_is_ptr() special case, and just do generic libbpf_is_mem_zeroed()
> check

You are right! I will fix it.

> 
>> +                       } else {
>> +                               if (libbpf_is_mem_zeroed(mdata, msize))
>> +                                       continue;
> 
> I think it's worth emitting informational message here, something like
> 
> pr_info("struct_ops %s: member %s not found in kernel, skipping it as
> it's set to zero\n", ...")
> 
> ?
> 
> and move that pr_warn("Cannot find member %s in kernel BTF") after
> this check, so we don't have scary-looking error-level message

Sure!

> 
>> +                       }
>> +
>>                          return -ENOTSUP;
>>                  }
>>
>> @@ -1147,13 +1169,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                          return -ENOTSUP;
>>                  }
>>
>> -               moff = member->offset / 8;
>>                  kern_moff = kern_member->offset / 8;
>> -
>> -               mdata = data + moff;
>>                  kern_mdata = kern_data + kern_moff;
>> -
>> -               mtype = skip_mods_and_typedefs(btf, member->type, &mtype_id);
>>                  kern_mtype = skip_mods_and_typedefs(kern_btf, kern_member->type,
>>                                                      &kern_mtype_id);
>>                  if (BTF_INFO_KIND(mtype->info) !=
>> @@ -1230,9 +1247,8 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
>>                          continue;
>>                  }
>>
>> -               msize = btf__resolve_size(btf, mtype_id);
>>                  kern_msize = btf__resolve_size(kern_btf, kern_mtype_id);
>> -               if (msize < 0 || kern_msize < 0 || msize != kern_msize) {
>> +               if (kern_msize < 0 || msize != kern_msize) {
>>                          pr_warn("struct_ops init_kern %s: Error in size of member %s: %zd != %zd(kernel)\n",
>>                                  map->name, mname, (ssize_t)msize,
>>                                  (ssize_t)kern_msize);
>> --
>> 2.34.1
>>

[PATCH bpf-next 2/2] selftests/bpf: Ensure libbpf skip all-zeros fields of struct_ops maps.

[Kui-Feng Lee]

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@gmail.com>
---
 .../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);
+
+	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,
+};
-- 
2.34.1

Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure libbpf skip all-zeros fields of struct_ops maps.

[Andrii Nakryiko]

On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> 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@gmail.com>
> ---
>  .../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?

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

> --
> 2.34.1
>

Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure libbpf skip all-zeros fields of struct_ops maps.

[Kui-Feng Lee]

On 3/12/24 16:10, Andrii Nakryiko wrote:
> On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> 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@gmail.com>
>> ---
>>   .../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
>>

Re: [PATCH bpf-next 2/2] selftests/bpf: Ensure libbpf skip all-zeros fields of struct_ops maps.

[Andrii Nakryiko]

On Tue, Mar 12, 2024 at 5:56 PM Kui-Feng Lee <sinquersw@gmail.com> wrote:
>
>
>
> On 3/12/24 16:10, Andrii Nakryiko wrote:
> > On Tue, Mar 12, 2024 at 11:32 AM Kui-Feng Lee <thinker.li@gmail.com> 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@gmail.com>
> >> ---
> >>   .../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?

Small demo/test was my goal here, to make sure this scenario works as expected.

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

I think libbpf could have checked local callback signature and kernel
callback signature and emit warning. If we don't enforce callback
signature match today and no one complained about that, I'd leave it
as is.

So let's start with a test for now and no more libbpf changes.


>
> If a program never accesses an extra argument, it should be allowed.(?)
> WDYT?
>
>
> >
> >> --
> >> 2.34.1
> >>