Re: [PATCH bpf 3/4] bpf: track changes_pkt_data property for global functions

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2024-12-06 at 12:43 -0800, Alexei Starovoitov wrote:
> On Thu, Dec 5, 2024 at 8:03 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index f4290c179bee..48b7b2eeb7e2 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -659,6 +659,7 @@ struct bpf_subprog_info {
> >         bool args_cached: 1;
> >         /* true if bpf_fastcall stack region is used by functions that can't be inlined */
> >         bool keep_fastcall_stack: 1;
> > +       bool changes_pkt_data: 1;
>
> since freplace was brought up in the other thread.
> Let's fix it all in one patch.
> I think propagating changes_pkt_data flag into prog_aux and
> into map->owner should do it.
> The handling will be similar to existing xdp_has_frags.
>
> Otherwise tail_call from static subprog will have the same issue.
> xdp_has_frags compatibility requires equality. All progs either
> have it or don't.
> changes_pkt_data flag doesn't need to be that strict:
> A prog with changes_pkt_data can be freplaced by prog without
> and tailcall into prog without it.
> But not the other way around.

I tried implementing this in:
https://github.com/eddyz87/bpf/tree/skb-pull-data-global-func-bug

The freplace part is simple and works as intended.

The tail call part won't work with check_cfg() based approach and
needs global functions traversal reordering Andrii talked about.
This is so, because of the need to inspect the value of register R1,
passed to the tail call helper, in order to check map owner's properties.

If the rules are simplified to consider each tail call such that
packet pointers are invalidated, the test case
tailcalls/tailcall_freplace fails. Here is how it looks:

    // tc_bpf2bpf.c
    __noinline                             freplace
    int subprog_tc(struct __sk_buff *skb) <--------.
    {                                              |
    	int ret = 1;                               |
                                                   |
    	__sink(skb);                               |
    	__sink(ret);                               |
    	return ret;                                |
    }                                              |
                                                   |
    SEC("tc")                                      |
    int entry_tc(struct __sk_buff *skb)            |
    {                                              |
    	return subprog_tc(skb);                    |
    }                                              |
                                                   |
    // tailcall_freplace.c                         |
    struct {                                       |
    	__uint(type, BPF_MAP_TYPE_PROG_ARRAY);     |
    	__uint(max_entries, 1);                    |
    	__uint(key_size, sizeof(__u32));           |
    	__uint(value_size, sizeof(__u32));         |
    } jmp_table SEC(".maps");                      |
                                                   |
    int count = 0;                                 |
                                                   |
    SEC("freplace")                                |
    int entry_freplace(struct __sk_buff *skb) -----'
    {
    	count++;
    	bpf_tail_call_static(skb, &jmp_table, 0);
    	return count;
    }

Here 'entry_freplace' is assumed to invalidate packet data because of
the bpf_tail_call_static(), and thus it can't replace 'subprog_tc'.
There is an option to add a dummy call to bpf_skb_pull_data(),
but this operation is not a noop, as far as I can tell.

Same situation was discussed in the sub-thread regarding use of tags.
(Note: because of the tail calls, some form of changes_pkt_data effect
 propagation similar to one done in check_cfg() would be needed with
 tags as well. That, or tags would be needed not only for global
 sub-programs but also for BPF_MAP_TYPE_PROG_ARRAY maps).

---

I'll continue with global sub-programs traversal reordering and share
the implementation on Monday, to facilitate further discussion.