From: Pu Lehui Currently in check_max_stack_depth_subprog, when the verifier enters a new callee branch, the local tail_call_reachable is not properly synchronized with the callee's state. Consider a main prog branching into multiple subprogs: subprog0 -> tailcall main < subprog1 -> subprog2 When the verifier finishes checking subprog0 and backtracks to main prog, the local tail_call_reachable state is left as true. As it proceeds to subprog1, this uncleared state leaks into the new branch, falsely marking subprog1 and subprog2 as tailcall reachable. Fix this by explicitly syncing tail_call_reachable with the callee's has_tail_call state on entry. The caller's state is safely preserved and restored via the existing backtracking logic. Fixes: ebf7d1f508a7 ("bpf, x64: rework pro/epilogue and tailcall handling in JIT") Reported-by: Sashiko Signed-off-by: Pu Lehui --- kernel/bpf/verifier.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index de816063ae63..782d939c38cd 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5263,8 +5263,8 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx, if (!priv_stack_supported) subprog[idx].priv_stack_mode = NO_PRIV_STACK; - if (subprog[idx].has_tail_call) - tail_call_reachable = true; + /* sync tail_call_reachable with callee state on entry */ + tail_call_reachable = subprog[idx].has_tail_call; frame = bpf_subprog_is_global(env, idx) ? 0 : frame + 1; if (frame >= MAX_CALL_FRAMES) { -- 2.34.1 From: Pu Lehui Some JIT compilers, such as x86_64, rely on a register to pass the TCC. When subprograms of synchronous callback invoke tailcall, C helpers invoking bpf callback clobber this register, and the corrupted TCC may bypass the TCC limit, leading to infinite tailcall. Fix this by rejecting tailcall inside all subprogs of sync callback. This also cleanly consolidates the existing async and exception callback checks into a single unified `is_cb` check. Reported-by: Sashiko Reported-by: Björn Töpel Acked-by: Eduard Zingerman Signed-off-by: Pu Lehui --- kernel/bpf/verifier.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 782d939c38cd..62d46b4c9962 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5237,10 +5237,6 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx, if (verifier_bug_if(sidx < 0, env, "callee not found at insn %d", next_insn)) return -EFAULT; if (subprog[sidx].is_async_cb) { - if (subprog[sidx].has_tail_call) { - verifier_bug(env, "subprog has tail_call and async cb"); - return -EFAULT; - } /* async callbacks don't increase bpf prog stack size unless called directly */ if (!bpf_pseudo_call(insn + i)) continue; @@ -5281,8 +5277,8 @@ static int check_max_stack_depth_subprog(struct bpf_verifier_env *env, int idx, */ if (tail_call_reachable) { for (tmp = idx; tmp >= 0; tmp = dinfo[tmp].caller) { - if (subprog[tmp].is_exception_cb) { - verbose(env, "cannot tail call within exception cb\n"); + if (subprog[tmp].is_cb) { + verbose(env, "cannot tail call within callback\n"); return -EINVAL; } if (subprog[tmp].stack_arg_cnt) { -- 2.34.1 From: Pu Lehui Add 2 testcases for callback with tailcall: 1. failure case: callback->subprog->tailcall. 2. success case: subprog with tailcall do not affect no-tailcall callback. Signed-off-by: Pu Lehui --- .../selftests/bpf/prog_tests/tailcalls.c | 7 ++ .../selftests/bpf/progs/tailcall_callback.c | 81 +++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/tailcall_callback.c diff --git a/tools/testing/selftests/bpf/prog_tests/tailcalls.c b/tools/testing/selftests/bpf/prog_tests/tailcalls.c index a5a226d0104c..c66037162da5 100644 --- a/tools/testing/selftests/bpf/prog_tests/tailcalls.c +++ b/tools/testing/selftests/bpf/prog_tests/tailcalls.c @@ -12,6 +12,7 @@ #include "tailcall_cgrp_storage_no_storage.skel.h" #include "tailcall_cgrp_storage.skel.h" #include "tailcall_sleepable.skel.h" +#include "tailcall_callback.skel.h" /* test_tailcall_1 checks basic functionality by patching multiple locations * in a single program for a single tail call slot with nop->jmp, jmp->nop @@ -1901,6 +1902,11 @@ static void test_tailcall_sleepable(void) tailcall_sleepable__destroy(skel); } +static void test_tailcall_callback(void) +{ + RUN_TESTS(tailcall_callback); +} + void test_tailcalls(void) { if (test__start_subtest("tailcall_1")) @@ -1967,4 +1973,5 @@ void test_tailcalls(void) test_tailcall_cgrp_storage_no_storage_leaf(); if (test__start_subtest("tailcall_cgrp_storage_no_storage_bridge")) test_tailcall_cgrp_storage_no_storage_bridge(); + test_tailcall_callback(); } diff --git a/tools/testing/selftests/bpf/progs/tailcall_callback.c b/tools/testing/selftests/bpf/progs/tailcall_callback.c new file mode 100644 index 000000000000..c41632cf423b --- /dev/null +++ b/tools/testing/selftests/bpf/progs/tailcall_callback.c @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: GPL-2.0 +#include +#include +#include "bpf_misc.h" +#include "bpf_test_utils.h" + +int classifier_0(struct __sk_buff *skb); + +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __uint(key_size, sizeof(__u32)); + __array(values, void (void)); +} jmp_table SEC(".maps") = { + .values = { + [0] = (void *) &classifier_0, + }, +}; + +__auxiliary +SEC("tc") +int classifier_0(struct __sk_buff *skb) +{ + return 0; +} + +static __noinline +int subprog_tail0(struct __sk_buff *skb) +{ + int ret = 0; + + bpf_tail_call_static(skb, &jmp_table, 0); + barrier_var(ret); + return ret; +} + +static __noinline +int callback_loop(int index, void **cb_ctx) +{ + int ret; + + ret = subprog_tail0(*cb_ctx); + barrier_var(ret); + return ret ? 1 : 0; +} + +static __noinline +int callback_empty(int index, void *data) +{ + return 0; +} + +/* callback involving subprog with tail call is rejected */ +SEC("tc") +__failure __msg("cannot tail call within callback") +int tailcall_callback_1(struct __sk_buff *skb) +{ + clobber_regs_stack(); + + bpf_loop(1, callback_loop, &skb, 0); + return 0; +} + +/* subprogs with tailcall do not affect no-tailcall callback */ +SEC("tc") +__success +__retval(0) +int tailcall_callback_2(struct __sk_buff *skb) +{ + int ret; + + clobber_regs_stack(); + + ret = subprog_tail0(skb); + __sink(ret); + + bpf_loop(1, callback_empty, NULL, 0); + return 0; +} + +char __license[] SEC("license") = "GPL"; -- 2.34.1