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
