On Fri, 2024-08-16 at 10:27 -0700, Martin KaFai Lau wrote:
[...]
> Thanks for checking!
>
> I think the bpf_map__attach_struct_ops() is not done such that st_ops is NULL.
>
> It probably needs another tag in the SEC("syscall") program to tell which st_ops
> map should be attached first before executing the "syscall" program.
>
> I like the idea of using the __xlated macro to check the patched prologue, ctx
> pointer saving, and epilogue. I will add this test in the respin. I will keep
> the current way in this patch to exercise syscall and the ops/func in st_ops for
> now. We can iterate on it later and use it as an example on what supports are
> needed on the test_loader side for st_ops map testing. On the repetitive-enough
> to worth test_loader refactoring side, I suspect some of the existing st_ops
> load-success/load-failure tests may be worth to look at also. Thoughts?
You are correct, this happens because bpf_map__attach_struct_ops() is
not called. Fortunately, the change for test_loader.c is not very big.
Please check two patches in the attachment.
> I suspect some of the existing st_ops load-success/load-failure
> tests may be worth to look at also.
I suspect this is the case, but would prefer not worry about it for now :)
From 7c77dc57572d68e206e2fcf230b3aa1c9403fa93 Mon Sep 17 00:00:00 2001
From: Eduard Zingerman <eddyz87@xxxxxxxxx>
Date: Fri, 16 Aug 2024 13:16:46 -0700
Subject: [PATCH bpf-next 1/2] selftests/bpf: attach struct_ops maps before
test prog runs
In test_loader based tests to bpf_map__attach_struct_ops()
before call to bpf_prog_test_run_opts() in order to trigger
bpf_struct_ops->reg() callbacks on kernel side.
This allows to use __retval macro for struct_ops tests.
Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
---
tools/testing/selftests/bpf/test_loader.c | 26 +++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index 12b0c41e8d64..67f8d427cfb5 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -729,11 +729,13 @@ void run_subtest(struct test_loader *tester,
{
struct test_subspec *subspec = unpriv ? &spec->unpriv : &spec->priv;
struct bpf_program *tprog = NULL, *tprog_iter;
+ struct bpf_link *link, *links[32] = {};
struct test_spec *spec_iter;
struct cap_state caps = {};
struct bpf_object *tobj;
struct bpf_map *map;
int retval, err, i;
+ int links_cnt = 0;
bool should_load;
if (!test__start_subtest(subspec->name))
@@ -823,6 +825,25 @@ void run_subtest(struct test_loader *tester,
if (restore_capabilities(&caps))
goto tobj_cleanup;
+ /* Do bpf_map__attach_struct_ops() for each struct_ops map.
+ * This should trigger bpf_struct_ops->reg callback on kernel side.
+ */
+ bpf_object__for_each_map(map, tobj) {
+ if (!bpf_map__autocreate(map) || bpf_map__type(map) != BPF_MAP_TYPE_STRUCT_OPS)
+ continue;
+ if (links_cnt >= ARRAY_SIZE(links)) {
+ PRINT_FAIL("too many struct_ops maps");
+ goto tobj_cleanup;
+ }
+ link = bpf_map__attach_struct_ops(map);
+ if (!link) {
+ PRINT_FAIL("bpf_map__attach_struct_ops failed for map %s: err=%d\n",
+ bpf_map__name(map), err);
+ goto tobj_cleanup;
+ }
+ links[links_cnt++] = link;
+ }
+
if (tester->pre_execution_cb) {
err = tester->pre_execution_cb(tobj);
if (err) {
@@ -837,9 +858,14 @@ void run_subtest(struct test_loader *tester,
PRINT_FAIL("Unexpected retval: %d != %d\n", retval, subspec->retval);
goto tobj_cleanup;
}
+ /* redo bpf_map__attach_struct_ops for each test */
+ while (links_cnt > 0)
+ bpf_link__destroy(links[--links_cnt]);
}
tobj_cleanup:
+ while (links_cnt > 0)
+ bpf_link__destroy(links[--links_cnt]);
bpf_object__close(tobj);
subtest_cleanup:
test__end_subtest();
--
2.45.2
From bbc36b2b0ec42f5994bef771bf8a85641aeb969e Mon Sep 17 00:00:00 2001
From: Eduard Zingerman <eddyz87@xxxxxxxxx>
Date: Fri, 16 Aug 2024 13:19:39 -0700
Subject: [PATCH bpf-next 2/2] selftests/bpf: example struct_ops test using
test_loader
This is based on struct_ops_syscall.c and aims to show usage of
__xlated and __retval macros when testing struct_ops related
functionality.
---
.../bpf/prog_tests/struct_ops_epilogue.c | 9 ++
.../selftests/bpf/progs/struct_ops_epilogue.c | 82 +++++++++++++++++++
2 files changed, 91 insertions(+)
create mode 100644 tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
diff --git a/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c b/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
new file mode 100644
index 000000000000..02825d9107ac
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/struct_ops_epilogue.c
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <test_progs.h>
+#include "struct_ops_epilogue.skel.h"
+
+void test_struct_ops_epilogue(void)
+{
+ RUN_TESTS(struct_ops_epilogue);
+}
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c b/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
new file mode 100644
index 000000000000..ca2343e5158a
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+
+char _license[] SEC("license") = "GPL";
+
+struct st_ops_args {
+ int a;
+};
+
+struct bpf_testmod_st_ops {
+ int (*test_prologue)(struct st_ops_args *args);
+ int (*test_epilogue)(struct st_ops_args *args);
+ int (*test_pro_epilogue)(struct st_ops_args *args);
+ struct module *owner;
+};
+
+__success
+__xlated("0: *(u64 *)(r10 -8) = r1")
+__xlated("1: r0 = 0")
+__xlated("2: r1 = *(u64 *)(r10 -8)")
+__xlated("3: r1 = *(u64 *)(r1 +0)")
+__xlated("4: r6 = *(u32 *)(r1 +0)")
+__xlated("5: w6 += 10000")
+__xlated("6: *(u32 *)(r1 +0) = r6")
+__xlated("7: r6 = r1")
+__xlated("8: call kernel-function")
+__xlated("9: r1 = r6")
+__xlated("10: call kernel-function")
+__xlated("11: w0 *= 2")
+__xlated("12: exit")
+SEC("struct_ops/test_epilogue")
+__naked int test_epilogue(void)
+{
+ asm volatile (
+ "r0 = 0;"
+ "exit;"
+ ::: __clobber_all);
+}
+
+__success
+__xlated("0: r6 = *(u64 *)(r1 +0)")
+__xlated("1: r7 = *(u32 *)(r6 +0)")
+__xlated("2: w7 += 1000")
+__xlated("3: *(u32 *)(r6 +0) = r7")
+__xlated("4: r7 = r1")
+__xlated("5: r1 = r6")
+__xlated("6: call kernel-function")
+__xlated("7: r1 = r6")
+__xlated("8: call kernel-function")
+__xlated("9: r1 = r7")
+__xlated("10: r0 = 0")
+__xlated("11: exit")
+SEC("struct_ops/test_prologue")
+__naked int test_prologue(void)
+{
+ asm volatile (
+ "r0 = 0;"
+ "exit;"
+ ::: __clobber_all);
+}
+
+struct st_ops_args;
+int bpf_kfunc_st_ops_test_prologue(struct st_ops_args *args) __ksym;
+
+SEC("syscall")
+__retval(1110)
+int syscall_prologue(void *ctx)
+{
+ struct st_ops_args args = {};
+ bpf_kfunc_st_ops_test_prologue(&args);
+ return args.a;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops st_ops = {
+ .test_epilogue = (void *)test_epilogue,
+ .test_prologue = (void *)test_prologue,
+};
--
2.45.2
