Re: [RFC PATCH bpf-next 3/6] selftests/test: test gen_prologue and gen_epilogue

[Eduard Zingerman <eddyz87@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

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


[-- Attachment #2: 0001-selftests-bpf-attach-struct_ops-maps-before-test-pro.patch --]
[-- Type: text/x-patch, Size: 2630 bytes --]

From 7c77dc57572d68e206e2fcf230b3aa1c9403fa93 Mon Sep 17 00:00:00 2001
From: Eduard Zingerman <eddyz87@gmail.com>
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@gmail.com>
---
 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


[-- Attachment #3: 0002-selftests-bpf-example-struct_ops-test-using-test_loa.patch --]
[-- Type: text/x-patch, Size: 3368 bytes --]

From bbc36b2b0ec42f5994bef771bf8a85641aeb969e Mon Sep 17 00:00:00 2001
From: Eduard Zingerman <eddyz87@gmail.com>
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