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 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.
Dedicated kfunc would be cleaner, though.