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.
