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 <[email protected]> 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/[email protected]?part=3
