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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message's second reason for the change does not match
  what the patch does.

--

virtio_net: Don't DMA map the kernel image for the AF_XDP TX header

This patch removes the static xsk_hdr that lives in the kernel image. In
its place, virtnet_xsk_pool_enable() now kzalloc's a header for each send
queue, and virtnet_xsk_pool_disable() frees it. The point is to stop
virtqueue_map_single_attrs() from rejecting the mapping on arm64, where
the kernel image sits in the vmalloc area.

> and binding an AF_XDP zero-copy socket fails with -ENOMEM. Even where
> the mapping succeeds, the header shares cache lines with unrelated data.

[Severity: Low]
Does the second sentence here match what the patch does? It reads as
though the patch also gives the header its own cache line.

virtnet_xsk_pool_enable() now uses a plain allocation:

    sq->xsk_hdr = kzalloc_obj(*sq->xsk_hdr);

This is a small object of about 24 bytes, the size of struct
virtio_net_hdr_v1_hash_tunnel. It has no cache-line alignment or padding.

__kmalloc_minalign() in mm/slab_common.c can also lower the kmalloc
minimum alignment:

    if (IS_ENABLED(CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC) &&
        is_swiotlb_allocated())
            minalign = ARCH_KMALLOC_MINALIGN;

So the new header can still share cache lines with other slab objects.

The header is also mapped DMA_TO_DEVICE only. That means sharing a cache
line would not cause a correctness problem for this mapping in the first
place.

Could this sentence be dropped, or reworded so it doesn't suggest the
patch provides cache-line isolation? The main reason given, the vmalloc
rejection on arm64, looks correct.

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

Re: [PATCH net 2/2] 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: 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