[PATCH net v2 0/3] virtio_net: AF_XDP zero-copy fixes for arm64 and suspend

[PATCH net v2 0/3] virtio_net: AF_XDP zero-copy fixes for arm64 and suspend

[Haishuang Yan]

Three AF_XDP zero-copy fixes for virtio_net.

Patch 1: on arm64, binding a zero-copy socket fails with -ENOMEM and a
"rejecting DMA map of vmalloc memory" warning whenever the device uses
the DMA API (e.g. VIRTIO_F_ACCESS_PLATFORM), because the static TX
header lives in the kernel image, which arm64 maps in the vmalloc area.

Patch 2: removing XDP can leave a zero-copy pool bound to a queue above
curr_queue_pairs. Closing the socket then fails in the driver, and the
queue is refilled from the freed pool.

Patch 3: freeze frees the rx/tx queues while the sockets stay bound in
the xsk core. The driver state of the pools is leaked, and closing the
socket after resume hits a "Missing register" warning and a NULL
pointer dereference in xp_dma_unmap(). v2 keeps the pools DMA mapped
across suspend and only moves the driver state to the new queues.

Freeze also drops the XDP program of virtio_net, so it has to be
attached again after resume. That is an older issue and this series
does not change it.

Testing used x86_64 KASAN kernels in QEMU/KVM (q35, intel-iommu,
virtio-net-pci with iommu_platform=on, x-pcie-pm-init=off) with a
small /init that binds an XDP_ZEROCOPY socket, redirects to it with an
XSKMAP program, and sends an ARP request through the TX ring and waits
for the reply on the RX ring, before suspend and after each of two
suspend cycles (pm_test=devices), then closes the socket:

- net: no TX or RX on the socket after resume, and an oops when it is
  closed. With a socket bound to an XDP queue and XDP removed, closing
  it gives "Failed to disable zero-copy!" and a KASAN use-after-free in
  xp_alloc_batch().
- v1: passes with an aligned umem, but with an unaligned umem nothing
  is received after resume. The XDP queue case behaves as on net.
- v2: all cases pass, without warnings, also with an unaligned umem and
  with a suspend cycle between removing XDP and closing the socket.

The test program is not part of the series; I can send it if useful.
Patch 1 is unchanged since v1, where it was tested on arm64 (QEMU TCG,
virt) with a KASAN kernel; arm64 was not tested again for v2. Real
hibernation and multiqueue traffic were not tested.

The issues were found with the help of an AI coding assistant.

Changes in v2:
- Patch 1: drop the sentence about cache lines from the changelog
  (Sashiko).
- New patch 2, for a pool left on a queue above curr_queue_pairs
  (Sashiko).
- Patch 3: keep the pools DMA mapped across suspend and set the driver
  state up on the new queues before NAPI is enabled, instead of
  disabling and enabling the pools while their sockets are still bound
  (Sashiko). This avoids a race with the RX path on freeze, stale DMA
  addresses with an unaligned umem, and transmitting before the TX
  header is mapped on restore.
- Rebased on net 30c71476d842.

v1: https://lore.kernel.org/netdev/20261107125723.33547-1-yanhaishuang@cmss.chinamobile.com/

Haishuang Yan (3):
  virtio_net: Don't DMA map the kernel image for the AF_XDP TX header
  virtio_net: Disable AF_XDP pools on queues above curr_queue_pairs
  virtio_net: Keep AF_XDP zero-copy pools bound across suspend

 drivers/net/virtio_net.c | 177 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 166 insertions(+), 11 deletions(-)


base-commit: 30c71476d842b094651bd7712f6c8ee8716147d0
-- 
2.43.0

[PATCH net v2 1/3] virtio_net: Don't DMA map the kernel image for the AF_XDP TX header

[Haishuang Yan]

virtnet_xsk_pool_enable() DMA maps the static xsk_hdr, which lives in
the kernel image. On arm64 the kernel image is mapped in the vmalloc
area, so when the device uses the DMA API the mapping is rejected:

  virtio_net virtio0: rejecting DMA map of vmalloc memory
  WARNING: drivers/virtio/virtio_ring.c:3882 ...

and binding an AF_XDP zero-copy socket fails with -ENOMEM.

Allocate the all-zero header per send queue when the pool is enabled
and free it when it is disabled.

This was found while testing AF_XDP in QEMU on arm64, not in
production. Tested in QEMU (TCG, arm64 virt, virtio-net-pci with
iommu_platform=on) with a KASAN kernel: binding an XDP_ZEROCOPY socket
fails without this patch and succeeds with it.

Fixes: 21a4e3ce6dc7 ("virtio_net: xsk: bind/unbind xsk for tx")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 drivers/net/virtio_net.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dd2c41ef6fbe..4f5d8812f75b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -315,6 +315,8 @@ struct send_queue {
 
 	struct xsk_buff_pool *xsk_pool;
 
+	/* All-zero header shared by the AF_XDP packets sent on this queue */
+	struct virtio_net_common_hdr *xsk_hdr;
 	dma_addr_t xsk_hdr_dma_addr;
 };
 
@@ -500,8 +502,6 @@ struct virtio_net_common_hdr {
 	};
 };
 
-static struct virtio_net_common_hdr xsk_hdr;
-
 static void virtnet_sq_free_unused_buf(struct virtqueue *vq, void *buf);
 static void virtnet_sq_free_unused_buf_done(struct virtqueue *vq);
 static int virtnet_xdp_handler(struct bpf_prog *xdp_prog, struct xdp_buff *xdp,
@@ -5971,11 +5971,21 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
 	if (!rq->xsk_buffs)
 		return -ENOMEM;
 
-	hdr_dma = virtqueue_map_single_attrs(sq->vq, &xsk_hdr, vi->hdr_len,
+	/*
+	 * Don't map a static header: the kernel image is not in the linear
+	 * map on all architectures (e.g. arm64), so it can't be DMA mapped.
+	 */
+	sq->xsk_hdr = kzalloc_obj(*sq->xsk_hdr);
+	if (!sq->xsk_hdr) {
+		err = -ENOMEM;
+		goto err_free_buffs;
+	}
+
+	hdr_dma = virtqueue_map_single_attrs(sq->vq, sq->xsk_hdr, vi->hdr_len,
 					     DMA_TO_DEVICE, 0);
 	if (virtqueue_map_mapping_error(sq->vq, hdr_dma)) {
 		err = -ENOMEM;
-		goto err_free_buffs;
+		goto err_free_hdr;
 	}
 
 	err = xsk_pool_dma_map(pool, dma_dev, 0);
@@ -6004,6 +6014,9 @@ static int virtnet_xsk_pool_enable(struct net_device *dev,
 err_xsk_map:
 	virtqueue_unmap_single_attrs(rq->vq, hdr_dma, vi->hdr_len,
 				     DMA_TO_DEVICE, 0);
+err_free_hdr:
+	kfree(sq->xsk_hdr);
+	sq->xsk_hdr = NULL;
 err_free_buffs:
 	kvfree(rq->xsk_buffs);
 	return err;
@@ -6032,6 +6045,8 @@ static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
 
 	virtqueue_unmap_single_attrs(sq->vq, sq->xsk_hdr_dma_addr,
 				     vi->hdr_len, DMA_TO_DEVICE, 0);
+	kfree(sq->xsk_hdr);
+	sq->xsk_hdr = NULL;
 	kvfree(rq->xsk_buffs);
 
 	return err;
-- 
2.43.0

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

[Haishuang Yan]

With an XDP program attached, virtnet_xdp_set() adds nr_cpu_ids queue
pairs for XDP_TX and raises curr_queue_pairs and real_num_rx_queues to
match. virtnet_xsk_pool_enable() accepts any queue below
curr_queue_pairs, so an AF_XDP zero-copy socket can be bound to one of
them.

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:

  WARNING: net/xdp/xsk_buff_pool.c:160 at xp_disable_drv_zc+0x1ee/0x260
  BUG: KASAN: slab-use-after-free in xp_alloc_batch+0x930/0xb40
  Read of size 8 at addr ffff8880028eb400 by task ksoftirqd/0/14
   xp_alloc_batch+0x930/0xb40
   try_fill_recv+0xb27/0x26e0

as rq->xsk_pool still points to it and the queue is refilled from it.

Removing XDP must not fail, so let virtnet_xsk_pool_disable() accept
any queue the driver has, below max_queue_pairs. The receive and send
queues above curr_queue_pairs still exist, so they are unbound as usual.

This was pointed out by the Sashiko review of v1 of this series, and
reproduced in QEMU by binding a socket to an XDP queue, removing XDP
and closing the socket.

Fixes: 09d2b3182c8e ("virtio_net: xsk: bind/unbind xsk for rx")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 drivers/net/virtio_net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4f5d8812f75b..f9d227060375 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;
 
 	sq = &vi->sq[qid];
-- 
2.43.0

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

[Haishuang Yan]

virtnet_freeze() frees the receive and send queues and virtnet_restore()
allocates new ones, but AF_XDP sockets stay bound to the device in the
xsk core. Whatever the driver set up for a zero-copy pool is lost on
freeze: the pool's DMA mapping, the xsk_buffs array, the TX header and
its mapping are leaked, and the queue comes back with no pool. When the
socket is closed afterwards, the core asks the driver to disable the
pool again:

  WARNING: net/core/xdp.c:140 ... Missing register, driver bug
  BUG: KASAN: null-ptr-deref in xp_dma_unmap
  Read of size 8 at addr 0000000000000090

as virtnet_xsk_pool_disable() unregisters an xdp_rxq_info that was
never registered and passes the NULL rq->xsk_pool to
xsk_pool_dma_unmap().

Keep the pools DMA mapped across suspend, as the DMA device does not
change, and only move the driver state of a pool from the old queues to
the new ones. On freeze, release it in remove_vq_common() once the
device is reset and the posted buffers are back in the pool. On restore,
set it up again before virtnet_open() fills the queues and enables NAPI.

If that fails for a queue, the pool is left DMA mapped and
virtnet_xsk_pool_disable() only unmaps it when the socket is closed. A
pool left above curr_queue_pairs by removing XDP is not set up again,
so it is unmapped on freeze.

This was found by code review of the suspend path and reproduced in
QEMU by running a suspend cycle with an AF_XDP socket bound, not in
production.

Fixes: 09d2b3182c8e ("virtio_net: xsk: bind/unbind xsk for rx")
Signed-off-by: Haishuang Yan <yanhaishuang@cmss.chinamobile.com>
---
 drivers/net/virtio_net.c | 151 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 145 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index f9d227060375..d9c920f42126 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -5774,6 +5774,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
 }
 
 static int init_vqs(struct virtnet_info *vi);
+static void virtnet_xsk_pools_release(struct virtnet_info *vi);
+static void virtnet_xsk_pools_attach(struct virtnet_info *vi);
 
 static int virtnet_restore_up(struct virtio_device *vdev)
 {
@@ -5790,13 +5792,13 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 
 	virtio_device_ready(vdev);
 
-	if (netif_running(vi->dev)) {
-		rtnl_lock();
+	rtnl_lock();
+	virtnet_xsk_pools_attach(vi);
+	if (netif_running(vi->dev))
 		err = virtnet_open(vi->dev);
-		rtnl_unlock();
-		if (err)
-			goto err_destroy_pools;
-	}
+	rtnl_unlock();
+	if (err)
+		goto err_destroy_pools;
 
 	netif_tx_lock_bh(vi->dev);
 	netif_device_attach(vi->dev);
@@ -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;
+	}
 
 	err = virtnet_rq_bind_xsk_pool(vi, rq, NULL);
 	err |= virtnet_sq_bind_xsk_pool(vi, sq, NULL);
@@ -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);
+			xdp_rxq_info_unreg(&rq->xsk_rxq_info);
+			kvfree(rq->xsk_buffs);
+			rq->xsk_buffs = NULL;
+			rq->xsk_pool = NULL;
+		}
+
+		if (sq->xsk_pool) {
+			virtqueue_unmap_single_attrs(sq->vq, sq->xsk_hdr_dma_addr,
+						     vi->hdr_len, DMA_TO_DEVICE, 0);
+			kfree(sq->xsk_hdr);
+			sq->xsk_hdr = NULL;
+			sq->xsk_pool = NULL;
+		}
+	}
+}
+
+/* Set up the driver state of a still DMA mapped pool on a new queue pair. */
+static int virtnet_xsk_pool_attach(struct virtnet_info *vi, u16 qid,
+				   struct xsk_buff_pool *pool)
+{
+	struct receive_queue *rq = &vi->rq[qid];
+	struct send_queue *sq = &vi->sq[qid];
+	struct device *dma_dev;
+	dma_addr_t hdr_dma;
+	int err;
+
+	dma_dev = virtqueue_dma_dev(rq->vq);
+	if (!rq->page_pool || !dma_dev || dma_dev != virtqueue_dma_dev(sq->vq))
+		return -EINVAL;
+
+	rq->xsk_buffs = kvzalloc_objs(*rq->xsk_buffs,
+				      virtqueue_get_vring_size(rq->vq));
+	if (!rq->xsk_buffs)
+		return -ENOMEM;
+
+	sq->xsk_hdr = kzalloc_obj(*sq->xsk_hdr);
+	if (!sq->xsk_hdr) {
+		err = -ENOMEM;
+		goto err_free_buffs;
+	}
+
+	hdr_dma = virtqueue_map_single_attrs(sq->vq, sq->xsk_hdr, vi->hdr_len,
+					     DMA_TO_DEVICE, 0);
+	if (virtqueue_map_mapping_error(sq->vq, hdr_dma)) {
+		err = -ENOMEM;
+		goto err_free_hdr;
+	}
+
+	err = xdp_rxq_info_reg(&rq->xsk_rxq_info, vi->dev, qid,
+			       rq->napi.napi_id);
+	if (err < 0)
+		goto err_unmap_hdr;
+
+	err = xdp_rxq_info_reg_mem_model(&rq->xsk_rxq_info,
+					 MEM_TYPE_XSK_BUFF_POOL, NULL);
+	if (err < 0)
+		goto err_unreg;
+
+	xsk_pool_set_rxq_info(pool, &rq->xsk_rxq_info);
+
+	sq->xsk_hdr_dma_addr = hdr_dma;
+	sq->xsk_pool = pool;
+	rq->xsk_pool = pool;
+
+	return 0;
+
+err_unreg:
+	xdp_rxq_info_unreg(&rq->xsk_rxq_info);
+err_unmap_hdr:
+	virtqueue_unmap_single_attrs(sq->vq, hdr_dma, vi->hdr_len,
+				     DMA_TO_DEVICE, 0);
+err_free_hdr:
+	kfree(sq->xsk_hdr);
+	sq->xsk_hdr = NULL;
+err_free_buffs:
+	kvfree(rq->xsk_buffs);
+	rq->xsk_buffs = NULL;
+	return err;
+}
+
+/*
+ * 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);
+		if (err)
+			netdev_warn(vi->dev,
+				    "failed to bind AF_XDP pool to queue %d: %d\n",
+				    i, err);
+	}
+}
+
 static int virtnet_xsk_pool_setup(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	if (xdp->xsk.pool)
@@ -7208,6 +7345,8 @@ static void remove_vq_common(struct virtnet_info *vi)
 	/* Free unused buffers in both send and recv, if any. */
 	free_unused_bufs(vi);
 
+	virtnet_xsk_pools_release(vi);
+
 	/*
 	 * Rule of thumb is netdev_tx_reset_queue() should follow any
 	 * skb freeing not followed by netdev_tx_completed_queue()
-- 
2.43.0

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

[netdev-bot+sashiko]

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 3 · 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_release() unmaps the DMA of a zero-copy pool
  sitting at a qid >= curr_queue_pairs (left there by removing XDP), but…
- [Medium] When virtnet_xsk_pool_attach() fails on restore, the pool
  stays DMA mapped while rq->xsk_pool stays NULL.
- [Medium] If virtnet_xsk_pool_attach() fails during restore,
  virtnet_xsk_pools_attach() only logs netdev_warn() and the resume…
- [Medium] virtnet_xsk_pools_release() is a new writer of AF_XDP driver
  state (rq->xsk_pool, sq->xsk_pool, rq->xsk_buffs, sq->xsk_hdr) that…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it defeats the
  patch's cleanup model on the failed-restore path.

--

virtio_net: Keep AF_XDP zero-copy pools bound across suspend

This patch keeps AF_XDP zero-copy pools DMA mapped across freeze and
restore. Only the driver state (xsk_buffs, TX header, xdp_rxq_info) moves
from the old queues to the new ones: it is released in remove_vq_common()
and attached again in virtnet_restore_up() before virtnet_open(). Pools
above curr_queue_pairs are unmapped on freeze. virtnet_xsk_pool_disable()
gets a fallback that unmaps pools whose attach failed 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;

[Severity: High]
This is a pre-existing issue that this patch did not introduce, but the
new error path depends on it.

When virtnet_restore_up() fails (init_vqs, virtnet_create_page_pools or
virtnet_open), or virtnet_cpu_notif_add() fails in virtnet_restore(),
virtnet_del_vqs()->virtnet_free_queues() does:

	kfree(vi->rq);
	kfree(vi->sq);

The pointers are not cleared, and the netdev stays registered.

On this path virtnet_xsk_pools_release() deliberately leaves the pools with
i < curr_queue_pairs DMA mapped. It expects virtnet_xsk_pool_disable() to
unmap them when the socket is closed. A socket close after thaw goes
through:

xp_release_deferred()
  xp_clear_dev()
    xp_disable_drv_zc()
      ndo_bpf()
        virtnet_xsk_pool_disable()

and that function starts with:

	sq = &vi->sq[qid];
	rq = &vi->rq[qid];

	pool = rq->xsk_pool;

Does this read the freed vi->rq before it ever reaches the new !pool
check? Depending on the stale contents, it could go on into
virtnet_rq_bind_xsk_pool() and virtqueue_reset() on freed queues.

xp_put_pool() queues the release on system_freezable_wq, so it cannot run
while the system is frozen. Only the failed-restore case applies. Other
ndo ops, such as virtnet_close(), also touch the dangling vi->rq after a
failed restore.

[ ... ]

> @@ -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 this fallback miss a pool that is still mapped? xsk_get_pool_from_qid()
filters on the current real queue counts:

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;
}

Consider this sequence:

1. A zc pool is bound at a queue Q in the XDP range
   [curr_qp, curr_qp + xdp_qp).

2. Suspend keeps it mapped because Q < curr_queue_pairs. On resume,
   virtnet_xsk_pool_attach() fails (allocation failure, header mapping
   error, or xdp_rxq_info_reg() failure) and only netdev_warn() is
   printed.

3. XDP is reinstalled and then removed. Removing it right after resume is
   a no-op, because freeze dropped rq->xdp_prog. The removal makes
   virtnet_xdp_set() call netif_set_real_num_rx_queues(dev, curr_qp).

4. When the socket closes, virtnet_xsk_pool_disable(Q) sees
   rq->xsk_pool == NULL. xsk_get_pool_from_qid() fails both real_num
   checks and returns NULL, so the function returns 0.

Is the DMA mapping, and the umem's xsk_dma_map refcount, leaked at that
point? A later freeze doesn't clean it up either, because
virtnet_xsk_pools_release() only acts when rq->xsk_pool is set. The umem
pages would later be unpinned while the IOMMU still maps them.

[ ... ]

> +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: High]
Does this leave a zero-copy pool registered in the xsk core but no longer
DMA mapped? The pool stays at dev->_rx[Q].pool with umem->zc set, and
xp_dma_unmap() does:

	kvfree(pool->dma_pages);
	pool->dma_pages = NULL;
	pool->dma_pages_cnt = 0;
	pool->dev = NULL;

It does not clear the per-xskb dma values in pool->heads.

curr_queue_pairs can later grow back over Q. One way is ethtool -L:
virtnet_set_channels() only rejects the change when rq[0].xdp_prog is set,
and that is NULL after resume. Reinstalling XDP has the same effect.

On the next suspend/resume, virtnet_xsk_pools_attach() only filters on:

	if (!pool || !pool->umem->zc)
		continue;

virtnet_xsk_pool_attach() only checks rq->page_pool and that the DMA
devices match. So the unmapped pool is installed as rq->xsk_pool and
sq->xsk_pool.

On TX the path is:

virtnet_poll_tx()
  virtnet_xsk_xmit()
    virtnet_xsk_xmit_one()
      xsk_buff_raw_get_dma()
        __xp_raw_get_dma()

which indexes pool->dma_pages[addr >> PAGE_SHIFT] while dma_pages is NULL,
at an offset chosen by userspace.

On RX the path is:

virtnet_open()
  try_fill_recv()
    virtnet_add_recvbuf_xsk()
      xsk_buff_alloc_batch()

pool->dev is NULL, so the slow path is skipped. Stale xskb->dma addresses
from the destroyed mapping are then posted with
virtqueue_add_inbuf_premapped(). With a vIOMMU or swiotlb (confidential VMs,
for example), those IOVAs or bounce slots have already been freed.

Doesn't this break the invariant behind the comment in
virtnet_xsk_pools_attach(), "The pool stays DMA mapped until it is
disabled"?

[ ... ]

> +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);
> +		if (err)
> +			netdev_warn(vi->dev,
> +				    "failed to bind AF_XDP pool to queue %d: %d\n",
> +				    i, err);
> +	}
> +}

[Severity: Medium]
What happens to the bound AF_XDP socket when this attach fails? The pool is
still registered at the qid with umem->zc set, but rq->xsk_pool and
sq->xsk_pool stay NULL. virtnet_poll_tx() only services XSK TX when
sq->xsk_pool is set:

	if (sq->xsk_pool)
		xsk_done = virtnet_xsk_xmit(sq, sq->xsk_pool, budget);
	else
		free_old_xmit(sq, txq, !!budget);

So the socket's TX ring is never consumed. virtnet_xsk_wakeup() still
returns 0, and the socket gets no sk_err or ENETDOWN, so userspace has no
signal that anything is wrong. RX on that queue quietly falls back to
page_pool buffers.

The socket stays stalled until the next suspend cycle or a rebind. The
commit message says:

    If that fails for a queue, the pool is left DMA mapped and
    virtnet_xsk_pool_disable() only unmaps it when the socket is closed.

It does not mention that the socket stops passing traffic. Should the
socket be told about the failure, or should this case be handled some
other way?

[ ... ]

> @@ -7208,6 +7345,8 @@ static void remove_vq_common(struct virtnet_info *vi)
>  	/* Free unused buffers in both send and recv, if any. */
>  	free_unused_bufs(vi);
>  
> +	virtnet_xsk_pools_release(vi);
> +

[Severity: Medium]
Should this run under RTNL? Every other writer of rq->xsk_pool,
sq->xsk_pool, rq->xsk_buffs and sq->xsk_hdr runs under RTNL through
ndo_bpf. remove_vq_common() is reached from virtnet_freeze() without RTNL.
The err_destroy_pools path in virtnet_restore_up() also calls
virtnet_xsk_pools_release() after rtnl_unlock().

The unmap path is:

xsk_pool_dma_unmap()
  xp_dma_unmap()
    xp_find_dma_map()
    xp_destroy_dma_map()

xp_find_dma_map() walks pool->umem->xsk_dma_list without a lock, and
xp_destroy_dma_map() does a plain list_del(). The ndo_bpf callers hold
RTNL, and that is the only serialization.

With XDP_SHARED_UMEM across two virtio-net devices, one umem->xsk_dma_list
holds one dma_map per netdev. PCI devices have async suspend enabled
(pci_pm_init()->device_enable_async_suspend()), so two virtnet_freeze()
calls can run at the same time. Both could take the
i >= curr_queue_pairs unmap branch.

Could that corrupt the shared list, or touch a dma_map that has already
been freed? free_receive_bufs() later in remove_vq_common() already takes
rtnl_lock(), so wrapping virtnet_xsk_pools_release() in RTNL should close
this.

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