netdev - Re: [PATCH RFC bpf-next v2 10/17] bnxt: Propagate trait presence to skb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87tt6d7utp.fsf@toke.dk>
Date: Thu, 24 Apr 2025 11:49:38 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Stanislav Fomichev <stfomichev@...il.com>, Arthur Fabre
 <arthur@...hurfabre.com>
Cc: netdev@...r.kernel.org, bpf@...r.kernel.org, jakub@...udflare.com,
 hawk@...nel.org, yan@...udflare.com, jbrandeburg@...udflare.com,
 lbiancon@...hat.com, ast@...nel.org, kuba@...nel.org, edumazet@...gle.com
Subject: Re: [PATCH RFC bpf-next v2 10/17] bnxt: Propagate trait presence to
 skb

Stanislav Fomichev <stfomichev@...il.com> writes:

> On 04/23, Arthur Fabre wrote:
>> On Wed Apr 23, 2025 at 6:36 PM CEST, Stanislav Fomichev wrote:
>> > On 04/22, Arthur Fabre wrote:
>> > > Call the common xdp_buff_update_skb() helper.
>> > > 
>> > > Signed-off-by: Arthur Fabre <arthur@...hurfabre.com>
>> > > ---
>> > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 4 ++++
>> > >  1 file changed, 4 insertions(+)
>> > > 
>> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > > index c8e3468eee612ad622bfbecfd7cc1ae3396061fd..0eba3e307a3edbc5fe1abf2fa45e6256d98574c2 100644
>> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
>> > > @@ -2297,6 +2297,10 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
>> > >  			}
>> > >  		}
>> > >  	}
>> > > +
>> > > +	if (xdp_active)
>> > > +		xdp_buff_update_skb(&xdp, skb);
>> >
>> > For me, the preference for reusing existing metadata area was
>> > because of the patches 10-16: we now need to care about two types of
>> > metadata explicitly.
>> 
>> Having to update all the drivers is definitely not ideal. Motivation is:
>> 
>> 1. Avoid trait_set() and xdp_adjust_meta() from corrupting each other's
>>    data. 
>>    But that's not a problem if we disallow trait_set() and
>>    xdp_adjust_meta() to be used at the same time, so maybe not a good
>>    reason anymore (except for maybe 3.)
>> 
>> 2. Not have the traits at the "end" of the headroom (ie right next to
>>    actual packet data).
>>    If it's at the "end", we need to move all of it to make room for
>>    every xdp_adjust_head() call.
>>    It seems more intrusive to the current SKB API: several funcs assume
>>    that there is headroom directly before the packet.
>
> [..]
>
>> 3. I'm not sure how this should be exposed with AF_XDP yet. Either:
>>    * Expose raw trait storage, and having it at the "end" of the
>>      headroom is nice. But userspace would need to know how to parse the
>> 	 header.
>>    * Require the XDP program to copy the traits it wants into the XDP
>>      metadata area, which is already exposed to userspace. That would
>> 	 need traits and XDP metadata to coexist.
>  
> By keeping the traits at the tail we can just expose raw trait storage
> and let userspace deal with it. But anyway, my main point is to avoid
> having drivers to deal with two separate cases. As long as we can hide
> everything behind a common call, we can change the placement later.

Being able to change the placement (and format) of the data store is the
reason why we should absolutely *not* expose the internal trait storage
to AF_XDP. Once we do that, we effectively make the whole thing UAPI.
The trait get/set API very deliberately does not expose any details
about the underlying storage for exactly this reason :)

-Toke