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

Jason Xing
2026-05-02T20:10:54Z

On Fri, May 1, 2026 at 6:29 AM Stanislav Fomichev <sdf.kernel@gmail.com> wrote:
>
> 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

My primary concern is this patch would be a little bit more
complicated (more insertions/deletions). IIUC, the fix should be as
simple as possible to easily get backported while we can do some
cleanup work on the next branch.

Please see this new version (v5) with your advice applied. Does it look good?
https://lore.kernel.org/all/20260502200722.53960-9-kerneljasonxing@gmail.com/

Thanks,
Jason