Re: [PATCH net v2 2/3] virtio_net: Disable AF_XDP pools on queues above curr_queue_pairs

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In the final state of the series, the widened qid range in
  virtnet_xsk_pool_disable() from this patch combines with the restore…
- [Low] The commit message names only one trigger: explicitly removing
  XDP and then closing the socket.

--

virtio_net: Disable AF_XDP pools on queues above curr_queue_pairs

This patch changes the qid check in virtnet_xsk_pool_disable() to use
max_queue_pairs instead of curr_queue_pairs. With this change, an AF_XDP
zero-copy pool bound to an XDP_TX queue can still be unbound after the XDP
program is removed. Before this patch, the core freed the pool while
rq->xsk_pool still pointed to it, which caused a use-after-free on refill.

> When the program is removed, the queue count goes down again while the
> pool stays bound to the driver. When the socket is closed afterwards,
> virtnet_xsk_pool_disable() rejects the queue with -EINVAL, and the core
> frees the pool anyway:

[Severity: Low]

Is explicit XDP removal followed by closing the socket the only trigger?

Ordinary netdev unregistration looks like it ends in the same failed
disable. Examples are virtio-net hot-unplug, driver unbind and module
unload. This needs an XDP program attached and a zero-copy socket bound
to one of the XDP_TX queues:

unregister_netdevice_many_notify()
    dev_xdp_uninstall(dev)
        virtnet_xdp_set(NULL)    <- lowers curr_queue_pairs
    call_netdevice_notifiers(NETDEV_UNREGISTER, dev)
        xsk_notifier()
            xp_clear_dev(xs->pool)
                xp_disable_drv_zc()
                    ndo_bpf(XDP_SETUP_XSK_POOL, NULL, qid)
                        virtnet_xsk_pool_disable()  <- -EINVAL before this patch

Before this patch, that path hit WARN(1, "Failed to disable zero-copy!")
in xp_disable_drv_zc(). It also left rq->xsk_pool, sq->xsk_pool, the
posted XSK buffers and the pool DMA map in place.

The new max_queue_pairs bound fixes this path as well. The commit message
only hints at it with "Removing XDP must not fail". Could the message
also mention the unregister path?

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f5d8812f75b0..f9d227060375c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -6030,7 +6030,8 @@ static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
>  	struct send_queue *sq;
>  	int err;
>  
> -	if (qid >= vi->curr_queue_pairs)
> +	/* Removing XDP can leave the pool on a queue above curr_queue_pairs */
> +	if (qid >= vi->max_queue_pairs)
>  		return -EINVAL;

[Severity: Medium]

How does this wider qid range interact with the restore fallback that a
later patch in this series adds to this function? That patch is
"virtio_net: Keep AF_XDP zero-copy pools bound across suspend", and the
fallback looks like this:

drivers/net/virtio_net.c:virtnet_xsk_pool_disable() {
    ...
	pool = rq->xsk_pool;
	if (!pool) {
		/* Binding it again on restore failed, only the DMA map is left */
		pool = xsk_get_pool_from_qid(dev, qid);
		if (pool)
			xsk_pool_dma_unmap(pool, 0);
		return 0;
	}
    ...
}

xsk_get_pool_from_qid() only returns a pool for a qid below
real_num_rx_queues or real_num_tx_queues:

net/xdp/xsk.c:xsk_get_pool_from_qid() {
	if (queue_id < dev->real_num_rx_queues)
		return dev->_rx[queue_id].pool;
	if (queue_id < dev->real_num_tx_queues)
		return dev->_tx[queue_id].pool;

	return NULL;
}

This patch now lets those same qids through after XDP removal. Consider
this sequence:

1. XDP is attached. virtnet_xdp_set() raises curr_queue_pairs and
   real_num_rx_queues, and real_num_tx_queues stays at the original
   count.

2. A zero-copy socket binds to a qid in the XDP_TX range.

3. On suspend, virtnet_xsk_pools_release() leaves the pool DMA mapped,
   because qid < curr_queue_pairs.

4. On resume, virtnet_xsk_pools_attach()->virtnet_xsk_pool_attach()
   fails. Possible causes are a kvzalloc()/kzalloc() failure, a TX header
   DMA mapping error or an xdp_rxq_info_reg() failure. Only a warning is
   printed. The pool stays DMA mapped and rq->xsk_pool is NULL.

5. XDP is removed. curr_queue_pairs, real_num_rx_queues and
   real_num_tx_queues are now all at or below qid.

6. The socket is closed. The qid passes the new max_queue_pairs check
   and rq->xsk_pool is NULL. xsk_get_pool_from_qid() returns NULL, so
   the unmap is skipped and 0 is returned.

The core then frees the pool through xp_release_deferred()->xp_destroy(),
which never unmaps:

net/xdp/xsk_buff_pool.c:xp_destroy() {
    ...
	kvfree(pool->tx_descs);
	kvfree(pool->heads);
	kvfree(pool);
}

Does this leak the xsk_dma_map, its dma_pages array and the DMA or IOMMU
mappings of the umem pages? Would the umem pages then be unpinned and
freed while they are still mapped for the device?

A few possible fixes:

- read dev->_rx[qid].pool directly in the fallback, which is bounded by
  num_rx_queues
- keep a driver-side pointer to a pool that is mapped but not attached
- unmap on restore when the attach fails

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108193845.73621-1-yanhaishuang%40cmss.chinamobile.com