It probably doesn't matter, but I don't like bpf_xdp_adjust_meta(xdp, 0)
hack to mark a program as pointer-invalidating either.
I would've preferred a simple static rule "calls to global sub programs
invalidate packet pointers" with an optional decl tag to mark a sub
program as non-invalidating, in line with "arg:nonnull".
> Making the change in bpf_helper_changes_pkt_data() automatically makes
> use of check_cfg() logic that computes 'changes_pkt_data' effect for
> global sub-programs, such that the following program could be
> rejected:
>
> int tail_call(struct __sk_buff *sk)
> {
> bpf_tail_call_static(sk, &jmp_table, 0);
> return 0;
> }
>
> SEC("tc")
> int not_safe(struct __sk_buff *sk)
> {
> int *p = (void *)(long)sk->data;
> ... make p valid ...
> tail_call(sk);
> *p = 42; /* this is unsafe */
> ...
> }
>
> The tc_bpf2bpf.c:subprog_tc() needs change: mark it as a function that
> can invalidate packet pointers. Otherwise, it can't be freplaced with
> tailcall_freplace.c:entry_freplace() that does a tail call.
>
> Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> ---
> net/core/filter.c | 2 ++
> tools/testing/selftests/bpf/progs/tc_bpf2bpf.c | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index efb75eed2e35..21131ec25f24 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -7924,6 +7924,8 @@ 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/tc_bpf2bpf.c b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
> index d1a57f7d09bd..fe6249d99b31 100644
> --- a/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
> +++ b/tools/testing/selftests/bpf/progs/tc_bpf2bpf.c
> @@ -11,6 +11,8 @@ int subprog_tc(struct __sk_buff *skb)
>
> __sink(skb);
> __sink(ret);
> + /* let verifier know that 'subprog_tc' can change pointers to skb->data */
> + bpf_skb_change_proto(skb, 0, 0);
> return ret;
> }
>
> --
> 2.47.0
>
