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 for Android: free password hash cracker in your pocket

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

Message-ID: <2ba29c9f-a44f-4be6-bd3a-eb9cdb34ac8a@kernel.org>
Date: Wed, 13 Aug 2025 10:43:21 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Alexander Lobakin <aleksander.lobakin@...el.com>,
 Jakub Kicinski <kuba@...nel.org>
Cc: 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 12/08/2025 18.48, Alexander Lobakin wrote:
> From: Jakub Kicinski <kuba@...nel.org>
> Date: Tue, 12 Aug 2025 09:15:28 -0700
> 
>> xdp_update_skb_shared_info() needs to update skb state which
>> was maintained in xdp_buff / frame. Pass full flags into it,
>> instead of breaking it out bit by bit. We will need to add
>> a bit for unreadable frags (even tho XDP doesn't support
>> those the driver paths may be common), at which point almost
>> all call sites would become:
>>
>>      xdp_update_skb_shared_info(skb, num_frags,
>>                                 sinfo->xdp_frags_size,
>>                                 MY_PAGE_SIZE * num_frags,
>>                                 xdp_buff_is_frag_pfmemalloc(xdp),
>>                                 xdp_buff_is_frag_unreadable(xdp));
> 
> Yeah I think this doesn't make sense, it just doesn't scale. We can make
> more flags in future and adding a new argument for each is not a good
> idea, even if more drivers would switch to generic
> xdp_build_skb_from_buff().
> 

I agree. And good reminder that some driver have already switched to the
generic xdp_build_skb_from_buff().

>>
>> Keep a helper for accessing the flags, in case we need to
>> transform them somehow in the future (e.g. to cover up xdp_buff
>> vs xdp_frame differences).
>>
>> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
>> ---
>> 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)


>> 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.).

--Jesper

>>
>> CC: ast@...nel.org
>> CC: daniel@...earbox.net
>> CC: hawk@...nel.org
>> CC: lorenzo@...nel.org
>> CC: toke@...hat.com
>> CC: john.fastabend@...il.com
>> CC: sdf@...ichev.me
>> CC: michael.chan@...adcom.com
>> CC: anthony.l.nguyen@...el.com
>> CC: przemyslaw.kitszel@...el.com
>> CC: marcin.s.wojtas@...il.com
>> CC: tariqt@...dia.com
>> CC: mbloch@...dia.com
>> CC: eperezma@...hat.com
>> CC: bpf@...r.kernel.org
>> ---
>>   include/net/xdp.h                             | 21 +++++++++----------
>>   drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  2 +-
>>   drivers/net/ethernet/intel/i40e/i40e_txrx.c   |  4 ++--
>>   drivers/net/ethernet/intel/ice/ice_txrx.c     |  4 ++--
>>   drivers/net/ethernet/marvell/mvneta.c         |  2 +-
>>   .../net/ethernet/mellanox/mlx5/core/en_rx.c   |  7 +++----
>>   drivers/net/virtio_net.c                      |  2 +-
>>   net/core/xdp.c                                | 11 +++++-----
>>   8 files changed, 26 insertions(+), 27 deletions(-)