Re: [PATCH net 2/2] 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: stale DMA addresses in free_list after unaligned
  XSK pool rebind
- [High] virtio_net: uninitialized xsk_hdr_dma_addr used in
  virtnet_xsk_pool_enable()
- [High] virtio_net: race between RX NAPI and xsk_pool_dma_unmap() in
  virtnet_freeze()
- [Medium] virtio_net: virtnet_xsk_pool_disable() fails for qid >=
  curr_queue_pairs
--

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

virtio_net: Keep AF_XDP zero-copy pools bound across suspend

This commit unbinds the driver side of active AF_XDP zero-copy buffer pools
before freezing virtio_net queues and rebinds them after restore.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4f5d8812f75b0..5995ad730a91a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -6052,6 +6055,42 @@ static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
>  	return err;
>  }
>  
> +/*
> + * Freeze frees the receive and send queues while the AF_XDP sockets stay
> + * bound to the device, so release the driver side of the zero-copy pools
> + * before and bind them again after restore.
> + */
> +static void __maybe_unused virtnet_xsk_pools_unbind(struct virtnet_info *vi)
> +{
> +	int i;
> +
> +	ASSERT_RTNL();
> +
> +	for (i = 0; i < vi->max_queue_pairs; i++)
> +		if (vi->rq[i].xsk_pool)
> +			virtnet_xsk_pool_disable(vi->dev, i);
> +}

[Severity: Medium]
Does virtnet_xsk_pool_disable() fail without unbinding the pool when
i >= vi->curr_queue_pairs?

Attaching an XDP program in virtnet_xdp_set() expands vi->curr_queue_pairs
and dev->real_num_rx_queues by xdp_qp, allowing an AF_XDP zero-copy pool to
be bound to a queue in that upper range, and detaching the XDP program
shrinks vi->curr_queue_pairs back down without unbinding the pool.  When
virtnet_xsk_pools_unbind() iterates up to vi->max_queue_pairs,
virtnet_xsk_pool_disable() rejects any qid >= vi->curr_queue_pairs:

drivers/net/virtio_net.c:virtnet_xsk_pool_disable() {
	if (qid >= vi->curr_queue_pairs)
		return -EINVAL;
}

As a result, remove_vq_common() frees vi->rq and vi->sq while leaving
rq->xsk_rxq_info registered, the pool DMA mapping active, and rq->xsk_buffs
and sq->xsk_hdr leaked.

> +
> +static void __maybe_unused virtnet_xsk_pools_rebind(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;
> +
> +		err = virtnet_xsk_pool_enable(vi->dev, pool, i);

[Severity: High]
When pool->unaligned is true, does rebinding the pool after freeze leave
stale pre-suspend DMA addresses in pool->free_list?

During virtnet_freeze(), virtnet_xsk_pool_disable() resets rq->vq and passes
each posted RX buffer to xsk_buff_free(), which places the xdp_buff_xsk
entries onto pool->free_list before xsk_pool_dma_unmap() unmaps and frees
the old DMA mapping.  On restore, virtnet_xsk_pool_enable() calls
xsk_pool_dma_map() -> xp_dma_map() -> xp_init_dma_info(), which skips
reinitializing xskb->dma when pool->unaligned is set:

net/xdp/xsk_buff_pool.c:xp_init_dma_info() {
	if (!pool->unaligned) {
		u32 i;

		for (i = 0; i < pool->heads_cnt; i++) {
			struct xdp_buff_xsk *xskb = &pool->heads[i];
			u64 orig_addr;

			orig_addr = xskb->xdp.data_hard_start - pool->addrs - pool->headroom;
			xp_init_xskb_dma(xskb, pool, dma_map->dma_pages, orig_addr);
		}
	}
}

When virtnet_rq_bind_xsk_pool() refills rq->vq via virtnet_rx_resume() ->
try_fill_recv() -> virtnet_add_recvbuf_xsk(), xsk_buff_alloc_batch() pops
those recycled entries from pool->free_list via xp_alloc_reused() without
updating xskb->dma, programming rq->vq with the unmapped pre-suspend DMA
addresses and causing IOMMU faults or DMA memory corruption.

> +		if (err)
> +			netdev_warn(vi->dev,
> +				    "failed to rebind AF_XDP pool to queue %d: %d\n",
> +				    i, err);
> +	}
> +}

[Severity: High]
Can virtnet_xsk_pool_enable() transmit from sq->xsk_pool during restore
before sq->xsk_hdr_dma_addr is initialized?

On restore, init_vqs() allocates fresh zeroed send queues (so
sq->xsk_hdr_dma_addr is 0), while bound AF_XDP sockets are already on
pool->xsk_tx_list and may have pending descriptors in their TX rings.
virtnet_xsk_pool_enable() calls virtnet_sq_bind_xsk_pool() before setting
sq->xsk_hdr_dma_addr:

drivers/net/virtio_net.c:virtnet_xsk_pool_enable() {
    ...
	err = virtnet_sq_bind_xsk_pool(vi, sq, pool);
	if (err)
		goto err_sq;

	/* Now, we do not support tx offload(such as tx csum), so all the tx
	 * virtnet hdr is zero. So all the tx packets can share a single hdr.
	 */
	sq->xsk_hdr_dma_addr = hdr_dma;
    ...
}

When the device is running, virtnet_sq_bind_xsk_pool() sets sq->xsk_pool =
pool and calls virtnet_tx_resume() -> virtnet_napi_tx_enable() ->
virtnet_napi_do_enable(), which schedules sq->napi and runs virtnet_poll_tx()
synchronously on local_bh_enable().  virtnet_poll_tx() -> virtnet_xsk_xmit()
-> virtnet_xsk_xmit_one() then submits sq->xsk_hdr_dma_addr (0) as the
virtio header DMA address to the virtqueue.

[ ... ]

> @@ -7246,6 +7285,10 @@ static __maybe_unused int virtnet_freeze(struct virtio_device *vdev)
>  {
>  	struct virtnet_info *vi = vdev->priv;
>  
> +	rtnl_lock();
> +	virtnet_xsk_pools_unbind(vi);
> +	rtnl_unlock();
> +
>  	virtnet_cpu_notif_remove(vi);
>  	virtnet_freeze_down(vdev);

[Severity: High]
Can unbinding the pools before virtnet_freeze_down() race with RX NAPI
redirecting to the still-bound AF_XDP socket while xsk_pool_dma_unmap()
frees pool->dma_pages?

Because virtnet_xsk_pools_unbind() runs while netif_running(vi->dev) is
true and before virtnet_freeze_down() closes the device,
virtnet_rq_bind_xsk_pool(vi, rq, NULL) clears rq->xsk_pool and calls
virtnet_rx_resume(vi, rq, true), which refills rq->vq with page_pool
buffers and re-enables and schedules rq->napi before xsk_pool_dma_unmap()
runs.  Unlike normal socket teardown, the AF_XDP socket is still bound in
xsks_map, sorq->xdp_prog can redirect received page_pool packets into
xsk_rcv(), which falls back to copy mode (__xsk_rcv() -> xsk_buff_alloc())
while xsk_pool_dma_unmap() concurrently tears down the pool's DMA state:

CPU0                              CPU1 (<softirq>)
virtnet_freeze()
  virtnet_xsk_pools_unbind()
    virtnet_xsk_pool_disable()
      virtnet_rq_bind_xsk_pool(..., NULL)
        rq->xsk_pool = NULL
        virtnet_rx_resume()
          virtnet_napi_enable(rq)
                                  virtnet_poll()
                                    ...
                                    xsk_rcv()
                                      __xsk_rcv()
                                        xsk_buff_alloc(xs->pool)
                                          xp_alloc()
      xsk_pool_dma_unmap(pool, 0)
        xp_dma_unmap()
          kvfree(pool->dma_pages)
                                            xp_get_xskb()
                                              xp_init_xskb_dma(...,
                                                pool->dma_pages, ...)
                                                <-- use-after-free
          pool->dma_pages = NULL
          pool->dev = NULL

In addition to reading freed pool->dma_pages in xp_check_unaligned() and
xp_init_xskb_dma(), xp_alloc() can call xp_dma_sync_for_cpu() as pool->dev
and the DMA mappings are being cleared, or leave uninitialized xskb->dma
entries on pool->free_list when the socket consumes the copied frame.

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