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