Re: [PATCH bpf v2 7/8] bpf: consider that tail calls invalidate packet pointers

[Nick Zavaritsky <mejedi@gmail.com>]

> Tail-called programs could execute any of the helpers that invalidate
> packet pointers. Hence, conservatively assume that each tail call
> invalidates packet pointers.

Tail calls look like a clear limitation of "auto-infer packet
invalidation effect" approach. Correct solution requires propagating
effects in the dynamic callee-caller graph, unlikely to ever happen.

I'm curious if assuming that every call to a global sub program
invalidates packet pointers might be an option. Does it break too many
programs in the wild?

From an end-user perspective, the presented solution makes debugging
verifier errors harder. An error message doesn't tell which call
invalidated pointers. Whether verifier considers a particular sub
program as pointer-invalidating is not revealed. I foresee exciting
debugging sessions.

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@gmail.com>
> ---
> 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
>