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

[sashiko-bot@kernel.org]

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