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

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

On 04/29, Jason Xing wrote:
> On Wed, Apr 29, 2026 at 2:11 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> >
> > 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?
> 
> 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?

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;
 }

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

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

Any comments on this?