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

[sashiko-bot@kernel.org]

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 <kernelxing@tencent.com>

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?

>  	/* 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?

> +	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).

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/20260415082654.21026-1-kerneljasonxing@gmail.com?part=13