Re: [PATCH RFC net-next v4 14/14] xsk: optimize xsk_build_skb for batch copy-mode fast path

On Wed, Apr 15, 2026 at 5:47 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [High] An SKB memory leak occurs when xsk_build_skb() returns an error for a newly assigned SKB.
> - [Medium] The prefetch instruction uses the raw AF_XDP descriptor address without extracting the offset, creating an invalid pointer in unaligned mode.
> --
>
> commit 7bdeef673e571c57c404cb8d05446f46b3e2676e
> Author: Jason Xing <[email protected]>
>
> xsk: optimize xsk_build_skb for batch copy-mode fast path
>
> This commit introduces three targeted optimizations for the batch
> copy-mode TX hot path in XDP sockets, including replacing skb_store_bits()
> with memcpy(), inlining UMEM address computation, and prefetching the
> next descriptor's UMEM data.
>
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index 3b3969adfd23f..0fccb0c8cbe90 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -751,14 +751,28 @@ int xsk_alloc_batch_skb(struct xdp_sock *xs, u32 nb_pkts, u32 nb_descs, int *err
> >       if (total_truesize)
> >               refcount_add(total_truesize, &xs->sk.sk_wmem_alloc);
> >
> > -     /* Phase 3: Build SKBs with packet data */
> > +     /* Phase 3: Build SKBs with packet data. */
> > +     struct xsk_buff_pool *pool = xs->pool;
> > +     void *pool_addrs = pool->addrs;
> > +     bool unaligned = pool->unaligned;
> > +
> >       for (j = 0; j < alloc_descs; j++) {
> > +             u64 addr = descs[j].addr;
> > +             void *buffer;
> > +
> > +             if (unaligned)
> > +                     addr = xp_unaligned_add_offset_to_addr(addr);
> > +             buffer = pool_addrs + addr;
> > +
> > +             if (j + 1 < alloc_descs)
> > +                     prefetch(pool_addrs + descs[j + 1].addr);
>
> Does this prefetch use the raw AF_XDP descriptor address without extracting
> the offset?
>
> In unaligned mode, an AF_XDP descriptor addr field encodes a chunk offset
> in its upper 16 bits. While xp_unaligned_add_offset_to_addr() is used to
> decode the current descriptor address, the prefetch directly adds
> descs[j + 1].addr to pool_addrs.
>
> Could this result in an out-of-bounds pointer in unaligned mode, failing to
> hide memory latency and causing cache pollution?

Will handle unaligned mode soon.

>
> > +
> >               if (!xs->skb) {
> >                       skb = skbs[skb_count - 1 - k];
> >                       k++;
> >               }
> >
> > -             skb = xsk_build_skb(xs, skb, &descs[j]);
> > +             skb = xsk_build_skb(xs, skb, &descs[j], buffer);
> >               if (IS_ERR(skb)) {
> >                       *err = PTR_ERR(skb);
> >                       break;
>
> Can this leak the newly assigned SKB if xsk_build_skb() returns an error?

This patch doesn't introduce any new stuff here...

>
> If xsk_build_skb() fails, it returns ERR_PTR(err) and the loop breaks. Since
> k was already incremented, the cleanup loop in Phase 4 (while (k < i)
> kfree_skb(...)) reclaims unused SKBs starting from the current k, skipping
> the failed SKB at index k - 1.
>
> Could a malicious userspace program exploit this to systematically leak SKBs
> by triggering metadata errors?

I confirm this claim is wrong because 1) xsk_build_skb() frees the skb
directly if frag is zero, or 2) __xsk_generic_xmit_batch() takes
responsibility to free xs->skb if frag is not zero and err is not
-EAGAIN.

Thanks,
Jason

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