[PATCH net 1/2] virtio_net: check AF_XDP queue index before use

[PATCH net 1/2] virtio_net: check AF_XDP queue index before use

[Xiong Weimin]

Validate the AF_XDP queue index before dereferencing the receive or
send queue arrays in virtnet_xsk_pool_enable().  This keeps an out of
range queue id from reaching vi->rq[qid] while checking page_pool.

Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
 drivers/net/virtio_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3e2a5876c..6160aa8ba 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -5896,15 +5896,15 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
 	if (vi->hdr_len > xsk_pool_get_headroom(pool))
 		return -EINVAL;
 
+	if (qid >= vi->curr_queue_pairs)
+		return -EINVAL;
+
 	/* In big_packets mode, xdp cannot work, so there is no need to
 	 * initialize xsk of rq.
 	 */
 	if (!vi->rq[qid].page_pool)
 		return -ENOENT;
 
-	if (qid >= vi->curr_queue_pairs)
-		return -EINVAL;
-
 	sq = &vi->sq[qid];
 	rq = &vi->rq[qid];
 
-- 
2.43.0

[PATCH net 2/2] virtio_net: unmap AF_XDP header with tx virtqueue

[Xiong Weimin]

virtnet_xsk_pool_enable() maps the shared AF_XDP transmit header with
the send virtqueue, and the disable path unmaps it with the same queue.
Use sq->vq on the setup error path as well so the DMA unmap matches the
mapping side.

Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6160aa8ba..a6f90f579 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -5959,7 +5959,7 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
 err_rq:
 	xsk_pool_dma_unmap(pool, 0);
 err_xsk_map:
-	virtqueue_unmap_single_attrs(rq->vq, hdr_dma, vi->hdr_len,
+	virtqueue_unmap_single_attrs(sq->vq, hdr_dma, vi->hdr_len,
 				     DMA_TO_DEVICE, 0);
 err_free_buffs:
 	kvfree(rq->xsk_buffs);
-- 
2.43.0

Re: [PATCH net 1/2] virtio_net: check AF_XDP queue index before use

[Jason Xing]

On Tue, Aug 4, 2026 at 6:11 PM Xiong Weimin <xiongweimin@kylinos.cn> wrote:
>
> Validate the AF_XDP queue index before dereferencing the receive or
> send queue arrays in virtnet_xsk_pool_enable().  This keeps an out of
> range queue id from reaching vi->rq[qid] while checking page_pool.
>
> Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>

Fixes: 24fbd3967f3f ("virtio_net: add page_pool support for buffer allocation")

Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>

Thanks!

> ---
>  drivers/net/virtio_net.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3e2a5876c..6160aa8ba 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5896,15 +5896,15 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
>         if (vi->hdr_len > xsk_pool_get_headroom(pool))
>                 return -EINVAL;
>
> +       if (qid >= vi->curr_queue_pairs)
> +               return -EINVAL;
> +
>         /* In big_packets mode, xdp cannot work, so there is no need to
>          * initialize xsk of rq.
>          */
>         if (!vi->rq[qid].page_pool)
>                 return -ENOENT;
>
> -       if (qid >= vi->curr_queue_pairs)
> -               return -EINVAL;
> -
>         sq = &vi->sq[qid];
>         rq = &vi->rq[qid];
>
> --
> 2.43.0
>
>

Re: [PATCH net 2/2] virtio_net: unmap AF_XDP header with tx virtqueue

[Jason Xing]

On Tue, Aug 4, 2026 at 5:22 PM Xiong Weimin <xiongweimin@kylinos.cn> wrote:
>
> virtnet_xsk_pool_enable() maps the shared AF_XDP transmit header with
> the send virtqueue, and the disable path unmaps it with the same queue.
> Use sq->vq on the setup error path as well so the DMA unmap matches the
> mapping side.
>
> Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>

Fixes: 21a4e3ce6dc7 ("virtio_net: xsk: bind/unbind xsk for tx")

Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>

Nice catch! Thanks for your work!

> ---
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6160aa8ba..a6f90f579 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5959,7 +5959,7 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
>  err_rq:
>         xsk_pool_dma_unmap(pool, 0);
>  err_xsk_map:
> -       virtqueue_unmap_single_attrs(rq->vq, hdr_dma, vi->hdr_len,
> +       virtqueue_unmap_single_attrs(sq->vq, hdr_dma, vi->hdr_len,
>                                      DMA_TO_DEVICE, 0);
>  err_free_buffs:
>         kvfree(rq->xsk_buffs);
> --
> 2.43.0
>
>

Re: [PATCH net 1/2] virtio_net: check AF_XDP queue index before use

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
virtio_net: check AF_XDP queue index before use

This moves the qid >= vi->curr_queue_pairs test in
virtnet_xsk_pool_enable() so that it runs before the vi->rq[qid].page_pool
check instead of after it.  The rest of the function is unchanged.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3e2a5876c6c8c..6160aa8ba595e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5896,15 +5896,15 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
>  	if (vi->hdr_len > xsk_pool_get_headroom(pool))
>  		return -EINVAL;
>  
> +	if (qid >= vi->curr_queue_pairs)
> +		return -EINVAL;
> +
>  	/* In big_packets mode, xdp cannot work, so there is no need to
>  	 * initialize xsk of rq.
>  	 */
>  	if (!vi->rq[qid].page_pool)
>  		return -ENOENT;
>  
> -	if (qid >= vi->curr_queue_pairs)
> -		return -EINVAL;
> -

[Severity: Medium]
The changelog says this "keeps an out of range queue id from reaching
vi->rq[qid] while checking page_pool".  Was an out of range qid actually
reachable here?

vi->rq and vi->sq are sized max_queue_pairs in virtnet_alloc_queues():

	vi->sq = kzalloc_objs(*vi->sq, vi->max_queue_pairs);
	...
	vi->rq = kzalloc_objs(*vi->rq, vi->max_queue_pairs);

and the netdev is created with alloc_etherdev_mq(..., max_queue_pairs), so
dev->num_rx_queues == dev->num_tx_queues == max_queue_pairs.

The only path into virtnet_xsk_pool_enable() is
xp_assign_dev() -> ndo_bpf(XDP_SETUP_XSK_POOL) -> virtnet_xsk_pool_setup(),
and xp_assign_dev() first goes through xsk_reg_pool_at_qid() in
net/xdp/xsk.c:

	if (queue_id >= max(dev->real_num_rx_queues,
			    dev->real_num_tx_queues))
		return -EINVAL;

real_num_rx_queues/real_num_tx_queues can never exceed num_rx_queues/
num_tx_queues, so any qid that reaches the driver is already inside the
allocated arrays and vi->rq[qid].page_pool was an in-bounds read.

For a qid in [curr_queue_pairs, max_queue_pairs) the old ordering still
rejected the request before sq/rq were used, so the only visible difference
after the reorder appears to be -EINVAL instead of -ENOENT.  Could the
changelog describe this as a defensive reordering rather than as preventing
an out of range dereference?  As written it reads like a memory safety fix
and may attract a stable backport.

[Severity: High]
This is a pre-existing issue and not something this patch introduces, but
since the patch touches the enable-side qid test, is the symmetric test in
virtnet_xsk_pool_disable() safe?

	if (qid >= vi->curr_queue_pairs)
		return -EINVAL;

	sq = &vi->sq[qid];
	rq = &vi->rq[qid];

On that path the early return skips virtnet_rq_bind_xsk_pool(vi, rq, NULL),
virtnet_sq_bind_xsk_pool(vi, sq, NULL), xsk_pool_dma_unmap(pool, 0), the
virtqueue_unmap_single_attrs() of sq->xsk_hdr_dma_addr and
kvfree(rq->xsk_buffs).

Can curr_queue_pairs shrink below a qid that already has a pool bound?
virtnet_xdp_set() raises it on attach:

	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
	if (err)
		goto err;
	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
	vi->xdp_queue_pairs = xdp_qp;

With max_queue_pairs=8, curr_qp=4 and xdp_qp=4 an XDP attach gives
curr_queue_pairs = real_num_rx_queues = 8, so a zero-copy socket can bind
to qid 5 (xsk_reg_pool_at_qid() accepts it, virtnet_xsk_pool_enable()
accepts it, and rq[5].page_pool is non-NULL because
virtnet_create_page_pools() covers all max_queue_pairs queues).  Detaching
the program then goes back to virtnet_set_queues(vi, curr_qp) with no
AF_XDP-in-use check, unlike the ethtool channel paths which are guarded by
netdev_queue_busy().

When the socket is closed, xp_release_deferred() -> xp_clear_dev() ->
xp_disable_drv_zc() calls ndo_bpf(XDP_SETUP_XSK_POOL, pool=NULL,
queue_id=5), which now hits qid(5) >= curr_queue_pairs(4) and returns
-EINVAL.  The core only warns:

	WARN(1, "Failed to disable zero-copy!");

so does this leak the kvzalloc'ed rq->xsk_buffs array, the xsk pool DMA
mapping and sq->xsk_hdr_dma_addr?

Beyond the leak, rq[5]->xsk_pool and sq[5]->xsk_pool still point at the
pool that xp_release_deferred() then frees via xp_destroy().  If the queue
count is grown again (XDP re-attach, or ethtool -L back to 8, which is now
allowed because xsk_clear_pool_at_qid() already cleared dev->_rx[5].pool),
the datapath dereferences it unconditionally in try_fill_recv():

	if (rq->xsk_pool) {
		err = virtnet_add_recvbuf_xsk(vi, rq, rq->xsk_pool, gfp);
		goto kick;
	}

and similarly in virtnet_receive() and virtnet_poll_tx().  Can that be a
use-after-free of the freed pool?

Related to that, rq->xsk_buffs is never set to NULL after kvfree() in
either virtnet_xsk_pool_disable() or the err_free_buffs path of
virtnet_xsk_pool_enable():

err_free_buffs:
	kvfree(rq->xsk_buffs);
	return err;

so its safety depends entirely on rq->xsk_pool having been cleared first.
Would it make sense to release the resources unconditionally in the
disable path, or to block shrinking curr_queue_pairs while a pool is bound?
-- 
pw-bot: cr

Re: [PATCH net 2/2] virtio_net: unmap AF_XDP header with tx virtqueue

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
virtio_net: unmap AF_XDP header with tx virtqueue

The patch changes the error unwind in virtnet_xsk_pool_enable() so that the
shared AF_XDP transmit header is unmapped with sq->vq instead of rq->vq,
matching the virtqueue used for the mapping.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6160aa8ba595e..a6f90f579b6e8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -5959,7 +5959,7 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
>  err_rq:
>  	xsk_pool_dma_unmap(pool, 0);
>  err_xsk_map:
> -	virtqueue_unmap_single_attrs(rq->vq, hdr_dma, vi->hdr_len,
> +	virtqueue_unmap_single_attrs(sq->vq, hdr_dma, vi->hdr_len,
>  				     DMA_TO_DEVICE, 0);

[Severity: Low]

The changelog says "Use sq->vq on the setup error path as well so the DMA
unmap matches the mapping side", which reads like a real wrong-virtqueue
unmap is being fixed.  Is there a configuration where the old rq->vq unmap
actually released a different mapping?

Earlier in virtnet_xsk_pool_enable() the function bails out unless both
virtqueues share one non-NULL dma dev:

	if (virtqueue_dma_dev(rq->vq) != virtqueue_dma_dev(sq->vq))
		return -EINVAL;

	dma_dev = virtqueue_dma_dev(rq->vq);
	if (!dma_dev)
		return -EINVAL;

So on any path that can reach err_xsk_map, rq->vq and sq->vq have the same
dma dev.  virtqueue_dma_dev() only returns non-NULL when the device has no
map ops, and virtqueue_unmap_single_attrs() in drivers/virtio/virtio_ring.c
then only looks at the per-device flag before going to
virtqueue_unmap_page_attrs():

	if (!vq->use_map_api)
		return;

	virtqueue_unmap_page_attrs(_vq, addr, size, dir, attrs);

use_map_api is set from vring_use_map_api(vdev), so it is the same for both
queues too.  With that, the emitted dma_unmap_page_attrs() call is identical
before and after the patch (same device, address, size and direction).

Would it be clearer to state "no functional change intended" and explain
that the two virtqueues are equivalent today, so this is a symmetry cleanup
that only starts to matter if the dma dev equality precondition is relaxed?
Alternatively, if a reachable mismatch does exist, could the changelog name
that configuration and carry a Fixes: tag?  As written, the message with no
Fixes: tag can still be picked up as a DMA fix by stable/AUTOSEL tooling.

Re: [PATCH net 2/2] virtio_net: unmap AF_XDP header with tx virtqueue

[Xiong Weimin]

On Thu, Aug 06, 2026 at 09:04:11AM -0700, Jakub Kicinski wrote:
> Would it be clearer to state "no functional change intended" ...?

Agreed - with the existing dma_dev equality check, rq->vq and sq->vq
are equivalent for this unmap today.  I will drop this as a standalone
"fix" and fold the sq->vq symmetry into the v2 series with an
explicit no-functional-change note (no Fixes: tag for that part).

Thanks,
Xiong Weimin

Re: [PATCH net 1/2] virtio_net: check AF_XDP queue index before use

[Xiong Weimin]

On Thu, Aug 06, 2026 at 09:04:10AM -0700, Jakub Kicinski wrote:
> Was an out of range qid actually reachable here?
> ...
> Could the changelog describe this as a defensive reordering rather than
> as preventing an out of range dereference?
> ...
> since the patch touches the enable-side qid test, is the symmetric test
> in virtnet_xsk_pool_disable() safe?

Thanks for the review.

You are right on both counts for this patch: qid is already bounded by
xsk_reg_pool_at_qid(), and the old ordering still rejected the request
before sq/rq were used.  The only visible difference was -EINVAL vs
-ENOENT for qid in [curr_queue_pairs, max_queue_pairs).  I will drop
this reorder (and the overstated changelog).

On the disable path / XDP detach interaction: I agree.  If
curr_queue_pairs shrinks while an AF_XDP pool is still bound,
virtnet_xsk_pool_disable() can return -EINVAL, skip cleanup, and leave
rq/sq->xsk_pool pointing at a pool that xp_clear_dev() then frees.

I will drop this series and post a v2 replacement as a new thread that:
1) lets disable clean up with a max_queue_pairs bounds check, and
2) refuses XDP-driven queue shrink while AF_XDP is still bound.

Thanks,
Xiong Weimin

Re: [PATCH net 1/2] virtio_net: check AF_XDP queue index before use

[Jason Xing]

On Fri, Aug 7, 2026 at 12:23 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> virtio_net: check AF_XDP queue index before use
>
> This moves the qid >= vi->curr_queue_pairs test in
> virtnet_xsk_pool_enable() so that it runs before the vi->rq[qid].page_pool
> check instead of after it.  The rest of the function is unchanged.
>
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 3e2a5876c6c8c..6160aa8ba595e 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -5896,15 +5896,15 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
> >       if (vi->hdr_len > xsk_pool_get_headroom(pool))
> >               return -EINVAL;
> >
> > +     if (qid >= vi->curr_queue_pairs)
> > +             return -EINVAL;
> > +
> >       /* In big_packets mode, xdp cannot work, so there is no need to
> >        * initialize xsk of rq.
> >        */
> >       if (!vi->rq[qid].page_pool)
> >               return -ENOENT;
> >
> > -     if (qid >= vi->curr_queue_pairs)
> > -             return -EINVAL;
> > -
>
> [Severity: Medium]
> The changelog says this "keeps an out of range queue id from reaching
> vi->rq[qid] while checking page_pool".  Was an out of range qid actually
> reachable here?
>
> vi->rq and vi->sq are sized max_queue_pairs in virtnet_alloc_queues():
>
>         vi->sq = kzalloc_objs(*vi->sq, vi->max_queue_pairs);
>         ...
>         vi->rq = kzalloc_objs(*vi->rq, vi->max_queue_pairs);
>
> and the netdev is created with alloc_etherdev_mq(..., max_queue_pairs), so
> dev->num_rx_queues == dev->num_tx_queues == max_queue_pairs.
>
> The only path into virtnet_xsk_pool_enable() is
> xp_assign_dev() -> ndo_bpf(XDP_SETUP_XSK_POOL) -> virtnet_xsk_pool_setup(),
> and xp_assign_dev() first goes through xsk_reg_pool_at_qid() in
> net/xdp/xsk.c:
>
>         if (queue_id >= max(dev->real_num_rx_queues,
>                             dev->real_num_tx_queues))
>                 return -EINVAL;
>
> real_num_rx_queues/real_num_tx_queues can never exceed num_rx_queues/
> num_tx_queues, so any qid that reaches the driver is already inside the
> allocated arrays and vi->rq[qid].page_pool was an in-bounds read.
>
> For a qid in [curr_queue_pairs, max_queue_pairs) the old ordering still
> rejected the request before sq/rq were used, so the only visible difference
> after the reorder appears to be -EINVAL instead of -ENOENT.  Could the
> changelog describe this as a defensive reordering rather than as preventing
> an out of range dereference?  As written it reads like a memory safety fix
> and may attract a stable backport.

Point taken. Sashiko is good at finding things like this stuff...

I don't see the necessity to keep this patch then.

Thanks,
Jason