On Sun, Dec 8, 2024 at 11:40 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Fri, 2024-12-06 at 12:43 -0800, Alexei Starovoitov wrote:
> > On Thu, Dec 5, 2024 at 8:03 PM Eduard Zingerman <eddyz87@xxxxxxxxx> 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;
> }
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.
> 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.
>
> ---
>
> I'll continue with global sub-programs traversal reordering and share
> the implementation on Monday, to facilitate further discussion.
>
