* [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop
@ 2023-09-03 15:14 Leon Hwang
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation Leon Hwang
` (4 more replies)
0 siblings, 5 replies; 16+ messages in thread
From: Leon Hwang @ 2023-09-03 15:14 UTC (permalink / raw)
To: ast, daniel, andrii, maciej.fijalkowski; +Cc: song, iii, jakub, hffilwlqm, bpf
This patch series fixes a tailcall infinite loop on x64.
From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and tailcall
handling in JIT"), the tailcall on x64 works better than before.
From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF subprograms
for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF program
to other BPF programs"), BPF program is able to trace other BPF programs.
How about combining them all together?
1. FENTRY/FEXIT on a BPF subprogram.
2. A tailcall runs in the BPF subprogram.
3. The tailcall calls the subprogram's caller.
As a result, a tailcall infinite loop comes up. And the loop would halt
the machine.
As we know, in tail call context, the tail_call_cnt propagates by stack
and rax register between BPF subprograms. So do in trampolines.
How did I discover the bug?
From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth 256 for
subprogs with tailcalls"), the total stack size limits to around 8KiB.
Then, I write some bpf progs to validate the stack consuming, that are
tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on bpf2bpf[1].
At that time, accidently, I made a tailcall loop. And then the loop halted
my VM. Without the loop, the bpf progs would consume over 8KiB stack size.
But the _stack-overflow_ did not halt my VM.
With bpf_printk(), I confirmed that the tailcall count limit did not work
expectedly. Next, read the code and fix it.
Finally, unfortunately, I only fix it on x64 but other arches. As a
result, CI tests failed because this bug hasn't been fixed on s390x.
Some helps on s390x are requested.
v3 -> v4:
* Suggestions from Maciej:
* Separate tail_call_cnt initialisation to a single patch.
* Unnecessary to check subprog.
v2 -> v3:
* Suggestions from Alexei:
* Fix comment style.
* Remove FIXME comment.
* Remove arch_prepare_bpf_trampoline() function comment update.
* Remove the unnecessary 'tgt_prog->aux->func[subprog]->is_func' check.
[1]: https://github.com/Asphaltt/learn-by-example/tree/main/ebpf/tailcall-stackoverflow
Leon Hwang (4):
bpf, x64: Comment tail_call_cnt initialisation
bpf, x64: Fix tailcall infinite loop
selftests/bpf: Correct map_fd to data_fd in tailcalls
selftests/bpf: Add testcases for tailcall infinite loop fixing
arch/x86/net/bpf_jit_comp.c | 32 +-
include/linux/bpf.h | 5 +
kernel/bpf/trampoline.c | 4 +-
kernel/bpf/verifier.c | 3 +
.../selftests/bpf/prog_tests/tailcalls.c | 311 +++++++++++++++++-
.../bpf/progs/tailcall_bpf2bpf_fentry.c | 18 +
.../bpf/progs/tailcall_bpf2bpf_fexit.c | 18 +
7 files changed, 377 insertions(+), 14 deletions(-)
create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
base-commit: 9930e4af4b509bcf6f060b09b16884f26102d110
--
2.41.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation
2023-09-03 15:14 [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
@ 2023-09-03 15:14 ` Leon Hwang
2023-09-05 19:26 ` Maciej Fijalkowski
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Leon Hwang @ 2023-09-03 15:14 UTC (permalink / raw)
To: ast, daniel, andrii, maciej.fijalkowski; +Cc: song, iii, jakub, hffilwlqm, bpf
Without understanding emit_prologue(), it is really hard to figure out
where does tail_call_cnt come from, even though searching tail_call_cnt
in the whole kernel repo.
By adding these comments, it is a little bit easy to understand
tail_call_cnt initialisation.
Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
arch/x86/net/bpf_jit_comp.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index a5930042139d3..bcca1c9b9a027 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
prog += X86_PATCH_SIZE;
if (!ebpf_from_cbpf) {
if (tail_call_reachable && !is_subprog)
+ /* When it's the entry of the whole tailcall context,
+ * zeroing rax means initialising tail_call_cnt.
+ */
EMIT2(0x31, 0xC0); /* xor eax, eax */
else
+ /* Keep the same instruction layout. */
EMIT2(0x66, 0x90); /* nop2 */
}
EMIT1(0x55); /* push rbp */
--
2.41.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop
2023-09-03 15:14 [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation Leon Hwang
@ 2023-09-03 15:14 ` Leon Hwang
2023-09-06 20:50 ` Maciej Fijalkowski
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls Leon Hwang
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Leon Hwang @ 2023-09-03 15:14 UTC (permalink / raw)
To: ast, daniel, andrii, maciej.fijalkowski; +Cc: song, iii, jakub, hffilwlqm, bpf
From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and tailcall
handling in JIT"), the tailcall on x64 works better than before.
From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF subprograms
for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF program
to other BPF programs"), BPF program is able to trace other BPF programs.
How about combining them all together?
1. FENTRY/FEXIT on a BPF subprogram.
2. A tailcall runs in the BPF subprogram.
3. The tailcall calls the subprogram's caller.
As a result, a tailcall infinite loop comes up. And the loop would halt
the machine.
As we know, in tail call context, the tail_call_cnt propagates by stack
and rax register between BPF subprograms. So do in trampolines.
Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
Fixes: e411901c0b77 ("bpf: allow for tailcalls in BPF subprograms for x64 JIT")
Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
arch/x86/net/bpf_jit_comp.c | 28 ++++++++++++++++++++++------
include/linux/bpf.h | 5 +++++
kernel/bpf/trampoline.c | 4 ++--
kernel/bpf/verifier.c | 3 +++
4 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index bcca1c9b9a027..2846c21d75bfa 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -1022,6 +1022,10 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
#define INSN_SZ_DIFF (((addrs[i] - addrs[i - 1]) - (prog - temp)))
+/* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
+#define RESTORE_TAIL_CALL_CNT(stack) \
+ EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
+
static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
int oldproglen, struct jit_context *ctx, bool jmp_padding)
{
@@ -1627,9 +1631,7 @@ st: if (is_imm8(insn->off))
func = (u8 *) __bpf_call_base + imm32;
if (tail_call_reachable) {
- /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
- EMIT3_off32(0x48, 0x8B, 0x85,
- -round_up(bpf_prog->aux->stack_depth, 8) - 8);
+ RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
if (!imm32)
return -EINVAL;
offs = 7 + x86_call_depth_emit_accounting(&prog, func);
@@ -2404,6 +2406,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
* [ ... ]
* [ stack_arg2 ]
* RBP - arg_stack_off [ stack_arg1 ]
+ * RSP [ tail_call_cnt ] BPF_TRAMP_F_TAIL_CALL_CTX
*/
/* room for return value of orig_call or fentry prog */
@@ -2468,6 +2471,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
else
/* sub rsp, stack_size */
EMIT4(0x48, 0x83, 0xEC, stack_size);
+ if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
+ EMIT1(0x50); /* push rax */
/* mov QWORD PTR [rbp - rbx_off], rbx */
emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_6, -rbx_off);
@@ -2520,9 +2525,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
restore_regs(m, &prog, regs_off);
save_args(m, &prog, arg_stack_off, true);
+ if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
+ /* Before calling the original function, restore the
+ * tail_call_cnt from stack to rax.
+ */
+ RESTORE_TAIL_CALL_CNT(stack_size);
+
if (flags & BPF_TRAMP_F_ORIG_STACK) {
- emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
- EMIT2(0xff, 0xd0); /* call *rax */
+ emit_ldx(&prog, BPF_DW, BPF_REG_6, BPF_REG_FP, 8);
+ EMIT2(0xff, 0xd3); /* call *rbx */
} else {
/* call original function */
if (emit_rsb_call(&prog, orig_call, prog)) {
@@ -2573,7 +2584,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
ret = -EINVAL;
goto cleanup;
}
- }
+ } else if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
+ /* Before running the original function, restore the
+ * tail_call_cnt from stack to rax.
+ */
+ RESTORE_TAIL_CALL_CNT(stack_size);
+
/* restore return value of orig_call or fentry prog back into RAX */
if (save_ret)
emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index cfabbcf47bdb8..c8df257ea435d 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1028,6 +1028,11 @@ struct btf_func_model {
*/
#define BPF_TRAMP_F_SHARE_IPMODIFY BIT(6)
+/* Indicate that current trampoline is in a tail call context. Then, it has to
+ * cache and restore tail_call_cnt to avoid infinite tail call loop.
+ */
+#define BPF_TRAMP_F_TAIL_CALL_CTX BIT(7)
+
/* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
* bytes on x86.
*/
diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
index 78acf28d48732..16ab5da7161f2 100644
--- a/kernel/bpf/trampoline.c
+++ b/kernel/bpf/trampoline.c
@@ -415,8 +415,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
goto out;
}
- /* clear all bits except SHARE_IPMODIFY */
- tr->flags &= BPF_TRAMP_F_SHARE_IPMODIFY;
+ /* clear all bits except SHARE_IPMODIFY and TAIL_CALL_CTX */
+ tr->flags &= (BPF_TRAMP_F_SHARE_IPMODIFY | BPF_TRAMP_F_TAIL_CALL_CTX);
if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4ccca1f6c9981..765da3007106a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -19629,6 +19629,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
if (!tr)
return -ENOMEM;
+ if (tgt_prog && tgt_prog->aux->tail_call_reachable)
+ tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX;
+
prog->aux->dst_trampoline = tr;
return 0;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls
2023-09-03 15:14 [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation Leon Hwang
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
@ 2023-09-03 15:14 ` Leon Hwang
2023-09-05 19:22 ` Maciej Fijalkowski
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing Leon Hwang
2023-09-04 13:10 ` [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Ilya Leoshkevich
4 siblings, 1 reply; 16+ messages in thread
From: Leon Hwang @ 2023-09-03 15:14 UTC (permalink / raw)
To: ast, daniel, andrii, maciej.fijalkowski; +Cc: song, iii, jakub, hffilwlqm, bpf
Get and check data_fd. It should not to check map_fd again.
Fixes: 79d49ba048ec ("bpf, testing: Add various tail call test cases")
Fixes: 3b0379111197 ("selftests/bpf: Add tailcall_bpf2bpf tests")
Fixes: 5e0b0a4c52d3 ("selftests/bpf: Test tail call counting with bpf2bpf and data on stack")
Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
tools/testing/selftests/bpf/prog_tests/tailcalls.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index 58fe2c586ed76..b20d7f77a5bce 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -274,7 +274,7 @@ static void test_tailcall_count(const char *which)
return;
data_fd = bpf_map__fd(data_map);
- if (CHECK_FAIL(map_fd < 0))
+ if (CHECK_FAIL(data_fd < 0))
return;
i = 0;
@@ -355,7 +355,7 @@ static void test_tailcall_4(void)
return;
data_fd = bpf_map__fd(data_map);
- if (CHECK_FAIL(map_fd < 0))
+ if (CHECK_FAIL(data_fd < 0))
return;
for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
@@ -445,7 +445,7 @@ static void test_tailcall_5(void)
return;
data_fd = bpf_map__fd(data_map);
- if (CHECK_FAIL(map_fd < 0))
+ if (CHECK_FAIL(data_fd < 0))
return;
for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
@@ -634,7 +634,7 @@ static void test_tailcall_bpf2bpf_2(void)
return;
data_fd = bpf_map__fd(data_map);
- if (CHECK_FAIL(map_fd < 0))
+ if (CHECK_FAIL(data_fd < 0))
return;
i = 0;
@@ -808,7 +808,7 @@ static void test_tailcall_bpf2bpf_4(bool noise)
return;
data_fd = bpf_map__fd(data_map);
- if (CHECK_FAIL(map_fd < 0))
+ if (CHECK_FAIL(data_fd < 0))
return;
i = 0;
@@ -872,7 +872,7 @@ static void test_tailcall_bpf2bpf_6(void)
ASSERT_EQ(topts.retval, 0, "tailcall retval");
data_fd = bpf_map__fd(obj->maps.bss);
- if (!ASSERT_GE(map_fd, 0, "bss map fd"))
+ if (!ASSERT_GE(data_fd, 0, "bss map fd"))
goto out;
i = 0;
--
2.41.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing
2023-09-03 15:14 [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
` (2 preceding siblings ...)
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls Leon Hwang
@ 2023-09-03 15:14 ` Leon Hwang
2023-09-06 21:18 ` Maciej Fijalkowski
2023-09-04 13:10 ` [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Ilya Leoshkevich
4 siblings, 1 reply; 16+ messages in thread
From: Leon Hwang @ 2023-09-03 15:14 UTC (permalink / raw)
To: ast, daniel, andrii, maciej.fijalkowski; +Cc: song, iii, jakub, hffilwlqm, bpf
Add 4 test cases to confirm the tailcall infinite loop bug has been fixed.
Like tailcall_bpf2bpf cases, do fentry/fexit on the bpf2bpf, and then
check the final count result.
tools/testing/selftests/bpf/test_progs -t tailcalls
226/13 tailcalls/tailcall_bpf2bpf_fentry:OK
226/14 tailcalls/tailcall_bpf2bpf_fexit:OK
226/15 tailcalls/tailcall_bpf2bpf_fentry_fexit:OK
226/16 tailcalls/tailcall_bpf2bpf_fentry_entry:OK
226 tailcalls:OK
Summary: 1/16 PASSED, 0 SKIPPED, 0 FAILED
Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
---
.../selftests/bpf/prog_tests/tailcalls.c | 299 ++++++++++++++++++
.../bpf/progs/tailcall_bpf2bpf_fentry.c | 18 ++
.../bpf/progs/tailcall_bpf2bpf_fexit.c | 18 ++
3 files changed, 335 insertions(+)
create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
index b20d7f77a5bce..331b4e455ad06 100644
--- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
+++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
@@ -884,6 +884,297 @@ static void test_tailcall_bpf2bpf_6(void)
tailcall_bpf2bpf6__destroy(obj);
}
+static void tailcall_bpf2bpf_fentry_fexit(bool test_fentry, bool test_fexit)
+{
+ struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL, *fexit_obj = NULL;
+ struct bpf_link *fentry_link = NULL, *fexit_link = NULL;
+ int err, map_fd, prog_fd, main_fd, data_fd, i, val;
+ struct bpf_map *prog_array, *data_map;
+ struct bpf_program *prog;
+ char buff[128] = {};
+
+ LIBBPF_OPTS(bpf_test_run_opts, topts,
+ .data_in = buff,
+ .data_size_in = sizeof(buff),
+ .repeat = 1,
+ );
+
+ err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
+ BPF_PROG_TYPE_SCHED_CLS,
+ &tgt_obj, &prog_fd);
+ if (!ASSERT_OK(err, "load tgt_obj"))
+ return;
+
+ prog = bpf_object__find_program_by_name(tgt_obj, "entry");
+ if (!ASSERT_OK_PTR(prog, "find entry prog"))
+ goto out;
+
+ main_fd = bpf_program__fd(prog);
+ if (!ASSERT_FALSE(main_fd < 0, "find entry prog fd"))
+ goto out;
+
+ prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
+ if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
+ goto out;
+
+ map_fd = bpf_map__fd(prog_array);
+ if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
+ goto out;
+
+ prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
+ if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
+ goto out;
+
+ prog_fd = bpf_program__fd(prog);
+ if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
+ goto out;
+
+ i = 0;
+ err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
+ if (!ASSERT_OK(err, "update jmp_table"))
+ goto out;
+
+ if (test_fentry) {
+ fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
+ NULL);
+ if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
+ goto out;
+
+ prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
+ if (!ASSERT_OK_PTR(prog, "find fentry prog"))
+ goto out;
+
+ err = bpf_program__set_attach_target(prog, prog_fd,
+ "subprog_tail");
+ if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
+ goto out;
+
+ err = bpf_object__load(fentry_obj);
+ if (!ASSERT_OK(err, "load fentry_obj"))
+ goto out;
+
+ fentry_link = bpf_program__attach_trace(prog);
+ if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
+ goto out;
+ }
+
+ if (test_fexit) {
+ fexit_obj = bpf_object__open_file("tailcall_bpf2bpf_fexit.bpf.o",
+ NULL);
+ if (!ASSERT_OK_PTR(fexit_obj, "open fexit_obj file"))
+ goto out;
+
+ prog = bpf_object__find_program_by_name(fexit_obj, "fexit");
+ if (!ASSERT_OK_PTR(prog, "find fexit prog"))
+ goto out;
+
+ err = bpf_program__set_attach_target(prog, prog_fd,
+ "subprog_tail");
+ if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
+ goto out;
+
+ err = bpf_object__load(fexit_obj);
+ if (!ASSERT_OK(err, "load fexit_obj"))
+ goto out;
+
+ fexit_link = bpf_program__attach_trace(prog);
+ if (!ASSERT_OK_PTR(fexit_link, "attach_trace"))
+ goto out;
+ }
+
+ err = bpf_prog_test_run_opts(main_fd, &topts);
+ ASSERT_OK(err, "tailcall");
+ ASSERT_EQ(topts.retval, 1, "tailcall retval");
+
+ data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
+ if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+ "find tailcall.bss map"))
+ goto out;
+
+ data_fd = bpf_map__fd(data_map);
+ if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
+ goto out;
+
+ i = 0;
+ err = bpf_map_lookup_elem(data_fd, &i, &val);
+ ASSERT_OK(err, "tailcall count");
+ ASSERT_EQ(val, 33, "tailcall count");
+
+ if (test_fentry) {
+ data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
+ if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+ "find tailcall_bpf2bpf_fentry.bss map"))
+ goto out;
+
+ data_fd = bpf_map__fd(data_map);
+ if (!ASSERT_FALSE(data_fd < 0,
+ "find tailcall_bpf2bpf_fentry.bss map fd"))
+ goto out;
+
+ i = 0;
+ err = bpf_map_lookup_elem(data_fd, &i, &val);
+ ASSERT_OK(err, "fentry count");
+ ASSERT_EQ(val, 33, "fentry count");
+ }
+
+ if (test_fexit) {
+ data_map = bpf_object__find_map_by_name(fexit_obj, ".bss");
+ if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+ "find tailcall_bpf2bpf_fexit.bss map"))
+ goto out;
+
+ data_fd = bpf_map__fd(data_map);
+ if (!ASSERT_FALSE(data_fd < 0,
+ "find tailcall_bpf2bpf_fexit.bss map fd"))
+ goto out;
+
+ i = 0;
+ err = bpf_map_lookup_elem(data_fd, &i, &val);
+ ASSERT_OK(err, "fexit count");
+ ASSERT_EQ(val, 33, "fexit count");
+ }
+
+out:
+ bpf_link__destroy(fentry_link);
+ bpf_link__destroy(fexit_link);
+ bpf_object__close(fentry_obj);
+ bpf_object__close(fexit_obj);
+ bpf_object__close(tgt_obj);
+}
+
+/* test_tailcall_bpf2bpf_fentry checks that the count value of the tail call
+ * limit enforcement matches with expectations when tailcall is preceded with
+ * bpf2bpf call, and the bpf2bpf call is traced by fentry.
+ */
+static void test_tailcall_bpf2bpf_fentry(void)
+{
+ tailcall_bpf2bpf_fentry_fexit(true, false);
+}
+
+/* test_tailcall_bpf2bpf_fexit checks that the count value of the tail call
+ * limit enforcement matches with expectations when tailcall is preceded with
+ * bpf2bpf call, and the bpf2bpf call is traced by fexit.
+ */
+static void test_tailcall_bpf2bpf_fexit(void)
+{
+ tailcall_bpf2bpf_fentry_fexit(false, true);
+}
+
+/* test_tailcall_bpf2bpf_fentry_fexit checks that the count value of the tail
+ * call limit enforcement matches with expectations when tailcall is preceded
+ * with bpf2bpf call, and the bpf2bpf call is traced by both fentry and fexit.
+ */
+static void test_tailcall_bpf2bpf_fentry_fexit(void)
+{
+ tailcall_bpf2bpf_fentry_fexit(true, true);
+}
+
+/* test_tailcall_bpf2bpf_fentry_entry checks that the count value of the tail
+ * call limit enforcement matches with expectations when tailcall is preceded
+ * with bpf2bpf call, and the bpf2bpf caller is traced by fentry.
+ */
+static void test_tailcall_bpf2bpf_fentry_entry(void)
+{
+ struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL;
+ int err, map_fd, prog_fd, data_fd, i, val;
+ struct bpf_map *prog_array, *data_map;
+ struct bpf_link *fentry_link = NULL;
+ struct bpf_program *prog;
+ char buff[128] = {};
+
+ LIBBPF_OPTS(bpf_test_run_opts, topts,
+ .data_in = buff,
+ .data_size_in = sizeof(buff),
+ .repeat = 1,
+ );
+
+ err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
+ BPF_PROG_TYPE_SCHED_CLS,
+ &tgt_obj, &prog_fd);
+ if (!ASSERT_OK(err, "load tgt_obj"))
+ return;
+
+ prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
+ if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
+ goto out;
+
+ map_fd = bpf_map__fd(prog_array);
+ if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
+ goto out;
+
+ prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
+ if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
+ goto out;
+
+ prog_fd = bpf_program__fd(prog);
+ if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
+ goto out;
+
+ i = 0;
+ err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
+ if (!ASSERT_OK(err, "update jmp_table"))
+ goto out;
+
+ fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
+ NULL);
+ if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
+ goto out;
+
+ prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
+ if (!ASSERT_OK_PTR(prog, "find fentry prog"))
+ goto out;
+
+ err = bpf_program__set_attach_target(prog, prog_fd, "classifier_0");
+ if (!ASSERT_OK(err, "set_attach_target classifier_0"))
+ goto out;
+
+ err = bpf_object__load(fentry_obj);
+ if (!ASSERT_OK(err, "load fentry_obj"))
+ goto out;
+
+ fentry_link = bpf_program__attach_trace(prog);
+ if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
+ goto out;
+
+ err = bpf_prog_test_run_opts(prog_fd, &topts);
+ ASSERT_OK(err, "tailcall");
+ ASSERT_EQ(topts.retval, 1, "tailcall retval");
+
+ data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
+ if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+ "find tailcall.bss map"))
+ goto out;
+
+ data_fd = bpf_map__fd(data_map);
+ if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
+ goto out;
+
+ i = 0;
+ err = bpf_map_lookup_elem(data_fd, &i, &val);
+ ASSERT_OK(err, "tailcall count");
+ ASSERT_EQ(val, 34, "tailcall count");
+
+ data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
+ if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
+ "find tailcall_bpf2bpf_fentry.bss map"))
+ goto out;
+
+ data_fd = bpf_map__fd(data_map);
+ if (!ASSERT_FALSE(data_fd < 0,
+ "find tailcall_bpf2bpf_fentry.bss map fd"))
+ goto out;
+
+ i = 0;
+ err = bpf_map_lookup_elem(data_fd, &i, &val);
+ ASSERT_OK(err, "fentry count");
+ ASSERT_EQ(val, 1, "fentry count");
+
+out:
+ bpf_link__destroy(fentry_link);
+ bpf_object__close(fentry_obj);
+ bpf_object__close(tgt_obj);
+}
+
void test_tailcalls(void)
{
if (test__start_subtest("tailcall_1"))
@@ -910,4 +1201,12 @@ void test_tailcalls(void)
test_tailcall_bpf2bpf_4(true);
if (test__start_subtest("tailcall_bpf2bpf_6"))
test_tailcall_bpf2bpf_6();
+ if (test__start_subtest("tailcall_bpf2bpf_fentry"))
+ test_tailcall_bpf2bpf_fentry();
+ if (test__start_subtest("tailcall_bpf2bpf_fexit"))
+ test_tailcall_bpf2bpf_fexit();
+ if (test__start_subtest("tailcall_bpf2bpf_fentry_fexit"))
+ test_tailcall_bpf2bpf_fentry_fexit();
+ if (test__start_subtest("tailcall_bpf2bpf_fentry_entry"))
+ test_tailcall_bpf2bpf_fentry_entry();
}
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
new file mode 100644
index 0000000000000..8436c6729167c
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Leon Hwang */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+int count = 0;
+
+SEC("fentry/subprog_tail")
+int BPF_PROG(fentry, struct sk_buff *skb)
+{
+ count++;
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
new file mode 100644
index 0000000000000..fe16412c6e6e9
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Leon Hwang */
+
+#include "vmlinux.h"
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+int count = 0;
+
+SEC("fexit/subprog_tail")
+int BPF_PROG(fexit, struct sk_buff *skb)
+{
+ count++;
+
+ return 0;
+}
+
+char _license[] SEC("license") = "GPL";
--
2.41.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop
2023-09-03 15:14 [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
` (3 preceding siblings ...)
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing Leon Hwang
@ 2023-09-04 13:10 ` Ilya Leoshkevich
2023-09-04 15:15 ` Leon Hwang
4 siblings, 1 reply; 16+ messages in thread
From: Ilya Leoshkevich @ 2023-09-04 13:10 UTC (permalink / raw)
To: Leon Hwang, ast, daniel, andrii, maciej.fijalkowski; +Cc: song, jakub, bpf
On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
> This patch series fixes a tailcall infinite loop on x64.
>
> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
> tailcall
> handling in JIT"), the tailcall on x64 works better than before.
>
> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
> subprograms
> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
>
> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
> program
> to other BPF programs"), BPF program is able to trace other BPF
> programs.
>
> How about combining them all together?
>
> 1. FENTRY/FEXIT on a BPF subprogram.
> 2. A tailcall runs in the BPF subprogram.
> 3. The tailcall calls the subprogram's caller.
>
> As a result, a tailcall infinite loop comes up. And the loop would
> halt
> the machine.
>
> As we know, in tail call context, the tail_call_cnt propagates by
> stack
> and rax register between BPF subprograms. So do in trampolines.
>
> How did I discover the bug?
>
> From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth 256
> for
> subprogs with tailcalls"), the total stack size limits to around
> 8KiB.
> Then, I write some bpf progs to validate the stack consuming, that
> are
> tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on bpf2bpf[1].
>
> At that time, accidently, I made a tailcall loop. And then the loop
> halted
> my VM. Without the loop, the bpf progs would consume over 8KiB stack
> size.
> But the _stack-overflow_ did not halt my VM.
>
> With bpf_printk(), I confirmed that the tailcall count limit did not
> work
> expectedly. Next, read the code and fix it.
>
> Finally, unfortunately, I only fix it on x64 but other arches. As a
> result, CI tests failed because this bug hasn't been fixed on s390x.
>
> Some helps on s390x are requested.
I will take a look, thanks for letting me know.
I noticed there was something peculiar in this area when implementing
the trampoline:
* Note 1: The callee can increment the tail call counter, but
* we do not load it back, since the x86 JIT does not do this
* either.
but I thought that this was intentional.
[...]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop
2023-09-04 13:10 ` [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Ilya Leoshkevich
@ 2023-09-04 15:15 ` Leon Hwang
2023-09-06 0:57 ` Ilya Leoshkevich
0 siblings, 1 reply; 16+ messages in thread
From: Leon Hwang @ 2023-09-04 15:15 UTC (permalink / raw)
To: Ilya Leoshkevich, ast, daniel, andrii, maciej.fijalkowski
Cc: song, jakub, bpf
On 2023/9/4 21:10, Ilya Leoshkevich wrote:
> On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
>> This patch series fixes a tailcall infinite loop on x64.
>>
>> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
>> tailcall
>> handling in JIT"), the tailcall on x64 works better than before.
>>
>> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
>> subprograms
>> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
>>
>> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
>> program
>> to other BPF programs"), BPF program is able to trace other BPF
>> programs.
>>
>> How about combining them all together?
>>
>> 1. FENTRY/FEXIT on a BPF subprogram.
>> 2. A tailcall runs in the BPF subprogram.
>> 3. The tailcall calls the subprogram's caller.
>>
>> As a result, a tailcall infinite loop comes up. And the loop would
>> halt
>> the machine.
>>
>> As we know, in tail call context, the tail_call_cnt propagates by
>> stack
>> and rax register between BPF subprograms. So do in trampolines.
>>
>> How did I discover the bug?
>>
>> From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth 256
>> for
>> subprogs with tailcalls"), the total stack size limits to around
>> 8KiB.
>> Then, I write some bpf progs to validate the stack consuming, that
>> are
>> tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on bpf2bpf[1].
>>
>> At that time, accidently, I made a tailcall loop. And then the loop
>> halted
>> my VM. Without the loop, the bpf progs would consume over 8KiB stack
>> size.
>> But the _stack-overflow_ did not halt my VM.
>>
>> With bpf_printk(), I confirmed that the tailcall count limit did not
>> work
>> expectedly. Next, read the code and fix it.
>>
>> Finally, unfortunately, I only fix it on x64 but other arches. As a
>> result, CI tests failed because this bug hasn't been fixed on s390x.
>>
>> Some helps on s390x are requested.
>
> I will take a look, thanks for letting me know.
Great.
> I noticed there was something peculiar in this area when implementing
> the trampoline:
>
> * Note 1: The callee can increment the tail call counter, but
> * we do not load it back, since the x86 JIT does not do this
> * either.>
> but I thought that this was intentional.
I do think so.
But wait, should we load it back?
Let me show a demo:
struct {
__uint(type, BPF_MAP_TYPE_PROG_ARRAY);
__uint(max_entries, 4);
__uint(key_size, sizeof(__u32));
__uint(value_size, sizeof(__u32));
} jmp_table SEC(".maps");
static __noinline
int subprog_tail_01(struct __sk_buff *skb)
{
if (load_byte(skb, 0))
bpf_tail_call_static(skb, &jmp_table, 1);
else
bpf_tail_call_static(skb, &jmp_table, 0);
return 1;
}
static __noinline
int subprog_tail_23(struct __sk_buff *skb)
{
if (load_byte(skb, 0))
bpf_tail_call_static(skb, &jmp_table, 3);
else
bpf_tail_call_static(skb, &jmp_table, 2);
return 1;
}
int count0 = 0;
SEC("tc")
int classifier_01(struct __sk_buff *skb)
{
count0++;
return subprog_tail_01(skb);
}
int count1 = 0;
SEC("tc")
int classifier_23(struct __sk_buff *skb)
{
count1++;
return subprog_tail_23(skb);
}
static __noinline
int subprog_tailcall(struct __sk_buff *skb, int index)
{
volatile int retval = 0;
bpf_tail_call(skb, &jmp_table, index);
return retval;
}
SEC("tc")
int entry(struct __sk_buff *skb)
{
subprog_tailcall(skb, 0);
subprog_tailcall(skb, 2);
return 0;
}
Finally, count0 is 33. And count1 is 33, too. It breaks the
MAX_TAIL_CALL_CNT limit by the way tailcall in subprog.
From 04fd61ab36ec065e ("bpf: allow bpf programs to tail-call other bpf
programs"):
The chain of tail calls can form unpredictable dynamic loops therefore
tail_call_cnt is used to limit the number of calls and currently is set
to 32.
It seems like that MAX_TAIL_CALL_CNT limits the max tailcall hierarchy
layers.
So, should we load it back?
Thanks,
Leon
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls Leon Hwang
@ 2023-09-05 19:22 ` Maciej Fijalkowski
2023-09-06 2:29 ` Leon Hwang
0 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2023-09-05 19:22 UTC (permalink / raw)
To: Leon Hwang; +Cc: ast, daniel, andrii, song, iii, jakub, bpf
On Sun, Sep 03, 2023 at 11:14:47PM +0800, Leon Hwang wrote:
> Get and check data_fd. It should not to check map_fd again.
>
> Fixes: 79d49ba048ec ("bpf, testing: Add various tail call test cases")
> Fixes: 3b0379111197 ("selftests/bpf: Add tailcall_bpf2bpf tests")
> Fixes: 5e0b0a4c52d3 ("selftests/bpf: Test tail call counting with bpf2bpf and data on stack")
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
This could be pulled out of this RFC set and sent separately to bpf tree,
given that Ilya is taking a look at addressing s390 jit.
> ---
> tools/testing/selftests/bpf/prog_tests/tailcalls.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index 58fe2c586ed76..b20d7f77a5bce 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -274,7 +274,7 @@ static void test_tailcall_count(const char *which)
> return;
>
> data_fd = bpf_map__fd(data_map);
> - if (CHECK_FAIL(map_fd < 0))
> + if (CHECK_FAIL(data_fd < 0))
> return;
>
> i = 0;
> @@ -355,7 +355,7 @@ static void test_tailcall_4(void)
> return;
>
> data_fd = bpf_map__fd(data_map);
> - if (CHECK_FAIL(map_fd < 0))
> + if (CHECK_FAIL(data_fd < 0))
> return;
>
> for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
> @@ -445,7 +445,7 @@ static void test_tailcall_5(void)
> return;
>
> data_fd = bpf_map__fd(data_map);
> - if (CHECK_FAIL(map_fd < 0))
> + if (CHECK_FAIL(data_fd < 0))
> return;
shouldn't this be 'goto out' ? applies to the rest of the code i believe.
>
> for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
> @@ -634,7 +634,7 @@ static void test_tailcall_bpf2bpf_2(void)
> return;
>
> data_fd = bpf_map__fd(data_map);
> - if (CHECK_FAIL(map_fd < 0))
> + if (CHECK_FAIL(data_fd < 0))
> return;
>
> i = 0;
> @@ -808,7 +808,7 @@ static void test_tailcall_bpf2bpf_4(bool noise)
> return;
>
> data_fd = bpf_map__fd(data_map);
> - if (CHECK_FAIL(map_fd < 0))
> + if (CHECK_FAIL(data_fd < 0))
> return;
>
> i = 0;
> @@ -872,7 +872,7 @@ static void test_tailcall_bpf2bpf_6(void)
> ASSERT_EQ(topts.retval, 0, "tailcall retval");
>
> data_fd = bpf_map__fd(obj->maps.bss);
> - if (!ASSERT_GE(map_fd, 0, "bss map fd"))
> + if (!ASSERT_GE(data_fd, 0, "bss map fd"))
> goto out;
>
> i = 0;
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation Leon Hwang
@ 2023-09-05 19:26 ` Maciej Fijalkowski
2023-09-06 2:23 ` Leon Hwang
0 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2023-09-05 19:26 UTC (permalink / raw)
To: Leon Hwang; +Cc: ast, daniel, andrii, song, iii, jakub, bpf
On Sun, Sep 03, 2023 at 11:14:45PM +0800, Leon Hwang wrote:
> Without understanding emit_prologue(), it is really hard to figure out
> where does tail_call_cnt come from, even though searching tail_call_cnt
> in the whole kernel repo.
>
> By adding these comments, it is a little bit easy to understand
s/easy/easier
> tail_call_cnt initialisation.
>
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
I was rather thinking about dropping these comments altogether. We should
be trying to provide fixes as small as possible IMHO.
Having this as a separate commit also breaks logistics of this set as
the fix + selftest should be routed towards bpf tree, whereas this
particular commit is rather a bpf-next candidate.
PTAL at https://www.kernel.org/doc/html/latest/bpf/bpf_devel_QA.html
> ---
> arch/x86/net/bpf_jit_comp.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index a5930042139d3..bcca1c9b9a027 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
> prog += X86_PATCH_SIZE;
> if (!ebpf_from_cbpf) {
> if (tail_call_reachable && !is_subprog)
> + /* When it's the entry of the whole tailcall context,
> + * zeroing rax means initialising tail_call_cnt.
> + */
> EMIT2(0x31, 0xC0); /* xor eax, eax */
> else
> + /* Keep the same instruction layout. */
> EMIT2(0x66, 0x90); /* nop2 */
> }
> EMIT1(0x55); /* push rbp */
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop
2023-09-04 15:15 ` Leon Hwang
@ 2023-09-06 0:57 ` Ilya Leoshkevich
2023-09-06 2:39 ` Leon Hwang
0 siblings, 1 reply; 16+ messages in thread
From: Ilya Leoshkevich @ 2023-09-06 0:57 UTC (permalink / raw)
To: Leon Hwang, ast, daniel, andrii, maciej.fijalkowski; +Cc: song, jakub, bpf
On Mon, 2023-09-04 at 23:15 +0800, Leon Hwang wrote:
>
>
> On 2023/9/4 21:10, Ilya Leoshkevich wrote:
> > On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
> > > This patch series fixes a tailcall infinite loop on x64.
> > >
> > > From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
> > > tailcall
> > > handling in JIT"), the tailcall on x64 works better than before.
> > >
> > > From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
> > > subprograms
> > > for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
> > >
> > > From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
> > > program
> > > to other BPF programs"), BPF program is able to trace other BPF
> > > programs.
> > >
> > > How about combining them all together?
> > >
> > > 1. FENTRY/FEXIT on a BPF subprogram.
> > > 2. A tailcall runs in the BPF subprogram.
> > > 3. The tailcall calls the subprogram's caller.
> > >
> > > As a result, a tailcall infinite loop comes up. And the loop
> > > would
> > > halt
> > > the machine.
> > >
> > > As we know, in tail call context, the tail_call_cnt propagates by
> > > stack
> > > and rax register between BPF subprograms. So do in trampolines.
> > >
> > > How did I discover the bug?
> > >
> > > From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth
> > > 256
> > > for
> > > subprogs with tailcalls"), the total stack size limits to around
> > > 8KiB.
> > > Then, I write some bpf progs to validate the stack consuming,
> > > that
> > > are
> > > tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on
> > > bpf2bpf[1].
> > >
> > > At that time, accidently, I made a tailcall loop. And then the
> > > loop
> > > halted
> > > my VM. Without the loop, the bpf progs would consume over 8KiB
> > > stack
> > > size.
> > > But the _stack-overflow_ did not halt my VM.
> > >
> > > With bpf_printk(), I confirmed that the tailcall count limit did
> > > not
> > > work
> > > expectedly. Next, read the code and fix it.
> > >
> > > Finally, unfortunately, I only fix it on x64 but other arches. As
> > > a
> > > result, CI tests failed because this bug hasn't been fixed on
> > > s390x.
> > >
> > > Some helps on s390x are requested.
> >
> > I will take a look, thanks for letting me know.
>
> Great.
>
> > I noticed there was something peculiar in this area when
> > implementing
> > the trampoline:
> >
> > * Note 1: The callee can increment the tail call counter,
> > but
> > * we do not load it back, since the x86 JIT does not do
> > this
> > * either.>
> > but I thought that this was intentional.
>
> I do think so.
>
> But wait, should we load it back?
>
> Let me show a demo:
>
> struct {
> __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
> __uint(max_entries, 4);
> __uint(key_size, sizeof(__u32));
> __uint(value_size, sizeof(__u32));
> } jmp_table SEC(".maps");
>
> static __noinline
> int subprog_tail_01(struct __sk_buff *skb)
> {
> if (load_byte(skb, 0))
> bpf_tail_call_static(skb, &jmp_table, 1);
> else
> bpf_tail_call_static(skb, &jmp_table, 0);
> return 1;
> }
>
> static __noinline
> int subprog_tail_23(struct __sk_buff *skb)
> {
> if (load_byte(skb, 0))
> bpf_tail_call_static(skb, &jmp_table, 3);
> else
> bpf_tail_call_static(skb, &jmp_table, 2);
> return 1;
> }
>
> int count0 = 0;
>
> SEC("tc")
> int classifier_01(struct __sk_buff *skb)
> {
> count0++;
> return subprog_tail_01(skb);
> }
>
> int count1 = 0;
>
> SEC("tc")
> int classifier_23(struct __sk_buff *skb)
> {
> count1++;
> return subprog_tail_23(skb);
> }
>
> static __noinline
> int subprog_tailcall(struct __sk_buff *skb, int index)
> {
> volatile int retval = 0;
> bpf_tail_call(skb, &jmp_table, index);
> return retval;
> }
>
> SEC("tc")
> int entry(struct __sk_buff *skb)
> {
> subprog_tailcall(skb, 0);
> subprog_tailcall(skb, 2);
>
> return 0;
> }
>
> Finally, count0 is 33. And count1 is 33, too. It breaks the
> MAX_TAIL_CALL_CNT limit by the way tailcall in subprog.
Thanks for coming up with an example, I could not create something like
this back then and just left a note for the future.
> From 04fd61ab36ec065e ("bpf: allow bpf programs to tail-call other
> bpf
> programs"):
> The chain of tail calls can form unpredictable dynamic loops
> therefore
> tail_call_cnt is used to limit the number of calls and currently is
> set
> to 32.
>
> It seems like that MAX_TAIL_CALL_CNT limits the max tailcall
> hierarchy
> layers.
>
> So, should we load it back?
I wonder if the current behavior can lead to high system load in some
cases. The current limit on the number of instructions is 1M; with tail
calls it goes up to MAX_TAIL_CALL_CNT * 1M. If one uses the technique
above to the fullest extent, i.e., on each tail call hierarchy level,
can't one execute (2 ** MAX_TAIL_CALL_CNT) * 1M instructions as a
result?
> Thanks,
> Leon
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation
2023-09-05 19:26 ` Maciej Fijalkowski
@ 2023-09-06 2:23 ` Leon Hwang
0 siblings, 0 replies; 16+ messages in thread
From: Leon Hwang @ 2023-09-06 2:23 UTC (permalink / raw)
To: Maciej Fijalkowski; +Cc: ast, daniel, andrii, song, iii, jakub, bpf
On 6/9/23 03:26, Maciej Fijalkowski wrote:
> On Sun, Sep 03, 2023 at 11:14:45PM +0800, Leon Hwang wrote:
>> Without understanding emit_prologue(), it is really hard to figure out
>> where does tail_call_cnt come from, even though searching tail_call_cnt
>> in the whole kernel repo.
>>
>> By adding these comments, it is a little bit easy to understand
>
> s/easy/easier
Got it.
>
>> tail_call_cnt initialisation.
>>
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
>
> I was rather thinking about dropping these comments altogether. We should
> be trying to provide fixes as small as possible IMHO.
>
> Having this as a separate commit also breaks logistics of this set as
> the fix + selftest should be routed towards bpf tree, whereas this
> particular commit is rather a bpf-next candidate.
>
> PTAL at https://www.kernel.org/doc/html/latest/bpf/bpf_devel_QA.html
Thanks for your review.
I'll keep this commit as one of this set.
Thanks,
Leon
>
>> ---
>> arch/x86/net/bpf_jit_comp.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
>> index a5930042139d3..bcca1c9b9a027 100644
>> --- a/arch/x86/net/bpf_jit_comp.c
>> +++ b/arch/x86/net/bpf_jit_comp.c
>> @@ -303,8 +303,12 @@ static void emit_prologue(u8 **pprog, u32 stack_depth, bool ebpf_from_cbpf,
>> prog += X86_PATCH_SIZE;
>> if (!ebpf_from_cbpf) {
>> if (tail_call_reachable && !is_subprog)
>> + /* When it's the entry of the whole tailcall context,
>> + * zeroing rax means initialising tail_call_cnt.
>> + */
>> EMIT2(0x31, 0xC0); /* xor eax, eax */
>> else
>> + /* Keep the same instruction layout. */
>> EMIT2(0x66, 0x90); /* nop2 */
>> }
>> EMIT1(0x55); /* push rbp */
>> --
>> 2.41.0
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls
2023-09-05 19:22 ` Maciej Fijalkowski
@ 2023-09-06 2:29 ` Leon Hwang
0 siblings, 0 replies; 16+ messages in thread
From: Leon Hwang @ 2023-09-06 2:29 UTC (permalink / raw)
To: Maciej Fijalkowski; +Cc: ast, daniel, andrii, song, iii, jakub, bpf
On 6/9/23 03:22, Maciej Fijalkowski wrote:
> On Sun, Sep 03, 2023 at 11:14:47PM +0800, Leon Hwang wrote:
>> Get and check data_fd. It should not to check map_fd again.
>>
>> Fixes: 79d49ba048ec ("bpf, testing: Add various tail call test cases")
>> Fixes: 3b0379111197 ("selftests/bpf: Add tailcall_bpf2bpf tests")
>> Fixes: 5e0b0a4c52d3 ("selftests/bpf: Test tail call counting with bpf2bpf and data on stack")
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
>
> This could be pulled out of this RFC set and sent separately to bpf tree,
> given that Ilya is taking a look at addressing s390 jit.
Yeah, I'll do it.
>
>> ---
>> tools/testing/selftests/bpf/prog_tests/tailcalls.c | 12 ++++++------
>> 1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> index 58fe2c586ed76..b20d7f77a5bce 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> @@ -274,7 +274,7 @@ static void test_tailcall_count(const char *which)
>> return;
>>
>> data_fd = bpf_map__fd(data_map);
>> - if (CHECK_FAIL(map_fd < 0))
>> + if (CHECK_FAIL(data_fd < 0))
>> return;
>>
>> i = 0;
>> @@ -355,7 +355,7 @@ static void test_tailcall_4(void)
>> return;
>>
>> data_fd = bpf_map__fd(data_map);
>> - if (CHECK_FAIL(map_fd < 0))
>> + if (CHECK_FAIL(data_fd < 0))
>> return;
>>
>> for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
>> @@ -445,7 +445,7 @@ static void test_tailcall_5(void)
>> return;
>>
>> data_fd = bpf_map__fd(data_map);
>> - if (CHECK_FAIL(map_fd < 0))
>> + if (CHECK_FAIL(data_fd < 0))
>> return;
>
> shouldn't this be 'goto out' ? applies to the rest of the code i believe.
Good point. I'll correct some other 'return' to 'goto out' meanwhile.
Thanks,
Leon
>
>>
>> for (i = 0; i < bpf_map__max_entries(prog_array); i++) {
>> @@ -634,7 +634,7 @@ static void test_tailcall_bpf2bpf_2(void)
>> return;
>>
>> data_fd = bpf_map__fd(data_map);
>> - if (CHECK_FAIL(map_fd < 0))
>> + if (CHECK_FAIL(data_fd < 0))
>> return;
>>
>> i = 0;
>> @@ -808,7 +808,7 @@ static void test_tailcall_bpf2bpf_4(bool noise)
>> return;
>>
>> data_fd = bpf_map__fd(data_map);
>> - if (CHECK_FAIL(map_fd < 0))
>> + if (CHECK_FAIL(data_fd < 0))
>> return;
>>
>> i = 0;
>> @@ -872,7 +872,7 @@ static void test_tailcall_bpf2bpf_6(void)
>> ASSERT_EQ(topts.retval, 0, "tailcall retval");
>>
>> data_fd = bpf_map__fd(obj->maps.bss);
>> - if (!ASSERT_GE(map_fd, 0, "bss map fd"))
>> + if (!ASSERT_GE(data_fd, 0, "bss map fd"))
>> goto out;
>>
>> i = 0;
>> --
>> 2.41.0
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop
2023-09-06 0:57 ` Ilya Leoshkevich
@ 2023-09-06 2:39 ` Leon Hwang
0 siblings, 0 replies; 16+ messages in thread
From: Leon Hwang @ 2023-09-06 2:39 UTC (permalink / raw)
To: Ilya Leoshkevich, ast, daniel, andrii, maciej.fijalkowski
Cc: song, jakub, bpf
On 6/9/23 08:57, Ilya Leoshkevich wrote:
> On Mon, 2023-09-04 at 23:15 +0800, Leon Hwang wrote:
>>
>>
>> On 2023/9/4 21:10, Ilya Leoshkevich wrote:
>>> On Sun, 2023-09-03 at 23:14 +0800, Leon Hwang wrote:
>>>> This patch series fixes a tailcall infinite loop on x64.
>>>>
>>>> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and
>>>> tailcall
>>>> handling in JIT"), the tailcall on x64 works better than before.
>>>>
>>>> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF
>>>> subprograms
>>>> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
>>>>
>>>> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF
>>>> program
>>>> to other BPF programs"), BPF program is able to trace other BPF
>>>> programs.
>>>>
>>>> How about combining them all together?
>>>>
>>>> 1. FENTRY/FEXIT on a BPF subprogram.
>>>> 2. A tailcall runs in the BPF subprogram.
>>>> 3. The tailcall calls the subprogram's caller.
>>>>
>>>> As a result, a tailcall infinite loop comes up. And the loop
>>>> would
>>>> halt
>>>> the machine.
>>>>
>>>> As we know, in tail call context, the tail_call_cnt propagates by
>>>> stack
>>>> and rax register between BPF subprograms. So do in trampolines.
>>>>
>>>> How did I discover the bug?
>>>>
>>>> From commit 7f6e4312e15a5c37 ("bpf: Limit caller's stack depth
>>>> 256
>>>> for
>>>> subprogs with tailcalls"), the total stack size limits to around
>>>> 8KiB.
>>>> Then, I write some bpf progs to validate the stack consuming,
>>>> that
>>>> are
>>>> tailcalls running in bpf2bpf and FENTRY/FEXIT tracing on
>>>> bpf2bpf[1].
>>>>
>>>> At that time, accidently, I made a tailcall loop. And then the
>>>> loop
>>>> halted
>>>> my VM. Without the loop, the bpf progs would consume over 8KiB
>>>> stack
>>>> size.
>>>> But the _stack-overflow_ did not halt my VM.
>>>>
>>>> With bpf_printk(), I confirmed that the tailcall count limit did
>>>> not
>>>> work
>>>> expectedly. Next, read the code and fix it.
>>>>
>>>> Finally, unfortunately, I only fix it on x64 but other arches. As
>>>> a
>>>> result, CI tests failed because this bug hasn't been fixed on
>>>> s390x.
>>>>
>>>> Some helps on s390x are requested.
>>>
>>> I will take a look, thanks for letting me know.
>>
>> Great.
>>
>>> I noticed there was something peculiar in this area when
>>> implementing
>>> the trampoline:
>>>
>>> * Note 1: The callee can increment the tail call counter,
>>> but
>>> * we do not load it back, since the x86 JIT does not do
>>> this
>>> * either.>
>>> but I thought that this was intentional.
>>
>> I do think so.
>>
>> But wait, should we load it back?
>>
>> Let me show a demo:
>>
>> struct {
>> __uint(type, BPF_MAP_TYPE_PROG_ARRAY);
>> __uint(max_entries, 4);
>> __uint(key_size, sizeof(__u32));
>> __uint(value_size, sizeof(__u32));
>> } jmp_table SEC(".maps");
>>
>> static __noinline
>> int subprog_tail_01(struct __sk_buff *skb)
>> {
>> if (load_byte(skb, 0))
>> bpf_tail_call_static(skb, &jmp_table, 1);
>> else
>> bpf_tail_call_static(skb, &jmp_table, 0);
>> return 1;
>> }
>>
>> static __noinline
>> int subprog_tail_23(struct __sk_buff *skb)
>> {
>> if (load_byte(skb, 0))
>> bpf_tail_call_static(skb, &jmp_table, 3);
>> else
>> bpf_tail_call_static(skb, &jmp_table, 2);
>> return 1;
>> }
>>
>> int count0 = 0;
>>
>> SEC("tc")
>> int classifier_01(struct __sk_buff *skb)
>> {
>> count0++;
>> return subprog_tail_01(skb);
>> }
>>
>> int count1 = 0;
>>
>> SEC("tc")
>> int classifier_23(struct __sk_buff *skb)
>> {
>> count1++;
>> return subprog_tail_23(skb);
>> }
>>
>> static __noinline
>> int subprog_tailcall(struct __sk_buff *skb, int index)
>> {
>> volatile int retval = 0;
>> bpf_tail_call(skb, &jmp_table, index);
>> return retval;
>> }
>>
>> SEC("tc")
>> int entry(struct __sk_buff *skb)
>> {
>> subprog_tailcall(skb, 0);
>> subprog_tailcall(skb, 2);
>>
>> return 0;
>> }
>>
>> Finally, count0 is 33. And count1 is 33, too. It breaks the
>> MAX_TAIL_CALL_CNT limit by the way tailcall in subprog.
>
> Thanks for coming up with an example, I could not create something like
> this back then and just left a note for the future.
>
>> From 04fd61ab36ec065e ("bpf: allow bpf programs to tail-call other
>> bpf
>> programs"):
>> The chain of tail calls can form unpredictable dynamic loops
>> therefore
>> tail_call_cnt is used to limit the number of calls and currently is
>> set
>> to 32.
>>
>> It seems like that MAX_TAIL_CALL_CNT limits the max tailcall
>> hierarchy
>> layers.
>>
>> So, should we load it back?
>
> I wonder if the current behavior can lead to high system load in some
> cases. The current limit on the number of instructions is 1M; with tail
> calls it goes up to MAX_TAIL_CALL_CNT * 1M. If one uses the technique
> above to the fullest extent, i.e., on each tail call hierarchy level,
> can't one execute (2 ** MAX_TAIL_CALL_CNT) * 1M instructions as a
> result?
Executing (2 ** MAX_TAIL_CALL_CNT) * 1M instructions is really harmful.
So, we should load it back.
I'll try to fix it with another patchset.
Thanks,
Leon
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
@ 2023-09-06 20:50 ` Maciej Fijalkowski
0 siblings, 0 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2023-09-06 20:50 UTC (permalink / raw)
To: Leon Hwang; +Cc: ast, daniel, andrii, song, iii, jakub, bpf
On Sun, Sep 03, 2023 at 11:14:46PM +0800, Leon Hwang wrote:
> From commit ebf7d1f508a73871 ("bpf, x64: rework pro/epilogue and tailcall
> handling in JIT"), the tailcall on x64 works better than before.
>
> From commit e411901c0b775a3a ("bpf: allow for tailcalls in BPF subprograms
> for x64 JIT"), tailcall is able to run in BPF subprograms on x64.
>
> From commit 5b92a28aae4dd0f8 ("bpf: Support attaching tracing BPF program
> to other BPF programs"), BPF program is able to trace other BPF programs.
>
> How about combining them all together?
>
> 1. FENTRY/FEXIT on a BPF subprogram.
> 2. A tailcall runs in the BPF subprogram.
> 3. The tailcall calls the subprogram's caller.
>
> As a result, a tailcall infinite loop comes up. And the loop would halt
> the machine.
>
> As we know, in tail call context, the tail_call_cnt propagates by stack
> and rax register between BPF subprograms. So do in trampolines.
>
> Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT")
> Fixes: e411901c0b77 ("bpf: allow for tailcalls in BPF subprograms for x64 JIT")
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
> arch/x86/net/bpf_jit_comp.c | 28 ++++++++++++++++++++++------
> include/linux/bpf.h | 5 +++++
> kernel/bpf/trampoline.c | 4 ++--
> kernel/bpf/verifier.c | 3 +++
> 4 files changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index bcca1c9b9a027..2846c21d75bfa 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1022,6 +1022,10 @@ static void emit_shiftx(u8 **pprog, u32 dst_reg, u8 src_reg, bool is64, u8 op)
>
> #define INSN_SZ_DIFF (((addrs[i] - addrs[i - 1]) - (prog - temp)))
>
> +/* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
> +#define RESTORE_TAIL_CALL_CNT(stack) \
> + EMIT3_off32(0x48, 0x8B, 0x85, -round_up(stack, 8) - 8)
> +
> static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image, u8 *rw_image,
> int oldproglen, struct jit_context *ctx, bool jmp_padding)
> {
> @@ -1627,9 +1631,7 @@ st: if (is_imm8(insn->off))
>
> func = (u8 *) __bpf_call_base + imm32;
> if (tail_call_reachable) {
> - /* mov rax, qword ptr [rbp - rounded_stack_depth - 8] */
> - EMIT3_off32(0x48, 0x8B, 0x85,
> - -round_up(bpf_prog->aux->stack_depth, 8) - 8);
> + RESTORE_TAIL_CALL_CNT(bpf_prog->aux->stack_depth);
> if (!imm32)
> return -EINVAL;
> offs = 7 + x86_call_depth_emit_accounting(&prog, func);
> @@ -2404,6 +2406,7 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> * [ ... ]
> * [ stack_arg2 ]
> * RBP - arg_stack_off [ stack_arg1 ]
> + * RSP [ tail_call_cnt ] BPF_TRAMP_F_TAIL_CALL_CTX
> */
>
> /* room for return value of orig_call or fentry prog */
> @@ -2468,6 +2471,8 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> else
> /* sub rsp, stack_size */
> EMIT4(0x48, 0x83, 0xEC, stack_size);
> + if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
> + EMIT1(0x50); /* push rax */
> /* mov QWORD PTR [rbp - rbx_off], rbx */
> emit_stx(&prog, BPF_DW, BPF_REG_FP, BPF_REG_6, -rbx_off);
>
> @@ -2520,9 +2525,15 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> restore_regs(m, &prog, regs_off);
> save_args(m, &prog, arg_stack_off, true);
>
> + if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
> + /* Before calling the original function, restore the
> + * tail_call_cnt from stack to rax.
> + */
> + RESTORE_TAIL_CALL_CNT(stack_size);
> +
> if (flags & BPF_TRAMP_F_ORIG_STACK) {
> - emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, 8);
> - EMIT2(0xff, 0xd0); /* call *rax */
> + emit_ldx(&prog, BPF_DW, BPF_REG_6, BPF_REG_FP, 8);
> + EMIT2(0xff, 0xd3); /* call *rbx */
> } else {
> /* call original function */
> if (emit_rsb_call(&prog, orig_call, prog)) {
> @@ -2573,7 +2584,12 @@ int arch_prepare_bpf_trampoline(struct bpf_tramp_image *im, void *image, void *i
> ret = -EINVAL;
> goto cleanup;
> }
> - }
> + } else if (flags & BPF_TRAMP_F_TAIL_CALL_CTX)
> + /* Before running the original function, restore the
> + * tail_call_cnt from stack to rax.
> + */
> + RESTORE_TAIL_CALL_CNT(stack_size);
> +
> /* restore return value of orig_call or fentry prog back into RAX */
> if (save_ret)
> emit_ldx(&prog, BPF_DW, BPF_REG_0, BPF_REG_FP, -8);
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index cfabbcf47bdb8..c8df257ea435d 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1028,6 +1028,11 @@ struct btf_func_model {
> */
> #define BPF_TRAMP_F_SHARE_IPMODIFY BIT(6)
>
> +/* Indicate that current trampoline is in a tail call context. Then, it has to
> + * cache and restore tail_call_cnt to avoid infinite tail call loop.
> + */
> +#define BPF_TRAMP_F_TAIL_CALL_CTX BIT(7)
> +
> /* Each call __bpf_prog_enter + call bpf_func + call __bpf_prog_exit is ~50
> * bytes on x86.
> */
> diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> index 78acf28d48732..16ab5da7161f2 100644
> --- a/kernel/bpf/trampoline.c
> +++ b/kernel/bpf/trampoline.c
> @@ -415,8 +415,8 @@ static int bpf_trampoline_update(struct bpf_trampoline *tr, bool lock_direct_mut
> goto out;
> }
>
> - /* clear all bits except SHARE_IPMODIFY */
> - tr->flags &= BPF_TRAMP_F_SHARE_IPMODIFY;
> + /* clear all bits except SHARE_IPMODIFY and TAIL_CALL_CTX */
> + tr->flags &= (BPF_TRAMP_F_SHARE_IPMODIFY | BPF_TRAMP_F_TAIL_CALL_CTX);
>
> if (tlinks[BPF_TRAMP_FEXIT].nr_links ||
> tlinks[BPF_TRAMP_MODIFY_RETURN].nr_links) {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4ccca1f6c9981..765da3007106a 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -19629,6 +19629,9 @@ static int check_attach_btf_id(struct bpf_verifier_env *env)
> if (!tr)
> return -ENOMEM;
>
> + if (tgt_prog && tgt_prog->aux->tail_call_reachable)
> + tr->flags = BPF_TRAMP_F_TAIL_CALL_CTX;
> +
> prog->aux->dst_trampoline = tr;
> return 0;
> }
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing Leon Hwang
@ 2023-09-06 21:18 ` Maciej Fijalkowski
2023-09-07 3:53 ` Leon Hwang
0 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2023-09-06 21:18 UTC (permalink / raw)
To: Leon Hwang; +Cc: ast, daniel, andrii, song, iii, jakub, bpf
On Sun, Sep 03, 2023 at 11:14:48PM +0800, Leon Hwang wrote:
> Add 4 test cases to confirm the tailcall infinite loop bug has been fixed.
>
> Like tailcall_bpf2bpf cases, do fentry/fexit on the bpf2bpf, and then
> check the final count result.
>
> tools/testing/selftests/bpf/test_progs -t tailcalls
> 226/13 tailcalls/tailcall_bpf2bpf_fentry:OK
> 226/14 tailcalls/tailcall_bpf2bpf_fexit:OK
> 226/15 tailcalls/tailcall_bpf2bpf_fentry_fexit:OK
> 226/16 tailcalls/tailcall_bpf2bpf_fentry_entry:OK
> 226 tailcalls:OK
> Summary: 1/16 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
> ---
> .../selftests/bpf/prog_tests/tailcalls.c | 299 ++++++++++++++++++
> .../bpf/progs/tailcall_bpf2bpf_fentry.c | 18 ++
> .../bpf/progs/tailcall_bpf2bpf_fexit.c | 18 ++
> 3 files changed, 335 insertions(+)
> create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
> create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> index b20d7f77a5bce..331b4e455ad06 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
> @@ -884,6 +884,297 @@ static void test_tailcall_bpf2bpf_6(void)
> tailcall_bpf2bpf6__destroy(obj);
> }
>
> +static void tailcall_bpf2bpf_fentry_fexit(bool test_fentry, bool test_fexit)
> +{
> + struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL, *fexit_obj = NULL;
> + struct bpf_link *fentry_link = NULL, *fexit_link = NULL;
> + int err, map_fd, prog_fd, main_fd, data_fd, i, val;
> + struct bpf_map *prog_array, *data_map;
> + struct bpf_program *prog;
> + char buff[128] = {};
> +
> + LIBBPF_OPTS(bpf_test_run_opts, topts,
> + .data_in = buff,
> + .data_size_in = sizeof(buff),
> + .repeat = 1,
> + );
> +
> + err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
> + BPF_PROG_TYPE_SCHED_CLS,
> + &tgt_obj, &prog_fd);
> + if (!ASSERT_OK(err, "load tgt_obj"))
> + return;
> +
> + prog = bpf_object__find_program_by_name(tgt_obj, "entry");
> + if (!ASSERT_OK_PTR(prog, "find entry prog"))
> + goto out;
> +
> + main_fd = bpf_program__fd(prog);
> + if (!ASSERT_FALSE(main_fd < 0, "find entry prog fd"))
> + goto out;
> +
> + prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
> + if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
> + goto out;
> +
> + map_fd = bpf_map__fd(prog_array);
> + if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
> + goto out;
> +
> + prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
> + if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
> + goto out;
> +
> + prog_fd = bpf_program__fd(prog);
> + if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
> + goto out;
> +
> + i = 0;
> + err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
> + if (!ASSERT_OK(err, "update jmp_table"))
> + goto out;
> +
> + if (test_fentry) {
> + fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
> + NULL);
> + if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
> + goto out;
> +
> + prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
> + if (!ASSERT_OK_PTR(prog, "find fentry prog"))
> + goto out;
> +
> + err = bpf_program__set_attach_target(prog, prog_fd,
> + "subprog_tail");
> + if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
> + goto out;
> +
> + err = bpf_object__load(fentry_obj);
> + if (!ASSERT_OK(err, "load fentry_obj"))
> + goto out;
> +
> + fentry_link = bpf_program__attach_trace(prog);
> + if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
> + goto out;
> + }
> +
> + if (test_fexit) {
> + fexit_obj = bpf_object__open_file("tailcall_bpf2bpf_fexit.bpf.o",
> + NULL);
> + if (!ASSERT_OK_PTR(fexit_obj, "open fexit_obj file"))
> + goto out;
> +
> + prog = bpf_object__find_program_by_name(fexit_obj, "fexit");
> + if (!ASSERT_OK_PTR(prog, "find fexit prog"))
> + goto out;
> +
> + err = bpf_program__set_attach_target(prog, prog_fd,
> + "subprog_tail");
> + if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
> + goto out;
> +
> + err = bpf_object__load(fexit_obj);
> + if (!ASSERT_OK(err, "load fexit_obj"))
> + goto out;
> +
> + fexit_link = bpf_program__attach_trace(prog);
> + if (!ASSERT_OK_PTR(fexit_link, "attach_trace"))
> + goto out;
> + }
> +
> + err = bpf_prog_test_run_opts(main_fd, &topts);
> + ASSERT_OK(err, "tailcall");
> + ASSERT_EQ(topts.retval, 1, "tailcall retval");
> +
> + data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> + "find tailcall.bss map"))
> + goto out;
> +
> + data_fd = bpf_map__fd(data_map);
> + if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
> + goto out;
> +
> + i = 0;
> + err = bpf_map_lookup_elem(data_fd, &i, &val);
> + ASSERT_OK(err, "tailcall count");
> + ASSERT_EQ(val, 33, "tailcall count");
> +
> + if (test_fentry) {
> + data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> + "find tailcall_bpf2bpf_fentry.bss map"))
> + goto out;
> +
> + data_fd = bpf_map__fd(data_map);
> + if (!ASSERT_FALSE(data_fd < 0,
> + "find tailcall_bpf2bpf_fentry.bss map fd"))
> + goto out;
> +
> + i = 0;
> + err = bpf_map_lookup_elem(data_fd, &i, &val);
> + ASSERT_OK(err, "fentry count");
> + ASSERT_EQ(val, 33, "fentry count");
> + }
> +
> + if (test_fexit) {
> + data_map = bpf_object__find_map_by_name(fexit_obj, ".bss");
> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> + "find tailcall_bpf2bpf_fexit.bss map"))
> + goto out;
> +
> + data_fd = bpf_map__fd(data_map);
> + if (!ASSERT_FALSE(data_fd < 0,
> + "find tailcall_bpf2bpf_fexit.bss map fd"))
> + goto out;
> +
> + i = 0;
> + err = bpf_map_lookup_elem(data_fd, &i, &val);
> + ASSERT_OK(err, "fexit count");
> + ASSERT_EQ(val, 33, "fexit count");
> + }
> +
> +out:
> + bpf_link__destroy(fentry_link);
> + bpf_link__destroy(fexit_link);
> + bpf_object__close(fentry_obj);
> + bpf_object__close(fexit_obj);
> + bpf_object__close(tgt_obj);
> +}
> +
> +/* test_tailcall_bpf2bpf_fentry checks that the count value of the tail call
> + * limit enforcement matches with expectations when tailcall is preceded with
> + * bpf2bpf call, and the bpf2bpf call is traced by fentry.
> + */
> +static void test_tailcall_bpf2bpf_fentry(void)
> +{
> + tailcall_bpf2bpf_fentry_fexit(true, false);
> +}
> +
> +/* test_tailcall_bpf2bpf_fexit checks that the count value of the tail call
> + * limit enforcement matches with expectations when tailcall is preceded with
> + * bpf2bpf call, and the bpf2bpf call is traced by fexit.
> + */
> +static void test_tailcall_bpf2bpf_fexit(void)
> +{
> + tailcall_bpf2bpf_fentry_fexit(false, true);
> +}
> +
> +/* test_tailcall_bpf2bpf_fentry_fexit checks that the count value of the tail
> + * call limit enforcement matches with expectations when tailcall is preceded
> + * with bpf2bpf call, and the bpf2bpf call is traced by both fentry and fexit.
> + */
> +static void test_tailcall_bpf2bpf_fentry_fexit(void)
> +{
> + tailcall_bpf2bpf_fentry_fexit(true, true);
Would it be possible to modify existing test_tailcall_count() to have
fentry/fexit testing within? __tailcall_bpf2bpf_fentry_fexit() basically
repeats the logic of test_tailcall_count(), right?
How about something like:
static void test_tailcall_bpf2bpf_fentry(void)
{
test_tailcall_count("tailcall_bpf2bpf2.bpf.o", true, false);
}
// rest of your test cases
and existing tailcall count tests:
static void test_tailcall_3(void)
{
test_tailcall_count("tailcall3.bpf.o", false, false);
}
static void test_tailcall_6(void)
{
test_tailcall_count("tailcall6.bpf.o", false, false);
}
> +}
> +
> +/* test_tailcall_bpf2bpf_fentry_entry checks that the count value of the tail
> + * call limit enforcement matches with expectations when tailcall is preceded
> + * with bpf2bpf call, and the bpf2bpf caller is traced by fentry.
> + */
> +static void test_tailcall_bpf2bpf_fentry_entry(void)
> +{
> + struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL;
> + int err, map_fd, prog_fd, data_fd, i, val;
> + struct bpf_map *prog_array, *data_map;
> + struct bpf_link *fentry_link = NULL;
> + struct bpf_program *prog;
> + char buff[128] = {};
> +
> + LIBBPF_OPTS(bpf_test_run_opts, topts,
> + .data_in = buff,
> + .data_size_in = sizeof(buff),
> + .repeat = 1,
> + );
> +
> + err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
> + BPF_PROG_TYPE_SCHED_CLS,
> + &tgt_obj, &prog_fd);
> + if (!ASSERT_OK(err, "load tgt_obj"))
> + return;
> +
> + prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
> + if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
> + goto out;
> +
> + map_fd = bpf_map__fd(prog_array);
> + if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
> + goto out;
> +
> + prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
> + if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
> + goto out;
> +
> + prog_fd = bpf_program__fd(prog);
> + if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
> + goto out;
> +
> + i = 0;
> + err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
> + if (!ASSERT_OK(err, "update jmp_table"))
> + goto out;
> +
> + fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
> + NULL);
> + if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
> + goto out;
> +
> + prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
> + if (!ASSERT_OK_PTR(prog, "find fentry prog"))
> + goto out;
> +
> + err = bpf_program__set_attach_target(prog, prog_fd, "classifier_0");
> + if (!ASSERT_OK(err, "set_attach_target classifier_0"))
> + goto out;
> +
> + err = bpf_object__load(fentry_obj);
> + if (!ASSERT_OK(err, "load fentry_obj"))
> + goto out;
> +
> + fentry_link = bpf_program__attach_trace(prog);
> + if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
> + goto out;
> +
> + err = bpf_prog_test_run_opts(prog_fd, &topts);
> + ASSERT_OK(err, "tailcall");
> + ASSERT_EQ(topts.retval, 1, "tailcall retval");
> +
> + data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> + "find tailcall.bss map"))
> + goto out;
> +
> + data_fd = bpf_map__fd(data_map);
> + if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
> + goto out;
> +
> + i = 0;
> + err = bpf_map_lookup_elem(data_fd, &i, &val);
> + ASSERT_OK(err, "tailcall count");
> + ASSERT_EQ(val, 34, "tailcall count");
> +
> + data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
> + "find tailcall_bpf2bpf_fentry.bss map"))
> + goto out;
> +
> + data_fd = bpf_map__fd(data_map);
> + if (!ASSERT_FALSE(data_fd < 0,
> + "find tailcall_bpf2bpf_fentry.bss map fd"))
> + goto out;
> +
> + i = 0;
> + err = bpf_map_lookup_elem(data_fd, &i, &val);
> + ASSERT_OK(err, "fentry count");
> + ASSERT_EQ(val, 1, "fentry count");
> +
> +out:
> + bpf_link__destroy(fentry_link);
> + bpf_object__close(fentry_obj);
> + bpf_object__close(tgt_obj);
> +}
> +
> void test_tailcalls(void)
> {
> if (test__start_subtest("tailcall_1"))
> @@ -910,4 +1201,12 @@ void test_tailcalls(void)
> test_tailcall_bpf2bpf_4(true);
> if (test__start_subtest("tailcall_bpf2bpf_6"))
> test_tailcall_bpf2bpf_6();
> + if (test__start_subtest("tailcall_bpf2bpf_fentry"))
> + test_tailcall_bpf2bpf_fentry();
> + if (test__start_subtest("tailcall_bpf2bpf_fexit"))
> + test_tailcall_bpf2bpf_fexit();
> + if (test__start_subtest("tailcall_bpf2bpf_fentry_fexit"))
> + test_tailcall_bpf2bpf_fentry_fexit();
> + if (test__start_subtest("tailcall_bpf2bpf_fentry_entry"))
> + test_tailcall_bpf2bpf_fentry_entry();
> }
> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
> new file mode 100644
> index 0000000000000..8436c6729167c
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright Leon Hwang */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +int count = 0;
> +
> +SEC("fentry/subprog_tail")
> +int BPF_PROG(fentry, struct sk_buff *skb)
> +{
> + count++;
> +
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
> new file mode 100644
> index 0000000000000..fe16412c6e6e9
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
> @@ -0,0 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright Leon Hwang */
> +
> +#include "vmlinux.h"
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +int count = 0;
> +
> +SEC("fexit/subprog_tail")
> +int BPF_PROG(fexit, struct sk_buff *skb)
> +{
> + count++;
> +
> + return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing
2023-09-06 21:18 ` Maciej Fijalkowski
@ 2023-09-07 3:53 ` Leon Hwang
0 siblings, 0 replies; 16+ messages in thread
From: Leon Hwang @ 2023-09-07 3:53 UTC (permalink / raw)
To: Maciej Fijalkowski; +Cc: ast, daniel, andrii, song, iii, jakub, bpf
On 7/9/23 05:18, Maciej Fijalkowski wrote:
> On Sun, Sep 03, 2023 at 11:14:48PM +0800, Leon Hwang wrote:
>> Add 4 test cases to confirm the tailcall infinite loop bug has been fixed.
>>
>> Like tailcall_bpf2bpf cases, do fentry/fexit on the bpf2bpf, and then
>> check the final count result.
>>
>> tools/testing/selftests/bpf/test_progs -t tailcalls
>> 226/13 tailcalls/tailcall_bpf2bpf_fentry:OK
>> 226/14 tailcalls/tailcall_bpf2bpf_fexit:OK
>> 226/15 tailcalls/tailcall_bpf2bpf_fentry_fexit:OK
>> 226/16 tailcalls/tailcall_bpf2bpf_fentry_entry:OK
>> 226 tailcalls:OK
>> Summary: 1/16 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Signed-off-by: Leon Hwang <hffilwlqm@gmail.com>
>> ---
>> .../selftests/bpf/prog_tests/tailcalls.c | 299 ++++++++++++++++++
>> .../bpf/progs/tailcall_bpf2bpf_fentry.c | 18 ++
>> .../bpf/progs/tailcall_bpf2bpf_fexit.c | 18 ++
>> 3 files changed, 335 insertions(+)
>> create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
>> create mode 100644 tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> index b20d7f77a5bce..331b4e455ad06 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c
>> @@ -884,6 +884,297 @@ static void test_tailcall_bpf2bpf_6(void)
>> tailcall_bpf2bpf6__destroy(obj);
>> }
>>
>> +static void tailcall_bpf2bpf_fentry_fexit(bool test_fentry, bool test_fexit)
>> +{
>> + struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL, *fexit_obj = NULL;
>> + struct bpf_link *fentry_link = NULL, *fexit_link = NULL;
>> + int err, map_fd, prog_fd, main_fd, data_fd, i, val;
>> + struct bpf_map *prog_array, *data_map;
>> + struct bpf_program *prog;
>> + char buff[128] = {};
>> +
>> + LIBBPF_OPTS(bpf_test_run_opts, topts,
>> + .data_in = buff,
>> + .data_size_in = sizeof(buff),
>> + .repeat = 1,
>> + );
>> +
>> + err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
>> + BPF_PROG_TYPE_SCHED_CLS,
>> + &tgt_obj, &prog_fd);
>> + if (!ASSERT_OK(err, "load tgt_obj"))
>> + return;
>> +
>> + prog = bpf_object__find_program_by_name(tgt_obj, "entry");
>> + if (!ASSERT_OK_PTR(prog, "find entry prog"))
>> + goto out;
>> +
>> + main_fd = bpf_program__fd(prog);
>> + if (!ASSERT_FALSE(main_fd < 0, "find entry prog fd"))
>> + goto out;
>> +
>> + prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
>> + if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
>> + goto out;
>> +
>> + map_fd = bpf_map__fd(prog_array);
>> + if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
>> + goto out;
>> +
>> + prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
>> + if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
>> + goto out;
>> +
>> + prog_fd = bpf_program__fd(prog);
>> + if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
>> + goto out;
>> +
>> + i = 0;
>> + err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
>> + if (!ASSERT_OK(err, "update jmp_table"))
>> + goto out;
>> +
>> + if (test_fentry) {
>> + fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
>> + NULL);
>> + if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
>> + goto out;
>> +
>> + prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
>> + if (!ASSERT_OK_PTR(prog, "find fentry prog"))
>> + goto out;
>> +
>> + err = bpf_program__set_attach_target(prog, prog_fd,
>> + "subprog_tail");
>> + if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
>> + goto out;
>> +
>> + err = bpf_object__load(fentry_obj);
>> + if (!ASSERT_OK(err, "load fentry_obj"))
>> + goto out;
>> +
>> + fentry_link = bpf_program__attach_trace(prog);
>> + if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
>> + goto out;
>> + }
>> +
>> + if (test_fexit) {
>> + fexit_obj = bpf_object__open_file("tailcall_bpf2bpf_fexit.bpf.o",
>> + NULL);
>> + if (!ASSERT_OK_PTR(fexit_obj, "open fexit_obj file"))
>> + goto out;
>> +
>> + prog = bpf_object__find_program_by_name(fexit_obj, "fexit");
>> + if (!ASSERT_OK_PTR(prog, "find fexit prog"))
>> + goto out;
>> +
>> + err = bpf_program__set_attach_target(prog, prog_fd,
>> + "subprog_tail");
>> + if (!ASSERT_OK(err, "set_attach_target subprog_tail"))
>> + goto out;
>> +
>> + err = bpf_object__load(fexit_obj);
>> + if (!ASSERT_OK(err, "load fexit_obj"))
>> + goto out;
>> +
>> + fexit_link = bpf_program__attach_trace(prog);
>> + if (!ASSERT_OK_PTR(fexit_link, "attach_trace"))
>> + goto out;
>> + }
>> +
>> + err = bpf_prog_test_run_opts(main_fd, &topts);
>> + ASSERT_OK(err, "tailcall");
>> + ASSERT_EQ(topts.retval, 1, "tailcall retval");
>> +
>> + data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
>> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> + "find tailcall.bss map"))
>> + goto out;
>> +
>> + data_fd = bpf_map__fd(data_map);
>> + if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
>> + goto out;
>> +
>> + i = 0;
>> + err = bpf_map_lookup_elem(data_fd, &i, &val);
>> + ASSERT_OK(err, "tailcall count");
>> + ASSERT_EQ(val, 33, "tailcall count");
>> +
>> + if (test_fentry) {
>> + data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
>> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> + "find tailcall_bpf2bpf_fentry.bss map"))
>> + goto out;
>> +
>> + data_fd = bpf_map__fd(data_map);
>> + if (!ASSERT_FALSE(data_fd < 0,
>> + "find tailcall_bpf2bpf_fentry.bss map fd"))
>> + goto out;
>> +
>> + i = 0;
>> + err = bpf_map_lookup_elem(data_fd, &i, &val);
>> + ASSERT_OK(err, "fentry count");
>> + ASSERT_EQ(val, 33, "fentry count");
>> + }
>> +
>> + if (test_fexit) {
>> + data_map = bpf_object__find_map_by_name(fexit_obj, ".bss");
>> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> + "find tailcall_bpf2bpf_fexit.bss map"))
>> + goto out;
>> +
>> + data_fd = bpf_map__fd(data_map);
>> + if (!ASSERT_FALSE(data_fd < 0,
>> + "find tailcall_bpf2bpf_fexit.bss map fd"))
>> + goto out;
>> +
>> + i = 0;
>> + err = bpf_map_lookup_elem(data_fd, &i, &val);
>> + ASSERT_OK(err, "fexit count");
>> + ASSERT_EQ(val, 33, "fexit count");
>> + }
>> +
>> +out:
>> + bpf_link__destroy(fentry_link);
>> + bpf_link__destroy(fexit_link);
>> + bpf_object__close(fentry_obj);
>> + bpf_object__close(fexit_obj);
>> + bpf_object__close(tgt_obj);
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fentry checks that the count value of the tail call
>> + * limit enforcement matches with expectations when tailcall is preceded with
>> + * bpf2bpf call, and the bpf2bpf call is traced by fentry.
>> + */
>> +static void test_tailcall_bpf2bpf_fentry(void)
>> +{
>> + tailcall_bpf2bpf_fentry_fexit(true, false);
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fexit checks that the count value of the tail call
>> + * limit enforcement matches with expectations when tailcall is preceded with
>> + * bpf2bpf call, and the bpf2bpf call is traced by fexit.
>> + */
>> +static void test_tailcall_bpf2bpf_fexit(void)
>> +{
>> + tailcall_bpf2bpf_fentry_fexit(false, true);
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fentry_fexit checks that the count value of the tail
>> + * call limit enforcement matches with expectations when tailcall is preceded
>> + * with bpf2bpf call, and the bpf2bpf call is traced by both fentry and fexit.
>> + */
>> +static void test_tailcall_bpf2bpf_fentry_fexit(void)
>> +{
>> + tailcall_bpf2bpf_fentry_fexit(true, true);
>
> Would it be possible to modify existing test_tailcall_count() to have
> fentry/fexit testing within? __tailcall_bpf2bpf_fentry_fexit() basically
> repeats the logic of test_tailcall_count(), right?
>
> How about something like:
>
> static void test_tailcall_bpf2bpf_fentry(void)
> {
> test_tailcall_count("tailcall_bpf2bpf2.bpf.o", true, false);
> }
>
> // rest of your test cases
>
> and existing tailcall count tests:
>
> static void test_tailcall_3(void)
> {
> test_tailcall_count("tailcall3.bpf.o", false, false);
> }
>
> static void test_tailcall_6(void)
> {
> test_tailcall_count("tailcall6.bpf.o", false, false);
> }
LGTM, I'll try it.
Thanks,
Leon
>
>> +}
>> +
>> +/* test_tailcall_bpf2bpf_fentry_entry checks that the count value of the tail
>> + * call limit enforcement matches with expectations when tailcall is preceded
>> + * with bpf2bpf call, and the bpf2bpf caller is traced by fentry.
>> + */
>> +static void test_tailcall_bpf2bpf_fentry_entry(void)
>> +{
>> + struct bpf_object *tgt_obj = NULL, *fentry_obj = NULL;
>> + int err, map_fd, prog_fd, data_fd, i, val;
>> + struct bpf_map *prog_array, *data_map;
>> + struct bpf_link *fentry_link = NULL;
>> + struct bpf_program *prog;
>> + char buff[128] = {};
>> +
>> + LIBBPF_OPTS(bpf_test_run_opts, topts,
>> + .data_in = buff,
>> + .data_size_in = sizeof(buff),
>> + .repeat = 1,
>> + );
>> +
>> + err = bpf_prog_test_load("tailcall_bpf2bpf2.bpf.o",
>> + BPF_PROG_TYPE_SCHED_CLS,
>> + &tgt_obj, &prog_fd);
>> + if (!ASSERT_OK(err, "load tgt_obj"))
>> + return;
>> +
>> + prog_array = bpf_object__find_map_by_name(tgt_obj, "jmp_table");
>> + if (!ASSERT_OK_PTR(prog_array, "find jmp_table map"))
>> + goto out;
>> +
>> + map_fd = bpf_map__fd(prog_array);
>> + if (!ASSERT_FALSE(map_fd < 0, "find jmp_table map fd"))
>> + goto out;
>> +
>> + prog = bpf_object__find_program_by_name(tgt_obj, "classifier_0");
>> + if (!ASSERT_OK_PTR(prog, "find classifier_0 prog"))
>> + goto out;
>> +
>> + prog_fd = bpf_program__fd(prog);
>> + if (!ASSERT_FALSE(prog_fd < 0, "find classifier_0 prog fd"))
>> + goto out;
>> +
>> + i = 0;
>> + err = bpf_map_update_elem(map_fd, &i, &prog_fd, BPF_ANY);
>> + if (!ASSERT_OK(err, "update jmp_table"))
>> + goto out;
>> +
>> + fentry_obj = bpf_object__open_file("tailcall_bpf2bpf_fentry.bpf.o",
>> + NULL);
>> + if (!ASSERT_OK_PTR(fentry_obj, "open fentry_obj file"))
>> + goto out;
>> +
>> + prog = bpf_object__find_program_by_name(fentry_obj, "fentry");
>> + if (!ASSERT_OK_PTR(prog, "find fentry prog"))
>> + goto out;
>> +
>> + err = bpf_program__set_attach_target(prog, prog_fd, "classifier_0");
>> + if (!ASSERT_OK(err, "set_attach_target classifier_0"))
>> + goto out;
>> +
>> + err = bpf_object__load(fentry_obj);
>> + if (!ASSERT_OK(err, "load fentry_obj"))
>> + goto out;
>> +
>> + fentry_link = bpf_program__attach_trace(prog);
>> + if (!ASSERT_OK_PTR(fentry_link, "attach_trace"))
>> + goto out;
>> +
>> + err = bpf_prog_test_run_opts(prog_fd, &topts);
>> + ASSERT_OK(err, "tailcall");
>> + ASSERT_EQ(topts.retval, 1, "tailcall retval");
>> +
>> + data_map = bpf_object__find_map_by_name(tgt_obj, "tailcall.bss");
>> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> + "find tailcall.bss map"))
>> + goto out;
>> +
>> + data_fd = bpf_map__fd(data_map);
>> + if (!ASSERT_FALSE(data_fd < 0, "find tailcall.bss map fd"))
>> + goto out;
>> +
>> + i = 0;
>> + err = bpf_map_lookup_elem(data_fd, &i, &val);
>> + ASSERT_OK(err, "tailcall count");
>> + ASSERT_EQ(val, 34, "tailcall count");
>> +
>> + data_map = bpf_object__find_map_by_name(fentry_obj, ".bss");
>> + if (!ASSERT_FALSE(!data_map || !bpf_map__is_internal(data_map),
>> + "find tailcall_bpf2bpf_fentry.bss map"))
>> + goto out;
>> +
>> + data_fd = bpf_map__fd(data_map);
>> + if (!ASSERT_FALSE(data_fd < 0,
>> + "find tailcall_bpf2bpf_fentry.bss map fd"))
>> + goto out;
>> +
>> + i = 0;
>> + err = bpf_map_lookup_elem(data_fd, &i, &val);
>> + ASSERT_OK(err, "fentry count");
>> + ASSERT_EQ(val, 1, "fentry count");
>> +
>> +out:
>> + bpf_link__destroy(fentry_link);
>> + bpf_object__close(fentry_obj);
>> + bpf_object__close(tgt_obj);
>> +}
>> +
>> void test_tailcalls(void)
>> {
>> if (test__start_subtest("tailcall_1"))
>> @@ -910,4 +1201,12 @@ void test_tailcalls(void)
>> test_tailcall_bpf2bpf_4(true);
>> if (test__start_subtest("tailcall_bpf2bpf_6"))
>> test_tailcall_bpf2bpf_6();
>> + if (test__start_subtest("tailcall_bpf2bpf_fentry"))
>> + test_tailcall_bpf2bpf_fentry();
>> + if (test__start_subtest("tailcall_bpf2bpf_fexit"))
>> + test_tailcall_bpf2bpf_fexit();
>> + if (test__start_subtest("tailcall_bpf2bpf_fentry_fexit"))
>> + test_tailcall_bpf2bpf_fentry_fexit();
>> + if (test__start_subtest("tailcall_bpf2bpf_fentry_entry"))
>> + test_tailcall_bpf2bpf_fentry_entry();
>> }
>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
>> new file mode 100644
>> index 0000000000000..8436c6729167c
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fentry.c
>> @@ -0,0 +1,18 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright Leon Hwang */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +int count = 0;
>> +
>> +SEC("fentry/subprog_tail")
>> +int BPF_PROG(fentry, struct sk_buff *skb)
>> +{
>> + count++;
>> +
>> + return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>> diff --git a/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
>> new file mode 100644
>> index 0000000000000..fe16412c6e6e9
>> --- /dev/null
>> +++ b/tools/testing/selftests/bpf/progs/tailcall_bpf2bpf_fexit.c
>> @@ -0,0 +1,18 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright Leon Hwang */
>> +
>> +#include "vmlinux.h"
>> +#include <bpf/bpf_helpers.h>
>> +#include <bpf/bpf_tracing.h>
>> +
>> +int count = 0;
>> +
>> +SEC("fexit/subprog_tail")
>> +int BPF_PROG(fexit, struct sk_buff *skb)
>> +{
>> + count++;
>> +
>> + return 0;
>> +}
>> +
>> +char _license[] SEC("license") = "GPL";
>> --
>> 2.41.0
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2023-09-07 3:53 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-03 15:14 [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 1/4] bpf, x64: Comment tail_call_cnt initialisation Leon Hwang
2023-09-05 19:26 ` Maciej Fijalkowski
2023-09-06 2:23 ` Leon Hwang
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 2/4] bpf, x64: Fix tailcall infinite loop Leon Hwang
2023-09-06 20:50 ` Maciej Fijalkowski
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 3/4] selftests/bpf: Correct map_fd to data_fd in tailcalls Leon Hwang
2023-09-05 19:22 ` Maciej Fijalkowski
2023-09-06 2:29 ` Leon Hwang
2023-09-03 15:14 ` [RFC PATCH bpf-next v4 4/4] selftests/bpf: Add testcases for tailcall infinite loop fixing Leon Hwang
2023-09-06 21:18 ` Maciej Fijalkowski
2023-09-07 3:53 ` Leon Hwang
2023-09-04 13:10 ` [RFC PATCH bpf-next v4 0/4] bpf, x64: Fix tailcall infinite loop Ilya Leoshkevich
2023-09-04 15:15 ` Leon Hwang
2023-09-06 0:57 ` Ilya Leoshkevich
2023-09-06 2:39 ` Leon Hwang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox