Re: [PATCH net v2 3/3] virtio_net: Keep AF_XDP zero-copy pools bound across suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] virtio_net: virtnet_xsk_pools_attach() attaches unmapped AF_XDP
  pool
- [Medium] virtio_net: UAF on vi->rq in virtnet_xsk_pool_disable() after
  failed restore
- [Medium] virtio_net: DMA map leak in virtnet_xsk_pool_disable() after
  XDP detach
- [Medium] virtio_net: missing rtnl_lock() around xsk_pool_dma_unmap()
  on freeze
--

commit f8bbdffa48d34460262d93d81019cacaef0e299f
Author: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>

virtio_net: Keep AF_XDP zero-copy pools bound across suspend

This commit preserves AF_XDP zero-copy pools across suspend and resume by
releasing only the per-queue driver state on freeze while keeping the pools
DMA-mapped, then re-attaching them to the newly allocated queues on
restore.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index f9d227060375c..d9c920f421268 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -5808,6 +5810,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>  err_destroy_pools:
>  	virtio_reset_device(vdev);
>  	free_unused_bufs(vi);
> +	virtnet_xsk_pools_release(vi);
>  	virtnet_destroy_page_pools(vi);
>  	virtnet_del_vqs(vi);
>  	return err;
> @@ -6038,6 +6041,13 @@ static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
>  	rq = &vi->rq[qid];
>  
>  	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;
> +	}

[Severity: Medium]
Can virtnet_xsk_pool_disable() dereference a freed or NULL vi->rq after a
failed restore?

If virtnet_restore_up() fails (for example at err_destroy_pools, or
earlier in init_vqs()), virtnet_del_vqs() -> virtnet_free_queues() frees
vi->rq and vi->sq while the net_device and bound AF_XDP pools remain
registered.  When the socket is later closed, virtnet_xsk_pool_disable()
reads vi->rq[qid].xsk_pool from the dangling vi->rq pointer.

[Severity: Medium]
Can this fallback also leak the pool's DMA mappings if XDP is detached
before the socket is closed?

When virtnet_xsk_pool_attach() fails on restore for a pool bound to an XDP
queue (qid >= curr_qp), rq->xsk_pool is left NULL while the pool stays
DMA-mapped.  If the XDP program is then removed via virtnet_xdp_set(),
netif_set_real_num_rx_queues() lowers dev->real_num_rx_queues to
curr_qp <= qid (and dev->real_num_tx_queues is also curr_qp).  When the
socket is subsequently closed, xsk_get_pool_from_qid(dev, qid) returns
NULL because qid is no longer below real_num_rx_queues or
real_num_tx_queues, skipping xsk_pool_dma_unmap().

Unmapping the pool directly in virtnet_xsk_pools_attach() when
virtnet_xsk_pool_attach() fails would avoid leaving a half-bound DMA map
behind.

> @@ -6053,6 +6063,133 @@ static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
>  	return err;
>  }
>  
> +/*
> + * Freeze frees the receive and send queues, but the AF_XDP sockets stay
> + * bound and the xsk core keeps their pools. Release only the driver state
> + * of the zero-copy pools here, once the device is reset and the buffers
> + * posted to it are back in the pools. The pools stay DMA mapped, since the
> + * DMA device is still the same after restore and the buffers on their free
> + * lists keep their DMA addresses.
> + *
> + * A pool left above curr_queue_pairs by removing XDP is not bound again
> + * on restore, so unmap it as well.
> + */
> +static void virtnet_xsk_pools_release(struct virtnet_info *vi)
> +{
> +	int i;
> +
> +	for (i = 0; i < vi->max_queue_pairs; i++) {
> +		struct receive_queue *rq = &vi->rq[i];
> +		struct send_queue *sq = &vi->sq[i];
> +
> +		if (rq->xsk_pool) {
> +			if (i >= vi->curr_queue_pairs)
> +				xsk_pool_dma_unmap(rq->xsk_pool, 0);

[Severity: Medium]
Does calling xsk_pool_dma_unmap() here without holding rtnl_lock() race on
pool->umem->xsk_dma_list?

remove_vq_common() calls virtnet_xsk_pools_release() outside rtnl_lock()
during virtnet_freeze().  xp_dma_unmap() walks and deletes entries from
pool->umem->xsk_dma_list, which is protected by rtnl_lock.  If an xdp_umem
is shared across multiple devices via XDP_SHARED_UMEM, concurrent async
suspends can corrupt xsk_dma_list.

[ ... ]

> +/*
> + * On restore, set the zero-copy pools up again on the new queues before
> + * they are filled and their NAPI is enabled.
> + */
> +static void virtnet_xsk_pools_attach(struct virtnet_info *vi)
> +{
> +	struct xsk_buff_pool *pool;
> +	int i, err;
> +
> +	ASSERT_RTNL();
> +
> +	for (i = 0; i < vi->curr_queue_pairs; i++) {
> +		pool = xsk_get_pool_from_qid(vi->dev, i);
> +		if (!pool || !pool->umem->zc)
> +			continue;
> +
> +		/* The pool stays DMA mapped until it is disabled */
> +		err = virtnet_xsk_pool_attach(vi, i, pool);

[Severity: High]
Can virtnet_xsk_pools_attach() re-attach a pool that was already
DMA-unmapped by virtnet_xsk_pools_release()?

If a pool is bound to an XDP queue qid, detaching XDP lowers
vi->curr_queue_pairs below qid, and the next freeze unmaps the pool in
virtnet_xsk_pools_release() via xsk_pool_dma_unmap().  xp_dma_unmap() sets
pool->dma_pages = NULL and pool->dev = NULL, but pool->umem->zc and
dev->_rx[qid].pool remain set.  If XDP is re-attached after resume (raising
vi->curr_queue_pairs above qid again) and the system suspends a second
time, virtnet_xsk_pools_attach() sees pool->umem->zc == true and attaches
the unmapped pool to the queue, leading to a NULL pointer dereference on
pool->dma_pages or pool->dev during RX/TX.

Should this check pool->dma_pages instead of pool->umem->zc, or re-map the
pool when pool->dma_pages is NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108193845.73621-1-yanhaishuang@cmss.chinamobile.com?part=3