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

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] virtnet_xsk_pools_unbind() calls virtnet_xsk_pool_disable() for
  pools whose AF_XDP sockets are still XSK_BOUND.
- [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()
- [Medium] virtnet_xsk_pools_unbind()
  (drivers/net/virtio_net.c:6063-6072) loops over max_queue_pairs and…

--

virtio_net: Keep AF_XDP zero-copy pools bound across suspend

This patch releases the driver side of AF_XDP zero-copy pools in
virtnet_freeze() and binds them again in virtnet_restore(). The xsk core
keeps the pools across suspend, so the driver needs to drop and rebuild its
own state for them. It also makes virtnet_xsk_pool_disable() return early
when a queue has no pool.

> 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: High]
Does this work for a pool created with XDP_UMEM_UNALIGNED_CHUNK_FLAG?

On freeze, virtnet_xsk_pool_disable() resets the RX virtqueue with
virtnet_rq_unmap_free_buf(), which hands every posted buffer back with
xsk_buff_free(). Those xdp_buff_xsk entries go onto pool->free_list
with xskb->dma and xskb->frame_dma still pointing into the old
mapping. xsk_pool_dma_unmap() then tears that mapping down.

On restore, xsk_pool_dma_map() ends up in xp_init_dma_info(), but that
only refreshes the per-buffer DMA addresses for aligned pools:

	if (!pool->unaligned) {
		...
		for (i = 0; i < pool->heads_cnt; i++) {
			...
			xp_init_xskb_dma(xskb, pool, ...);
		}
	}

For unaligned pools the address is filled in by xp_get_xskb() when a
buffer is taken from the fill queue. Buffers recycled from
pool->free_list in xp_alloc_reused() or xp_alloc() are handed out as
they are.

So when virtnet_rq_bind_xsk_pool() refills the ring through
virtnet_rx_resume()->try_fill_recv()->virtnet_add_recvbuf_xsk(),
won't xsk_buff_xdp_get_dma() return the pre-suspend address for the
recycled buffers? With an IOMMU or swiotlb, that address is no longer
mapped or may already belong to someone else. The device would then
either fault or write received data into the wrong memory.
xp_dma_sync_for_device() would also be called on the stale address.

The normal unbind path never hits this, because the pool is destroyed
together with the socket. Here the same pool is unmapped and mapped
again while buffers sit on its free_list.

Should the driver keep the pool DMA mapping across freeze instead of
dropping it? Or does the xsk core need to reinitialise the free_list
entries when an unaligned pool is mapped again?

[Severity: Medium]

What happens here for a pool bound to a queue at or above
curr_queue_pairs? virtnet_xsk_pool_disable() returns before it releases
anything:

virtnet_xsk_pool_disable() {
	...
	if (qid >= vi->curr_queue_pairs)
		return -EINVAL;
	...
}

The return value is dropped here, so nothing is reported.

This state looks reachable. With native XDP attached, curr_queue_pairs is
curr_qp + nr_cpu_ids and real_num_rx_queues goes up to match. A zero-copy
socket can then bind to one of the XDP queues, because
virtnet_xsk_pool_enable() only requires qid < curr_queue_pairs.

When XDP is detached, virtnet_xdp_set() lowers curr_queue_pairs without
checking for bound pools:

	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;

On a later suspend, remove_vq_common()->virtnet_free_queues() frees vi->rq
and vi->sq. For those queues, wouldn't the pool DMA mapping,
rq->xsk_buffs, sq->xsk_hdr and the hdr DMA mapping still leak?

virtnet_xsk_pools_rebind() only walks curr_queue_pairs, so these queues
are not rebound on restore either. If the socket is closed after that, the
core gets -EINVAL back from virtnet_xsk_pool_disable() and warns "Failed
to disable zero-copy!". If XDP is attached again before the close, the new
!pool early return reports success and the umem DMA mapping is never
unmapped.

The missing check in virtnet_xdp_set() predates this patch. However, the
commit message says the patch releases "the driver side of all zero-copy
pools before freezing", and this case is still skipped without any error
being reported.

> +}

[ ... ]

> @@ -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();
> +

[Severity: High]

Can this race with the receive datapath? At this point the AF_XDP sockets
are still XSK_BOUND. The device is also still live, because
virtnet_freeze_down() has not run yet.

When the xsk core disables a pool itself, it quiesces the datapath first
in xsk_unbind_dev():

	WRITE_ONCE(xs->state, XSK_UNBOUND);

	/* Wait for driver to stop using the xdp socket. */
	xp_del_xsk(xs->pool, xs);
	synchronize_net();

Here, virtnet_xsk_pool_disable() does this instead:

	err = virtnet_rq_bind_xsk_pool(vi, rq, NULL);
	err |= virtnet_sq_bind_xsk_pool(vi, sq, NULL);

	xsk_pool_dma_unmap(pool, 0);

virtnet_rq_bind_xsk_pool(NULL) ends with virtnet_rx_resume(), which
refills the ring with page_pool buffers and re-enables NAPI. rq->xdp_prog
is still set, so an XDP_REDIRECT into the XSKMAP reaches xsk_rcv(). Since
the socket is still bound, xsk_rcv_check() passes and the copy path runs:

xsk_rcv()->__xsk_rcv()->xsk_buff_alloc()->xp_alloc()

xp_alloc() reads pool->dev and calls xp_dma_sync_for_device(). In
unaligned mode, xp_get_xskb() reads pool->dma_pages.

At the same time, xp_dma_unmap() can be running on another CPU. It unmaps
the pages, kvfree()s pool->dma_pages and sets pool->dev to NULL. RTNL does
not exclude the softirq.

Could this cause a use-after-free of pool->dma_pages, a DMA sync on an
address that was already unmapped (with swiotlb the bounce slot may have
been reused), or a NULL dev being passed to dma_sync_single_for_device()?

Restore seems to have the same window in reverse. virtnet_restore_up()
re-enables NAPI first. Then
virtnet_xsk_pools_rebind()->virtnet_xsk_pool_enable() calls
xsk_pool_dma_map() before virtnet_rq_bind_xsk_pool() pauses NAPI.

xp_init_dma_info() publishes pool->dev before it fills in the dma_pages
contents. A concurrent copy-mode receive, for example from a generic XDP
program still attached to the netdev, could then see a half-initialised
pool.

Before this patch, the driver's enable and disable paths only ran while the
socket was not bound, so this concurrency could not happen.

>  	virtnet_cpu_notif_remove(vi);
>  	virtnet_freeze_down(vdev);
>  	remove_vq_common(vi);

[ ... ]

> @@ -7270,6 +7313,10 @@ static __maybe_unused int virtnet_restore(struct virtio_device *vdev)
>  		return err;
>  	}
>  
> +	rtnl_lock();
> +	virtnet_xsk_pools_rebind(vi);
> +	rtnl_unlock();
> +
>  	return 0;
>  }
>  

[Severity: High]
Is the ordering in virtnet_xsk_pool_enable() still safe now that it
can be called for a pool whose sockets are already bound?

virtnet_restore_up() allocates a new, zeroed vi->sq array, so
sq->xsk_hdr_dma_addr is 0 when virtnet_xsk_pools_rebind() runs.
virtnet_xsk_pool_enable() maps the header into the local hdr_dma.
However, it only stores it in sq->xsk_hdr_dma_addr after
virtnet_sq_bind_xsk_pool() has returned.

virtnet_sq_bind_xsk_pool() sets sq->xsk_pool and then calls
virtnet_tx_resume(), which goes through virtnet_napi_tx_enable() to
virtnet_napi_do_enable(). That schedules the TX NAPI and runs it from
local_bh_enable(), so virtnet_poll_tx() can execute before
virtnet_sq_bind_xsk_pool() returns. A wakeup from userspace through
virtnet_xsk_wakeup() or a TX interrupt can get there as well.

virtnet_poll_tx() sees sq->xsk_pool set and calls virtnet_xsk_xmit().
On the initial bind there is nothing to send at that point, because
xp_assign_dev() calls ndo_bpf before the socket is added to
pool->xsk_tx_list. On restore the sockets are already on xsk_tx_list
and may still have descriptors queued in their TX ring from before
suspend. virtnet_xsk_xmit_one() would then pass sq->xsk_hdr_dma_addr,
which is still 0, to sg_fill_dma() and post it with
virtqueue_add_outbuf_premapped().

That hands DMA address 0 to the device as the virtio-net header for
every packet in the batch. Under an IOMMU this would fault. Without
one, the device would send whatever happens to be at that address as
the header.

The ordering predates this patch. Before it, though, the driver enable
path only ran before the socket was on xsk_tx_list. Should
sq->xsk_hdr_dma_addr be set before virtnet_sq_bind_xsk_pool() is
called, with the error path clearing it again?

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