Re: [PATCH bpf] bpf: tail calls do not modify packet data

[Eduard Zingerman <eddyz87@gmail.com>]

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";