On 3/18/24 11:34, Andrii Nakryiko wrote:
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.
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
