On Mon, Dec 9, 2024 at 5:24 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2024-12-09 at 16:48 -0800, Alexei Starovoitov wrote:
>
> [...]
>
> > Also either inline global sub-prog traversal or the simple rule above
> > both suffer from the following issue:
> > in case prog_array is empty map->changes_pkt_data will be false.
> > It will be set to true only when the first
> > prog_fd_array_get_ptr()->bpf_prog_map_compatible()
> > will record changes_pkt_data from the first prog inserted in prog_array.
> >
> > So main prog reading skb->data after calling subprog that tail_calls
> > somewhere should assume that skb->data is invalidated.
>
> Yes, that's what I was planning to do.
>
> > That's pretty much your rule "every tail call changes packet data".
> >
> > I think we can go with this simplest approach as well.
> > The test you mentioned have to be adjusted. Not a big deal.
> >
> > Or we can do:
> > "if prog_array empty assume adjusts_pkt_data == true,
> > otherwise adj_pkt_data | = for each map in used_maps {
> > map->owner.adj_pkt_data }"
> >
> > The fancy inline global subprog traversal would have to have the same
> > "simple" (or call it dumb) rule.
> > So at the end both inline or check_cfg are not accurate at all,
> > but check_cfg approach is so much simpler.
>
> I don't like the '|= map->owner.adj_pkt_data' thing, tbh.
> I think "any tail call changes packet data" is simpler to reason about.
> But then, the question is, how to modify the test?
> You suggested bpf_xdp_adjust_meta(xdp, 0) for 'xdp', and it is a good fit,
> as it does not do much.
> For 'skb' I don't see anything better than 'bpf_skb_pull_data(sk, 0)',
> it does skb_ensure_writable(), but everything else does more.
bpf_skb_change_proto(skb, skb->protocol, 0)
is a guaranteed success and as close to nop as possible.
Or the same with invalid flags or invalid proto.
bpf_skb_change_proto(skb, 0, 0)
Guaranteed -EINVAL.
> Dedicated kfunc would be cleaner, though.
Sorry I disagree.
