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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Haishuang Yan]

Two 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: 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().

Testing used KASAN kernels with a small /init that binds an
XDP_ZEROCOPY socket to eth0 queue 0, runs one suspend cycle with
pm_test=devices and closes the socket, on virtio-net-pci with
iommu_platform=on,disable-legacy=on:

- x86_64, QEMU/KVM on bare metal and nested inside a KVM guest:
  - without the series: WARNING + KASAN null-ptr-deref + oops on close
    (patch 1 does not apply to x86, binding works there)
  - with both patches:  no warnings
- A full x86_64 guest kernel with the series passed virtio-net ping,
  a 100MB HTTP download with checksum check and suspend cycles.

Haishuang Yan (2):
  virtio_net: Don't DMA map the kernel image for the AF_XDP TX header
  virtio_net: Keep AF_XDP zero-copy pools bound across suspend

 drivers/net/virtio_net.c | 70 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 66 insertions(+), 4 deletions(-)


base-commit: 37dfd0efbfffc20a57e41c2eb349ac49b1ca1ff8
-- 
2.43.0

[PATCH net 1/2] 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. Even where
the mapping succeeds, the header shares cache lines with unrelated data.

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

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 2/2] 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().

Release the driver side of all zero-copy pools before freezing and bind
the pools the core still has for the queues again after restore. Also
return early from virtnet_xsk_pool_disable() for a queue without a pool,
in case rebinding failed.

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

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4f5d8812f75b..5995ad730a91 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -6037,6 +6037,9 @@ static int virtnet_xsk_pool_disable(struct net_device *dev, u16 qid)
 	rq = &vi->rq[qid];
 
 	pool = rq->xsk_pool;
+	/* Not bound to the driver, e.g. if rebinding it on restore failed */
+	if (!pool)
+		return 0;
 
 	err = virtnet_rq_bind_xsk_pool(vi, rq, NULL);
 	err |= virtnet_sq_bind_xsk_pool(vi, sq, NULL);
@@ -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);
+}
+
+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);
+		if (err)
+			netdev_warn(vi->dev,
+				    "failed to rebind 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)
@@ -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);
 	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;
 }
 
-- 
2.43.0