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 6:14 PM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
> >
> > 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?
> 
> 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?
> 
> >
> > 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?
> 
> >
> > > 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.
> 
> >
> > > >
> > > > >  }
> > > > >
> > > > >  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?
> 
> 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?