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

[Eduard Zingerman <eddyz87@gmail.com>]

On Mon, 2024-12-09 at 08:53 -0800, Alexei Starovoitov wrote:

[...]

> > 
> >     // 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;
> >     }
> 
> hmm. none of the above changes pkt_data, so it should be allowed.
> The prog doesn't read skb->data either.
> So I don't quite see the problem.

The problem is when I use simplified rule: "every tail call changes packet data",
as a substitute for proper map content effects tracking.

If map content effects are tracked, there should be no problems
verifying this program. However, that can't be done in check_cfg(),
as it does not track register values, and register value is needed to
identify the map. Hence, mechanics with "in-line" global sub-program
traversal is needed (as described by Andrii):
- during a regular verification pass get to a global sub-program call:
  - if sub-program had not been visited yet, verify it completely
    and compute changes_pkt_data effect;
  - continue from the call-site using the computed effect;
- during a regular verification pass get to a tail call:
  - check the map pointed to by R1 to see whether it has
    changes_pkt_data effect.

> > 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.
> 
> skb_pull is not, but there are plenty that are practically nop helpers.
> bpf_helper_changes_pkt_data() lists them all.
> Like bpf_xdp_adjust_meta(xdp, 0)
> 
> > 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).
> 
> nack to tags approach.

Understood.