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

On Sun, May 3, 2026 at 11:09 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] In the copy-mode TX path (`xsk_build_skb`), if a multi-buffer fragment exceeds the `MAX_SKB_FRAGS` limit, the buffer's address is leaked to userspace because `0` is erroneously submitted to the completion queue.
> --
>
> commit 4a776aefc60aa4d3123e7e63fd8f927f3748d008
> Author: Jason Xing <[email protected]>
>
> xsk: fix u64 descriptor address truncation on 32-bit architectures
>
> This commit fixes a 32-bit truncation issue when storing 64-bit descriptor
> addresses in the SKB destructor argument by allocating an xsk_addrs struct.
> It consolidates the allocation logic into xsk_addrs_alloc() and uses it in
> both zerocopy and copy modes.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 079abd4bcb69..5e5786cd9af5 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
>
> [ ... ]
>
> > @@ -929,19 +959,10 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >                       struct page *page;
> >                       u8 *vaddr;
> >
> > -                     if (xsk_skb_destructor_is_addr(skb)) {
> > -                             xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache,
> > -                                                          GFP_KERNEL);
> > -                             if (!xsk_addr) {
> > -                                     err = -ENOMEM;
> > -                                     goto free_err;
> > -                             }
> > -
> > -                             xsk_addr->num_descs = 1;
> > -                             xsk_addr->addrs[0] = xsk_skb_destructor_get_addr(skb);
> > -                             skb_shinfo(skb)->destructor_arg = (void *)xsk_addr;
> > -                     } else {
> > -                             xsk_addr = (struct xsk_addrs *)skb_shinfo(skb)->destructor_arg;
> > +                     xsk_addr = xsk_addrs_alloc(skb);
> > +                     if (!xsk_addr) {
> > +                             err = -ENOMEM;
> > +                             goto free_err;
> >                       }
> >
> >                       if (unlikely(nr_frags == (MAX_SKB_FRAGS - 1) && xp_mb_desc(desc))) {
>
> This isn't a regression introduced by this patch, but while looking at the
> modified code, does this error handling path leak the buffer's address to
> userspace?
>
> If this overflow check evaluates to true, the code sets err = -EOVERFLOW and
> jumps to the free_err label before assigning desc->addr to
> xsk_addr->addrs[xsk_addr->num_descs].
>
> In the free_err block, it handles -EOVERFLOW by calling
> xsk_inc_num_desc(xs->skb) and xsk_drop_skb(xs->skb). This increments
> num_descs to account for the dropped descriptor and eventually calls
> xsk_destruct_skb() which calls xsk_cq_submit_addr_locked().
>
> Since xsk_addr->addrs was never initialized with desc->addr for this
> descriptor, does this mean it submits 0 (from kmem_cache_zalloc) to the
> completion queue instead of the actual buffer address, causing userspace
> to permanently lose track of the buffer?
>
> By comparison, xsk_build_skb_zerocopy() assigns desc->addr to the array
> before checking the fragment limit:
>
> net/xdp/xsk.c:xsk_build_skb_zerocopy() {
>     ...
>                 xsk_addr = xsk_addrs_alloc(skb);
>                 if (!xsk_addr)
>                         return ERR_PTR(-ENOMEM);
>
>                 /* in case of -EOVERFLOW that could happen below,
>                  * xsk_consume_skb() will release this node as whole skb
>                  * would be dropped, which implies freeing all list elements
>                  */
>                 xsk_addr->addrs[xsk_addr->num_descs] = desc->addr;
>         }
>
>         len = desc->len;
>     ...
> }
>
> Should xsk_build_skb() also record the descriptor address before checking
> the -EOVERFLOW condition?

It has already been uncovered by you, maybe in v4 or v3 :) Sure, it
will be fixed after this patch gets merged.

Thanks,
Jason

>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8