Re: [PATCH RFC net-next v4 13/14] xsk: retire old xmit path in copy mode

On Wed, Apr 15, 2026 at 5:18 PM <[email protected]> wrote:
>
> Sashiko AI review found 3 potential issue(s):
> - [Critical] Time-of-Check to Time-of-Use (TOCTOU) Use-After-Free (UAF) vulnerability on `batch->desc_cache`.
> - [High] Leaked socket structure on error path in `xsk_create()`.
> - [High] Unbounded `kmalloc` size for user-controlled batch size triggers `WARN_ON` and potential panic.
> --
>
> commit 5f2cebbff2efe715b882067f337741749751ac65
> Author: Jason Xing <[email protected]>
>
> xsk: retire old xmit path in copy mode
>
> This commit adds a new helper xsk_init_batch() with a default value of 1,
> and obsoletes __xsk_generic_xmit() by substituting it with
> __xsk_generic_xmit_batch() in the generic transmit path.
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 16e47f6fc30ec..232e7bf96b342 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> [ ... ]
> > @@ -1030,101 +1030,14 @@ static int __xsk_generic_xmit_batch(struct xdp_sock *xs)
> >       return err;
> >  }
> >
> [ ... ]
> >  static int xsk_generic_xmit(struct sock *sk)
> >  {
> > +     struct xdp_sock *xs = xdp_sk(sk);
> >       int ret;
> >
> >       /* Drop the RCU lock since the SKB path might sleep. */
> >       rcu_read_unlock();
> > -     ret = __xsk_generic_xmit(sk);
> > +     ret = __xsk_generic_xmit_batch(xs);
>
> By switching to __xsk_generic_xmit_batch(), does this expose a
> use-after-free race with xsk_setsockopt()?
>
> In __xsk_generic_xmit_batch(), the local pointer descs is assigned
> batch->desc_cache before acquiring xs->mutex.
>
> Concurrently, a user can call setsockopt(..., XDP_GENERIC_XMIT_BATCH),
> which acquires xs->mutex and updates the batch size via xsk_init_batch().
> This safely frees the old desc_cache array using kvfree() in
> xsk_batch_reset() because the setsockopt path does not check for
> xs->state != XSK_READY.
>
> Once setsockopt drops the mutex, the blocked __xsk_generic_xmit_batch()
> thread acquires the mutex and proceeds to use its stale descs pointer.
> Can this lead to a use-after-free write into the freed descriptor array?

Ack. This is the issue we've discussed previously. I will move those
read operations underneath the mutex lock in
__xsk_generic_xmit_batch().

>
> >       /* Reaquire RCU lock before going into common code. */
> >       rcu_read_lock();
> [ ... ]
> > @@ -1620,6 +1533,34 @@ struct xdp_umem_reg_v1 {
> >       __u32 headroom;
> >  };
> >
> > +static int xsk_init_batch(struct xsk_batch *batch, unsigned int size)
> > +{
> > +     struct xdp_desc *descs;
> > +     struct sk_buff **skbs;
> > +     void **data;
> > +
> > +     skbs = kmalloc(size * sizeof(struct sk_buff *), GFP_KERNEL);
>
> Can this trigger a WARN_ON_ONCE in the page allocator if a user requests
> a very large batch size?
>
> The size parameter is controlled by userspace via the XDP_GENERIC_XMIT_BATCH
> setsockopt and is bounded by the TX ring size (xs->max_tx_budget), which
> can be quite large.
>
> Since these pointer arrays can exceed KMALLOC_MAX_SIZE, should this be
> allocated using kvmalloc_array() similar to how the descs array is
> allocated below?

This is also what you pointed out. Even though I thought it's trivial,
I decided to use the _array() to keep safe.

>
> > +     if (!skbs)
> > +             return -ENOMEM;
> > +
> > +     data = kmalloc_array(size, sizeof(void *), GFP_KERNEL);
> > +     if (!data) {
> > +             kfree(skbs);
> > +             return -ENOMEM;
> > +     }
> > +
> > +     descs = kvcalloc(size, sizeof(struct xdp_desc), GFP_KERNEL);
> [ ... ]
> > @@ -2065,6 +1984,11 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
> >       if (!sk)
> >               return -ENOBUFS;
> >
> > +     xs = xdp_sk(sk);
> > +     ret = xsk_init_batch(&xs->batch, 1);
> > +     if (ret)
> > +             return ret;
> > +
>
> Does this leak the sk object if xsk_init_batch() fails?
>
> If xsk_init_batch() returns an error here, the function returns early
> without calling sk_free(sk).

My bad. I should have noticed this....

I'm totally getting lost in so many checks and conditions while
refactoring the xmit path...

Thanks,
Jason


>
> Because sock_init_data() has not been executed yet to link the sk to the
> sock structure, the VFS socket layer will not free the socket memory when
> tearing down the failed creation. Should this path clean up the allocated
> socket before returning?
>
> >       sock->ops = &xsk_proto_ops;
> >
> >       sock_init_data(sock, sk);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13