Jakub Sitnicki via Intel-wired-lan <[email protected]> writes:
> On Tue, Jan 13, 2026 at 07:52 PM +01, Jesper Dangaard Brouer wrote: >> *BUT* this patchset isn't doing that. To me it looks like a cleanup >> patchset that simply makes it consistent when skb_metadata_set() called. >> Selling it as a pre-requirement for doing copy later seems fishy. > > Fair point on the framing. The interface cleanup is useful on its own - > I should have presented it that way rather than tying it to future work. > >> Instead of blindly copying XDP data_meta area into a single SKB >> extension. What if we make it the responsibility of the TC-ingress BPF- >> hook to understand the data_meta format and via (kfunc) helpers >> transfer/create the SKB extension that it deems relevant. >> Would this be an acceptable approach that makes it easier to propagate >> metadata deeper in netstack? > > I think you and Jakub are actually proposing the same thing. > > If we can access a buffer tied to an skb extension from BPF, this could > act as skb-local storage and solves the problem (with some operational > overhead to set up TC on ingress). > > I'd also like to get Alexei's take on this. We had a discussion before > about not wanting to maintain two different storage areas for skb > metadata. > > That was one of two reasons why we abandoned Arthur's patches and why I > tried to make the existing headroom-backed metadata area work. > > But perhaps I misunderstood the earlier discussion. Alexei's point may > have been that we don't want another *headroom-backed* metadata area > accessible from XDP, because we already have that. > > Looks like we have two options on the table: > > Option A) Headroom-backed metadata > - Use existing skb metadata area > - Patch skb_push/pull call sites to preserve it > > Option B) Extension-backed metadata > - Store metadata in skb extension from BPF > - TC BPF copies/extracts what it needs from headroom-metadata > > Or is there an Option C I'm missing? Not sure if it's really an option C, but would it be possible to consolidate them using verifier tricks? I.e., the data_meta field in the __sk_buff struct is really a virtual pointer that the verifier rewrites to loading an actual pointer from struct bpf_skb_data_end in skb->cb. So in principle this could be loaded from an skb extension instead with the BPF programs being none the wiser. There's the additional wrinkle that the end of the data_meta pointer is compared to the 'data' start pointer to check for overflow, which wouldn't work anymore. Not sure if there's a way to make the verifier rewrite those checks in a compatible way, or if this is even a path we want to go down. But it would be a pretty neat way to make the whole thing transparent and backwards compatible, I think :) Other than that, I like the extention-backed metadata idea! -Toke
