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

Stanislav Fomichev
2026-04-28T23:11:37Z

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