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

Message-ID: <20250813144439.71a09e9a@kernel.org>
Date: Wed, 13 Aug 2025 14:44:39 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Jesper Dangaard Brouer <hawk@...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 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.

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