[v4,bpf-next,9/9] selftests/bpf: Test epilogue patching when the main prog has multiple BPF_EXIT

Commit Message

Martin KaFai Lau Aug. 27, 2024, 7:48 p.m. UTC

From: Martin KaFai Lau <[e-mail]>

This patch tests the epilogue patching when the main prog has
multiple BPF_EXIT. The verifier should have patched the 2nd (and
later) BPF_EXIT with a BPF_JA that goes back to the earlier
patched epilogue instructions.

Signed-off-by: Martin KaFai Lau <[e-mail]>
---
 .../selftests/bpf/prog_tests/pro_epilogue.c   |  2 +
 .../selftests/bpf/progs/epilogue_exit.c       | 78 +++++++++++++++++++
 2 files changed, 80 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/epilogue_exit.c

Comments

Martin KaFai Lau Aug. 28, 2024, 12:58 a.m. UTC | #1

On 8/27/24 12:48 PM, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <[e-mail]>
> 
> This patch tests the epilogue patching when the main prog has
> multiple BPF_EXIT. The verifier should have patched the 2nd (and
> later) BPF_EXIT with a BPF_JA that goes back to the earlier
> patched epilogue instructions.
> 
> Signed-off-by: Martin KaFai Lau <[e-mail]>
> ---
>   .../selftests/bpf/prog_tests/pro_epilogue.c   |  2 +
>   .../selftests/bpf/progs/epilogue_exit.c       | 78 +++++++++++++++++++
>   2 files changed, 80 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/epilogue_exit.c
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
> index b2e467cf15fe..58c18529a802 100644
> --- a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
> +++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
> @@ -6,6 +6,7 @@
>   #include "pro_epilogue_kfunc.skel.h"
>   #include "epilogue_tailcall.skel.h"
>   #include "pro_epilogue_goto_start.skel.h"
> +#include "epilogue_exit.skel.h"
>   
>   struct st_ops_args {
>   	int a;
> @@ -47,6 +48,7 @@ void test_pro_epilogue(void)
>   	RUN_TESTS(pro_epilogue_subprog);
>   	RUN_TESTS(pro_epilogue_kfunc);
>   	RUN_TESTS(pro_epilogue_goto_start);
> +	RUN_TESTS(epilogue_exit);
>   	if (test__start_subtest("tailcall"))
>   		test_tailcall();
>   }
> diff --git a/tools/testing/selftests/bpf/progs/epilogue_exit.c b/tools/testing/selftests/bpf/progs/epilogue_exit.c
> new file mode 100644
> index 000000000000..8c03256c7491
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/epilogue_exit.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
> +
> +#include <vmlinux.h>
> +#include <bpf/bpf_tracing.h>
> +#include "bpf_misc.h"
> +#include "../bpf_testmod/bpf_testmod.h"
> +#include "../bpf_testmod/bpf_testmod_kfunc.h"
> +
> +char _license[] SEC("license") = "GPL";
> +
> +__success
> +/* save __u64 *ctx to stack */
> +__xlated("0: *(u64 *)(r10 -8) = r1")
> +/* main prog */
> +__xlated("1: r1 = *(u64 *)(r1 +0)")
> +__xlated("2: r2 = *(u32 *)(r1 +0)")
> +__xlated("3: if r2 == 0x0 goto pc+10")
> +__xlated("4: r0 = 0")
> +__xlated("5: *(u32 *)(r1 +0) = 0")
> +/* epilogue */
> +__xlated("6: r1 = *(u64 *)(r10 -8)")
> +__xlated("7: r1 = *(u64 *)(r1 +0)")
> +__xlated("8: r6 = *(u32 *)(r1 +0)")
> +__xlated("9: w6 += 10000")
> +__xlated("10: *(u32 *)(r1 +0) = r6")
> +__xlated("11: w0 = w6")
> +__xlated("12: w0 *= 2")
> +__xlated("13: exit")
> +/* 2nd part of the main prog after the first exit */
> +__xlated("14: *(u32 *)(r1 +0) = 1")
> +__xlated("15: r0 = 1")
> +/* Clear the r1 to ensure it does not have
> + * off-by-1 error and ensure it jumps back to the
> + * beginning of epilogue which initializes
> + * the r1 with the ctx ptr.
> + */
> +__xlated("16: r1 = 0")
> +__xlated("17: gotol pc-12")
> +SEC("struct_ops/test_epilogue_exit")
> +__naked int test_epilogue_exit(void)
> +{
> +	asm volatile (
> +	"r1 = *(u64 *)(r1 +0);"
> +	"r2 = *(u32 *)(r1 +0);"
> +	"if r2 == 0 goto +3;"
> +	"r0 = 0;"
> +	"*(u32 *)(r1 + 0) = 0;"

llvm17 cannot take "*(u32 *)(r1 +0) = 0".

Instead:

r3 = 0;
*(u32 *)(r1 + 0) = r3;

The above solved the llvm17 error:
https://github.com/kernel-patches/bpf/actions/runs/10586206183/job/29334690461

However, there is still a zext with s390 that added extra insn and failed the 
__xlated check. will try an adjustment in the tests to avoid the zext.

pw-bot: cr


> +	"exit;"
> +	"*(u32 *)(r1 + 0) = 1;"
> +	"r0 = 1;"
> +	"r1 = 0;"
> +	"exit;"
> +	::: __clobber_all);
> +}
> +
> +SEC(".struct_ops.link")
> +struct bpf_testmod_st_ops epilogue_exit = {
> +	.test_epilogue = (void *)test_epilogue_exit,
> +};
> +
> +SEC("syscall")
> +__retval(20000)
> +int syscall_epilogue_exit0(void *ctx)
> +{
> +	struct st_ops_args args = { .a = 1 };
> +
> +	return bpf_kfunc_st_ops_test_epilogue(&args);
> +}
> +
> +SEC("syscall")
> +__retval(20002)
> +int syscall_epilogue_exit1(void *ctx)
> +{
> +	struct st_ops_args args = {};
> +
> +	return bpf_kfunc_st_ops_test_epilogue(&args);
> +}

Eduard Zingerman Aug. 29, 2024, 6:25 a.m. UTC | #2

On Tue, 2024-08-27 at 12:48 -0700, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <[e-mail]>
> 
> This patch tests the epilogue patching when the main prog has
> multiple BPF_EXIT. The verifier should have patched the 2nd (and
> later) BPF_EXIT with a BPF_JA that goes back to the earlier
> patched epilogue instructions.
> 
> Signed-off-by: Martin KaFai Lau <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

[...]

Eduard Zingerman Aug. 29, 2024, 6:28 a.m. UTC | #3

On Tue, 2024-08-27 at 17:58 -0700, Martin KaFai Lau wrote:

[...]

> > +SEC("struct_ops/test_epilogue_exit")
> > +__naked int test_epilogue_exit(void)
> > +{
> > +	asm volatile (
> > +	"r1 = *(u64 *)(r1 +0);"
> > +	"r2 = *(u32 *)(r1 +0);"
> > +	"if r2 == 0 goto +3;"
> > +	"r0 = 0;"
> > +	"*(u32 *)(r1 + 0) = 0;"
> 
> llvm17 cannot take "*(u32 *)(r1 +0) = 0".
> 
> Instead:
> 
> r3 = 0;
> *(u32 *)(r1 + 0) = r3;
> 
> The above solved the llvm17 error:
> https://github.com/kernel-patches/bpf/actions/runs/10586206183/job/29334690461
> 
> However, there is still a zext with s390 that added extra insn and failed the 
> __xlated check. will try an adjustment in the tests to avoid the zext.

Another option would be to limit archs for the test, e.g. use
__arch_x86_64 and __arch_arm64.

Martin KaFai Lau Aug. 29, 2024, 8:09 p.m. UTC | #4

On 8/28/24 11:28 PM, Eduard Zingerman wrote:
> On Tue, 2024-08-27 at 17:58 -0700, Martin KaFai Lau wrote:
> 
> [...]
> 
>>> +SEC("struct_ops/test_epilogue_exit")
>>> +__naked int test_epilogue_exit(void)
>>> +{
>>> +	asm volatile (
>>> +	"r1 = *(u64 *)(r1 +0);"
>>> +	"r2 = *(u32 *)(r1 +0);"
>>> +	"if r2 == 0 goto +3;"
>>> +	"r0 = 0;"
>>> +	"*(u32 *)(r1 + 0) = 0;"
>>
>> llvm17 cannot take "*(u32 *)(r1 +0) = 0".
>>
>> Instead:
>>
>> r3 = 0;
>> *(u32 *)(r1 + 0) = r3;
>>
>> The above solved the llvm17 error:
>> https://github.com/kernel-patches/bpf/actions/runs/10586206183/job/29334690461
>>
>> However, there is still a zext with s390 that added extra insn and failed the
>> __xlated check. will try an adjustment in the tests to avoid the zext.
> 
> Another option would be to limit archs for the test, e.g. use
> __arch_x86_64 and __arch_arm64.

Ah. good to know.

I have used all 64bits ops to solve it. It passed the earlier bpf CI run:
https://github.com/kernel-patches/bpf/actions/runs/10590714532
I will respin with the 64bits ops solution.

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
index b2e467cf15fe..58c18529a802 100644
--- a/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
+++ b/tools/testing/selftests/bpf/prog_tests/pro_epilogue.c
@@ -6,6 +6,7 @@ 
 #include "pro_epilogue_kfunc.skel.h"
 #include "epilogue_tailcall.skel.h"
 #include "pro_epilogue_goto_start.skel.h"
+#include "epilogue_exit.skel.h"
 
 struct st_ops_args {
 	int a;
@@ -47,6 +48,7 @@  void test_pro_epilogue(void)
 	RUN_TESTS(pro_epilogue_subprog);
 	RUN_TESTS(pro_epilogue_kfunc);
 	RUN_TESTS(pro_epilogue_goto_start);
+	RUN_TESTS(epilogue_exit);
 	if (test__start_subtest("tailcall"))
 		test_tailcall();
 }
diff --git a/tools/testing/selftests/bpf/progs/epilogue_exit.c b/tools/testing/selftests/bpf/progs/epilogue_exit.c
new file mode 100644
index 000000000000..8c03256c7491
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/epilogue_exit.c
@@ -0,0 +1,78 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2024 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include "bpf_misc.h"
+#include "../bpf_testmod/bpf_testmod.h"
+#include "../bpf_testmod/bpf_testmod_kfunc.h"
+
+char _license[] SEC("license") = "GPL";
+
+__success
+/* save __u64 *ctx to stack */
+__xlated("0: *(u64 *)(r10 -8) = r1")
+/* main prog */
+__xlated("1: r1 = *(u64 *)(r1 +0)")
+__xlated("2: r2 = *(u32 *)(r1 +0)")
+__xlated("3: if r2 == 0x0 goto pc+10")
+__xlated("4: r0 = 0")
+__xlated("5: *(u32 *)(r1 +0) = 0")
+/* epilogue */
+__xlated("6: r1 = *(u64 *)(r10 -8)")
+__xlated("7: r1 = *(u64 *)(r1 +0)")
+__xlated("8: r6 = *(u32 *)(r1 +0)")
+__xlated("9: w6 += 10000")
+__xlated("10: *(u32 *)(r1 +0) = r6")
+__xlated("11: w0 = w6")
+__xlated("12: w0 *= 2")
+__xlated("13: exit")
+/* 2nd part of the main prog after the first exit */
+__xlated("14: *(u32 *)(r1 +0) = 1")
+__xlated("15: r0 = 1")
+/* Clear the r1 to ensure it does not have
+ * off-by-1 error and ensure it jumps back to the
+ * beginning of epilogue which initializes
+ * the r1 with the ctx ptr.
+ */
+__xlated("16: r1 = 0")
+__xlated("17: gotol pc-12")
+SEC("struct_ops/test_epilogue_exit")
+__naked int test_epilogue_exit(void)
+{
+	asm volatile (
+	"r1 = *(u64 *)(r1 +0);"
+	"r2 = *(u32 *)(r1 +0);"
+	"if r2 == 0 goto +3;"
+	"r0 = 0;"
+	"*(u32 *)(r1 + 0) = 0;"
+	"exit;"
+	"*(u32 *)(r1 + 0) = 1;"
+	"r0 = 1;"
+	"r1 = 0;"
+	"exit;"
+	::: __clobber_all);
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_st_ops epilogue_exit = {
+	.test_epilogue = (void *)test_epilogue_exit,
+};
+
+SEC("syscall")
+__retval(20000)
+int syscall_epilogue_exit0(void *ctx)
+{
+	struct st_ops_args args = { .a = 1 };
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}
+
+SEC("syscall")
+__retval(20002)
+int syscall_epilogue_exit1(void *ctx)
+{
+	struct st_ops_args args = {};
+
+	return bpf_kfunc_st_ops_test_epilogue(&args);
+}