* [PATCH bpf] bpf: tail calls do not modify packet data
@ 2025-10-29 10:58 Martin Teichmann
2025-10-31 19:24 ` Eduard Zingerman
0 siblings, 1 reply; 43+ messages in thread
From: Martin Teichmann @ 2025-10-29 10:58 UTC (permalink / raw)
To: bpf; +Cc: Martin Teichmann
The bpf verifier checks whether packet data is modified within a helper
function, and if so invalidates the pointer to that data. Currently the
verifier always invalidates if the helper function called is a tail
call, as it cannot tell whether the called function does or does not
modify the packet data.
However, in this case, the fact that the packet might be modified is
irrelevant in the code following the helper call, as the tail call only
returns if there is nothing to execute, otherwise the calling
(sub)program will return directly after the tail call finished.
So it is this (sub)program for which the pointer to packet data needs to
be invalidated.
Fortunately, there are already two distinct points in the code for
invalidating packet pointers directly after a helper call, and for
entire (sub)programs. This commit assures that the pointer is only
invalidated in the relevant case.
Note that this is a regression bug: taking care of tail calls only
became necessary when subprograms were introduced, before commit
1a4607ffba35 using a packet pointer after a tail call was working fine,
as it should.
Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers")
Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu>
---
kernel/bpf/verifier.c | 3 ++-
net/core/filter.c | 2 --
.../selftests/bpf/progs/verifier_sock.c | 18 ++++++++++++++++--
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6d175849e57a..4bc36a1efe93 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17879,7 +17879,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
*/
if (ret == 0 && fp->might_sleep)
mark_subprog_might_sleep(env, t);
- if (bpf_helper_changes_pkt_data(insn->imm))
+ if (bpf_helper_changes_pkt_data(insn->imm)
+ || insn->imm == BPF_FUNC_tail_call)
mark_subprog_changes_pkt_data(env, t);
} else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) {
struct bpf_kfunc_call_arg_meta meta;
diff --git a/net/core/filter.c b/net/core/filter.c
index 9d67a34a6650..71a1cfed49f1 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -8038,8 +8038,6 @@ bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id)
case BPF_FUNC_xdp_adjust_head:
case BPF_FUNC_xdp_adjust_meta:
case BPF_FUNC_xdp_adjust_tail:
- /* tail-called program could call any of the above */
- case BPF_FUNC_tail_call:
return true;
default:
return false;
diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c
index 2b4610b53382..3e22b4f8aec4 100644
--- a/tools/testing/selftests/bpf/progs/verifier_sock.c
+++ b/tools/testing/selftests/bpf/progs/verifier_sock.c
@@ -1117,10 +1117,10 @@ int tail_call(struct __sk_buff *sk)
return 0;
}
-/* Tail calls invalidate packet pointers. */
+/* Tail calls in sub-programs invalidate packet pointers. */
SEC("tc")
__failure __msg("invalid mem access")
-int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk)
+int invalidate_pkt_pointers_by_indirect_tail_call(struct __sk_buff *sk)
{
int *p = (void *)(long)sk->data;
@@ -1131,4 +1131,18 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk)
return TCX_PASS;
}
+/* Direct tail calls do not invalidate packet pointers. */
+SEC("tc")
+__success
+int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk)
+{
+ int *p = (void *)(long)sk->data;
+
+ if ((void *)(p + 1) > (void *)(long)sk->data_end)
+ return TCX_DROP;
+ bpf_tail_call_static(sk, &jmp_table, 0);
+ *p = 42; /* this is NOT unsafe: tail calls don't return */
+ return TCX_PASS;
+}
+
char _license[] SEC("license") = "GPL";
--
2.43.0
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH bpf] bpf: tail calls do not modify packet data 2025-10-29 10:58 [PATCH bpf] bpf: tail calls do not modify packet data Martin Teichmann @ 2025-10-31 19:24 ` Eduard Zingerman 2025-11-03 8:56 ` Teichmann, Martin ` (4 more replies) 0 siblings, 5 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-10-31 19:24 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Wed, 2025-10-29 at 11:58 +0100, Martin Teichmann wrote: > The bpf verifier checks whether packet data is modified within a helper > function, and if so invalidates the pointer to that data. Currently the > verifier always invalidates if the helper function called is a tail > call, as it cannot tell whether the called function does or does not > modify the packet data. > > However, in this case, the fact that the packet might be modified is > irrelevant in the code following the helper call, as the tail call only > returns if there is nothing to execute, otherwise the calling > (sub)program will return directly after the tail call finished. > > So it is this (sub)program for which the pointer to packet data needs to > be invalidated. > > Fortunately, there are already two distinct points in the code for > invalidating packet pointers directly after a helper call, and for > entire (sub)programs. This commit assures that the pointer is only > invalidated in the relevant case. > > Note that this is a regression bug: taking care of tail calls only > became necessary when subprograms were introduced, before commit > 1a4607ffba35 using a packet pointer after a tail call was working fine, > as it should. > > Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") > Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> > --- Hi Martin, Sorry for delayed response, missed this patch. I don't think this is safe to do, consider the following example: main: p = pkt foo() use p foo: // assume that 'foo' is a static function (local subprogram) if (something) do tail call don't modify packet data Verifier does not track if jump table map contains programs that modify or don't packet pointers, meaning that tail call can do both. When local subprograms are processed, the logic checking .changes_pkt_data does not kick in: static int check_func_call(...) { ... if (subprog_is_global(env, subprog)) { ... if (env->subprog_info[subprog].changes_pkt_data) clear_all_pkt_pointers(env); ... } And verifier relies on reaching: static int check_helper_call(...) { ... changes_data = bpf_helper_changes_pkt_data(func_id); ... if (changes_data) clear_all_pkt_pointers(env); ... } To decide if to invalidate packet pointers. Meaning that in the example above, the 'use p' part of 'main' won't be safe if packet pointers are not invalidated upon seeing tail call. --- Alexei vaguely remembers discussion about using decl_tag's to mark maps containing programs that don't modify packet pointers. > kernel/bpf/verifier.c | 3 ++- > net/core/filter.c | 2 -- > .../selftests/bpf/progs/verifier_sock.c | 18 ++++++++++++++++-- > 3 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 6d175849e57a..4bc36a1efe93 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -17879,7 +17879,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env) > */ > if (ret == 0 && fp->might_sleep) > mark_subprog_might_sleep(env, t); > - if (bpf_helper_changes_pkt_data(insn->imm)) > + if (bpf_helper_changes_pkt_data(insn->imm) > + || insn->imm == BPF_FUNC_tail_call) > mark_subprog_changes_pkt_data(env, t); > } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { > struct bpf_kfunc_call_arg_meta meta; > diff --git a/net/core/filter.c b/net/core/filter.c > index 9d67a34a6650..71a1cfed49f1 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -8038,8 +8038,6 @@ bool bpf_helper_changes_pkt_data(enum bpf_func_id func_id) > case BPF_FUNC_xdp_adjust_head: > case BPF_FUNC_xdp_adjust_meta: > case BPF_FUNC_xdp_adjust_tail: > - /* tail-called program could call any of the above */ > - case BPF_FUNC_tail_call: > return true; > default: > return false; > diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c > index 2b4610b53382..3e22b4f8aec4 100644 > --- a/tools/testing/selftests/bpf/progs/verifier_sock.c > +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c > @@ -1117,10 +1117,10 @@ int tail_call(struct __sk_buff *sk) > return 0; > } > > -/* Tail calls invalidate packet pointers. */ > +/* Tail calls in sub-programs invalidate packet pointers. */ > SEC("tc") > __failure __msg("invalid mem access") > -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) > +int invalidate_pkt_pointers_by_indirect_tail_call(struct __sk_buff *sk) > { > int *p = (void *)(long)sk->data; > > @@ -1131,4 +1131,18 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) > return TCX_PASS; > } > > +/* Direct tail calls do not invalidate packet pointers. */ > +SEC("tc") > +__success > +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) > +{ > + int *p = (void *)(long)sk->data; > + > + if ((void *)(p + 1) > (void *)(long)sk->data_end) > + return TCX_DROP; > + bpf_tail_call_static(sk, &jmp_table, 0); > + *p = 42; /* this is NOT unsafe: tail calls don't return */ > + return TCX_PASS; > +} > + > char _license[] SEC("license") = "GPL"; ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH bpf] bpf: tail calls do not modify packet data 2025-10-31 19:24 ` Eduard Zingerman @ 2025-11-03 8:56 ` Teichmann, Martin 2025-11-03 17:34 ` Eduard Zingerman 2025-11-04 13:30 ` [PATCH v2 bpf] bpf: properly verify tail call behavior Martin Teichmann ` (3 subsequent siblings) 4 siblings, 1 reply; 43+ messages in thread From: Teichmann, Martin @ 2025-11-03 8:56 UTC (permalink / raw) To: Eduard Zingerman; +Cc: bpf, ast, andrii Dear Eduard, thanks for the review! > I don't think this is safe to do, consider the following example: > > main: > p = pkt > foo() > use p > > foo: // assume that 'foo' is a static function (local subprogram) > if (something) do tail call > don't modify packet data You are absolutely right, this would not work. This should actually be covered by tests... I'll write a test. I also already have an idea how to fix also this problem, and will come back to you once I'm done. > Alexei vaguely remembers discussion about using decl_tag's to mark > maps containing programs that don't modify packet pointers. I am actually against that, I think this would be the wrong way to go. In my use case, I have written a dispatcher for packets that tail call other programs depending on the content of the packet processed. These programs do change during runtime. Until now I had no restrictions on those programs, they could modify the packet or not, as they wished, as the code does not return at all anyways. Tagging the programs would only limit their usefulness, without giving any benefits. I know that what I am doing is a bit crazy, I actually do motion control with EBPF, and all the EPBF programs are generated directly from Python, so I am not protected by any checking that compilers like LLVM might do. So I am kindof stress testing the EBPF subsystem... (if there is any interest, my code is at https://github.com/tecki/ebpfcat) Cheers Martin ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH bpf] bpf: tail calls do not modify packet data 2025-11-03 8:56 ` Teichmann, Martin @ 2025-11-03 17:34 ` Eduard Zingerman 2025-11-04 12:54 ` Teichmann, Martin 0 siblings, 1 reply; 43+ messages in thread From: Eduard Zingerman @ 2025-11-03 17:34 UTC (permalink / raw) To: Teichmann, Martin; +Cc: bpf, ast, andrii On Mon, 2025-11-03 at 09:56 +0100, Teichmann, Martin wrote: > Dear Eduard, > > thanks for the review! > > > I don't think this is safe to do, consider the following example: > > > > main: > > p = pkt > > foo() > > use p > > > > foo: // assume that 'foo' is a static function (local subprogram) > > if (something) do tail call > > don't modify packet data > > You are absolutely right, this would not work. This should actually > be covered by tests... I'll write a test. I also already have an > idea how to fix also this problem, and will come back to you once > I'm done. > > > Alexei vaguely remembers discussion about using decl_tag's to mark > > maps containing programs that don't modify packet pointers. > > I am actually against that, I think this would be the wrong way to > go. In my use case, I have written a dispatcher for packets that > tail call other programs depending on the content of the packet > processed. These programs do change during runtime. > > Until now I had no restrictions on those programs, they could modify > the packet or not, as they wished, as the code does not return at > all anyways. Tagging the programs would only limit their usefulness, > without giving any benefits. The idea behind annotation was that map without annotation would only allow programs that do not modify packet data as values, while map with annotation would allow programs that do modify and those that don't. So, for your use-case it would be sufficient to add annotations to the map. But really, I don't see that many options on the table: a. Be conservative and assume that every tail call modifies packed data. b. Use map annotations as described above. c. Assume that the map is pre-filled before main program verification and infer the "allows packet modifying programs" property from programs already there, after which seal the property. What do you suggest? ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH bpf] bpf: tail calls do not modify packet data 2025-11-03 17:34 ` Eduard Zingerman @ 2025-11-04 12:54 ` Teichmann, Martin 0 siblings, 0 replies; 43+ messages in thread From: Teichmann, Martin @ 2025-11-04 12:54 UTC (permalink / raw) To: Eduard Zingerman; +Cc: bpf, ast, andrii Dear Eduard, > But really, I don't see that many options on the table: > a. Be conservative and assume that every tail call modifies packed data. That's actually what I want, be conservative. But that means that old programs should continue to run. I have worked on this in the meantime and I think I found a solution for all cases, I will send it soon. The origin of the problem is that tail calls just were not designed for ever returning. This way once we required the same scrutiny for the tail call as for the caller, the verifier could just stop at the tail call. Letting tail calls return is actually really weird, especially given that we also have constraints on the return values of a tail call, which are indeed verified in check_return_code. But why would we check that at all if that is not actually the end of the program, but we just return to the caller, which cannot do anything with that return value, or can it? So if we go along this path, we would like to have new PROG_TYPEs for tail calls, which just check what is needed inside the tail call, but not the return value, as it returns to another program anyways. We would have types like BPF_PROG_TYPE_TAIL_CALL and BPF_PROC_TYPE_TAIL_CALL_PACKET_MODIFYING or so. Then we should start asking why it is a tail call at all, why can't we just call functions from maps, like we call global functions? Then we could even have proper calling conventions like r1 to r5 are parameters, r0 is return. I do not advocate for any of this, I think this would be a lot of work for not much benefit. So for the time being, I will just try to make my old code work again, as it should. As said, I'll post a patch soon. Greetings Martin ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v2 bpf] bpf: properly verify tail call behavior 2025-10-31 19:24 ` Eduard Zingerman 2025-11-03 8:56 ` Teichmann, Martin @ 2025-11-04 13:30 ` Martin Teichmann 2025-11-04 13:58 ` bot+bpf-ci 2025-11-04 22:30 ` Eduard Zingerman 2025-11-05 17:40 ` [PATCH v3 bpf-next 0/2] " Martin Teichmann ` (2 subsequent siblings) 4 siblings, 2 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-04 13:30 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann A successful ebpf tail call does not return to the caller, but to the caller-of-the-caller, often just finishing the ebpf program altogether. Any restrictions that the verifier needs to take into account - notably the fact that the tail call might have modified packet pointers - are to be checked on the caller-of-the-caller. Checking it on the caller made the verifier refuse perfectly fine programs that would use the packet pointers after a tail call, which is no problem as this code is only executed if the tail call was unsuccessful, i.e. nothing happened. This patch simulates the behavior of a tail call in the verifier. A conditional jump to the code after the tail call is added for the case of an unsucessful tail call, and a return to the caller is simulated for a successful tail call. For the successful case we assume that the tail call returns an int, as tail calls are currently only allowed in functions that return and int. We always assume that the tail call modified the packet pointers, as we do not know what the tail call did. For the unsuccessful case we know nothing happened, so we do not need to add new constraints. Some test are added, notably one corner case found by Eduard Zingerman. Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- kernel/bpf/verifier.c | 25 ++++++++++-- .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++++++- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e4928846e763..9a091e0f2f07 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11005,6 +11005,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) bool in_callback_fn; int err; + err = bpf_update_live_stack(env); + if (err) + return err; + callee = state->frame[state->curframe]; r0 = &callee->regs[BPF_REG_0]; if (r0->type == PTR_TO_STACK) { @@ -11911,6 +11915,24 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn env->prog->call_get_func_ip = true; } + if (func_id == BPF_FUNC_tail_call) { + if (env->cur_state->curframe) { + struct bpf_verifier_state *branch; + mark_reg_scratched(env, BPF_REG_0); + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); + if (IS_ERR(branch)) + return PTR_ERR(branch); + clear_all_pkt_pointers(env); + mark_reg_unknown(env, regs, BPF_REG_0); + err = prepare_func_exit(env, &env->insn_idx); + if (err) + return err; + env->insn_idx--; + } else { + changes_data = false; + } + } + if (changes_data) clear_all_pkt_pointers(env); return 0; @@ -19876,9 +19898,6 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, return PROCESS_BPF_EXIT; if (env->cur_state->curframe) { - err = bpf_update_live_stack(env); - if (err) - return err; /* exit from nested function */ err = prepare_func_exit(env, &env->insn_idx); if (err) diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c index 2b4610b53382..a2132c72d3b8 100644 --- a/tools/testing/selftests/bpf/progs/verifier_sock.c +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c @@ -1117,10 +1117,17 @@ int tail_call(struct __sk_buff *sk) return 0; } -/* Tail calls invalidate packet pointers. */ +static __noinline +int static_tail_call(struct __sk_buff *sk) +{ + bpf_tail_call_static(sk, &jmp_table, 0); + return 0; +} + +/* Tail calls in sub-programs invalidate packet pointers. */ SEC("tc") __failure __msg("invalid mem access") -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +int invalidate_pkt_pointers_by_global_tail_call(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; @@ -1131,4 +1138,32 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) return TCX_PASS; } +/* Tail calls in static sub-programs invalidate packet pointers. */ +SEC("tc") +__failure __msg("invalid mem access") +int invalidate_pkt_pointers_by_static_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + static_tail_call(sk); + *p = 42; /* this is unsafe */ + return TCX_PASS; +} + +/* Direct tail calls do not invalidate packet pointers. */ +SEC("tc") +__success +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + bpf_tail_call_static(sk, &jmp_table, 0); + *p = 42; /* this is NOT unsafe: tail calls don't return */ + return TCX_PASS; +} + char _license[] SEC("license") = "GPL"; -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v2 bpf] bpf: properly verify tail call behavior 2025-11-04 13:30 ` [PATCH v2 bpf] bpf: properly verify tail call behavior Martin Teichmann @ 2025-11-04 13:58 ` bot+bpf-ci 2025-11-04 18:05 ` Alexei Starovoitov 2025-11-04 22:30 ` Eduard Zingerman 1 sibling, 1 reply; 43+ messages in thread From: bot+bpf-ci @ 2025-11-04 13:58 UTC (permalink / raw) To: martin.teichmann, bpf Cc: eddyz87, ast, andrii, martin.teichmann, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 1989 bytes --] > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index ff40e5e65..4b7439a48 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c [ ... ] > @@ -11884,6 +11888,24 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > env->prog->call_get_func_ip = true; > } > > + if (func_id == BPF_FUNC_tail_call) { > + if (env->cur_state->curframe) { > + struct bpf_verifier_state *branch; > + mark_reg_scratched(env, BPF_REG_0); > + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); > + if (IS_ERR(branch)) > + return PTR_ERR(branch); Can push_stack() return an ERR_PTR? Looking at the push_stack() function in verifier.c, it returns NULL on all error paths (allocation failure at line 2095, copy failure at line 2105, and stack overflow at line 2110). Every other call site in the verifier checks for NULL: - cluster_alloc_swap_entry() at line 9017: if (!queued_st) - push_callback_call() at line 10608: if (!callback_state) - check_reference_leaks_function() at line 13852: if (!branch) - check_cond_jmp_op() at line 14314: if (branch && insn) - check_iter_next_call() at line 16739: if (!queued_st) - do_check() at line 16858: if (!other_branch) If push_stack() returns NULL due to memory pressure, IS_ERR(NULL) evaluates to false, so the error check won't trigger and the code will continue with a NULL branch pointer. > + clear_all_pkt_pointers(env); > + mark_reg_unknown(env, regs, BPF_REG_0); > + err = prepare_func_exit(env, &env->insn_idx); > + if (err) > + return err; > + env->insn_idx--; > + } else { > + changes_data = false; > + } > + } > + > if (changes_data) > clear_all_pkt_pointers(env); > return 0; [ ... ] --- AI reviewed your patch. Please fix the bug or email reply why it's not a bug. See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19070634029 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 bpf] bpf: properly verify tail call behavior 2025-11-04 13:58 ` bot+bpf-ci @ 2025-11-04 18:05 ` Alexei Starovoitov 0 siblings, 0 replies; 43+ messages in thread From: Alexei Starovoitov @ 2025-11-04 18:05 UTC (permalink / raw) To: bot+bpf-ci Cc: martin.teichmann, bpf, Eduard, Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann, Martin KaFai Lau, Yonghong Song, Chris Mason, Ihor Solodrai On Tue, Nov 4, 2025 at 5:58 AM <bot+bpf-ci@kernel.org> wrote: > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index ff40e5e65..4b7439a48 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > [ ... ] > > > @@ -11884,6 +11888,24 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > > env->prog->call_get_func_ip = true; > > } > > > > + if (func_id == BPF_FUNC_tail_call) { > > + if (env->cur_state->curframe) { > > + struct bpf_verifier_state *branch; add empty line. > > + mark_reg_scratched(env, BPF_REG_0); > > + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); > > + if (IS_ERR(branch)) > > + return PTR_ERR(branch); > > Can push_stack() return an ERR_PTR? Looking at the push_stack() > function in verifier.c, it returns NULL on all error paths (allocation > failure at line 2095, copy failure at line 2105, and stack overflow at > line 2110). Every other call site in the verifier checks for NULL: > > - cluster_alloc_swap_entry() at line 9017: if (!queued_st) > - push_callback_call() at line 10608: if (!callback_state) > - check_reference_leaks_function() at line 13852: if (!branch) > - check_cond_jmp_op() at line 14314: if (branch && insn) > - check_iter_next_call() at line 16739: if (!queued_st) > - do_check() at line 16858: if (!other_branch) > > If push_stack() returns NULL due to memory pressure, IS_ERR(NULL) > evaluates to false, so the error check won't trigger and the code will > continue with a NULL branch pointer. AI is correct, since it reviewed the patch against bpf tree where push_stack() returns NULL. This is a non-trivial change. If we land it it will go to bpf-next. So pls respin with bpf-next tag, so CI can process it properly. Overall it's an interesting idea! pw-bot: cr ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v2 bpf] bpf: properly verify tail call behavior 2025-11-04 13:30 ` [PATCH v2 bpf] bpf: properly verify tail call behavior Martin Teichmann 2025-11-04 13:58 ` bot+bpf-ci @ 2025-11-04 22:30 ` Eduard Zingerman 1 sibling, 0 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-04 22:30 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Tue, 2025-11-04 at 14:30 +0100, Martin Teichmann wrote: > A successful ebpf tail call does not return to the caller, but to the > caller-of-the-caller, often just finishing the ebpf program altogether. > > Any restrictions that the verifier needs to take into account - notably > the fact that the tail call might have modified packet pointers - are to > be checked on the caller-of-the-caller. Checking it on the caller made > the verifier refuse perfectly fine programs that would use the packet > pointers after a tail call, which is no problem as this code is only > executed if the tail call was unsuccessful, i.e. nothing happened. > > This patch simulates the behavior of a tail call in the verifier. A > conditional jump to the code after the tail call is added for the case > of an unsucessful tail call, and a return to the caller is simulated for > a successful tail call. > > For the successful case we assume that the tail call returns an int, > as tail calls are currently only allowed in functions that return and > int. We always assume that the tail call modified the packet pointers, > as we do not know what the tail call did. > > For the unsuccessful case we know nothing happened, so we do not need to > add new constraints. Some test are added, notably one corner case found > by Eduard Zingerman. > > Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") > Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ > Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> > --- Hi Martin, I think this is a viable idea. Please see a few notes below. > kernel/bpf/verifier.c | 25 ++++++++++-- > .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++++++- > 2 files changed, 59 insertions(+), 5 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index e4928846e763..9a091e0f2f07 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -11005,6 +11005,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) > bool in_callback_fn; > int err; > > + err = bpf_update_live_stack(env); > + if (err) > + return err; > + > callee = state->frame[state->curframe]; > r0 = &callee->regs[BPF_REG_0]; > if (r0->type == PTR_TO_STACK) { > @@ -11911,6 +11915,24 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > env->prog->call_get_func_ip = true; > } > > + if (func_id == BPF_FUNC_tail_call) { > + if (env->cur_state->curframe) { > + struct bpf_verifier_state *branch; > + mark_reg_scratched(env, BPF_REG_0); > + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); > + if (IS_ERR(branch)) > + return PTR_ERR(branch); > + clear_all_pkt_pointers(env); > + mark_reg_unknown(env, regs, BPF_REG_0); > + err = prepare_func_exit(env, &env->insn_idx); > + if (err) > + return err; > + env->insn_idx--; > + } else { > + changes_data = false; > + } > + } > + Could you please update bpf_insn_successors()? Technically, it should do something similar to what Anton does in [1]. W/o bpf_insn_successors() reflecting that control flow might jump over the instructions between tail call and function exit, verifier might assume that some writes to parent stack always happen, which is not the case. I think this is a long standing bug, was here even before my changes for stack liveness. [1] https://lore.kernel.org/bpf/20251102205722.3266908-7-a.s.protopopov@gmail.com/ > if (changes_data) > clear_all_pkt_pointers(env); > return 0; > @@ -19876,9 +19898,6 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, > return PROCESS_BPF_EXIT; > > if (env->cur_state->curframe) { > - err = bpf_update_live_stack(env); > - if (err) > - return err; > /* exit from nested function */ > err = prepare_func_exit(env, &env->insn_idx); > if (err) > diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c > index 2b4610b53382..a2132c72d3b8 100644 > --- a/tools/testing/selftests/bpf/progs/verifier_sock.c > +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c > @@ -1117,10 +1117,17 @@ int tail_call(struct __sk_buff *sk) > return 0; > } > > -/* Tail calls invalidate packet pointers. */ > +static __noinline > +int static_tail_call(struct __sk_buff *sk) > +{ > + bpf_tail_call_static(sk, &jmp_table, 0); > + return 0; > +} > + > +/* Tail calls in sub-programs invalidate packet pointers. */ > SEC("tc") > __failure __msg("invalid mem access") > -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) > +int invalidate_pkt_pointers_by_global_tail_call(struct __sk_buff *sk) > { > int *p = (void *)(long)sk->data; > > @@ -1131,4 +1138,32 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) > return TCX_PASS; > } > Usually, test cases are added as separate commits. Changes to tests are bundled with changes to kernel only if that is necessary to keep bisect happy (tests passing on each commit). -- Also, could you please add a test case verifying that: - Precision propagation works correctly when processing program fragment containing tail call. (I think that is_jmp_point() logic should kick-in automatically here, but would be nice to have a test). - Live stack tracking works correctly under the same circumstances. (Test case exercising bpf_update_live_stack() call you inserted). See verifier_subprog_precision.c and verifier_live_stack.c for examples. > +/* Tail calls in static sub-programs invalidate packet pointers. */ > +SEC("tc") > +__failure __msg("invalid mem access") > +int invalidate_pkt_pointers_by_static_tail_call(struct __sk_buff *sk) > +{ > + int *p = (void *)(long)sk->data; > + > + if ((void *)(p + 1) > (void *)(long)sk->data_end) > + return TCX_DROP; > + static_tail_call(sk); > + *p = 42; /* this is unsafe */ > + return TCX_PASS; > +} > + > +/* Direct tail calls do not invalidate packet pointers. */ > +SEC("tc") > +__success > +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) > +{ > + int *p = (void *)(long)sk->data; > + > + if ((void *)(p + 1) > (void *)(long)sk->data_end) > + return TCX_DROP; > + bpf_tail_call_static(sk, &jmp_table, 0); > + *p = 42; /* this is NOT unsafe: tail calls don't return */ > + return TCX_PASS; > +} > + > char _license[] SEC("license") = "GPL"; ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3 bpf-next 0/2] bpf: properly verify tail call behavior 2025-10-31 19:24 ` Eduard Zingerman 2025-11-03 8:56 ` Teichmann, Martin 2025-11-04 13:30 ` [PATCH v2 bpf] bpf: properly verify tail call behavior Martin Teichmann @ 2025-11-05 17:40 ` Martin Teichmann 2025-11-05 19:08 ` Eduard Zingerman 2025-11-05 17:40 ` [PATCH v3 bpf-next 1/2] bpf: properly verify tail call behavior Martin Teichmann 2025-11-05 17:40 ` [PATCH v3 bpf-next 2/2] bpf: test the proper verification of tail calls Martin Teichmann 4 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-05 17:40 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann I added the changes you proposed. regarding bpf_insn_successors(), nothing needs to be done, call already have the next instruction as successor by default, and this is all we need. The fact that a tail call can also be an exit in disguise is handled by calling bpf_update_live_stack(). In total, this patch now fixed three bugs: a regression wheras programs that modify packet data after a tail call are unnecessarily rejected, proper treatment of precision propagation and live stack tracking. Martin Teichmann (2): bpf: properly verify tail call behavior bpf: test the proper verification of tail calls kernel/bpf/verifier.c | 26 ++++++++-- .../selftests/bpf/progs/verifier_live_stack.c | 46 ++++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 +++++++++++++++++++ 4 files changed, 153 insertions(+), 5 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v3 bpf-next 0/2] bpf: properly verify tail call behavior 2025-11-05 17:40 ` [PATCH v3 bpf-next 0/2] " Martin Teichmann @ 2025-11-05 19:08 ` Eduard Zingerman 2025-11-06 10:52 ` [PATCH v4 " Martin Teichmann ` (2 more replies) 0 siblings, 3 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-05 19:08 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Wed, 2025-11-05 at 18:40 +0100, Martin Teichmann wrote: > I added the changes you proposed. > > regarding bpf_insn_successors(), nothing needs to be done, call > already have the next instruction as successor by default, and this > is all we need. The fact that a tail call can also be an exit in > disguise is handled by calling bpf_update_live_stack(). Consider the following scenario: main: int a; int b[10] = { 1, 2, ... }; if <random>: a = 100500; foo(&a); return b[a]; foo(int *a): tail call; *a = 0; return; W/o bpf_insn_successors() knowing that tail call can jump directly to return, verifier will assume that write to *a always happens, and accept the above program. While trying to create a reproducer for the above scenario I found an issue with current patch-set and precision tracking. Consider a modifier version of you test case: SEC("socket") __log_level(2) __flag(BPF_F_TEST_STATE_FREQ) __naked unsigned long caller_stack_write_tail_call(void) { asm volatile ( "r6 = r1;" "*(u64 *)(r10 - 8) = -8;" "call %[bpf_get_prandom_u32];" "if r0 != 42 goto 1f;" "goto 2f;" "1:" "*(u64 *)(r10 - 8) = -1024;" "2:" "r1 = r6;" "r2 = r10;" "r2 += -8;" "call write_tail_call;" "r1 = *(u64 *)(r10 - 8);" "r2 = r10;" "r2 += r1;" "r0 = *(u64 *)(r2 + 0);" "exit;" :: __imm(bpf_get_prandom_u32) : __clobber_all); } static __used __naked unsigned long write_tail_call(void) { asm volatile ( "r6 = r2;" "r2 = %[map_array] ll;" "r3 = 0;" "call %[bpf_tail_call];" "*(u64 *)(r6 + 0) = -16;" "r0 = 0;" "exit;" : : __imm(bpf_tail_call), __imm_addr(map_array) : __clobber_all); } Currently, it hits the following error: 10: (79) r1 = *(u64 *)(r10 -8) ; R1=-8 R10=fp0 fp-8=-8 11: (bf) r2 = r10 ; R2=fp0 R10=fp0 12: (0f) r2 += r1 mark_precise: frame0: last_idx 12 first_idx 10 subseq_idx -1 mark_precise: frame0: regs=r1 stack= before 11: (bf) r2 = r10 mark_precise: frame0: regs=r1 stack= before 10: (79) r1 = *(u64 *)(r10 -8) mark_precise: frame0: parent state regs= stack=-8: R0=scalar() R6=ctx() R10=fp0 fp-8=P-8 mark_precise: frame0: last_idx 19 first_idx 15 subseq_idx 10 mark_precise: frame0: regs= stack=-8 before 19: (85) call bpf_tail_call#12 mark_precise: frame0: regs= stack=-8 before 18: (b7) r3 = 0 mark_precise: frame0: regs= stack=-8 before 16: (18) r2 = 0xff11000109035000 mark_precise: frame0: regs= stack=-8 before 15: (bf) r6 = r2 mark_precise: frame0: parent state regs= stack=-8: R6=ctx() R10=fp0 fp-8=P-8 mark_precise: frame0: last_idx 9 first_idx 9 subseq_idx 15 mark_precise: frame0: regs= stack=-8 before 9: (85) call pc+5 verifier bug: static subprog leftover stack slots 1 processed 16 insns (limit 1000000) max_states_per_insn 0 total_states 5 peak_states 5 mark_read 0 ============= This happens, because __mark_chain_precision() moves from a history entry with frame index 0, to a history entry with frame index 1. backtrack_insn() assumes that such changes are impossible, because there always would be an EXIT instruction for a subprogram, where it would adjust bt->frame by +1. Which is not the case with patch-set. So, one needs to track frame index in struct bpf_jmp_history_entry, or fake a jump from tail call to EXIT, instead of directly leaving subprogram. After that is resolved, I think that bpf_insn_successors() issue described above will manifest itself. > In total, this patch now fixed three bugs: a regression wheras > programs that modify packet data after a tail call are unnecessarily > rejected, proper treatment of precision propagation and live stack > tracking. > > Martin Teichmann (2): > bpf: properly verify tail call behavior > bpf: test the proper verification of tail calls > > kernel/bpf/verifier.c | 26 ++++++++-- > .../selftests/bpf/progs/verifier_live_stack.c | 46 ++++++++++++++++++ > .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++- > .../bpf/progs/verifier_subprog_precision.c | 47 +++++++++++++++++++ > 4 files changed, 153 insertions(+), 5 deletions(-) ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 bpf-next 0/2] bpf: properly verify tail call behavior 2025-11-05 19:08 ` Eduard Zingerman @ 2025-11-06 10:52 ` Martin Teichmann 2025-11-06 10:52 ` [PATCH v4 bpf-next 1/2] " Martin Teichmann 2025-11-06 10:52 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls Martin Teichmann 2 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-06 10:52 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Hi Eduard, you are right, backtrack_insn() did not properly deal with tail calls. I fixed that, and added your test, which now passes properly. What my code does is to effectively treat (following your example): foo(int *a): tail call; *a = 0; return; as foo(int *a); if (prog_map_has_entry) { tail call; return; } *a = 0; return; In bpf_insn_successors(), a BPF_EXIT is declared as can_jump=false and can_fallthrough=false. This is how the return after tail call should be treated. Let's go through the three lines: the if statement has two successors, the jump and the fallthrough. The fallthrough is the tail call, which has the return as its only successor, which in turn has no successor as BPF_EXIT has no successor. So we are left with the jump successor of the if statement, which is nothing else than the fallthrough of the entire three lines. So if we go back to the original code, we have an effective can_jump=false, can_fallthrough=true. This is the default for BPF_CALL in bpf_insn_successors(), so no code change is needed. Martin Teichmann (2): bpf: properly verify tail call behavior bpf: test the proper verification of tail calls kernel/bpf/verifier.c | 31 +++++++++- .../selftests/bpf/progs/verifier_live_stack.c | 60 +++++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 +++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 +++++++++++++++ 4 files changed, 172 insertions(+), 5 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 bpf-next 1/2] bpf: properly verify tail call behavior 2025-11-05 19:08 ` Eduard Zingerman 2025-11-06 10:52 ` [PATCH v4 " Martin Teichmann @ 2025-11-06 10:52 ` Martin Teichmann 2025-11-06 10:52 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls Martin Teichmann 2 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-06 10:52 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann A successful ebpf tail call does not return to the caller, but to the caller-of-the-caller, often just finishing the ebpf program altogether. Any restrictions that the verifier needs to take into account - notably the fact that the tail call might have modified packet pointers - are to be checked on the caller-of-the-caller. Checking it on the caller made the verifier refuse perfectly fine programs that would use the packet pointers after a tail call, which is no problem as this code is only executed if the tail call was unsuccessful, i.e. nothing happened. This patch simulates the behavior of a tail call in the verifier. A conditional jump to the code after the tail call is added for the case of an unsucessful tail call, and a return to the caller is simulated for a successful tail call. For the successful case we assume that the tail call returns an int, as tail calls are currently only allowed in functions that return and int. We always assume that the tail call modified the packet pointers, as we do not know what the tail call did. For the unsuccessful case we know nothing happened, so we do not need to add new constraints. Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- kernel/bpf/verifier.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1268fa075d4c..7a817777fbb3 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4411,6 +4411,11 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, bt_reg_mask(bt)); return -EFAULT; } + if (insn->src_reg == BPF_REG_0 && insn->imm == BPF_FUNC_tail_call + && subseq_idx - idx != 1) { + if (bt_subprog_enter(bt)) + return -EFAULT; + } } else if (opcode == BPF_EXIT) { bool r0_precise; @@ -11034,6 +11039,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) bool in_callback_fn; int err; + err = bpf_update_live_stack(env); + if (err) + return err; + callee = state->frame[state->curframe]; r0 = &callee->regs[BPF_REG_0]; if (r0->type == PTR_TO_STACK) { @@ -11940,6 +11949,25 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn env->prog->call_get_func_ip = true; } + if (func_id == BPF_FUNC_tail_call) { + if (env->cur_state->curframe) { + struct bpf_verifier_state *branch; + + mark_reg_scratched(env, BPF_REG_0); + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); + if (IS_ERR(branch)) + return PTR_ERR(branch); + clear_all_pkt_pointers(env); + mark_reg_unknown(env, regs, BPF_REG_0); + err = prepare_func_exit(env, &env->insn_idx); + if (err) + return err; + env->insn_idx--; + } else { + changes_data = false; + } + } + if (changes_data) clear_all_pkt_pointers(env); return 0; @@ -20110,9 +20138,6 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, return PROCESS_BPF_EXIT; if (env->cur_state->curframe) { - err = bpf_update_live_stack(env); - if (err) - return err; /* exit from nested function */ err = prepare_func_exit(env, &env->insn_idx); if (err) -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls 2025-11-05 19:08 ` Eduard Zingerman 2025-11-06 10:52 ` [PATCH v4 " Martin Teichmann 2025-11-06 10:52 ` [PATCH v4 bpf-next 1/2] " Martin Teichmann @ 2025-11-06 10:52 ` Martin Teichmann 2025-11-06 19:50 ` Eduard Zingerman 2 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-06 10:52 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Four tests are added: - invalidate_pkt_pointers_by_tail_call checks that one can use the packet pointer after a tail call. This was originally possible and also poses not problems, but was made impossible by 1a4607ffba35. - invalidate_pkt_pointers_by_static_tail_call tests a corner case found by Eduard Zingerman during the discussion of the original fix, which was broken in that fix. - subprog_result_tail_call tests that precision propagation works correctly across tail calls. This did not work before. - caller_stack_write_tail_call tests that the live stack is correctly tracked for a tail call. --- .../selftests/bpf/progs/verifier_live_stack.c | 60 +++++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 +++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 +++++++++++++++ 3 files changed, 144 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/progs/verifier_live_stack.c b/tools/testing/selftests/bpf/progs/verifier_live_stack.c index c0e808509268..c6ce9a4a9fa2 100644 --- a/tools/testing/selftests/bpf/progs/verifier_live_stack.c +++ b/tools/testing/selftests/bpf/progs/verifier_live_stack.c @@ -292,3 +292,63 @@ __naked void syzbot_postorder_bug1(void) "exit;" ::: __clobber_all); } + +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +SEC("socket") +__log_level(2) +__msg("19: (85) call bpf_tail_call#12") +__msg("(0) frame 0 insn 1 +written -8") +__msg("(0) live stack update done in 2 iterations") +__msg("14: (95) exit") +__msg("(0) frame 0 insn 13 +live -8") +__msg("(0) live stack update done in 2 iterations") +__msg("22: (95) exit") +__msg("(9,15) frame 0 insn 20 +written -8") +__msg("(9,15) live stack update done in 2 iterations") +__msg("14: (95) exit") +__msg("(0) frame 0 insn 13 +live -16") +__naked unsigned long caller_stack_write_tail_call(void) +{ + asm volatile ( + "r6 = r1;" + "*(u64 *)(r10 - 8) = -8;" + "call %[bpf_get_prandom_u32];" + "if r0 != 42 goto 1f;" + "goto 2f;" + "1:" + "*(u64 *)(r10 - 8) = -1024;" + "2:" + "r1 = r6;" + "r2 = r10;" + "r2 += -8;" + "call write_tail_call;" + "r1 = *(u64 *)(r10 - 8);" + "r2 = r10;" + "r2 += r1;" + "r0 = *(u64 *)(r2 + 0);" + "exit;" + :: __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +static __used __naked unsigned long write_tail_call(void) +{ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "*(u64 *)(r6 + 0) = -16;" + "r0 = 0;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c index 2b4610b53382..a2132c72d3b8 100644 --- a/tools/testing/selftests/bpf/progs/verifier_sock.c +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c @@ -1117,10 +1117,17 @@ int tail_call(struct __sk_buff *sk) return 0; } -/* Tail calls invalidate packet pointers. */ +static __noinline +int static_tail_call(struct __sk_buff *sk) +{ + bpf_tail_call_static(sk, &jmp_table, 0); + return 0; +} + +/* Tail calls in sub-programs invalidate packet pointers. */ SEC("tc") __failure __msg("invalid mem access") -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +int invalidate_pkt_pointers_by_global_tail_call(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; @@ -1131,4 +1138,32 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) return TCX_PASS; } +/* Tail calls in static sub-programs invalidate packet pointers. */ +SEC("tc") +__failure __msg("invalid mem access") +int invalidate_pkt_pointers_by_static_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + static_tail_call(sk); + *p = 42; /* this is unsafe */ + return TCX_PASS; +} + +/* Direct tail calls do not invalidate packet pointers. */ +SEC("tc") +__success +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + bpf_tail_call_static(sk, &jmp_table, 0); + *p = 42; /* this is NOT unsafe: tail calls don't return */ + return TCX_PASS; +} + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c index ac3e418c2a96..de5ef3152567 100644 --- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c +++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c @@ -793,4 +793,51 @@ __naked int stack_slot_aliases_precision(void) ); } +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +__naked __noinline __used +static unsigned long identity_tail_call(void) +{ + /* the simplest identity function involving a tail call */ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "r0 = r6;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} + +SEC("?raw_tp") +__failure __log_level(2) +__msg("6: (0f) r1 += r0") +__msg("mark_precise: frame0: regs=r0 stack= before 5: (bf) r1 = r6") +__msg("mark_precise: frame0: regs=r0 stack= before 4: (27) r0 *= 4") +__msg("mark_precise: frame0: parent state regs=r0 stack=: R0=Pscalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") +__msg("math between map_value pointer and register with unbounded min value is not allowed") +__naked int subprog_result_tail_call(void) +{ + asm volatile ( + "r2 = 3;" + "call identity_tail_call;" + "r0 *= 4;" + "r1 = %[vals];" + "r1 += r0;" + "r0 = *(u32 *)(r1 + 0);" + "exit;" + : + : __imm_ptr(vals) + : __clobber_common + ); +} + char _license[] SEC("license") = "GPL"; -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls 2025-11-06 10:52 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls Martin Teichmann @ 2025-11-06 19:50 ` Eduard Zingerman 2025-11-10 15:18 ` [PATCH v4 bpf-next 0/2] bpf: properly verify tail call behavior Martin Teichmann ` (2 more replies) 0 siblings, 3 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-06 19:50 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Thu, 2025-11-06 at 11:52 +0100, Martin Teichmann wrote: [...] > +SEC("socket") > +__log_level(2) > +__msg("19: (85) call bpf_tail_call#12") > +__msg("(0) frame 0 insn 1 +written -8") > +__msg("(0) live stack update done in 2 iterations") > +__msg("14: (95) exit") > +__msg("(0) frame 0 insn 13 +live -8") > +__msg("(0) live stack update done in 2 iterations") > +__msg("22: (95) exit") > +__msg("(9,15) frame 0 insn 20 +written -8") > +__msg("(9,15) live stack update done in 2 iterations") > +__msg("14: (95) exit") > +__msg("(0) frame 0 insn 13 +live -16") > +__naked unsigned long caller_stack_write_tail_call(void) > +{ > + asm volatile ( > + "r6 = r1;" > + "*(u64 *)(r10 - 8) = -8;" > + "call %[bpf_get_prandom_u32];" > + "if r0 != 42 goto 1f;" > + "goto 2f;" > + "1:" > + "*(u64 *)(r10 - 8) = -1024;" > + "2:" > + "r1 = r6;" > + "r2 = r10;" > + "r2 += -8;" > + "call write_tail_call;" > + "r1 = *(u64 *)(r10 - 8);" > + "r2 = r10;" > + "r2 += r1;" > + "r0 = *(u64 *)(r2 + 0);" > + "exit;" > + :: __imm(bpf_get_prandom_u32) > + : __clobber_all); > +} This program is not safe, but verifier accepts it as safe. Note the following log: 0: R1=ctx() R10=fp0 ; asm volatile ( @ verifier_live_stack.c:318 0: (bf) r6 = r1 ; R1=ctx() R6=ctx() 1: (7a) *(u64 *)(r10 -8) = -8 ; R10=fp0 fp-8=-8 2: (85) call bpf_get_prandom_u32#7 ; R0=scalar() 3: (55) if r0 != 0x2a goto pc+1 ; R0=42 4: (05) goto pc+1 6: (bf) r1 = r6 ; R1=ctx() R6=ctx() 7: (bf) r2 = r10 ; R2=fp0 R10=fp0 8: (07) r2 += -8 ; R2=fp-8 9: (85) call pc+5 ... from 3 to 5: R0=scalar() R6=ctx() R10=fp0 fp-8=-8 5: R0=scalar() R6=ctx() R10=fp0 fp-8=-8 5: (7a) *(u64 *)(r10 -8) = -1024 ; R10=fp0 fp-8=-1024 6: (bf) r1 = r6 ; R1=ctx() R6=ctx() 7: (bf) r2 = r10 ; R2=fp0 R10=fp0 8: (07) r2 += -8 9: safe First time instruction #9 is verified for fp-8 == -8, on a seocnd time verifier does not proceed with abstract interpretation when fp-8 == -1024. This happens because verifier assumes that call at (9) always sets fp-8 to -16. And it does so, because bpf_insn_successors() does not tell it about possible function exit after tail call. Note that sack liveness marks are propagated over the program control flow graph as defined by bpf_insn_successors(). kernel/bpf/liveness.c has extensive comments about that. W/o tweak to bpf_insn_successors() verifier assumes that write_tail_call always sets fp-8 to -16, from which verifier infers that fp-8 is dead at instruction (9). Verifier does not compare states for dead stack slots when considering states_equal() in is_state_visited(). Hence it proceeds with state pruning logic and stops. Please adjust check_cfg() in a way, similar to visit_gotox_insn(): for tail calls, store env->insn_aux_data[t].jt pointing either to the next instruciton or to program exit. And use this information in bpf_insn_successors(). (You will need to rebase to do that, the changes for visit_gotox_insn() landed just yesterday). > +static __used __naked unsigned long write_tail_call(void) > +{ > + asm volatile ( > + "r6 = r2;" > + "r2 = %[map_array] ll;" > + "r3 = 0;" > + "call %[bpf_tail_call];" > + "*(u64 *)(r6 + 0) = -16;" > + "r0 = 0;" > + "exit;" > + : > + : __imm(bpf_tail_call), > + __imm_addr(map_array) > + : __clobber_all); > +} [...] ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 bpf-next 0/2] bpf: properly verify tail call behavior 2025-11-06 19:50 ` Eduard Zingerman @ 2025-11-10 15:18 ` Martin Teichmann 2025-11-10 15:18 ` [PATCH v4 bpf-next 1/2] " Martin Teichmann 2025-11-10 15:18 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification " Martin Teichmann 2 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-10 15:18 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Hi Eduard, sorry, it took me a while to get my head around this, but now I found a nice solution. The test you proposed is now correctly found to fail. Martin Teichmann (2): bpf: properly verify tail call behavior bpf: test the proper verification of tail calls kernel/bpf/liveness.c | 4 ++ kernel/bpf/verifier.c | 31 ++++++++++-- .../selftests/bpf/progs/verifier_live_stack.c | 49 +++++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 ++++++++++++++++++ 5 files changed, 165 insertions(+), 5 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 bpf-next 1/2] bpf: properly verify tail call behavior 2025-11-06 19:50 ` Eduard Zingerman 2025-11-10 15:18 ` [PATCH v4 bpf-next 0/2] bpf: properly verify tail call behavior Martin Teichmann @ 2025-11-10 15:18 ` Martin Teichmann 2025-11-10 20:28 ` Eduard Zingerman 2025-11-10 15:18 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification " Martin Teichmann 2 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-10 15:18 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann A successful ebpf tail call does not return to the caller, but to the caller-of-the-caller, often just finishing the ebpf program altogether. Any restrictions that the verifier needs to take into account - notably the fact that the tail call might have modified packet pointers - are to be checked on the caller-of-the-caller. Checking it on the caller made the verifier refuse perfectly fine programs that would use the packet pointers after a tail call, which is no problem as this code is only executed if the tail call was unsuccessful, i.e. nothing happened. This patch simulates the behavior of a tail call in the verifier. A conditional jump to the code after the tail call is added for the case of an unsucessful tail call, and a return to the caller is simulated for a successful tail call. For the successful case we assume that the tail call returns an int, as tail calls are currently only allowed in functions that return and int. We always assume that the tail call modified the packet pointers, as we do not know what the tail call did. For the unsuccessful case we know nothing happened, so we do not need to add new constraints. This approach also allows to check other problems that may occur with tail calls, namely we are now able to check that precision is properly propagated into subprograms using tail calls, as well as checking the live slots in such a subprogram. Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- kernel/bpf/liveness.c | 4 ++++ kernel/bpf/verifier.c | 31 ++++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index a7240013fd9d..54f4772d990c 100644 --- a/kernel/bpf/liveness.c +++ b/kernel/bpf/liveness.c @@ -500,6 +500,10 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx) if (opcode_info->can_jump) succ->items[succ->cnt++] = idx + bpf_jmp_offset(insn) + 1; + if (unlikely(insn->code == (BPF_JMP | BPF_CALL) && insn->src_reg == 0 + && insn->imm == BPF_FUNC_tail_call)) + succ->items[succ->cnt++] = idx; + return succ; } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1268fa075d4c..7a817777fbb3 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4411,6 +4411,11 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, bt_reg_mask(bt)); return -EFAULT; } + if (insn->src_reg == BPF_REG_0 && insn->imm == BPF_FUNC_tail_call + && subseq_idx - idx != 1) { + if (bt_subprog_enter(bt)) + return -EFAULT; + } } else if (opcode == BPF_EXIT) { bool r0_precise; @@ -11034,6 +11039,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) bool in_callback_fn; int err; + err = bpf_update_live_stack(env); + if (err) + return err; + callee = state->frame[state->curframe]; r0 = &callee->regs[BPF_REG_0]; if (r0->type == PTR_TO_STACK) { @@ -11940,6 +11949,25 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn env->prog->call_get_func_ip = true; } + if (func_id == BPF_FUNC_tail_call) { + if (env->cur_state->curframe) { + struct bpf_verifier_state *branch; + + mark_reg_scratched(env, BPF_REG_0); + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); + if (IS_ERR(branch)) + return PTR_ERR(branch); + clear_all_pkt_pointers(env); + mark_reg_unknown(env, regs, BPF_REG_0); + err = prepare_func_exit(env, &env->insn_idx); + if (err) + return err; + env->insn_idx--; + } else { + changes_data = false; + } + } + if (changes_data) clear_all_pkt_pointers(env); return 0; @@ -20110,9 +20138,6 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, return PROCESS_BPF_EXIT; if (env->cur_state->curframe) { - err = bpf_update_live_stack(env); - if (err) - return err; /* exit from nested function */ err = prepare_func_exit(env, &env->insn_idx); if (err) -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: properly verify tail call behavior 2025-11-10 15:18 ` [PATCH v4 bpf-next 1/2] " Martin Teichmann @ 2025-11-10 20:28 ` Eduard Zingerman 2025-11-10 23:39 ` Eduard Zingerman ` (6 more replies) 0 siblings, 7 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-10 20:28 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii [-- Attachment #1: Type: text/plain, Size: 1069 bytes --] On Mon, 2025-11-10 at 16:18 +0100, Martin Teichmann wrote: [...] > diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c > index a7240013fd9d..54f4772d990c 100644 > --- a/kernel/bpf/liveness.c > +++ b/kernel/bpf/liveness.c > @@ -500,6 +500,10 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx) > if (opcode_info->can_jump) > succ->items[succ->cnt++] = idx + bpf_jmp_offset(insn) + 1; > > + if (unlikely(insn->code == (BPF_JMP | BPF_CALL) && insn->src_reg == 0 > + && insn->imm == BPF_FUNC_tail_call)) > + succ->items[succ->cnt++] = idx; > + > return succ; > } > Hi Martin, This is a clever hack and I like it, but let's not do that. It is going to be a footgun if e.g. someone would use bpf_insn_successors() to build intra-procedural CFG. Instead, please allocate a jt object for tail calls as in the diff attached (on top of your patch-set). Please also extend compute_live_registers.c to cover this logic. Other than that, I think that patch logic and tests are fine. Thanks, Eduard [...] [-- Attachment #2: tail-call-jt.patch --] [-- Type: text/x-patch, Size: 4484 bytes --] diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 5441341f1ab9..8d0b60fa5f2b 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -527,7 +527,6 @@ struct bpf_insn_aux_data { struct { u32 map_index; /* index into used_maps[] */ u32 map_off; /* offset from value base address */ - struct bpf_iarray *jt; /* jump table for gotox instruction */ }; struct { enum bpf_reg_type reg_type; /* type of pseudo_btf_id */ @@ -550,6 +549,7 @@ struct bpf_insn_aux_data { /* remember the offset of node field within type to rewrite */ u64 insert_off; }; + struct bpf_iarray *jt; /* jump table for gotox or bpf_tailcall call instruction */ struct btf_struct_meta *kptr_struct_meta; u64 map_key_state; /* constant (32 bit) key tracking for maps */ int ctx_field_size; /* the ctx field size for load insn, maybe 0 */ @@ -652,6 +652,7 @@ struct bpf_subprog_info { u32 start; /* insn idx of function entry point */ u32 linfo_idx; /* The idx to the main_prog->aux->linfo */ u32 postorder_start; /* The idx to the env->cfg.insn_postorder */ + u32 exit_idx; /* Index of one of the BPF_EXIT instructions in this subprogram */ u16 stack_depth; /* max. stack depth used by this function */ u16 stack_extra; /* offsets in range [stack_depth .. fastcall_stack_off) @@ -669,9 +670,9 @@ struct bpf_subprog_info { bool keep_fastcall_stack: 1; bool changes_pkt_data: 1; bool might_sleep: 1; + u8 arg_cnt:3; enum priv_stack_mode priv_stack_mode; - u8 arg_cnt; struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS]; }; diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index 54f4772d990c..60db5d655495 100644 --- a/kernel/bpf/liveness.c +++ b/kernel/bpf/liveness.c @@ -482,11 +482,12 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx) struct bpf_prog *prog = env->prog; struct bpf_insn *insn = &prog->insnsi[idx]; const struct opcode_info *opcode_info; - struct bpf_iarray *succ; + struct bpf_iarray *succ, *jt; int insn_sz; - if (unlikely(insn_is_gotox(insn))) - return env->insn_aux_data[idx].jt; + jt = env->insn_aux_data[idx].jt; + if (unlikely(jt)) + return jt; /* pre-allocated array of size up to 2; reset cnt, as it may have been used already */ succ = env->succ; @@ -500,10 +501,6 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx) if (opcode_info->can_jump) succ->items[succ->cnt++] = idx + bpf_jmp_offset(insn) + 1; - if (unlikely(insn->code == (BPF_JMP | BPF_CALL) && insn->src_reg == 0 - && insn->imm == BPF_FUNC_tail_call)) - succ->items[succ->cnt++] = idx; - return succ; } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 7a817777fbb3..dc129a59718b 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3528,8 +3528,12 @@ static int check_subprogs(struct bpf_verifier_env *env) subprog[cur_subprog].has_ld_abs = true; if (BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32) goto next; - if (BPF_OP(code) == BPF_EXIT || BPF_OP(code) == BPF_CALL) + if (BPF_OP(code) == BPF_CALL) goto next; + if (BPF_OP(code) == BPF_EXIT) { + subprog[cur_subprog].exit_idx = i; + goto next; + } off = i + bpf_jmp_offset(&insn[i]) + 1; if (off < subprog_start || off >= subprog_end) { verbose(env, "jump out of range from insn %d to %d\n", i, off); @@ -18120,6 +18124,25 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env) return keep_exploring ? KEEP_EXPLORING : DONE_EXPLORING; } +static int visit_tailcall_insn(struct bpf_verifier_env *env, int t) +{ + static struct bpf_subprog_info *subprog; + struct bpf_iarray *jt; + + if (env->insn_aux_data[t].jt) + return 0; + + jt = iarray_realloc(NULL, 2); + if (!jt) + return -ENOMEM; + + subprog = bpf_find_containing_subprog(env, t); + jt->items[0] = t + 1; + jt->items[1] = subprog->exit_idx; + env->insn_aux_data[t].jt = jt; + return 0; +} + /* Visits the instruction at index t and returns one of the following: * < 0 - an error occurred * DONE_EXPLORING - the instruction was fully explored @@ -18180,6 +18203,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env) mark_subprog_might_sleep(env, t); if (bpf_helper_changes_pkt_data(insn->imm)) mark_subprog_changes_pkt_data(env, t); + if (insn->imm == BPF_FUNC_tail_call) + visit_tailcall_insn(env, t); } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { struct bpf_kfunc_call_arg_meta meta; ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: properly verify tail call behavior 2025-11-10 20:28 ` Eduard Zingerman @ 2025-11-10 23:39 ` Eduard Zingerman 2025-11-13 11:46 ` Teichmann, Martin ` (5 subsequent siblings) 6 siblings, 0 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-10 23:39 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Mon, 2025-11-10 at 12:28 -0800, Eduard Zingerman wrote: > On Mon, 2025-11-10 at 16:18 +0100, Martin Teichmann wrote: > > [...] > > > diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c > > index a7240013fd9d..54f4772d990c 100644 > > --- a/kernel/bpf/liveness.c > > +++ b/kernel/bpf/liveness.c > > @@ -500,6 +500,10 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx) > > if (opcode_info->can_jump) > > succ->items[succ->cnt++] = idx + bpf_jmp_offset(insn) + 1; > > > > + if (unlikely(insn->code == (BPF_JMP | BPF_CALL) && insn->src_reg == 0 > > + && insn->imm == BPF_FUNC_tail_call)) > > + succ->items[succ->cnt++] = idx; > > + > > return succ; > > } > > > > Hi Martin, > > This is a clever hack and I like it, but let's not do that. > It is going to be a footgun if e.g. someone would use > bpf_insn_successors() to build intra-procedural CFG. > Instead, please allocate a jt object for tail calls as in the diff > attached (on top of your patch-set). Please also extend > compute_live_registers.c to cover this logic. clear_insn_aux_data() will also need an update to avoid leaking env->insn_aux_data[...].jt for tail calls... > Other than that, I think that patch logic and tests are fine. > > Thanks, > Eduard > > [...] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: properly verify tail call behavior 2025-11-10 20:28 ` Eduard Zingerman 2025-11-10 23:39 ` Eduard Zingerman @ 2025-11-13 11:46 ` Teichmann, Martin 2025-11-13 16:09 ` Alexei Starovoitov 2025-11-18 13:39 ` [PATCH v5 bpf-next 0/4] " Martin Teichmann ` (4 subsequent siblings) 6 siblings, 1 reply; 43+ messages in thread From: Teichmann, Martin @ 2025-11-13 11:46 UTC (permalink / raw) To: Eduard Zingerman; +Cc: bpf, ast, andrii Hi Eduard, Hi List, sorry for the late response. > This is a clever hack and I like it, but let's not do that. > It is going to be a footgun if e.g. someone would use > bpf_insn_successors() to build intra-procedural CFG. I appreciate the effort you've put into your solution. However, I feel that we might be leaning towards a YAGNI (You Ain't Gonna Need It) situation here. Sure, if somebody wants to do something like your CFG, my "hack" might pose problems. But what if not? In that case, your proposed solution just leads to additional complexity and unused code. My implementation is quite concise at just three lines, making it straightforward to replace if necessary. It maintains clarity by using BPF_FUNC_tail_call markers, which can help anyone reviewing the code quickly identify its purpose. If someone is not focused on tail calls, they can easily bypass that section, ensuring minimal distractions. Additionally, I've discovered that the bug in the stack liveness code is actually independent of the bug I’m addressing. My patch doesn’t introduce this issue, so it doesn’t need to resolve it. In line with the tradition of the Linux kernel of keeping patches small, I can extract my three lines and the corresponding test, allowing us to proceed with my patch as a bug fix. You can then apply your adjustments for the stack liveness issue in your preferred manner. I hope that helps. Greetings Martin ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v4 bpf-next 1/2] bpf: properly verify tail call behavior 2025-11-13 11:46 ` Teichmann, Martin @ 2025-11-13 16:09 ` Alexei Starovoitov 0 siblings, 0 replies; 43+ messages in thread From: Alexei Starovoitov @ 2025-11-13 16:09 UTC (permalink / raw) To: Teichmann, Martin; +Cc: Eduard Zingerman, bpf, ast, andrii On Thu, Nov 13, 2025 at 3:46 AM Teichmann, Martin <martin.teichmann@xfel.eu> wrote: > > Hi Eduard, Hi List, > > sorry for the late response. > > > This is a clever hack and I like it, but let's not do that. > > It is going to be a footgun if e.g. someone would use > > bpf_insn_successors() to build intra-procedural CFG. > > I appreciate the effort you've put into your solution. However, I feel that we might be leaning towards a YAGNI (You Ain't Gonna Need It) situation here. Sure, if somebody wants to do something like your CFG, my "hack" might pose problems. But what if not? In that case, your proposed solution just leads to additional complexity and unused code. Disagree. Please do what Eduard suggested. > In line with the tradition of the Linux kernel of keeping patches small, I can extract my three lines and the corresponding test, allowing us to proceed with my patch as a bug fix. You can then apply your adjustments for the stack liveness issue in your preferred manner. that's not how it works. Please address the feedback. pw-bot: cr ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 bpf-next 0/4] bpf: properly verify tail call behavior 2025-11-10 20:28 ` Eduard Zingerman 2025-11-10 23:39 ` Eduard Zingerman 2025-11-13 11:46 ` Teichmann, Martin @ 2025-11-18 13:39 ` Martin Teichmann 2025-11-18 13:39 ` [PATCH v5 bpf-next 1/4] " Martin Teichmann ` (3 subsequent siblings) 6 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-18 13:39 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Hi List, sorry for the late response, somehow the continuous integration was down and I did not want to submit untested code... This patch set adresses the fact that tail calls may return from the calling function, which needs to be reflected in the verifier. The first patch changes the verifier such that it simulates the actual behavior of a tail call, the second patch tests the correct behavior. During the discussion of this patch, Eduard Zingerman found another bug in the stack liveness calculation, and fixed it. This is the third patch, tested by the fourth. Both patches are written by Eduard, the only thing I changed was to assure it properly applies on top of my patches, and a small addition that Eduard asked for. He asked to add his patches on top of mine, yet all credits for these patches should go to him, I do not intent to steal code here. Eduard also asked for a test for the register liveness in compute_live_registers.c. I thought long about it, but unfortunately found no way to properly test the new behavior, as I could not come up with an example where it would be different from the original one. Cheers Martin Martin Teichmann (4): bpf: properly verify tail call behavior bpf: test the proper verification of tail calls bpf: correct stack liveness for tail calls bpf: test the correct stack liveness of tail calls include/linux/bpf_verifier.h | 5 +- kernel/bpf/liveness.c | 7 ++- kernel/bpf/verifier.c | 60 +++++++++++++++++-- .../selftests/bpf/progs/verifier_live_stack.c | 50 ++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 +++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 +++++++++++++++ 6 files changed, 196 insertions(+), 12 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 bpf-next 1/4] bpf: properly verify tail call behavior 2025-11-10 20:28 ` Eduard Zingerman ` (2 preceding siblings ...) 2025-11-18 13:39 ` [PATCH v5 bpf-next 0/4] " Martin Teichmann @ 2025-11-18 13:39 ` Martin Teichmann 2025-11-18 19:34 ` Eduard Zingerman 2025-11-18 13:39 ` [PATCH v5 bpf-next 2/4] bpf: test the proper verification " Martin Teichmann ` (2 subsequent siblings) 6 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-18 13:39 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann A successful ebpf tail call does not return to the caller, but to the caller-of-the-caller, often just finishing the ebpf program altogether. Any restrictions that the verifier needs to take into account - notably the fact that the tail call might have modified packet pointers - are to be checked on the caller-of-the-caller. Checking it on the caller made the verifier refuse perfectly fine programs that would use the packet pointers after a tail call, which is no problem as this code is only executed if the tail call was unsuccessful, i.e. nothing happened. This patch simulates the behavior of a tail call in the verifier. A conditional jump to the code after the tail call is added for the case of an unsucessful tail call, and a return to the caller is simulated for a successful tail call. For the successful case we assume that the tail call returns an int, as tail calls are currently only allowed in functions that return and int. We always assume that the tail call modified the packet pointers, as we do not know what the tail call did. For the unsuccessful case we know nothing happened, so we do not need to add new constraints. This approach also allows to check other problems that may occur with tail calls, namely we are now able to check that precision is properly propagated into subprograms using tail calls, as well as checking the live slots in such a subprogram. Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- kernel/bpf/verifier.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 098dd7f21c89..117a2b1cf87c 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4438,6 +4438,11 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, bt_reg_mask(bt)); return -EFAULT; } + if (insn->src_reg == BPF_REG_0 && insn->imm == BPF_FUNC_tail_call + && subseq_idx - idx != 1) { + if (bt_subprog_enter(bt)) + return -EFAULT; + } } else if (opcode == BPF_EXIT) { bool r0_precise; @@ -11064,6 +11069,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) bool in_callback_fn; int err; + err = bpf_update_live_stack(env); + if (err) + return err; + callee = state->frame[state->curframe]; r0 = &callee->regs[BPF_REG_0]; if (r0->type == PTR_TO_STACK) { @@ -11970,6 +11979,25 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn env->prog->call_get_func_ip = true; } + if (func_id == BPF_FUNC_tail_call) { + if (env->cur_state->curframe) { + struct bpf_verifier_state *branch; + + mark_reg_scratched(env, BPF_REG_0); + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); + if (IS_ERR(branch)) + return PTR_ERR(branch); + clear_all_pkt_pointers(env); + mark_reg_unknown(env, regs, BPF_REG_0); + err = prepare_func_exit(env, &env->insn_idx); + if (err) + return err; + env->insn_idx--; + } else { + changes_data = false; + } + } + if (changes_data) clear_all_pkt_pointers(env); return 0; @@ -20140,9 +20168,6 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, return PROCESS_BPF_EXIT; if (env->cur_state->curframe) { - err = bpf_update_live_stack(env); - if (err) - return err; /* exit from nested function */ err = prepare_func_exit(env, &env->insn_idx); if (err) -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v5 bpf-next 1/4] bpf: properly verify tail call behavior 2025-11-18 13:39 ` [PATCH v5 bpf-next 1/4] " Martin Teichmann @ 2025-11-18 19:34 ` Eduard Zingerman 2025-11-19 16:03 ` [PATCH v6 bpf-next 0/4] " Martin Teichmann ` (4 more replies) 0 siblings, 5 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-18 19:34 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Tue, 2025-11-18 at 14:39 +0100, Martin Teichmann wrote: > A successful ebpf tail call does not return to the caller, but to the > caller-of-the-caller, often just finishing the ebpf program altogether. > > Any restrictions that the verifier needs to take into account - notably > the fact that the tail call might have modified packet pointers - are to > be checked on the caller-of-the-caller. Checking it on the caller made > the verifier refuse perfectly fine programs that would use the packet > pointers after a tail call, which is no problem as this code is only > executed if the tail call was unsuccessful, i.e. nothing happened. > > This patch simulates the behavior of a tail call in the verifier. A > conditional jump to the code after the tail call is added for the case > of an unsucessful tail call, and a return to the caller is simulated for > a successful tail call. > > For the successful case we assume that the tail call returns an int, > as tail calls are currently only allowed in functions that return and > int. We always assume that the tail call modified the packet pointers, > as we do not know what the tail call did. > > For the unsuccessful case we know nothing happened, so we do not need to > add new constraints. > > This approach also allows to check other problems that may occur with > tail calls, namely we are now able to check that precision is properly > propagated into subprograms using tail calls, as well as checking the > live slots in such a subprogram. > > Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") > Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ > Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> > --- Acked-by: Eduard Zingerman <eddyz87@gmail.com> [...] > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 098dd7f21c89..117a2b1cf87c 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c [...] > @@ -11970,6 +11979,25 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn > env->prog->call_get_func_ip = true; > } > > + if (func_id == BPF_FUNC_tail_call) { [...] > + if (env->cur_state->curframe) { > + struct bpf_verifier_state *branch; > + > + mark_reg_scratched(env, BPF_REG_0); > + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); > + if (IS_ERR(branch)) > + return PTR_ERR(branch); > + clear_all_pkt_pointers(env); > + mark_reg_unknown(env, regs, BPF_REG_0); > + err = prepare_func_exit(env, &env->insn_idx); > + if (err) > + return err; > + env->insn_idx--; This insn_idx adjustment is a bit unfortunate, but refactoring getting rid of it is probably out of scope for this series. ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v6 bpf-next 0/4] bpf: properly verify tail call behavior 2025-11-18 19:34 ` Eduard Zingerman @ 2025-11-19 16:03 ` Martin Teichmann 2025-11-19 16:03 ` [PATCH v6 bpf-next 1/4] " Martin Teichmann ` (3 subsequent siblings) 4 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-19 16:03 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Hi Eduard, Hi Alexei, Hi List, thanks for the good feedback. Some details: Eduard wrote: > > + env->insn_idx--; > > This insn_idx adjustment is a bit unfortunate, but refactoring getting > rid of it is probably out of scope for this series. Indeed. I tried hard to avoid it but couldn't find a way that would not have let this patch explode. Similarly, check_helper_call by now ends with a lot of if's checking func_id. This should at some point probably be replaced by a switch statement. Eduard also wrote: > > +__msg("mark_precise: frame0: parent state regs=r0 stack=: R0=Pscalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") > > Nit: I'd add a couple more lines to this __msg sequence to check that > backtrack_insn correctly moved one frame down. I added some more __msg lines where it enters the subprogram, that's where the patch changes behavior. Cheers Martin Eduard Zingerman (2): bpf: correct stack liveness for tail calls bpf: test the correct stack liveness of tail calls Martin Teichmann (2): bpf: properly verify tail call behavior bpf: test the proper verification of tail calls include/linux/bpf_verifier.h | 5 +- kernel/bpf/liveness.c | 7 ++- kernel/bpf/verifier.c | 60 +++++++++++++++++-- .../selftests/bpf/progs/verifier_live_stack.c | 50 ++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 +++++++++++- .../bpf/progs/verifier_subprog_precision.c | 53 ++++++++++++++++ 6 files changed, 202 insertions(+), 12 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v6 bpf-next 1/4] bpf: properly verify tail call behavior 2025-11-18 19:34 ` Eduard Zingerman 2025-11-19 16:03 ` [PATCH v6 bpf-next 0/4] " Martin Teichmann @ 2025-11-19 16:03 ` Martin Teichmann 2025-11-22 2:00 ` patchwork-bot+netdevbpf 2025-11-19 16:03 ` [PATCH v6 bpf-next 2/4] bpf: test the proper verification of tail calls Martin Teichmann ` (2 subsequent siblings) 4 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-19 16:03 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann A successful ebpf tail call does not return to the caller, but to the caller-of-the-caller, often just finishing the ebpf program altogether. Any restrictions that the verifier needs to take into account - notably the fact that the tail call might have modified packet pointers - are to be checked on the caller-of-the-caller. Checking it on the caller made the verifier refuse perfectly fine programs that would use the packet pointers after a tail call, which is no problem as this code is only executed if the tail call was unsuccessful, i.e. nothing happened. This patch simulates the behavior of a tail call in the verifier. A conditional jump to the code after the tail call is added for the case of an unsucessful tail call, and a return to the caller is simulated for a successful tail call. For the successful case we assume that the tail call returns an int, as tail calls are currently only allowed in functions that return and int. We always assume that the tail call modified the packet pointers, as we do not know what the tail call did. For the unsuccessful case we know nothing happened, so we do not need to add new constraints. This approach also allows to check other problems that may occur with tail calls, namely we are now able to check that precision is properly propagated into subprograms using tail calls, as well as checking the live slots in such a subprogram. Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> Acked-by: Eduard Zingerman <eddyz87@gmail.com> --- kernel/bpf/verifier.c | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 098dd7f21c89..117a2b1cf87c 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4438,6 +4438,11 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx, bt_reg_mask(bt)); return -EFAULT; } + if (insn->src_reg == BPF_REG_0 && insn->imm == BPF_FUNC_tail_call + && subseq_idx - idx != 1) { + if (bt_subprog_enter(bt)) + return -EFAULT; + } } else if (opcode == BPF_EXIT) { bool r0_precise; @@ -11064,6 +11069,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) bool in_callback_fn; int err; + err = bpf_update_live_stack(env); + if (err) + return err; + callee = state->frame[state->curframe]; r0 = &callee->regs[BPF_REG_0]; if (r0->type == PTR_TO_STACK) { @@ -11970,6 +11979,25 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn env->prog->call_get_func_ip = true; } + if (func_id == BPF_FUNC_tail_call) { + if (env->cur_state->curframe) { + struct bpf_verifier_state *branch; + + mark_reg_scratched(env, BPF_REG_0); + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); + if (IS_ERR(branch)) + return PTR_ERR(branch); + clear_all_pkt_pointers(env); + mark_reg_unknown(env, regs, BPF_REG_0); + err = prepare_func_exit(env, &env->insn_idx); + if (err) + return err; + env->insn_idx--; + } else { + changes_data = false; + } + } + if (changes_data) clear_all_pkt_pointers(env); return 0; @@ -20140,9 +20168,6 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, return PROCESS_BPF_EXIT; if (env->cur_state->curframe) { - err = bpf_update_live_stack(env); - if (err) - return err; /* exit from nested function */ err = prepare_func_exit(env, &env->insn_idx); if (err) -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v6 bpf-next 1/4] bpf: properly verify tail call behavior 2025-11-19 16:03 ` [PATCH v6 bpf-next 1/4] " Martin Teichmann @ 2025-11-22 2:00 ` patchwork-bot+netdevbpf 0 siblings, 0 replies; 43+ messages in thread From: patchwork-bot+netdevbpf @ 2025-11-22 2:00 UTC (permalink / raw) To: Martin Teichmann; +Cc: bpf, eddyz87, ast, andrii Hello: This series was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Wed, 19 Nov 2025 17:03:52 +0100 you wrote: > A successful ebpf tail call does not return to the caller, but to the > caller-of-the-caller, often just finishing the ebpf program altogether. > > Any restrictions that the verifier needs to take into account - notably > the fact that the tail call might have modified packet pointers - are to > be checked on the caller-of-the-caller. Checking it on the caller made > the verifier refuse perfectly fine programs that would use the packet > pointers after a tail call, which is no problem as this code is only > executed if the tail call was unsuccessful, i.e. nothing happened. > > [...] Here is the summary with links: - [v6,bpf-next,1/4] bpf: properly verify tail call behavior https://git.kernel.org/bpf/bpf-next/c/e3245f899043 - [v6,bpf-next,2/4] bpf: test the proper verification of tail calls https://git.kernel.org/bpf/bpf-next/c/978da762ea45 - [v6,bpf-next,3/4] bpf: correct stack liveness for tail calls https://git.kernel.org/bpf/bpf-next/c/e40f5a6bf88a - [v6,bpf-next,4/4] bpf: test the correct stack liveness of tail calls https://git.kernel.org/bpf/bpf-next/c/8f7cf305a15e You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v6 bpf-next 2/4] bpf: test the proper verification of tail calls 2025-11-18 19:34 ` Eduard Zingerman 2025-11-19 16:03 ` [PATCH v6 bpf-next 0/4] " Martin Teichmann 2025-11-19 16:03 ` [PATCH v6 bpf-next 1/4] " Martin Teichmann @ 2025-11-19 16:03 ` Martin Teichmann 2025-11-19 16:03 ` [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann 2025-11-19 16:03 ` [PATCH v6 bpf-next 4/4] bpf: test the correct stack liveness of " Martin Teichmann 4 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-19 16:03 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Three tests are added: - invalidate_pkt_pointers_by_tail_call checks that one can use the packet pointer after a tail call. This was originally possible and also poses not problems, but was made impossible by 1a4607ffba35. - invalidate_pkt_pointers_by_static_tail_call tests a corner case found by Eduard Zingerman during the discussion of the original fix, which was broken in that fix. - subprog_result_tail_call tests that precision propagation works correctly across tail calls. This did not work before. Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> Acked-by: Eduard Zingerman <eddyz87@gmail.com> --- .../selftests/bpf/progs/verifier_sock.c | 39 +++++++++++++- .../bpf/progs/verifier_subprog_precision.c | 53 +++++++++++++++++++ 2 files changed, 90 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c index 2b4610b53382..a2132c72d3b8 100644 --- a/tools/testing/selftests/bpf/progs/verifier_sock.c +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c @@ -1117,10 +1117,17 @@ int tail_call(struct __sk_buff *sk) return 0; } -/* Tail calls invalidate packet pointers. */ +static __noinline +int static_tail_call(struct __sk_buff *sk) +{ + bpf_tail_call_static(sk, &jmp_table, 0); + return 0; +} + +/* Tail calls in sub-programs invalidate packet pointers. */ SEC("tc") __failure __msg("invalid mem access") -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +int invalidate_pkt_pointers_by_global_tail_call(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; @@ -1131,4 +1138,32 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) return TCX_PASS; } +/* Tail calls in static sub-programs invalidate packet pointers. */ +SEC("tc") +__failure __msg("invalid mem access") +int invalidate_pkt_pointers_by_static_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + static_tail_call(sk); + *p = 42; /* this is unsafe */ + return TCX_PASS; +} + +/* Direct tail calls do not invalidate packet pointers. */ +SEC("tc") +__success +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + bpf_tail_call_static(sk, &jmp_table, 0); + *p = 42; /* this is NOT unsafe: tail calls don't return */ + return TCX_PASS; +} + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c index ac3e418c2a96..61886ed554de 100644 --- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c +++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c @@ -793,4 +793,57 @@ __naked int stack_slot_aliases_precision(void) ); } +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +__naked __noinline __used +static unsigned long identity_tail_call(void) +{ + /* the simplest identity function involving a tail call */ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "r0 = r6;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} + +SEC("?raw_tp") +__failure __log_level(2) +__msg("13: (85) call bpf_tail_call#12") +__msg("mark_precise: frame1: last_idx 13 first_idx 0 subseq_idx -1 ") +__msg("returning from callee:") +__msg("frame1: R0=scalar() R6=3 R10=fp0") +__msg("to caller at 4:") +__msg("R0=scalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") +__msg("6: (0f) r1 += r0") +__msg("mark_precise: frame0: regs=r0 stack= before 5: (bf) r1 = r6") +__msg("mark_precise: frame0: regs=r0 stack= before 4: (27) r0 *= 4") +__msg("mark_precise: frame0: parent state regs=r0 stack=: R0=Pscalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") +__msg("math between map_value pointer and register with unbounded min value is not allowed") +__naked int subprog_result_tail_call(void) +{ + asm volatile ( + "r2 = 3;" + "call identity_tail_call;" + "r0 *= 4;" + "r1 = %[vals];" + "r1 += r0;" + "r0 = *(u32 *)(r1 + 0);" + "exit;" + : + : __imm_ptr(vals) + : __clobber_common + ); +} + char _license[] SEC("license") = "GPL"; -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for tail calls 2025-11-18 19:34 ` Eduard Zingerman ` (2 preceding siblings ...) 2025-11-19 16:03 ` [PATCH v6 bpf-next 2/4] bpf: test the proper verification of tail calls Martin Teichmann @ 2025-11-19 16:03 ` Martin Teichmann 2025-11-19 16:33 ` bot+bpf-ci 2025-12-12 2:06 ` Chris Mason 2025-11-19 16:03 ` [PATCH v6 bpf-next 4/4] bpf: test the correct stack liveness of " Martin Teichmann 4 siblings, 2 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-19 16:03 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann From: Eduard Zingerman <eddyz87@gmail.com> This updates bpf_insn_successors() reflecting that control flow might jump over the instructions between tail call and function exit, verifier might assume that some writes to parent stack always happen, which is not the case. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- include/linux/bpf_verifier.h | 5 +++-- kernel/bpf/liveness.c | 7 ++++--- kernel/bpf/verifier.c | 29 +++++++++++++++++++++++++++-- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 5441341f1ab9..8d0b60fa5f2b 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -527,7 +527,6 @@ struct bpf_insn_aux_data { struct { u32 map_index; /* index into used_maps[] */ u32 map_off; /* offset from value base address */ - struct bpf_iarray *jt; /* jump table for gotox instruction */ }; struct { enum bpf_reg_type reg_type; /* type of pseudo_btf_id */ @@ -550,6 +549,7 @@ struct bpf_insn_aux_data { /* remember the offset of node field within type to rewrite */ u64 insert_off; }; + struct bpf_iarray *jt; /* jump table for gotox or bpf_tailcall call instruction */ struct btf_struct_meta *kptr_struct_meta; u64 map_key_state; /* constant (32 bit) key tracking for maps */ int ctx_field_size; /* the ctx field size for load insn, maybe 0 */ @@ -652,6 +652,7 @@ struct bpf_subprog_info { u32 start; /* insn idx of function entry point */ u32 linfo_idx; /* The idx to the main_prog->aux->linfo */ u32 postorder_start; /* The idx to the env->cfg.insn_postorder */ + u32 exit_idx; /* Index of one of the BPF_EXIT instructions in this subprogram */ u16 stack_depth; /* max. stack depth used by this function */ u16 stack_extra; /* offsets in range [stack_depth .. fastcall_stack_off) @@ -669,9 +670,9 @@ struct bpf_subprog_info { bool keep_fastcall_stack: 1; bool changes_pkt_data: 1; bool might_sleep: 1; + u8 arg_cnt:3; enum priv_stack_mode priv_stack_mode; - u8 arg_cnt; struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS]; }; diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index a7240013fd9d..60db5d655495 100644 --- a/kernel/bpf/liveness.c +++ b/kernel/bpf/liveness.c @@ -482,11 +482,12 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx) struct bpf_prog *prog = env->prog; struct bpf_insn *insn = &prog->insnsi[idx]; const struct opcode_info *opcode_info; - struct bpf_iarray *succ; + struct bpf_iarray *succ, *jt; int insn_sz; - if (unlikely(insn_is_gotox(insn))) - return env->insn_aux_data[idx].jt; + jt = env->insn_aux_data[idx].jt; + if (unlikely(jt)) + return jt; /* pre-allocated array of size up to 2; reset cnt, as it may have been used already */ succ = env->succ; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 117a2b1cf87c..f564150ec807 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3555,8 +3555,12 @@ static int check_subprogs(struct bpf_verifier_env *env) subprog[cur_subprog].has_ld_abs = true; if (BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32) goto next; - if (BPF_OP(code) == BPF_EXIT || BPF_OP(code) == BPF_CALL) + if (BPF_OP(code) == BPF_CALL) goto next; + if (BPF_OP(code) == BPF_EXIT) { + subprog[cur_subprog].exit_idx = i; + goto next; + } off = i + bpf_jmp_offset(&insn[i]) + 1; if (off < subprog_start || off >= subprog_end) { verbose(env, "jump out of range from insn %d to %d\n", i, off); @@ -18150,6 +18154,25 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env) return keep_exploring ? KEEP_EXPLORING : DONE_EXPLORING; } +static int visit_tailcall_insn(struct bpf_verifier_env *env, int t) +{ + static struct bpf_subprog_info *subprog; + struct bpf_iarray *jt; + + if (env->insn_aux_data[t].jt) + return 0; + + jt = iarray_realloc(NULL, 2); + if (!jt) + return -ENOMEM; + + subprog = bpf_find_containing_subprog(env, t); + jt->items[0] = t + 1; + jt->items[1] = subprog->exit_idx; + env->insn_aux_data[t].jt = jt; + return 0; +} + /* Visits the instruction at index t and returns one of the following: * < 0 - an error occurred * DONE_EXPLORING - the instruction was fully explored @@ -18210,6 +18233,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env) mark_subprog_might_sleep(env, t); if (bpf_helper_changes_pkt_data(insn->imm)) mark_subprog_changes_pkt_data(env, t); + if (insn->imm == BPF_FUNC_tail_call) + visit_tailcall_insn(env, t); } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { struct bpf_kfunc_call_arg_meta meta; @@ -21471,7 +21496,7 @@ static void clear_insn_aux_data(struct bpf_verifier_env *env, int start, int len int i; for (i = start; i < end; i++) { - if (insn_is_gotox(&insns[i])) { + if (aux_data[i].jt) { kvfree(aux_data[i].jt); aux_data[i].jt = NULL; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for tail calls 2025-11-19 16:03 ` [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann @ 2025-11-19 16:33 ` bot+bpf-ci 2025-12-12 2:06 ` Chris Mason 1 sibling, 0 replies; 43+ messages in thread From: bot+bpf-ci @ 2025-11-19 16:33 UTC (permalink / raw) To: martin.teichmann, bpf Cc: eddyz87, ast, andrii, martin.teichmann, ast, andrii, daniel, martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai [-- Attachment #1: Type: text/plain, Size: 2547 bytes --] > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h > index 5441341f1..8d0b60fa5 100644 > --- a/include/linux/bpf_verifier.h > +++ b/include/linux/bpf_verifier.h [ ... ] > @@ -652,6 +652,7 @@ struct bpf_subprog_info { > u32 start; /* insn idx of function entry point */ > u32 linfo_idx; /* The idx to the main_prog->aux->linfo */ > u32 postorder_start; /* The idx to the env->cfg.insn_postorder */ > + u32 exit_idx; /* Index of one of the BPF_EXIT instructions in this subprogram */ > u16 stack_depth; /* max. stack depth used by this function */ [ ... ] > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 117a2b1cf..f564150ec 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3555,8 +3555,12 @@ static int check_subprogs(struct bpf_verifier_env *env) > subprog[cur_subprog].has_ld_abs = true; > if (BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32) > goto next; > - if (BPF_OP(code) == BPF_EXIT || BPF_OP(code) == BPF_CALL) > + if (BPF_OP(code) == BPF_CALL) > goto next; > + if (BPF_OP(code) == BPF_EXIT) { > + subprog[cur_subprog].exit_idx = i; > + goto next; > + } [ ... ] > +static int visit_tailcall_insn(struct bpf_verifier_env *env, int t) > +{ > + static struct bpf_subprog_info *subprog; > + struct bpf_iarray *jt; > + > + if (env->insn_aux_data[t].jt) > + return 0; > + > + jt = iarray_realloc(NULL, 2); > + if (!jt) > + return -ENOMEM; > + > + subprog = bpf_find_containing_subprog(env, t); > + jt->items[0] = t + 1; > + jt->items[1] = subprog->exit_idx; ^^^^^^^^^^^^^^^^^^ Can a subprogram with a tail_call have no BPF_EXIT instruction? The verifier allows subprograms to end with an unconditional jump instead of BPF_EXIT (see check_subprogs() which accepts BPF_JA as the last instruction). In check_subprogs(), exit_idx is only set when a BPF_EXIT is encountered. For subprograms ending with only jumps (like infinite loops with tail_calls), exit_idx remains 0 from the kvzalloc() in bpf_check(). If visit_tailcall_insn() uses exit_idx when it's 0, the jump table will incorrectly point to instruction 0 instead of the actual exit, potentially causing incorrect liveness analysis for stack slots. > + env->insn_aux_data[t].jt = jt; > + return 0; > +} --- AI reviewed your patch. Please fix the bug or email reply why it's not a bug. See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19508226347 ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for tail calls 2025-11-19 16:03 ` [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann 2025-11-19 16:33 ` bot+bpf-ci @ 2025-12-12 2:06 ` Chris Mason 1 sibling, 0 replies; 43+ messages in thread From: Chris Mason @ 2025-12-12 2:06 UTC (permalink / raw) To: Martin Teichmann; +Cc: Chris Mason, bpf, eddyz87, ast, andrii On Wed, 19 Nov 2025 17:03:54 +0100 Martin Teichmann <martin.teichmann@xfel.eu> wrote: > From: Eduard Zingerman <eddyz87@gmail.com> > > This updates bpf_insn_successors() reflecting that control flow might > jump over the instructions between tail call and function exit, verifier > might assume that some writes to parent stack always happen, which is > not the case. > Hi everyone, I ran recent upstream commits through claude and it flagged this one. Looks like a different comment than it had made earlier: > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 9426367fc9113..0828718a8ba71 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c [ ... ] > @@ -18156,6 +18160,25 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env) > return keep_exploring ? KEEP_EXPLORING : DONE_EXPLORING; > } > > +static int visit_tailcall_insn(struct bpf_verifier_env *env, int t) > +{ > + static struct bpf_subprog_info *subprog; > + struct bpf_iarray *jt; > + > + if (env->insn_aux_data[t].jt) > + return 0; > + > + jt = iarray_realloc(NULL, 2); > + if (!jt) > + return -ENOMEM; > + > + subprog = bpf_find_containing_subprog(env, t); > + jt->items[0] = t + 1; > + jt->items[1] = subprog->exit_idx; > + env->insn_aux_data[t].jt = jt; > + return 0; > +} > + [ ... ] > @@ -18216,6 +18239,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env) > mark_subprog_might_sleep(env, t); > if (bpf_helper_changes_pkt_data(insn->imm)) > mark_subprog_changes_pkt_data(env, t); > + if (insn->imm == BPF_FUNC_tail_call) > + visit_tailcall_insn(env, t); ^^^^^^^^^^^^^^^^^^^^^^^ The return value of visit_tailcall_insn() is not checked here. If iarray_realloc() fails and returns -ENOMEM, the error is silently discarded. This is inconsistent with visit_gotox_insn() which properly returns its value. Under memory pressure, the jt allocation fails silently, leaving env->insn_aux_data[t].jt as NULL. Later, bpf_insn_successors() will not return the expected successors for the tail call, and the stack liveness fix this patch implements would not take effect. Should this be something like: if (insn->imm == BPF_FUNC_tail_call) { ret = visit_tailcall_insn(env, t); if (ret < 0) return ret; } > + } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { [ ... ] ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v6 bpf-next 4/4] bpf: test the correct stack liveness of tail calls 2025-11-18 19:34 ` Eduard Zingerman ` (3 preceding siblings ...) 2025-11-19 16:03 ` [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann @ 2025-11-19 16:03 ` Martin Teichmann 4 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-19 16:03 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann From: Eduard Zingerman <eddyz87@gmail.com> A new test is added: caller_stack_write_tail_call tests that the live stack is correctly tracked for a tail call. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- .../selftests/bpf/progs/verifier_live_stack.c | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/verifier_live_stack.c b/tools/testing/selftests/bpf/progs/verifier_live_stack.c index c0e808509268..2de105057bbc 100644 --- a/tools/testing/selftests/bpf/progs/verifier_live_stack.c +++ b/tools/testing/selftests/bpf/progs/verifier_live_stack.c @@ -292,3 +292,53 @@ __naked void syzbot_postorder_bug1(void) "exit;" ::: __clobber_all); } + +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +SEC("socket") +__failure __msg("invalid read from stack R2 off=-1024 size=8") +__flag(BPF_F_TEST_STATE_FREQ) +__naked unsigned long caller_stack_write_tail_call(void) +{ + asm volatile ( + "r6 = r1;" + "*(u64 *)(r10 - 8) = -8;" + "call %[bpf_get_prandom_u32];" + "if r0 != 42 goto 1f;" + "goto 2f;" + "1:" + "*(u64 *)(r10 - 8) = -1024;" + "2:" + "r1 = r6;" + "r2 = r10;" + "r2 += -8;" + "call write_tail_call;" + "r1 = *(u64 *)(r10 - 8);" + "r2 = r10;" + "r2 += r1;" + "r0 = *(u64 *)(r2 + 0);" + "exit;" + :: __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +static __used __naked unsigned long write_tail_call(void) +{ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "*(u64 *)(r6 + 0) = -16;" + "r0 = 0;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v5 bpf-next 2/4] bpf: test the proper verification of tail calls 2025-11-10 20:28 ` Eduard Zingerman ` (3 preceding siblings ...) 2025-11-18 13:39 ` [PATCH v5 bpf-next 1/4] " Martin Teichmann @ 2025-11-18 13:39 ` Martin Teichmann 2025-11-18 22:47 ` Eduard Zingerman 2025-11-18 13:39 ` [PATCH v5 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann 2025-11-18 13:39 ` [PATCH v5 bpf-next 4/4] bpf: test the correct stack liveness of " Martin Teichmann 6 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-18 13:39 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Three tests are added: - invalidate_pkt_pointers_by_tail_call checks that one can use the packet pointer after a tail call. This was originally possible and also poses not problems, but was made impossible by 1a4607ffba35. - invalidate_pkt_pointers_by_static_tail_call tests a corner case found by Eduard Zingerman during the discussion of the original fix, which was broken in that fix. - subprog_result_tail_call tests that precision propagation works correctly across tail calls. This did not work before. Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 +++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c index 2b4610b53382..a2132c72d3b8 100644 --- a/tools/testing/selftests/bpf/progs/verifier_sock.c +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c @@ -1117,10 +1117,17 @@ int tail_call(struct __sk_buff *sk) return 0; } -/* Tail calls invalidate packet pointers. */ +static __noinline +int static_tail_call(struct __sk_buff *sk) +{ + bpf_tail_call_static(sk, &jmp_table, 0); + return 0; +} + +/* Tail calls in sub-programs invalidate packet pointers. */ SEC("tc") __failure __msg("invalid mem access") -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +int invalidate_pkt_pointers_by_global_tail_call(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; @@ -1131,4 +1138,32 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) return TCX_PASS; } +/* Tail calls in static sub-programs invalidate packet pointers. */ +SEC("tc") +__failure __msg("invalid mem access") +int invalidate_pkt_pointers_by_static_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + static_tail_call(sk); + *p = 42; /* this is unsafe */ + return TCX_PASS; +} + +/* Direct tail calls do not invalidate packet pointers. */ +SEC("tc") +__success +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + bpf_tail_call_static(sk, &jmp_table, 0); + *p = 42; /* this is NOT unsafe: tail calls don't return */ + return TCX_PASS; +} + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c index ac3e418c2a96..de5ef3152567 100644 --- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c +++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c @@ -793,4 +793,51 @@ __naked int stack_slot_aliases_precision(void) ); } +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +__naked __noinline __used +static unsigned long identity_tail_call(void) +{ + /* the simplest identity function involving a tail call */ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "r0 = r6;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} + +SEC("?raw_tp") +__failure __log_level(2) +__msg("6: (0f) r1 += r0") +__msg("mark_precise: frame0: regs=r0 stack= before 5: (bf) r1 = r6") +__msg("mark_precise: frame0: regs=r0 stack= before 4: (27) r0 *= 4") +__msg("mark_precise: frame0: parent state regs=r0 stack=: R0=Pscalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") +__msg("math between map_value pointer and register with unbounded min value is not allowed") +__naked int subprog_result_tail_call(void) +{ + asm volatile ( + "r2 = 3;" + "call identity_tail_call;" + "r0 *= 4;" + "r1 = %[vals];" + "r1 += r0;" + "r0 = *(u32 *)(r1 + 0);" + "exit;" + : + : __imm_ptr(vals) + : __clobber_common + ); +} + char _license[] SEC("license") = "GPL"; -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v5 bpf-next 2/4] bpf: test the proper verification of tail calls 2025-11-18 13:39 ` [PATCH v5 bpf-next 2/4] bpf: test the proper verification " Martin Teichmann @ 2025-11-18 22:47 ` Eduard Zingerman 0 siblings, 0 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-18 22:47 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Tue, 2025-11-18 at 14:39 +0100, Martin Teichmann wrote: > Three tests are added: > > - invalidate_pkt_pointers_by_tail_call checks that one can use the > packet pointer after a tail call. This was originally possible > and also poses not problems, but was made impossible by 1a4607ffba35. > > - invalidate_pkt_pointers_by_static_tail_call tests a corner case > found by Eduard Zingerman during the discussion of the original fix, > which was broken in that fix. > > - subprog_result_tail_call tests that precision propagation works > correctly across tail calls. This did not work before. > > Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> > --- Acked-by: Eduard Zingerman <eddyz87@gmail.com> [...] > diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c > index ac3e418c2a96..de5ef3152567 100644 > --- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c > +++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c > @@ -793,4 +793,51 @@ __naked int stack_slot_aliases_precision(void) > ); > } [...] > +SEC("?raw_tp") > +__failure __log_level(2) > +__msg("6: (0f) r1 += r0") > +__msg("mark_precise: frame0: regs=r0 stack= before 5: (bf) r1 = r6") > +__msg("mark_precise: frame0: regs=r0 stack= before 4: (27) r0 *= 4") > +__msg("mark_precise: frame0: parent state regs=r0 stack=: R0=Pscalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") Nit: I'd add a couple more lines to this __msg sequence to check that backtrack_insn correctly moved one frame down. > +__msg("math between map_value pointer and register with unbounded min value is not allowed") > +__naked int subprog_result_tail_call(void) > +{ > + asm volatile ( > + "r2 = 3;" > + "call identity_tail_call;" > + "r0 *= 4;" > + "r1 = %[vals];" > + "r1 += r0;" > + "r0 = *(u32 *)(r1 + 0);" > + "exit;" > + : > + : __imm_ptr(vals) > + : __clobber_common > + ); > +} > + > char _license[] SEC("license") = "GPL"; ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 bpf-next 3/4] bpf: correct stack liveness for tail calls 2025-11-10 20:28 ` Eduard Zingerman ` (4 preceding siblings ...) 2025-11-18 13:39 ` [PATCH v5 bpf-next 2/4] bpf: test the proper verification " Martin Teichmann @ 2025-11-18 13:39 ` Martin Teichmann 2025-11-18 22:54 ` Eduard Zingerman 2025-11-18 13:39 ` [PATCH v5 bpf-next 4/4] bpf: test the correct stack liveness of " Martin Teichmann 6 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-18 13:39 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann This updates bpf_insn_successors() reflecting that control flow might jump over the instructions between tail call and function exit, verifier might assume that some writes to parent stack always happen, which is not the case. --- include/linux/bpf_verifier.h | 5 +++-- kernel/bpf/liveness.c | 7 ++++--- kernel/bpf/verifier.c | 29 +++++++++++++++++++++++++++-- 3 files changed, 34 insertions(+), 7 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 5441341f1ab9..8d0b60fa5f2b 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -527,7 +527,6 @@ struct bpf_insn_aux_data { struct { u32 map_index; /* index into used_maps[] */ u32 map_off; /* offset from value base address */ - struct bpf_iarray *jt; /* jump table for gotox instruction */ }; struct { enum bpf_reg_type reg_type; /* type of pseudo_btf_id */ @@ -550,6 +549,7 @@ struct bpf_insn_aux_data { /* remember the offset of node field within type to rewrite */ u64 insert_off; }; + struct bpf_iarray *jt; /* jump table for gotox or bpf_tailcall call instruction */ struct btf_struct_meta *kptr_struct_meta; u64 map_key_state; /* constant (32 bit) key tracking for maps */ int ctx_field_size; /* the ctx field size for load insn, maybe 0 */ @@ -652,6 +652,7 @@ struct bpf_subprog_info { u32 start; /* insn idx of function entry point */ u32 linfo_idx; /* The idx to the main_prog->aux->linfo */ u32 postorder_start; /* The idx to the env->cfg.insn_postorder */ + u32 exit_idx; /* Index of one of the BPF_EXIT instructions in this subprogram */ u16 stack_depth; /* max. stack depth used by this function */ u16 stack_extra; /* offsets in range [stack_depth .. fastcall_stack_off) @@ -669,9 +670,9 @@ struct bpf_subprog_info { bool keep_fastcall_stack: 1; bool changes_pkt_data: 1; bool might_sleep: 1; + u8 arg_cnt:3; enum priv_stack_mode priv_stack_mode; - u8 arg_cnt; struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS]; }; diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c index a7240013fd9d..60db5d655495 100644 --- a/kernel/bpf/liveness.c +++ b/kernel/bpf/liveness.c @@ -482,11 +482,12 @@ bpf_insn_successors(struct bpf_verifier_env *env, u32 idx) struct bpf_prog *prog = env->prog; struct bpf_insn *insn = &prog->insnsi[idx]; const struct opcode_info *opcode_info; - struct bpf_iarray *succ; + struct bpf_iarray *succ, *jt; int insn_sz; - if (unlikely(insn_is_gotox(insn))) - return env->insn_aux_data[idx].jt; + jt = env->insn_aux_data[idx].jt; + if (unlikely(jt)) + return jt; /* pre-allocated array of size up to 2; reset cnt, as it may have been used already */ succ = env->succ; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 117a2b1cf87c..f564150ec807 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3555,8 +3555,12 @@ static int check_subprogs(struct bpf_verifier_env *env) subprog[cur_subprog].has_ld_abs = true; if (BPF_CLASS(code) != BPF_JMP && BPF_CLASS(code) != BPF_JMP32) goto next; - if (BPF_OP(code) == BPF_EXIT || BPF_OP(code) == BPF_CALL) + if (BPF_OP(code) == BPF_CALL) goto next; + if (BPF_OP(code) == BPF_EXIT) { + subprog[cur_subprog].exit_idx = i; + goto next; + } off = i + bpf_jmp_offset(&insn[i]) + 1; if (off < subprog_start || off >= subprog_end) { verbose(env, "jump out of range from insn %d to %d\n", i, off); @@ -18150,6 +18154,25 @@ static int visit_gotox_insn(int t, struct bpf_verifier_env *env) return keep_exploring ? KEEP_EXPLORING : DONE_EXPLORING; } +static int visit_tailcall_insn(struct bpf_verifier_env *env, int t) +{ + static struct bpf_subprog_info *subprog; + struct bpf_iarray *jt; + + if (env->insn_aux_data[t].jt) + return 0; + + jt = iarray_realloc(NULL, 2); + if (!jt) + return -ENOMEM; + + subprog = bpf_find_containing_subprog(env, t); + jt->items[0] = t + 1; + jt->items[1] = subprog->exit_idx; + env->insn_aux_data[t].jt = jt; + return 0; +} + /* Visits the instruction at index t and returns one of the following: * < 0 - an error occurred * DONE_EXPLORING - the instruction was fully explored @@ -18210,6 +18233,8 @@ static int visit_insn(int t, struct bpf_verifier_env *env) mark_subprog_might_sleep(env, t); if (bpf_helper_changes_pkt_data(insn->imm)) mark_subprog_changes_pkt_data(env, t); + if (insn->imm == BPF_FUNC_tail_call) + visit_tailcall_insn(env, t); } else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL) { struct bpf_kfunc_call_arg_meta meta; @@ -21471,7 +21496,7 @@ static void clear_insn_aux_data(struct bpf_verifier_env *env, int start, int len int i; for (i = start; i < end; i++) { - if (insn_is_gotox(&insns[i])) { + if (aux_data[i].jt) { kvfree(aux_data[i].jt); aux_data[i].jt = NULL; } -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v5 bpf-next 3/4] bpf: correct stack liveness for tail calls 2025-11-18 13:39 ` [PATCH v5 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann @ 2025-11-18 22:54 ` Eduard Zingerman 0 siblings, 0 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-18 22:54 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Tue, 2025-11-18 at 14:39 +0100, Martin Teichmann wrote: > This updates bpf_insn_successors() reflecting that control flow might > jump over the instructions between tail call and function exit, verifier > might assume that some writes to parent stack always happen, which is > not the case. > --- Acked-by: Eduard Zingerman <eddyz87@gmail.com> Please add your signed-off-by. If you'd like you can put me as a co-developed-by but that's not a big deal. [...] ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v5 bpf-next 4/4] bpf: test the correct stack liveness of tail calls 2025-11-10 20:28 ` Eduard Zingerman ` (5 preceding siblings ...) 2025-11-18 13:39 ` [PATCH v5 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann @ 2025-11-18 13:39 ` Martin Teichmann 2025-11-18 22:55 ` Eduard Zingerman 6 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-18 13:39 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann A new test is added: caller_stack_write_tail_call tests that the live stack is correctly tracked for a tail call. --- .../selftests/bpf/progs/verifier_live_stack.c | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tools/testing/selftests/bpf/progs/verifier_live_stack.c b/tools/testing/selftests/bpf/progs/verifier_live_stack.c index c0e808509268..2de105057bbc 100644 --- a/tools/testing/selftests/bpf/progs/verifier_live_stack.c +++ b/tools/testing/selftests/bpf/progs/verifier_live_stack.c @@ -292,3 +292,53 @@ __naked void syzbot_postorder_bug1(void) "exit;" ::: __clobber_all); } + +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +SEC("socket") +__failure __msg("invalid read from stack R2 off=-1024 size=8") +__flag(BPF_F_TEST_STATE_FREQ) +__naked unsigned long caller_stack_write_tail_call(void) +{ + asm volatile ( + "r6 = r1;" + "*(u64 *)(r10 - 8) = -8;" + "call %[bpf_get_prandom_u32];" + "if r0 != 42 goto 1f;" + "goto 2f;" + "1:" + "*(u64 *)(r10 - 8) = -1024;" + "2:" + "r1 = r6;" + "r2 = r10;" + "r2 += -8;" + "call write_tail_call;" + "r1 = *(u64 *)(r10 - 8);" + "r2 = r10;" + "r2 += r1;" + "r0 = *(u64 *)(r2 + 0);" + "exit;" + :: __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +static __used __naked unsigned long write_tail_call(void) +{ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "*(u64 *)(r6 + 0) = -16;" + "r0 = 0;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v5 bpf-next 4/4] bpf: test the correct stack liveness of tail calls 2025-11-18 13:39 ` [PATCH v5 bpf-next 4/4] bpf: test the correct stack liveness of " Martin Teichmann @ 2025-11-18 22:55 ` Eduard Zingerman 2025-11-19 0:13 ` Alexei Starovoitov 0 siblings, 1 reply; 43+ messages in thread From: Eduard Zingerman @ 2025-11-18 22:55 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Tue, 2025-11-18 at 14:39 +0100, Martin Teichmann wrote: > A new test is added: caller_stack_write_tail_call tests that the live > stack is correctly tracked for a tail call. > --- Acked-by: Eduard Zingerman <eddyz87@gmail.com> Same here, please add signed-off-by. [...] ^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH v5 bpf-next 4/4] bpf: test the correct stack liveness of tail calls 2025-11-18 22:55 ` Eduard Zingerman @ 2025-11-19 0:13 ` Alexei Starovoitov 0 siblings, 0 replies; 43+ messages in thread From: Alexei Starovoitov @ 2025-11-19 0:13 UTC (permalink / raw) To: Eduard Zingerman Cc: Martin Teichmann, bpf, Alexei Starovoitov, Andrii Nakryiko On Tue, Nov 18, 2025 at 2:55 PM Eduard Zingerman <eddyz87@gmail.com> wrote: > > On Tue, 2025-11-18 at 14:39 +0100, Martin Teichmann wrote: > > A new test is added: caller_stack_write_tail_call tests that the live > > stack is correctly tracked for a tail call. > > --- > > Acked-by: Eduard Zingerman <eddyz87@gmail.com> > > Same here, please add signed-off-by. Martin, small process comment. If you want to give authorship to Eduard, please use his name in 'From:', then add his SOB, and add your SOB afterwards. If it's too complicated, just use your From, your SOB and Co-developed-by: Eduard Sounds like Eduard doesn't pay attention to patch count :) Which is a good thing. Really. ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls 2025-11-06 19:50 ` Eduard Zingerman 2025-11-10 15:18 ` [PATCH v4 bpf-next 0/2] bpf: properly verify tail call behavior Martin Teichmann 2025-11-10 15:18 ` [PATCH v4 bpf-next 1/2] " Martin Teichmann @ 2025-11-10 15:18 ` Martin Teichmann 2025-11-10 20:32 ` Eduard Zingerman 2 siblings, 1 reply; 43+ messages in thread From: Martin Teichmann @ 2025-11-10 15:18 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Four tests are added: - invalidate_pkt_pointers_by_tail_call checks that one can use the packet pointer after a tail call. This was originally possible and also poses not problems, but was made impossible by 1a4607ffba35. - invalidate_pkt_pointers_by_static_tail_call tests a corner case found by Eduard Zingerman during the discussion of the original fix, which was broken in that fix. - subprog_result_tail_call tests that precision propagation works correctly across tail calls. This did not work before. - caller_stack_write_tail_call tests that the live stack is correctly tracked for a tail call, again a corner case found by Eduard Zingerman. Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- .../selftests/bpf/progs/verifier_live_stack.c | 49 +++++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 ++++++++++++++++++ 3 files changed, 133 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/progs/verifier_live_stack.c b/tools/testing/selftests/bpf/progs/verifier_live_stack.c index c0e808509268..9cc53eb1a545 100644 --- a/tools/testing/selftests/bpf/progs/verifier_live_stack.c +++ b/tools/testing/selftests/bpf/progs/verifier_live_stack.c @@ -292,3 +292,52 @@ __naked void syzbot_postorder_bug1(void) "exit;" ::: __clobber_all); } + +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +SEC("socket") +__failure __msg("invalid read from stack R2 off=-1024 size=8") +__naked unsigned long caller_stack_write_tail_call(void) +{ + asm volatile ( + "r6 = r1;" + "*(u64 *)(r10 - 8) = -8;" + "call %[bpf_get_prandom_u32];" + "if r0 != 42 goto 1f;" + "goto 2f;" + "1:" + "*(u64 *)(r10 - 8) = -1024;" + "2:" + "r1 = r6;" + "r2 = r10;" + "r2 += -8;" + "call write_tail_call;" + "r1 = *(u64 *)(r10 - 8);" + "r2 = r10;" + "r2 += r1;" + "r0 = *(u64 *)(r2 + 0);" + "exit;" + :: __imm(bpf_get_prandom_u32) + : __clobber_all); +} + +static __used __naked unsigned long write_tail_call(void) +{ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "*(u64 *)(r6 + 0) = -16;" + "r0 = 0;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c index 2b4610b53382..a2132c72d3b8 100644 --- a/tools/testing/selftests/bpf/progs/verifier_sock.c +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c @@ -1117,10 +1117,17 @@ int tail_call(struct __sk_buff *sk) return 0; } -/* Tail calls invalidate packet pointers. */ +static __noinline +int static_tail_call(struct __sk_buff *sk) +{ + bpf_tail_call_static(sk, &jmp_table, 0); + return 0; +} + +/* Tail calls in sub-programs invalidate packet pointers. */ SEC("tc") __failure __msg("invalid mem access") -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +int invalidate_pkt_pointers_by_global_tail_call(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; @@ -1131,4 +1138,32 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) return TCX_PASS; } +/* Tail calls in static sub-programs invalidate packet pointers. */ +SEC("tc") +__failure __msg("invalid mem access") +int invalidate_pkt_pointers_by_static_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + static_tail_call(sk); + *p = 42; /* this is unsafe */ + return TCX_PASS; +} + +/* Direct tail calls do not invalidate packet pointers. */ +SEC("tc") +__success +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + bpf_tail_call_static(sk, &jmp_table, 0); + *p = 42; /* this is NOT unsafe: tail calls don't return */ + return TCX_PASS; +} + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c index ac3e418c2a96..de5ef3152567 100644 --- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c +++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c @@ -793,4 +793,51 @@ __naked int stack_slot_aliases_precision(void) ); } +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +__naked __noinline __used +static unsigned long identity_tail_call(void) +{ + /* the simplest identity function involving a tail call */ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "r0 = r6;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} + +SEC("?raw_tp") +__failure __log_level(2) +__msg("6: (0f) r1 += r0") +__msg("mark_precise: frame0: regs=r0 stack= before 5: (bf) r1 = r6") +__msg("mark_precise: frame0: regs=r0 stack= before 4: (27) r0 *= 4") +__msg("mark_precise: frame0: parent state regs=r0 stack=: R0=Pscalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") +__msg("math between map_value pointer and register with unbounded min value is not allowed") +__naked int subprog_result_tail_call(void) +{ + asm volatile ( + "r2 = 3;" + "call identity_tail_call;" + "r0 *= 4;" + "r1 = %[vals];" + "r1 += r0;" + "r0 = *(u32 *)(r1 + 0);" + "exit;" + : + : __imm_ptr(vals) + : __clobber_common + ); +} + char _license[] SEC("license") = "GPL"; -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls 2025-11-10 15:18 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification " Martin Teichmann @ 2025-11-10 20:32 ` Eduard Zingerman 0 siblings, 0 replies; 43+ messages in thread From: Eduard Zingerman @ 2025-11-10 20:32 UTC (permalink / raw) To: Martin Teichmann, bpf; +Cc: ast, andrii On Mon, 2025-11-10 at 16:18 +0100, Martin Teichmann wrote: [...] > diff --git a/tools/testing/selftests/bpf/progs/verifier_live_stack.c b/tools/testing/selftests/bpf/progs/verifier_live_stack.c > index c0e808509268..9cc53eb1a545 100644 > --- a/tools/testing/selftests/bpf/progs/verifier_live_stack.c > +++ b/tools/testing/selftests/bpf/progs/verifier_live_stack.c > @@ -292,3 +292,52 @@ __naked void syzbot_postorder_bug1(void) > "exit;" > ::: __clobber_all); > } > + > +struct { > + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); > + __uint(max_entries, 1); > + __type(key, __u32); > + __type(value, __u32); > +} map_array SEC(".maps"); > + > +SEC("socket") > +__failure __msg("invalid read from stack R2 off=-1024 size=8") Please also add `__flag(BPF_F_TEST_STATE_FREQ)` here, so that it is guaranteed that checkpoint is created at the `call write_tail_call` instruction. Otherwise the test case would depend on add_new_state heuristic in is_state_visited() remaining unchanged. > +__naked unsigned long caller_stack_write_tail_call(void) > +{ > + asm volatile ( > + "r6 = r1;" > + "*(u64 *)(r10 - 8) = -8;" > + "call %[bpf_get_prandom_u32];" > + "if r0 != 42 goto 1f;" > + "goto 2f;" > + "1:" > + "*(u64 *)(r10 - 8) = -1024;" > + "2:" > + "r1 = r6;" > + "r2 = r10;" > + "r2 += -8;" > + "call write_tail_call;" > + "r1 = *(u64 *)(r10 - 8);" > + "r2 = r10;" > + "r2 += r1;" > + "r0 = *(u64 *)(r2 + 0);" > + "exit;" > + :: __imm(bpf_get_prandom_u32) > + : __clobber_all); > +} > + > +static __used __naked unsigned long write_tail_call(void) > +{ > + asm volatile ( > + "r6 = r2;" > + "r2 = %[map_array] ll;" > + "r3 = 0;" > + "call %[bpf_tail_call];" > + "*(u64 *)(r6 + 0) = -16;" > + "r0 = 0;" > + "exit;" > + : > + : __imm(bpf_tail_call), > + __imm_addr(map_array) > + : __clobber_all); > +} [...] ^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH v3 bpf-next 1/2] bpf: properly verify tail call behavior 2025-10-31 19:24 ` Eduard Zingerman ` (2 preceding siblings ...) 2025-11-05 17:40 ` [PATCH v3 bpf-next 0/2] " Martin Teichmann @ 2025-11-05 17:40 ` Martin Teichmann 2025-11-05 17:40 ` [PATCH v3 bpf-next 2/2] bpf: test the proper verification of tail calls Martin Teichmann 4 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-05 17:40 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann A successful ebpf tail call does not return to the caller, but to the caller-of-the-caller, often just finishing the ebpf program altogether. Any restrictions that the verifier needs to take into account - notably the fact that the tail call might have modified packet pointers - are to be checked on the caller-of-the-caller. Checking it on the caller made the verifier refuse perfectly fine programs that would use the packet pointers after a tail call, which is no problem as this code is only executed if the tail call was unsuccessful, i.e. nothing happened. This patch simulates the behavior of a tail call in the verifier. A conditional jump to the code after the tail call is added for the case of an unsucessful tail call, and a return to the caller is simulated for a successful tail call. For the successful case we assume that the tail call returns an int, as tail calls are currently only allowed in functions that return and int. We always assume that the tail call modified the packet pointers, as we do not know what the tail call did. For the unsuccessful case we know nothing happened, so we do not need to add new constraints. Fixes: 1a4607ffba35 ("bpf: consider that tail calls invalidate packet pointers") Link: https://lore.kernel.org/bpf/20251029105828.1488347-1-martin.teichmann@xfel.eu/ Signed-off-by: Martin Teichmann <martin.teichmann@xfel.eu> --- kernel/bpf/verifier.c | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index e4928846e763..b9e54acbd74e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11005,6 +11005,10 @@ static int prepare_func_exit(struct bpf_verifier_env *env, int *insn_idx) bool in_callback_fn; int err; + err = bpf_update_live_stack(env); + if (err) + return err; + callee = state->frame[state->curframe]; r0 = &callee->regs[BPF_REG_0]; if (r0->type == PTR_TO_STACK) { @@ -11911,6 +11915,25 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn env->prog->call_get_func_ip = true; } + if (func_id == BPF_FUNC_tail_call) { + if (env->cur_state->curframe) { + struct bpf_verifier_state *branch; + + mark_reg_scratched(env, BPF_REG_0); + branch = push_stack(env, env->insn_idx + 1, env->insn_idx, false); + if (IS_ERR(branch)) + return PTR_ERR(branch); + clear_all_pkt_pointers(env); + mark_reg_unknown(env, regs, BPF_REG_0); + err = prepare_func_exit(env, &env->insn_idx); + if (err) + return err; + env->insn_idx--; + } else { + changes_data = false; + } + } + if (changes_data) clear_all_pkt_pointers(env); return 0; @@ -19876,9 +19899,6 @@ static int process_bpf_exit_full(struct bpf_verifier_env *env, return PROCESS_BPF_EXIT; if (env->cur_state->curframe) { - err = bpf_update_live_stack(env); - if (err) - return err; /* exit from nested function */ err = prepare_func_exit(env, &env->insn_idx); if (err) -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH v3 bpf-next 2/2] bpf: test the proper verification of tail calls 2025-10-31 19:24 ` Eduard Zingerman ` (3 preceding siblings ...) 2025-11-05 17:40 ` [PATCH v3 bpf-next 1/2] bpf: properly verify tail call behavior Martin Teichmann @ 2025-11-05 17:40 ` Martin Teichmann 4 siblings, 0 replies; 43+ messages in thread From: Martin Teichmann @ 2025-11-05 17:40 UTC (permalink / raw) To: bpf; +Cc: eddyz87, ast, andrii, Martin Teichmann Four tests are added: - invalidate_pkt_pointers_by_tail_call checks that one can use the packet pointer after a tail call. This was originally possible and also poses not problems, but was made impossible by 1a4607ffba35. - invalidate_pkt_pointers_by_static_tail_call tests a corner case found by Eduard Zingerman during the discussion of the original fix, which was broken in that fix. - subprog_result_tail_call tests that precision propagation works correctly across tail calls. This did not work before. - caller_stack_write_tail_call tests that the live stack is correctly tracked for a tail call. --- .../selftests/bpf/progs/verifier_live_stack.c | 46 ++++++++++++++++++ .../selftests/bpf/progs/verifier_sock.c | 39 ++++++++++++++- .../bpf/progs/verifier_subprog_precision.c | 47 +++++++++++++++++++ 3 files changed, 130 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/progs/verifier_live_stack.c b/tools/testing/selftests/bpf/progs/verifier_live_stack.c index c0e808509268..17c33a88f069 100644 --- a/tools/testing/selftests/bpf/progs/verifier_live_stack.c +++ b/tools/testing/selftests/bpf/progs/verifier_live_stack.c @@ -292,3 +292,49 @@ __naked void syzbot_postorder_bug1(void) "exit;" ::: __clobber_all); } + +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +SEC("socket") +__log_level(2) +__msg("10: (85) call bpf_tail_call#12") +__msg("(2,5) frame 0 insn 5 +written -16") +__msg("(2,5) live stack update done in 2 iterations") +__msg("4: (95) exit") +__msg("(0) frame 0 insn 3 +live -24") +__msg("(0) live stack update done in 2 iterations") +__msg("13: (95) exit") +__msg("(2,5) frame 0 insn 11 +written -8") +__msg("(2,5) live stack update done in 2 iterations") +__naked unsigned long caller_stack_write_tail_call(void) +{ + asm volatile ( + "r2 = r10;" + "r2 += -8;" + "call write_tail_call;" + "r0 = *(u64 *)(r10 - 24);" + "exit;" + ::: __clobber_all); +} + +static __used __naked unsigned long write_tail_call(void) +{ + asm volatile ( + "*(u64 *)(r2 - 8) = 7;" + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "*(u64 *)(r6 + 0) = 7;" + "r0 = 0;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} diff --git a/tools/testing/selftests/bpf/progs/verifier_sock.c b/tools/testing/selftests/bpf/progs/verifier_sock.c index 2b4610b53382..a2132c72d3b8 100644 --- a/tools/testing/selftests/bpf/progs/verifier_sock.c +++ b/tools/testing/selftests/bpf/progs/verifier_sock.c @@ -1117,10 +1117,17 @@ int tail_call(struct __sk_buff *sk) return 0; } -/* Tail calls invalidate packet pointers. */ +static __noinline +int static_tail_call(struct __sk_buff *sk) +{ + bpf_tail_call_static(sk, &jmp_table, 0); + return 0; +} + +/* Tail calls in sub-programs invalidate packet pointers. */ SEC("tc") __failure __msg("invalid mem access") -int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +int invalidate_pkt_pointers_by_global_tail_call(struct __sk_buff *sk) { int *p = (void *)(long)sk->data; @@ -1131,4 +1138,32 @@ int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) return TCX_PASS; } +/* Tail calls in static sub-programs invalidate packet pointers. */ +SEC("tc") +__failure __msg("invalid mem access") +int invalidate_pkt_pointers_by_static_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + static_tail_call(sk); + *p = 42; /* this is unsafe */ + return TCX_PASS; +} + +/* Direct tail calls do not invalidate packet pointers. */ +SEC("tc") +__success +int invalidate_pkt_pointers_by_tail_call(struct __sk_buff *sk) +{ + int *p = (void *)(long)sk->data; + + if ((void *)(p + 1) > (void *)(long)sk->data_end) + return TCX_DROP; + bpf_tail_call_static(sk, &jmp_table, 0); + *p = 42; /* this is NOT unsafe: tail calls don't return */ + return TCX_PASS; +} + char _license[] SEC("license") = "GPL"; diff --git a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c index ac3e418c2a96..de5ef3152567 100644 --- a/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c +++ b/tools/testing/selftests/bpf/progs/verifier_subprog_precision.c @@ -793,4 +793,51 @@ __naked int stack_slot_aliases_precision(void) ); } +struct { + __uint(type, BPF_MAP_TYPE_PROG_ARRAY); + __uint(max_entries, 1); + __type(key, __u32); + __type(value, __u32); +} map_array SEC(".maps"); + +__naked __noinline __used +static unsigned long identity_tail_call(void) +{ + /* the simplest identity function involving a tail call */ + asm volatile ( + "r6 = r2;" + "r2 = %[map_array] ll;" + "r3 = 0;" + "call %[bpf_tail_call];" + "r0 = r6;" + "exit;" + : + : __imm(bpf_tail_call), + __imm_addr(map_array) + : __clobber_all); +} + +SEC("?raw_tp") +__failure __log_level(2) +__msg("6: (0f) r1 += r0") +__msg("mark_precise: frame0: regs=r0 stack= before 5: (bf) r1 = r6") +__msg("mark_precise: frame0: regs=r0 stack= before 4: (27) r0 *= 4") +__msg("mark_precise: frame0: parent state regs=r0 stack=: R0=Pscalar() R6=map_value(map=.data.vals,ks=4,vs=16) R10=fp0") +__msg("math between map_value pointer and register with unbounded min value is not allowed") +__naked int subprog_result_tail_call(void) +{ + asm volatile ( + "r2 = 3;" + "call identity_tail_call;" + "r0 *= 4;" + "r1 = %[vals];" + "r1 += r0;" + "r0 = *(u32 *)(r1 + 0);" + "exit;" + : + : __imm_ptr(vals) + : __clobber_common + ); +} + char _license[] SEC("license") = "GPL"; -- 2.43.0 ^ permalink raw reply related [flat|nested] 43+ messages in thread
end of thread, other threads:[~2025-12-12 2:07 UTC | newest] Thread overview: 43+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-29 10:58 [PATCH bpf] bpf: tail calls do not modify packet data Martin Teichmann 2025-10-31 19:24 ` Eduard Zingerman 2025-11-03 8:56 ` Teichmann, Martin 2025-11-03 17:34 ` Eduard Zingerman 2025-11-04 12:54 ` Teichmann, Martin 2025-11-04 13:30 ` [PATCH v2 bpf] bpf: properly verify tail call behavior Martin Teichmann 2025-11-04 13:58 ` bot+bpf-ci 2025-11-04 18:05 ` Alexei Starovoitov 2025-11-04 22:30 ` Eduard Zingerman 2025-11-05 17:40 ` [PATCH v3 bpf-next 0/2] " Martin Teichmann 2025-11-05 19:08 ` Eduard Zingerman 2025-11-06 10:52 ` [PATCH v4 " Martin Teichmann 2025-11-06 10:52 ` [PATCH v4 bpf-next 1/2] " Martin Teichmann 2025-11-06 10:52 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification of tail calls Martin Teichmann 2025-11-06 19:50 ` Eduard Zingerman 2025-11-10 15:18 ` [PATCH v4 bpf-next 0/2] bpf: properly verify tail call behavior Martin Teichmann 2025-11-10 15:18 ` [PATCH v4 bpf-next 1/2] " Martin Teichmann 2025-11-10 20:28 ` Eduard Zingerman 2025-11-10 23:39 ` Eduard Zingerman 2025-11-13 11:46 ` Teichmann, Martin 2025-11-13 16:09 ` Alexei Starovoitov 2025-11-18 13:39 ` [PATCH v5 bpf-next 0/4] " Martin Teichmann 2025-11-18 13:39 ` [PATCH v5 bpf-next 1/4] " Martin Teichmann 2025-11-18 19:34 ` Eduard Zingerman 2025-11-19 16:03 ` [PATCH v6 bpf-next 0/4] " Martin Teichmann 2025-11-19 16:03 ` [PATCH v6 bpf-next 1/4] " Martin Teichmann 2025-11-22 2:00 ` patchwork-bot+netdevbpf 2025-11-19 16:03 ` [PATCH v6 bpf-next 2/4] bpf: test the proper verification of tail calls Martin Teichmann 2025-11-19 16:03 ` [PATCH v6 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann 2025-11-19 16:33 ` bot+bpf-ci 2025-12-12 2:06 ` Chris Mason 2025-11-19 16:03 ` [PATCH v6 bpf-next 4/4] bpf: test the correct stack liveness of " Martin Teichmann 2025-11-18 13:39 ` [PATCH v5 bpf-next 2/4] bpf: test the proper verification " Martin Teichmann 2025-11-18 22:47 ` Eduard Zingerman 2025-11-18 13:39 ` [PATCH v5 bpf-next 3/4] bpf: correct stack liveness for " Martin Teichmann 2025-11-18 22:54 ` Eduard Zingerman 2025-11-18 13:39 ` [PATCH v5 bpf-next 4/4] bpf: test the correct stack liveness of " Martin Teichmann 2025-11-18 22:55 ` Eduard Zingerman 2025-11-19 0:13 ` Alexei Starovoitov 2025-11-10 15:18 ` [PATCH v4 bpf-next 2/2] bpf: test the proper verification " Martin Teichmann 2025-11-10 20:32 ` Eduard Zingerman 2025-11-05 17:40 ` [PATCH v3 bpf-next 1/2] bpf: properly verify tail call behavior Martin Teichmann 2025-11-05 17:40 ` [PATCH v3 bpf-next 2/2] bpf: test the proper verification of tail calls Martin Teichmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox