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: 1978 bytes --]

Hi Martin,

Please note that after changes for struct_ops map autoload by libbpf,
test_loader could be use to test struct_ops related changes.
Also, test_loader now supports __xlated macro which allows to verify
rewrites applied by verifier.
For example, the sample below works:

    struct st_ops_args;
    
    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);
    }
    
    SEC(".struct_ops.link")
    struct bpf_testmod_st_ops st_ops = {
    	.test_epilogue = (void *)test_epilogue,
    };

(Complete example is in the attachment).
test_loader based tests can also trigger program execution via __retval() macro.
The only (minor) shortcoming that I see, is that test_loader would
load/unload st_ops map multiple times because of the following
interaction:
- test_loader assumes that each bpf program defines a test;
- test_loader re-creates all maps before each test;
- libbpf struct_ops autocreate logic marks all programs referenced
  from struct_ops map as autoloaded.

I think that writing tests this way is easier to follow,
compared to arithmetic manipulations done currently.
What do you think?

Thanks,
Eduard


[-- Attachment #2: struct_ops-test_loader-example.patch --]
[-- Type: text/x-patch, Size: 2395 bytes --]

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..8702c9375023
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_epilogue.c
@@ -0,0 +1,70 @@
+// 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);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops st_ops = {
+	.test_epilogue = (void *)test_epilogue,
+	.test_prologue = (void *)test_prologue,
+};