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: <46470d2b-4828-48ad-a94e-9d874de1b2fc@intel.com>
Date: Tue, 12 Aug 2025 18:48:25 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: <netdev@...r.kernel.org>, <bpf@...r.kernel.org>, <ast@...nel.org>,
	<daniel@...earbox.net>, <hawk@...nel.org>, <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

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

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

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

Thanks,
Olek