Re: [PATCH v2 bpf] xsk: fix immature cq descriptor production

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Date: Sat,  5 Jul 2025 15:55:12 +0200

> Eryk reported an issue that I have put under Closes: tag, related to
> umem addrs being prematurely produced onto pool's completion queue.
> Let us make the skb's destructor responsible for producing all addrs
> that given skb used.
> 
> Commit from fixes tag introduced the buggy behavior, it was not broken
> from day 1, but rather when xsk multi-buffer got introduced.
> 
> Introduce a struct which will carry descriptor count with array of
> addresses taken from processed descriptors that will be carried via
> skb_shared_info::destructor_arg. This way we can refer to it within
> xsk_destruct_skb().
> 
> To summarize, behavior is changed from:
> - produce addr to cq, increase cq's cached_prod
> - increment descriptor count and store it on
> - (xmit and rest of path...)
>   skb_shared_info::destructor_arg
> - use destructor_arg on skb destructor to update global state of cq
>   producer
> 
> to the following:
> - increment cq's cached_prod
> - increment descriptor count, save xdp_desc::addr in custom array and
>   store this custom array on skb_shared_info::destructor_arg
> - (xmit and rest of path...)
> - use destructor_arg on skb destructor to walk the array of addrs and
>   write them to cq and finally update global state of cq producer
> 
> Fixes: b7f72a30e9ac ("xsk: introduce wrappers and helpers for supporting multi-buffer in Tx path")
> Reported-by: Eryk Kubanski <e.kubanski@partner.samsung.com>
> Closes: https://lore.kernel.org/netdev/20250530103456.53564-1-e.kubanski@partner.samsung.com/
> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> ---
> v1:
> https://lore.kernel.org/bpf/20250702101648.1942562-1-maciej.fijalkowski@intel.com/
> 
> v1->v2:
> * store addrs in array carried via destructor_arg instead having them
>   stored in skb headroom; cleaner and less hacky approach;

Might look cleaner, but what about the performance given that you're
adding a memory allocation?

(I realize that's only for the skb mode, still)

Yeah we anyway allocate an skb and may even copy the whole frame, just
curious.
I could recommend using skb->cb for that, but its 48 bytes would cover
only 6 addresses =\

Headroom is no that hacky; we even place &xdp_frame there :D

> ---
>  net/xdp/xsk.c       | 79 ++++++++++++++++++++++++++++++++++-----------
>  net/xdp/xsk_queue.h | 12 +++++++
>  2 files changed, 73 insertions(+), 18 deletions(-)

[...]

> @@ -619,6 +646,12 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  			return ERR_PTR(err);
>  
>  		skb_reserve(skb, hr);
> +
> +		addrs = kzalloc(sizeof(*addrs), GFP_KERNEL);

Are you sure you can use sleeping GFP_KERNEL here, not GFP_ATOMIC?

> +		if (!addrs)
> +			return ERR_PTR(-ENOMEM);
> +
> +		xsk_set_destructor_arg(skb, addrs);
>  	}
>  
>  	addr = desc->addr;

Thanks,
Olek