[PATCH 0/3] virtio_net: introduce an RX queue watchdog for stuck detection

[PATCH 0/3] virtio_net: introduce an RX queue watchdog for stuck detection

[Longjun Tang]

From: Longjun Tang <tanglongjun@kylinos.cn>

Hi Michael, Kuba,

I once ran into a problem where the virtio_net receive queue became
permanently stuck.

The virtio_net RX path relies on backend interrupt notifications. 
The backend(vhost_net/vhost-user) appends buffers to the used ring
and then notifies the guest, relying on the hypervisor(KVM) to inject
the interrupt. On receiving it, virtio_net schedules NAPI, drains the
used ring and refills descriptors.

If the hypervisor fails to inject the interrupt(e.g. a transient KVM
failure), the guest never schedules NAPI, so it neither consumes buffers
nor returns descriptors. The backend keeps appending until the ring is full,
then stops and, with no free descriptors left, also stops sending
notifications. Both sides now wait for the other, and the RX queue is
permanently stuck.

The root cause is the lost interrupt in the hypervisor, and the proper
fix belongs there. Nevertheless, I believe virtio_net needs a way to
observe and diagnose it. Today, once a queue hangs, there is no signal
to the operator that anything is wrong: the RX path has no equivalent of
the TX watchdog (ndo_tx_timeout). This series lets virtio_net detect and
recover a stuck RX queue.


Implementation
--------------

Patch 1 exports virtqueue_get_last_used_idx(), a read-only accessor for
the last consumed used ring index. Together with the existing
virtqueue_poll(), it lets a driver ask whether the device has produced
buffers that the driver has not consumed yet ("backlog").

Patch 2 adds a per-device RX watchdog that scans all RX queues once per
second. A queue is considered stuck when, for rx_watchdog_timeo seconds
(default 5, 0 disables it), all of the following hold simultaneously:
  - the queue has a non-zero backlog (unconsumed buffers in the used ring);
  - the driver makes no consumption progress (last_used unchanged);
  - no new interrupt arrives (rq->calls unchanged).
On detection it logs a warning, rate-limited to once per timeout, and
reschedules NAPI, which drains the used ring and refills the descriptors,
so the queue recovers.

Patch 3 adds a per-queue rx_timeouts statistic, exposed via ethtool -S,
so the number of stuck events can be observed per queue.

questions and comments are welcome.

Thanks!
Lange

Longjun Tang (3):
  virtio: add virtqueue_get_last_used_idx() helper
  virtio_net: introduce an RX queue watchdog for stuck detection
  virtio_net: add rx_timeouts per-queue statistic

 drivers/net/virtio_net.c     | 112 +++++++++++++++++++++++++++++++++++
 drivers/virtio/virtio_ring.c |  18 +++++-
 include/linux/virtio.h       |   2 +
 3 files changed, 130 insertions(+), 2 deletions(-)

-- 
2.51.0

[PATCH 1/3] virtio: add virtqueue_get_last_used_idx() helper

[Longjun Tang]

From: Longjun Tang <tanglongjun@kylinos.cn>

Export a read-only accessor for the last consumed used ring index.
It's useful for drivers that need to detect whether the device has
produced buffers that have not been consumed yet, by passing the
returned value to virtqueue_poll().

The split ring now writes the field with WRITE_ONCE() (the packed
ring already did) to document that the new reader can run concurrently
with the poll path.

Signed-off-by: Longjun Tang <tanglongjun@kylinos.cn>
---
 drivers/virtio/virtio_ring.c | 18 ++++++++++++++++--
 include/linux/virtio.h       |  2 ++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index db678f5a80e0..6d8485c73881 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1001,7 +1001,7 @@ static void *virtqueue_get_buf_ctx_split(struct vring_virtqueue *vq,
 	/* detach_buf_split clears data, so grab it now. */
 	ret = vq->split.desc_state[i].data;
 	detach_buf_split(vq, i, ctx);
-	vq->last_used_idx++;
+	WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1);
 	/* If we expect an interrupt for the next entry, tell host
 	 * by writing event index and flush out the write before
 	 * the read in the next get_buf call. */
@@ -1068,7 +1068,7 @@ static void *virtqueue_get_buf_ctx_split_in_order(struct vring_virtqueue *vq,
 	ret = vq->split.desc_state[last_used].data;
 	detach_buf_split_in_order(vq, last_used, ctx);
 
-	vq->last_used_idx++;
+	WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1);
 	vq->last_used += (vq->vq.num_free - num_free);
 	/* If we expect an interrupt for the next entry, tell host
 	 * by writing event index and flush out the write before
@@ -3227,6 +3227,20 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned int last_used_idx)
 }
 EXPORT_SYMBOL_GPL(virtqueue_poll);
 
+/**
+ * virtqueue_get_last_used_idx - get the last consumed used ring index
+ * @_vq: the struct virtqueue we're talking about.
+ *
+ * Returns the index of the most recently consumed used buffer.
+ */
+unsigned int virtqueue_get_last_used_idx(const struct virtqueue *_vq)
+{
+	const struct vring_virtqueue *vq = to_vvq(_vq);
+
+	return READ_ONCE(vq->last_used_idx);
+}
+EXPORT_SYMBOL_GPL(virtqueue_get_last_used_idx);
+
 /**
  * virtqueue_enable_cb - restart callbacks after disable_cb.
  * @_vq: the struct virtqueue we're talking about.
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index f923e42cfd01..1aed4d9235c8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -112,6 +112,8 @@ unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq);
 
 bool virtqueue_poll(struct virtqueue *vq, unsigned);
 
+unsigned int virtqueue_get_last_used_idx(const struct virtqueue *vq);
+
 bool virtqueue_enable_cb_delayed(struct virtqueue *vq);
 
 void *virtqueue_detach_unused_buf(struct virtqueue *vq);
-- 
2.51.0

[PATCH 2/3] virtio_net: introduce an RX queue watchdog for stuck detection

[Longjun Tang]

From: Longjun Tang <tanglongjun@kylinos.cn>

The backend raises an interrupt after adding buffers to the used ring,
and the hypervisor injects it. If one is lost (e.g., a transient KVM
failure), NAPI will not be scheduled and the driver stops consuming
buffers. The ring eventually fills up, the backend stops notifying, and
the queue is left permanently stuck.

Add a per-device watchdog that scans the RX queues once per second and
detects a queue that has a non-zero backlog while making no consumption
progress and receiving no new interrupt for rx_watchdog_timeo seconds
(default 5, 0 disables it). On detection, reschedule NAPI.

Signed-off-by: Longjun Tang <tanglongjun@kylinos.cn>
---
 drivers/net/virtio_net.c | 104 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index dd2c41ef6fbe..ec7c248080c5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -36,6 +36,10 @@ module_param(csum, bool, 0444);
 module_param(gso, bool, 0444);
 module_param(napi_tx, bool, 0644);
 
+/* RX queue stall timeout in seconds; 0 disables the watchdog. */
+static unsigned int rx_watchdog_timeo = 5;
+module_param(rx_watchdog_timeo, uint, 0644);
+
 #define VIRTIO_OFFLOAD_MAP_MIN	46
 #define VIRTIO_OFFLOAD_MAP_MAX	47
 #define VIRTIO_FEATURES_MAP_MIN	65
@@ -318,6 +322,13 @@ struct send_queue {
 	dma_addr_t xsk_hdr_dma_addr;
 };
 
+struct virtnet_rq_watchdog {
+	u16 last_used;
+	u16 calls;
+	unsigned long jiffies;
+	bool stalled;
+};
+
 /* Internal representation of a receive virtqueue */
 struct receive_queue {
 	/* Virtqueue associated with this receive_queue */
@@ -332,6 +343,9 @@ struct receive_queue {
 	/* The number of rx notifications */
 	u16 calls;
 
+	/* RX watchdog state. */
+	struct virtnet_rq_watchdog watchdog;
+
 	/* Is dynamic interrupt moderation enabled? */
 	bool dim_enabled;
 
@@ -443,6 +457,9 @@ struct virtnet_info {
 	/* Work struct for setting rx mode */
 	struct work_struct rx_mode_work;
 
+	/* RX watchdog timer for stuck detection */
+	struct timer_list rx_watchdog;
+
 	/* Does the affinity hint is set for virtqueues? */
 	bool affinity_hint_set;
 
@@ -3067,6 +3084,80 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 	return received;
 }
 
+/*
+ * An RX queue is stalled if it has a backlog but makes no progress and
+ * gets no new interrupt for rx_watchdog_timeo seconds, usually because
+ * an interrupt was lost.  Mark it stalled and reschedule NAPI to drain
+ * the ring, refill the buffers.
+ */
+static void virtnet_rx_watchdog(struct timer_list *t)
+{
+	struct virtnet_info *vi = timer_container_of(vi, t, rx_watchdog);
+	unsigned long timeout = rx_watchdog_timeo * HZ;
+	int i;
+
+	if (!rx_watchdog_timeo)
+		return;
+
+	for (i = 0; i < vi->curr_queue_pairs; i++) {
+		struct receive_queue *rq = &vi->rq[i];
+		u16 last_used = virtqueue_get_last_used_idx(rq->vq);
+		u16 calls = rq->calls;
+		bool backlog = virtqueue_poll(rq->vq, last_used);
+		bool progress = last_used != rq->watchdog.last_used;
+
+		if (!backlog || progress || calls != rq->watchdog.calls) {
+			rq->watchdog.last_used = last_used;
+			rq->watchdog.calls = calls;
+			rq->watchdog.jiffies = jiffies;
+			continue;
+		}
+
+		if (time_after(jiffies, rq->watchdog.jiffies + timeout)) {
+			unsigned int stall_ms =
+				jiffies_to_msecs(jiffies - rq->watchdog.jiffies);
+
+			netdev_warn(vi->dev,
+				    "RX queue %u stalled for %u ms, rescheduling NAPI\n",
+				    i, stall_ms);
+
+			rq->watchdog.stalled = true;
+			virtqueue_napi_schedule(&rq->napi, rq->vq);
+
+			/* Rate-limit to one event per timeout. */
+			rq->watchdog.jiffies = jiffies;
+		}
+	}
+
+	mod_timer(&vi->rx_watchdog, jiffies + HZ);
+}
+
+static void virtnet_rx_watchdog_init_queue(struct receive_queue *rq)
+{
+	rq->watchdog.last_used = virtqueue_get_last_used_idx(rq->vq);
+	rq->watchdog.calls = rq->calls;
+	rq->watchdog.jiffies = jiffies;
+	rq->watchdog.stalled = false;
+}
+
+static void virtnet_rx_watchdog_start(struct virtnet_info *vi)
+{
+	int i;
+
+	if (!rx_watchdog_timeo)
+		return;
+
+	for (i = 0; i < vi->curr_queue_pairs; i++)
+		virtnet_rx_watchdog_init_queue(&vi->rq[i]);
+
+	mod_timer(&vi->rx_watchdog, jiffies + HZ);
+}
+
+static void virtnet_rx_watchdog_stop(struct virtnet_info *vi)
+{
+	timer_delete_sync(&vi->rx_watchdog);
+}
+
 static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
 {
 	virtnet_napi_tx_disable(&vi->sq[qp_index]);
@@ -3230,6 +3321,8 @@ static int virtnet_open(struct net_device *dev)
 		netif_carrier_on(dev);
 	}
 
+	virtnet_rx_watchdog_start(vi);
+
 	return 0;
 
 err_enable_qp:
@@ -3759,6 +3852,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 	struct virtio_net_rss_config_trailer old_rss_trailer;
 	struct net_device *dev = vi->dev;
 	struct scatterlist sg;
+	u16 old_queue_pairs = vi->curr_queue_pairs;
 
 	if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
 		return 0;
@@ -3816,7 +3910,14 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 	if (vi->has_rss)
 		vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
 succ:
+	if (dev->flags & IFF_UP) {
+		/* Init new queues */
+		for (int i = old_queue_pairs; i < queue_pairs; i++)
+			virtnet_rx_watchdog_init_queue(&vi->rq[i]);
+	}
+
 	vi->curr_queue_pairs = queue_pairs;
+
 	if (dev->flags & IFF_UP) {
 		local_bh_disable();
 		for (int i = 0; i < vi->curr_queue_pairs; ++i)
@@ -3832,6 +3933,8 @@ static int virtnet_close(struct net_device *dev)
 	struct virtnet_info *vi = netdev_priv(dev);
 	int i;
 
+	virtnet_rx_watchdog_stop(vi);
+
 	/* Prevent the config change callback from changing carrier
 	 * after close
 	 */
@@ -6900,6 +7003,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	INIT_WORK(&vi->config_work, virtnet_config_changed_work);
 	INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work);
 	disable_work(&vi->rx_mode_work);
+	timer_setup(&vi->rx_watchdog, virtnet_rx_watchdog, 0);
 
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) {
 		vi->mergeable_rx_bufs = true;
-- 
2.51.0

[PATCH 3/3] virtio_net: add rx_timeouts per-queue statistic

[Longjun Tang]

From: Longjun Tang <tanglongjun@kylinos.cn>

Add an rx_timeouts field to the per-queue RX statistics and expose it
via ethtool. Whenever an RX queue gets stuck, the NAPI poll increments
rx_timeouts, so it counts RX queue stuck events.

Signed-off-by: Longjun Tang <tanglongjun@kylinos.cn>
---
 drivers/net/virtio_net.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ec7c248080c5..feb6bfbb1c5a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -135,6 +135,7 @@ struct virtnet_rq_stats {
 	u64_stats_t xdp_redirects;
 	u64_stats_t xdp_drops;
 	u64_stats_t kicks;
+	u64_stats_t rx_timeouts;
 };
 
 #define VIRTNET_SQ_STAT(name, m) {name, offsetof(struct virtnet_sq_stats, m), -1}
@@ -168,6 +169,7 @@ static const struct virtnet_stat_desc virtnet_rq_stats_desc[] = {
 	VIRTNET_RQ_STAT("xdp_redirects", xdp_redirects),
 	VIRTNET_RQ_STAT("xdp_drops",     xdp_drops),
 	VIRTNET_RQ_STAT("kicks",         kicks),
+	VIRTNET_RQ_STAT("rx_timeouts",   rx_timeouts),
 };
 
 static const struct virtnet_stat_desc virtnet_sq_stats_desc_qstat[] = {
@@ -2983,6 +2985,12 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
 		u64_stats_add(item, u64_stats_read(src));
 	}
 
+	/* Count a stall recovered by this poll. */
+	if (rq->watchdog.stalled) {
+		u64_stats_inc(&rq->stats.rx_timeouts);
+		rq->watchdog.stalled = false;
+	}
+
 	u64_stats_add(&rq->stats.packets, u64_stats_read(&stats.packets));
 	u64_stats_add(&rq->stats.bytes, u64_stats_read(&stats.bytes));
 	u64_stats_add(&rq->stats.alloc_fail, u64_stats_read(&stats.alloc_fail));
-- 
2.51.0

Re: [PATCH 1/3] virtio: add virtqueue_get_last_used_idx() helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] virtio_ring: 4-byte WRITE_ONCE() on u16 last_used_idx in
  tools/virtio
- [Medium] virtio_ring: virtqueue_get_last_used_idx() misses in-order
  batch buffers
--

commit e72b0bb03b2a71f4d30e308519407a5817a43cd2
Author: Longjun Tang <tanglongjun@kylinos.cn>

virtio: add virtqueue_get_last_used_idx() helper

This commit adds virtqueue_get_last_used_idx() to read vq->last_used_idx
and updates split ring buffer detachment to write vq->last_used_idx using
WRITE_ONCE().

> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index f683a2d6a2002..f5f592de9ed59 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -1005,7 +1005,7 @@ static void *virtqueue_get_buf_ctx_split(struct vring_virtqueue *vq,
>  	/* detach_buf_split clears data, so grab it now. */
>  	ret = vq->split.desc_state[i].data;
>  	detach_buf_split(vq, i, ctx);
> -	vq->last_used_idx++;
> +	WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1);
>  	/* If we expect an interrupt for the next entry, tell host
>  	 * by writing event index and flush out the write before
>  	 * the read in the next get_buf call. */
> @@ -1072,7 +1072,7 @@ static void *virtqueue_get_buf_ctx_split_in_order(struct vring_virtqueue *vq,
>  	ret = vq->split.desc_state[last_used].data;
>  	detach_buf_split_in_order(vq, last_used, ctx);
>  
> -	vq->last_used_idx++;
> +	WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1);
>  	vq->last_used += (vq->vq.num_free - num_free);

[Severity: Medium]
Does WRITE_ONCE(vq->last_used_idx, vq->last_used_idx + 1) corrupt the
adjacent u16 last_used field when drivers/virtio/virtio_ring.c is built
under tools/virtio/?

In struct vring_virtqueue, u16 last_used_idx is immediately followed by
u16 last_used.  The expression vq->last_used_idx + 1 promotes to int
(4 bytes), and tools/virtio/linux/compiler.h defines WRITE_ONCE() using
typeof(val):

tools/virtio/linux/compiler.h:
#define WRITE_ONCE(var, val) \
	(*((volatile typeof(val) *)(&(var))) = (val))

In tools/virtio builds, both WRITE_ONCE() calls perform a 4-byte store to
&vq->last_used_idx, overwriting vq->last_used right before
virtqueue_get_buf_ctx_split_in_order() increments it (and failing to
update vq->last_used_idx on big-endian hosts).  Casting the value to u16
or fixing the tools/virtio stub to use typeof(var) would avoid the
4-byte store.

[ ... ]

> @@ -3236,6 +3236,20 @@ bool virtqueue_poll(struct virtqueue *_vq, unsigned int last_used_idx)
>  }
>  EXPORT_SYMBOL_GPL(virtqueue_poll);
>  
> +/**
> + * virtqueue_get_last_used_idx - get the last consumed used ring index
> + * @_vq: the struct virtqueue we're talking about.
> + *
> + * Returns the index of the most recently consumed used buffer.
> + */
> +unsigned int virtqueue_get_last_used_idx(const struct virtqueue *_vq)
> +{
> +	const struct vring_virtqueue *vq = to_vvq(_vq);
> +
> +	return READ_ONCE(vq->last_used_idx);
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_get_last_used_idx);

[Severity: Medium]
Can passing virtqueue_get_last_used_idx() to virtqueue_poll() falsely
report no pending buffers when VIRTIO_F_IN_ORDER is negotiated?

On a packed in-order ring, the device only writes the first descriptor of
a completed batch, and virtqueue_get_buf_ctx_packed_in_order() caches the
batch end in vq->batch_last.id while advancing vq->last_used_idx to the
next descriptor in the batch:

drivers/virtio/virtio_ring.c:more_used_packed_in_order() {
	if (vq->batch_last.id != UINT_MAX)
		return true;

	return virtqueue_poll_packed(vq, READ_ONCE(vq->last_used_idx));
}

Because the intermediate descriptors in the batch do not have their USED
flag updated by the device, virtqueue_poll() (which dispatches to
virtqueue_poll_packed() without checking vq->batch_last.id) returns false
whenever a batch has been partially consumed, such as when NAPI poll stops
mid-batch after exhausting its budget.

Also, the kernel-doc says this returns the index of the most recently
consumed used buffer, whereas vq->last_used_idx is the next index to be
consumed and, for packed rings, also packs the wrap counter in bit
VRING_PACKED_EVENT_F_WRAP_CTR, matching the opaque token returned by
virtqueue_enable_cb_prepare().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094310.103979-1-lange_tang@163.com?part=1

Re: [PATCH 2/3] virtio_net: introduce an RX queue watchdog for stuck detection

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] virtio_net: virtnet_rx_watchdog() races with
  virtqueue_resize() and reset
- [Low] virtio_net: broken runtime re-enable and overflow of
  rx_watchdog_timeo
- [Low] virtio_net: virtnet_rx_watchdog() counts idle time toward stall
  timeout
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] virtio_net: memory leak in virtnet_probe() [1]
--

commit 8f4864b7d541d6a73ad04b5a7ad25842595fc1aa
Author: Longjun Tang <tanglongjun@kylinos.cn>

virtio_net: introduce an RX queue watchdog for stuck detection

This commit adds a per-device periodic timer that checks active RX queues
for unconsumed used buffers without progress or new interrupts for
rx_watchdog_timeo seconds, and reschedules NAPI when a stall is detected.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index dd2c41ef6fbe7..ec7c248080c57 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -36,6 +36,10 @@ module_param(csum, bool, 0444);
>  module_param(gso, bool, 0444);
>  module_param(napi_tx, bool, 0644);
>  
> +/* RX queue stall timeout in seconds; 0 disables the watchdog. */
> +static unsigned int rx_watchdog_timeo = 5;
> +module_param(rx_watchdog_timeo, uint, 0644);

[ ... ]

> @@ -3067,6 +3084,80 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>  	return received;
>  }
>  
> +/*
> + * An RX queue is stalled if it has a backlog but makes no progress and
> + * gets no new interrupt for rx_watchdog_timeo seconds, usually because
> + * an interrupt was lost.  Mark it stalled and reschedule NAPI to drain
> + * the ring, refill the buffers.
> + */
> +static void virtnet_rx_watchdog(struct timer_list *t)
> +{
> +	struct virtnet_info *vi = timer_container_of(vi, t, rx_watchdog);
> +	unsigned long timeout = rx_watchdog_timeo * HZ;
> +	int i;
> +
> +	if (!rx_watchdog_timeo)
> +		return;

[Severity: Low]
If rx_watchdog_timeo is set to 0 at boot or via sysfs (0644) and later
written back to a non-zero value while the interface is up, how does
vi->rx_watchdog get started again?  Both virtnet_rx_watchdog_start() and
virtnet_rx_watchdog() return without arming the timer when
rx_watchdog_timeo is 0, and there is no module parameter callback to
restart it on open devices.

Also, virtnet_rx_watchdog() reads rx_watchdog_timeo twice without
READ_ONCE() and multiplies rx_watchdog_timeo * HZ in 32-bit unsigned int
arithmetic before widening to unsigned long.  A concurrent update from 0
to non-zero can compute timeout = 0 while passing the zero check, and a
large value can wrap 32-bit multiplication, causing immediate false stall
warnings.

> +
> +	for (i = 0; i < vi->curr_queue_pairs; i++) {
> +		struct receive_queue *rq = &vi->rq[i];
> +		u16 last_used = virtqueue_get_last_used_idx(rq->vq);
> +		u16 calls = rq->calls;
> +		bool backlog = virtqueue_poll(rq->vq, last_used);

[Severity: Medium]
Can virtnet_rx_watchdog() race with virtnet_rx_resize() or
virtnet_rq_bind_xsk_pool() while the interface is up?

virtnet_rx_pause() disables NAPI and DIM before resizing or resetting
rq->vq, but it does not stop vi->rx_watchdog.  virtqueue_resize_split()
and virtqueue_resize_packed() free the old vring before attaching the new
one, and virtqueue_poll() only checks vq->broken:

CPU0                                  CPU1 (<softirq>)
virtnet_set_ringparam()
  virtnet_rx_resize()
    virtnet_rx_pause(vi, rq)
    virtqueue_resize(rq->vq, ...)
      vring_free(&vq->vq)
                                      virtnet_rx_watchdog()
                                        virtqueue_poll(rq->vq, last_used)
                                          <-- use-after-free of vring
      virtqueue_init(...)
      virtqueue_vring_attach_*(...)

On packed virtqueues, if virtnet_rx_resize() shrinks the ring after
virtnet_rx_watchdog() reads last_used, is_used_desc_packed() can also
index vq->packed.vring.desc[used_idx] out of bounds.  In addition,
virtqueue_resize() and virtqueue_reset() reset vq->last_used_idx to 0,
but virtnet_rx_resume() does not call virtnet_rx_watchdog_init_queue(rq),
leaving stale state in rq->watchdog.

> +		bool progress = last_used != rq->watchdog.last_used;
> +
> +		if (!backlog || progress || calls != rq->watchdog.calls) {
> +			rq->watchdog.last_used = last_used;
> +			rq->watchdog.calls = calls;
> +			rq->watchdog.jiffies = jiffies;
> +			continue;
> +		}
> +
> +		if (time_after(jiffies, rq->watchdog.jiffies + timeout)) {

[Severity: Low]
Does the first timer tick that sees backlog == true count the preceding
idle second toward timeout?

When a queue is idle (!backlog), rq->watchdog.jiffies is updated to the
idle tick's jiffies and the timer is re-armed for jiffies + HZ.  If a
packet arrives just before the next tick and has not been consumed yet,
backlog is true, progress is false, and calls == rq->watchdog.calls, so
virtnet_rx_watchdog() immediately compares jiffies against the timestamp
from the previous idle tick.  When rx_watchdog_timeo is 1 (or the timer
softirq is delayed), time_after(jiffies, rq->watchdog.jiffies + timeout)
can trigger on the very first observation of a backlog and report an
inflated stall_ms.

Furthermore, sampling last_used and calls before virtqueue_poll() means
that if an interrupt arrives and NAPI drains the packet between reading
last_used/calls and calling virtqueue_poll(rq->vq, last_used),
virtqueue_poll() compares the new used index against the stale last_used
and sees a false backlog with no apparent progress.

[ ... ]

> @@ -6900,6 +7003,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	INIT_WORK(&vi->config_work, virtnet_config_changed_work);
>  	INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work);
>  	disable_work(&vi->rx_mode_work);
> +	timer_setup(&vi->rx_watchdog, virtnet_rx_watchdog, 0);

[Severity: Medium]
This problem wasn't introduced by this patch, but can a failed
virtnet_cpu_notif_add() in virtnet_probe() leak queued RX buffers and DMA
mappings if the device was opened concurrently [1]?

Once virtnet_probe() drops rtnl_unlock(), userspace can open the device
and populate the receive queues via virtnet_open() -> try_fill_recv():

CPU0                                  CPU1
virtnet_probe()
  register_netdevice(dev)
  rtnl_unlock()
                                      virtnet_open()
                                        try_fill_recv()
  virtnet_cpu_notif_add(vi) -> fails
  unregister_netdev(dev)
  ...
  virtio_reset_device(vdev)
  virtnet_del_vqs(vi)  <-- leaks unused buffers

Unlike remove_vq_common(), the free_unregister_netdev error path resets
the device and deletes the virtqueues without calling free_unused_bufs().

[1] https://sashiko.dev/#/bug/linux-701d943e-275e-4959-aa9f-bafbe242f070

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094310.103979-1-lange_tang@163.com?part=2

Re: [PATCH 3/3] virtio_net: add rx_timeouts per-queue statistic

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] virtio_net: data race and lost rx_timeouts events via
  rq->watchdog.stalled
- [Medium] virtio_net: race between virtnet_rx_watchdog() and RX queue
  pause/reset
--

commit 4a62fe0fe638e28583b6ebe514a0626f687513ee
Author: Longjun Tang <tanglongjun@kylinos.cn>

virtio_net: add rx_timeouts per-queue statistic

This commit adds an rx_timeouts per-queue RX statistic exposed via ethtool
that is incremented in virtnet_receive() when rq->watchdog.stalled is set.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index ec7c248080c57..feb6bfbb1c5a8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c

[ ... ]

> @@ -2983,6 +2985,12 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
>  		u64_stats_add(item, u64_stats_read(src));
>  	}
>  
> +	/* Count a stall recovered by this poll. */
> +	if (rq->watchdog.stalled) {
> +		u64_stats_inc(&rq->stats.rx_timeouts);
> +		rq->watchdog.stalled = false;
> +	}
> +
>  	u64_stats_add(&rq->stats.packets, u64_stats_read(&stats.packets));
>  	u64_stats_add(&rq->stats.bytes, u64_stats_read(&stats.bytes));
>  	u64_stats_add(&rq->stats.alloc_fail, u64_stats_read(&stats.alloc_fail));

[Severity: Medium]
Does passing timeout events through the plain boolean rq->watchdog.stalled
race with virtnet_rx_watchdog() and lose rx_timeouts increments?

CPU0 (timer softirq)              CPU1 (NAPI poll)
virtnet_rx_watchdog()             virtnet_receive()
                                    if (rq->watchdog.stalled)
                                      u64_stats_inc(&rq->stats.rx_timeouts)
  rq->watchdog.stalled = true;
                                      rq->watchdog.stalled = false;
                                        <-- data race / lost timeout

Because virtnet_rx_watchdog() writes rq->watchdog.stalled = true on each
timeout and virtnet_receive() reads and clears it without atomic operations,
concurrent execution triggers a KCSAN data race and can clobber a newly
detected stall.  Counting in virtnet_receive() also leaves rx_timeouts at 0
while NAPI remains wedged, collapses multiple watchdog expirations before a
poll into a single increment, loses the count if the device is reopened via
virtnet_rx_watchdog_init_queue() before NAPI runs, and clears stalled even
when virtnet_poll() is called with budget == 0 (such as via netpoll) without
draining packets.

[Severity: Medium]
Can virtnet_rx_watchdog() also race with RX queue pause, resize, and reset?

CPU0                              CPU1 (timer softirq)
virtnet_rx_resize()               virtnet_rx_watchdog()
  virtnet_rx_pause(vi, rq)
  virtqueue_resize(rq->vq, ...)
    virtqueue_resize_split()
      vring_free(&vq->vq)
                                    virtqueue_poll(rq->vq, last_used)
                                      <-- use-after-free / OOB read
      virtqueue_vring_attach_split()

commit "virtio_net: introduce an RX queue watchdog for stuck detection"
starts vi->rx_watchdog in virtnet_open() and only stops it in
virtnet_close(), while virtnet_rx_pause() only disables rq->napi and cancels
rq->dim:

drivers/net/virtio_net.c:virtnet_rx_pause() {
	bool running = netif_running(vi->dev);

	if (running) {
		virtnet_napi_disable(rq);
		virtnet_cancel_dim(vi, &rq->dim);
	}
}

When virtnet_rx_resize() or virtnet_rq_bind_xsk_pool() pauses a queue to
resize or reset rq->vq, virtnet_rx_watchdog() continues polling rq->vq every
second and can read freed vring memory or index past a newly shrunk packed
descriptor ring, or call virtqueue_napi_schedule() while rq->vq is being
reset.  Because neither path reinitializes rq->watchdog after resetting
vq->last_used_idx, any rq->watchdog.stalled state left from before the
reset is also consumed by virtnet_receive() on resume and falsely increments
rq->stats.rx_timeouts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109094310.103979-1-lange_tang@163.com?part=3