netdev - Re: [RFC] xdp: pass flags to xdp_update_skb_shared

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] [day] [month] [year] [list]

Message-ID: <902cf005-7731-4a20-9cb1-287318ab8a9a@kernel.org>
Date: Thu, 14 Aug 2025 10:11:35 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Alexander Lobakin <aleksander.lobakin@...el.com>, netdev@...r.kernel.org,
 bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
 lorenzo@...nel.org, toke@...hat.com, john.fastabend@...il.com,
 sdf@...ichev.me, michael.chan@...adcom.com, anthony.l.nguyen@...el.com,
 przemyslaw.kitszel@...el.com, marcin.s.wojtas@...il.com, tariqt@...dia.com,
 mbloch@...dia.com, eperezma@...hat.com
Subject: Re: [RFC] xdp: pass flags to xdp_update_skb_shared_info() directly



On 13/08/2025 23.44, Jakub Kicinski wrote:
> On Wed, 13 Aug 2025 10:43:21 +0200 Jesper Dangaard Brouer wrote:
>>>> Does anyone prefer the current form of the API, or can we change
>>>> as prosposed?
>>
>> I like the proposed change.
>> The only thing that confuses me was that the u32 flags is named
>> "skb_flags" and not "xdp_flags".
>>
>> @@ -314,7 +313,7 @@
>>    static inline void
>>    xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags,
>>    			   unsigned int size, unsigned int truesize,
>> -			   bool pfmemalloc)
>> +			   u32 skb_flags)
> 
> It was matching the helper names: xdp_buff_get_skb_flags()
> 
> If we rename it to xdp_flags here do you want me to keep
> the helpers (xdp_buff_get_flags()?) or access buf->flags
> directly in the caller?
> 
> The idea was that the helper could filter / transform
> the flags to whatever the update function takes. And the skb_
> in the helper name was matching the skb_ of the arg.
> 

It makes sense to have a helper, as you argue.

>>>> Bonus question: while Im messing with this API could I rename
>>>> xdp_update_skb_shared_info()? Maybe to xdp_update_skb_state() ?
>>>> Not sure why the function name has "shared_info" when most of
>>>> what it updates is skb fields.
>>>
>>> I can only suspect that the author decided to name it this way due to
>>> that it's only used when xdp_buff has frags (and frags are in shinfo).
>>> But I agree it's not the best choice. xdp_update_skb_state() sounds fine
>>> to me, but given that it's all about frags, maybe something like
>>> xdp_update_skb_frags_info/state() or so?
>>
>> Yes, function is only used when skb_shared_info have already been touched.
>>
>> Performance wise it can be expensive to touch the cache-line for
>> skb_shared_info, so the code carefully checks xdp_buff_has_frags() (flag
>> XDP_FLAGS_HAS_FRAGS) before deref of skb_shared_info memory area.
>>
>> Calling it xdp_update_skb_state() seems misleading. As Olek says, this
>> is about updating the "skb_frags".  The original intent is that
>> xdp_buff/xdp_frame is using same skb_shared_info area as SKB, and when
>> transitioning to a "full" SKB then we need to do some adjustments.
>> (Looking at function code, it is of-cause confusing that it doesn't
>> touch sinfo->frags[] array, but that is because we don't need to, as
>> non-linear XDP and SKB have same layout.).
> 
> Let's go with xdp_update_skb_frags_info(), then.

Fine with me. It was Olek's naming suggestions (and I liked both).

Thanks
--Jesper