Re: [PATCH net v4 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures

[Stanislav Fomichev <sdf.kernel@gmail.com>]

On 04/24, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> In copy mode TX, xsk_skb_destructor_set_addr() stores the 64-bit
> descriptor address into skb_shinfo(skb)->destructor_arg (void *) via a
> uintptr_t cast:
> 
>     skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);
> 
> On 32-bit architectures uintptr_t is 32 bits, so the upper 32 bits of
> the descriptor address are silently dropped. In unaligned mode the chunk
> offset is encoded in bits 48-63 of the descriptor address
> (XSK_UNALIGNED_BUF_OFFSET_SHIFT = 48), meaning the offset is lost
> entirely. The completion queue then returns a truncated address to
> userspace, making buffer recycling impossible.
> 
> Fix this by handling the 32-bit case in the destructor_arg helpers:
> 
> - xsk_skb_destructor_set_addr(): on !CONFIG_64BIT, allocate an
>   xsk_addrs struct via kmem_cache_zalloc() to store the full u64
>   address. Leave num_descs as 0 (zalloc) so that the subsequent
>   xsk_inc_num_desc() brings it to the correct count of 1.
> 
> - xsk_skb_destructor_is_addr(): on !CONFIG_64BIT, return true only
>   when destructor_arg is NULL (not yet set), false when it points to
>   an xsk_addrs struct.
> 
> - xsk_skb_init_misc(): call xsk_skb_destructor_set_addr() first
>   before touching any other skb fields; on failure return early so
>   the skb destructor is never changed from sock_wfree.
> 
> The existing xsk_consume_skb() already handles 32-bit correctly after
> these changes: xsk_skb_destructor_is_addr() returns false for any
> allocated xsk_addrs, so the kmem_cache_free path is always taken.
> 
> The overhead is one extra kmem_cache_zalloc per first descriptor on
> 32-bit only; 64-bit builds are completely unchanged.
> 
> Closes: https://lore.kernel.org/all/20260419045824.D9E5EC2BCAF@smtp.kernel.org/
> Fixes: 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
>  net/xdp/xsk.c | 38 +++++++++++++++++++++++++++++++-------
>  1 file changed, 31 insertions(+), 7 deletions(-)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index ed96f6ec8ff2..fe88f47741b5 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -558,7 +558,10 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)
>  
>  static bool xsk_skb_destructor_is_addr(struct sk_buff *skb)
>  {
> -	return (uintptr_t)skb_shinfo(skb)->destructor_arg & 0x1UL;
> +	if (IS_ENABLED(CONFIG_64BIT))
> +		return (uintptr_t)skb_shinfo(skb)->destructor_arg & 0x1UL;
> +	else
> +		return !skb_shinfo(skb)->destructor_arg;

Don't understand why we need to special case CONFIG_64BIT here?
Shouldn't the same existing condition work on 32bit?

>  }
>  
>  static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)
> @@ -566,9 +569,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)
>  	return (u64)((uintptr_t)skb_shinfo(skb)->destructor_arg & ~0x1UL);
>  }
>  
> -static void xsk_skb_destructor_set_addr(struct sk_buff *skb, u64 addr)
> +static int xsk_skb_destructor_set_addr(struct sk_buff *skb, u64 addr)
>  {

[..]

> +	if (!IS_ENABLED(CONFIG_64BIT)) {
> +		struct xsk_addrs *xsk_addr;
> +
> +		xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
> +		if (!xsk_addr)
> +			return -ENOMEM;
> +		xsk_addr->addrs[0] = addr;
> +		skb_shinfo(skb)->destructor_arg = (void *)xsk_addr;
> +		return 0;
> +	}
> +
>  	skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);
> +	return 0;

I think this is gonna be a 3rd copy paste of the same logic? Let's
move to a new helper and replace existing kmem_cache_zalloc places?

xsk_skb_destructor_alloc_list(prev_addr) ?