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

On 04/29, Jason Xing wrote:

On Wed, Apr 29, 2026 at 6:14 PM Stanislav Fomichev [off-list ref] wrote:

quoted

On 04/29, Jason Xing wrote:

quoted

On Wed, Apr 29, 2026 at 2:11 AM Stanislav Fomichev [off-list ref] wrote:

quoted

On 04/24, Jason Xing wrote:

quoted

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/ (local)
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?

Because 0x1UL is the particular semantic applied on a 64-bit arch.
xsk_skb_destructor_set_addr() sets it while
xsk_skb_destructor_is_addr() recognizes it. They are a pair.

As you noticed, one liner works but is not that appropriate: on a
32-bit arch, this member should be either a NULL point or a valid
pointer pointing to a memory region. Testing if it's NULL can be
helpful as to the long term maintenance because of its readability and
robustness/safety.

The error path in allocation of skb is really complex, which is why
I'm so cautious to take care of it :)

Let's cleanup the error path instead of adding more complexity? Similar to what
you do with your "xsk: fix xsk_addrs slab leak on multi-buffer error path",
but maybe add a few NULL checks?

Good suggestion. I think I can cook a follow up patch to do such a
thing targetting net-next tree. This patch 8 belongs to net material
which means it will be backported to the older stable kernel as soon
as it gets merged. IIUC, the better way is to make it as simple as
possible?

quoted

Instead of 32 vs 64, I'd like to reason about whether destructor_arg
is an address or an allocated array (not whether we have 1 or >1
descriptors). And we special case 32 bit by always allocating it.

Haven't checked, but maybe this is all you need (besides your _set_addr
changes)?

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 6149f6a79897..03f217e85d31 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c

@@ -589,6 +589,8 @@ static u32 xsk_get_num_desc(struct sk_buff *skb)
                return 1;

        xsk_addr = (struct xsk_addrs *)skb_shinfo(skb)->destructor_arg;
+       if (!xsk_addr)
+               return 0;

        return xsk_addr->num_descs;
 }

Right, as I mentioned, how about posting a new cleanup patch with your
suggested-by tag?

quoted

quoted

I've noticed the status has been changed to 'changes requested'. Does
that mean one way or another I have to post a new version?

That wasn't me :-) From my POW, patches 1-7 are good to go..

Great! Thanks for the review. My hope is to get this series merged
soon in the net tree.

quoted

quoted

quoted

quoted

 }

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

[..]

quoted

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

Any comments on this?

I didn't comment on this because I thought I was not that sure if we
needed to wrap it up in the stable kernels :)

Of course, it would be easier for me to work on the net-next tree to
make the code look
more neat and elegant.

Are you concerned that you're gonna break something in the net tree? Why not
do it properly from the start? If you want, you can post 1-7 patches
separately and we follow up with this one but still into net?