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

[PATCH bpf-next v2 0/3] 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.

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

 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

[PATCH bpf-next v2 1/3] 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 | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index efab29b8935b..d0ba6eb605f8 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -1132,8 +1132,26 @@ static int bpf_map__init_kern_struct_ops(struct bpf_map *map)
 		const char *mname;
 
 		mname = btf__name_by_offset(btf, member->name_off);
+		moff = member->offset / 8;
+		mdata = data + moff;
+		msize = btf__resolve_size(btf, member->type);
+		if (msize < 0) {
+			pr_warn("struct_ops init_kern %s: failed 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) {
+			/* Skip all zeros or null fields if they are not
+			 * presented in the kernel BTF.
+			 */
+			if (libbpf_is_mem_zeroed(mdata, msize)) {
+				pr_info("struct_ops %s: member %s not found in kernel, skipping it as it's set to zero\n",
+					map->name, mname);
+				continue;
+			}
+
 			pr_warn("struct_ops init_kern %s: Cannot find member %s in kernel BTF\n",
 				map->name, mname);
 			return -ENOTSUP;
@@ -1147,10 +1165,7 @@ 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);
@@ -1230,9 +1245,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 v2 2/3] 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   | 47 +++++++++++++++++++
 .../selftests/bpf/progs/struct_ops_module.c   | 16 ++++++-
 2 files changed, 62 insertions(+), 1 deletion(-)

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..098776d00ab4 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,56 @@ 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;
+
+	/* zeroed is 0, and zeroed_op is null */
+	skel = struct_ops_module__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_module_open"))
+		return;
+
+	err = struct_ops_module__load(skel);
+	ASSERT_OK(err, "struct_ops_module_load");
+
+	struct_ops_module__destroy(skel);
+
+	/* zeroed is not 0 */
+	skel = struct_ops_module__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_not_zeroed"))
+		return;
+
+	/* libbpf should reject the testmod_zeroed since struct
+	 * bpf_testmod_ops in the kernel has no "zeroed" field and the
+	 * value of "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);
+
+	/* zeroed_op is not null */
+	skel = struct_ops_module__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_not_zeroed_op"))
+		return;
+
+	/* libbpf should reject the testmod_zeroed since the value of its
+	 * "zeroed_op" is not null.
+	 */
+	skel->struct_ops.testmod_zeroed->zeroed_op = skel->progs.test_3;
+	err = struct_ops_module__load(skel);
+	ASSERT_ERR(err, "struct_ops_module_load_not_zeroed_op");
+
+	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..86e1e50c5531 100644
--- a/tools/testing/selftests/bpf/progs/struct_ops_module.c
+++ b/tools/testing/selftests/bpf/progs/struct_ops_module.c
@@ -23,7 +23,7 @@ void BPF_PROG(test_2, int a, int b)
 	test_2_result = a + b;
 }
 
-SEC("struct_ops/test_3")
+SEC("?struct_ops/test_3")
 int BPF_PROG(test_3, int a, int b)
 {
 	test_2_result = a + b + 3;
@@ -54,3 +54,17 @@ 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);
+	void (*zeroed_op)(int a, int b);
+	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

[PATCH bpf-next v2 3/3] selftests/bpf: Accept extra arguments if they are not used.

[Kui-Feng Lee]

A struct_ops program can have more arguments than what the corresponding
function pointer in the kernel has if the program doesn't access the
arguments. For example, a struct_ops operator may have 2 arguments, but
your program may defined with 3 or more arguments. It is acceptable as long
as the program doesn't use these arguments.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 .../bpf/prog_tests/test_struct_ops_module.c   | 56 +++++++++++++++++++
 .../bpf/progs/struct_ops_extra_arg.c          | 49 ++++++++++++++++
 2 files changed, 105 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_extra_arg.c

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..a146bf079a60 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
@@ -4,6 +4,7 @@
 #include <time.h>
 
 #include "struct_ops_module.skel.h"
+#include "struct_ops_extra_arg.skel.h"
 
 static void check_map_info(struct bpf_map_info *info)
 {
@@ -138,11 +139,66 @@ static void test_struct_ops_not_zeroed(void)
 	struct_ops_module__destroy(skel);
 }
 
+/* Handle BPF programs with additional arguments that don't appear in the
+ * function pointer prototype in the kernel.
+ */
+static void test_struct_ops_extra_arg(void)
+{
+	struct struct_ops_extra_arg *skel;
+	int err;
+
+	/* test_extra_arg() has an extra argument, so testmod_1 should fail
+	 * to load.
+	 *
+	 * Since extra_arg is used in test_extra_arg(), it should be
+	 * rejected by the verifier.
+	 */
+	skel = struct_ops_extra_arg__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_extra_arg_open_extra_arg"))
+		return;
+
+	err = struct_ops_extra_arg__load(skel);
+	ASSERT_ERR(err, "struct_ops_extra_arg_load_extra_arg");
+
+	struct_ops_extra_arg__destroy(skel);
+
+	/* Switch to test_2() */
+	skel = struct_ops_extra_arg__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_extra_arg_open"))
+		return;
+
+	skel->struct_ops.testmod_1->test_2 = skel->progs.test_2;
+
+	err = struct_ops_extra_arg__load(skel);
+	ASSERT_OK(err, "struct_ops_extra_arg_load");
+
+	struct_ops_extra_arg__destroy(skel);
+
+	/* Switch to test_extra_arg_unused()
+	 *
+	 * Since extra_arg is never used, it should be accepted by the
+	 * verifier. The verifier would accept a program with extra
+	 * arguments as long as they are not used.
+	 */
+	skel = struct_ops_extra_arg__open();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_extra_arg_open_unused"))
+		return;
+
+	skel->struct_ops.testmod_1->test_2 = skel->progs.test_extra_arg_unused;
+
+	err = struct_ops_extra_arg__load(skel);
+	ASSERT_OK(err, "struct_ops_extra_arg_load_unused");
+
+	struct_ops_extra_arg__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();
+	if (test__start_subtest("test_struct_ops_extra_arg"))
+		test_struct_ops_extra_arg();
 }
 
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_extra_arg.c b/tools/testing/selftests/bpf/progs/struct_ops_extra_arg.c
new file mode 100644
index 000000000000..4b73b279ad22
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_extra_arg.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+int test_1_result = 0;
+int test_2_result = 0;
+
+SEC("struct_ops/test_1")
+int BPF_PROG(test_1)
+{
+	test_1_result = 0xdeadbeef;
+	return 0;
+}
+
+SEC("?struct_ops/test_2")
+void BPF_PROG(test_2, int a, int b)
+{
+	test_2_result = a + b;
+}
+
+SEC("?struct_ops/test_extra_arg")
+void BPF_PROG(test_extra_arg, int a, int b, int extra_arg)
+{
+	/* Accessing extra_arg will cause a verifier error since it
+	 * accesses the context data beyond the size of the context.
+	 */
+	test_2_result = a + b + extra_arg + 3;
+}
+
+SEC("?struct_ops/test_extra_arg_unused")
+void BPF_PROG(test_extra_arg_unused, int a, int b, int extra_arg)
+{
+	/* The extra_arg is not used, so it should not cause a verifier
+	 * error.
+	 */
+	test_2_result = a + b + 3;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+	.test_1 = (void *)test_1,
+	.test_2 = (void *)test_extra_arg,
+	.data = 0x1,
+};
-- 
2.34.1

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

[Andrii Nakryiko]

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,
+};


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
>

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 13 Mar 2024 14:41:36 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [bpf-next,v2,1/3] libbpf: Skip zeroed or null fields if not found in the kernel type.
    https://git.kernel.org/bpf/bpf-next/c/c911fc61a7ce
  - [bpf-next,v2,2/3] selftests/bpf: Ensure libbpf skip all-zeros fields of struct_ops maps.
    https://git.kernel.org/bpf/bpf-next/c/26a7cf2bbea6
  - [bpf-next,v2,3/3] selftests/bpf: Accept extra arguments if they are not used.
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Kui-Feng Lee]

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

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

[Andrii Nakryiko]

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.

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

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

[Kui-Feng Lee]

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