Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] virtio_balloon: infinite spin in virtballoon_free_page_report() on broken vq - [Medium] virtio_balloon: callback toggle side effects in wait_event() condition --
commit bced8a77808ca2850eb132d7f72c3d6fc5ae5935 Author: Michael S. Tsirkin <[email protected]> virtio_balloon: submit reported pages as individual buffers This commit submits each reported free page as an individual virtqueue buffer rather than a single scatter-gather chain to avoid exceeding the virtio 4 GiB descriptor chain limit. To prevent interrupt storms from per-page buffers, it disables reporting virtqueue callbacks at initialization and uses virtqueue_enable_cb_delayed_max() while waiting for completion in virtballoon_free_page_report(). > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index ab3e3d887e00f..0a331acf2c8ef 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -205,25 +205,53 @@ static int virtballoon_free_page_report(struct > page_reporting_dev_info *pr_dev_i > struct virtio_balloon *vb = > container_of(pr_dev_info, struct virtio_balloon, pr_dev_info); > struct virtqueue *vq = vb->reporting_vq; > - unsigned int unused, err; > + unsigned int i, err = 0; > > /* We should always be able to add these buffers to an empty queue. */ > - err = virtqueue_add_inbuf(vq, sg, nents, vb, GFP_NOWAIT); > + for (i = 0; i < nents; i++) { > + struct scatterlist one; > > - /* > - * In the extremely unlikely case that something has occurred and we > - * are able to trigger an error we will simply display a warning > - * and exit without actually processing the pages. > - */ > - if (WARN_ON_ONCE(err)) > - return err; > + sg_init_table(&one, 1); > + sg_set_page(&one, sg_page(&sg[i]), sg[i].length, > + sg[i].offset); > + err = virtqueue_add_inbuf(vq, &one, 1, &sg[i], GFP_NOWAIT); > > - virtqueue_kick(vq); > + /* > + * In the extremely unlikely case that something has occurred > + * and we are able to trigger an error we will simply display > + * a warning and exit without actually processing the pages. > + */ > + if (WARN_ON_ONCE(err)) { > + nents = i; > + break; > + } > + } > > - /* When host has read buffer, this completes via balloon_ack */ > - wait_event(vb->acked, virtqueue_get_buf(vq, &unused)); > + if (nents) { > + unsigned int unused; > > - return 0; > + virtqueue_kick(vq); > + > + /* When host has read buffer, this completes via balloon_ack. */ > + i = 0; > + wait_event(vb->acked, ({ > + for (;;) { > + virtqueue_disable_cb(vq); > + while (virtqueue_get_buf(vq, &unused)) > + i++; > + > + /* Stop waiting if device used all that we > added. */ > + if (i >= nents) > + break; > + > + if (virtqueue_enable_cb_delayed_max(vq)) > + break; > + } > + i >= nents; > + })); [Severity: High] Can this inner for (;;) loop spin indefinitely in kernel workqueue context when vq->broken becomes true before all nents buffers are drained? In page_reporting_process() -> page_reporting_process_zone() -> page_reporting_cycle() -> virtballoon_free_page_report(), if the device marks buffers as used in the ring and vq->broken becomes true before all nents buffers are consumed (either because virtqueue_get_buf() encounters a bad descriptor and calls BAD_RING(), or because virtio_break_device() runs concurrently during surprise removal or reset), virtqueue_get_buf() returns NULL without advancing vq->last_used_idx or incrementing i. Because i < nents, the loop then calls virtqueue_enable_cb_delayed_max(vq). Unlike virtqueue_enable_cb_delayed(), which checks vq->broken: drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed() { struct vring_virtqueue *vq = to_vvq(_vq); if (vq->event_triggered) data_race(vq->event_triggered = false); if (unlikely(vq->broken)) return true; ... } virtqueue_enable_cb_delayed_max() omits that check: drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max() { struct vring_virtqueue *vq = to_vvq(_vq); if (vq->event_triggered) data_race(vq->event_triggered = false); return VIRTQUEUE_CALL(vq, enable_cb_delayed_max); } When unconsumed used entries remain in the ring, virtqueue_enable_cb_delayed_max_split() and virtqueue_enable_cb_delayed_max_packed() both return false on every call: drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max_split() { ... if (unlikely((u16)(virtio16_to_cpu(vq->vq.vdev, vq->split.vring.used->idx) - vq->last_used_idx) > bufs)) { END_USE(vq); return false; } return true; } drivers/virtio/virtio_ring.c:virtqueue_enable_cb_delayed_max_packed() { ... if (is_used_desc_packed(vq, used_idx, wrap_counter)) { END_USE(vq); return false; } return true; } As a result, neither break condition in the for (;;) loop is ever met, and the thread spins in a tight non-sleeping loop inside the wait_event() condition rather than sleeping as noted in the commit message. Also, should virtqueue_enable_cb_delayed_max_split() read used->idx via vring_read_split_used_idx(vq) instead of a plain access? [Severity: Medium] Does placing virtqueue_disable_cb() and virtqueue_enable_cb_delayed_max() inside the wait_event() condition cause unintended callback state changes when wait_event() evaluates the condition multiple times? In include/linux/wait.h, wait_event() evaluates the condition once before calling __wait_event(), and ___wait_event() calls prepare_to_wait_event() followed immediately by a second evaluation of the condition before calling schedule(): include/linux/wait.h:wait_event() { might_sleep(); if (condition) break; __wait_event(wq_head, condition); } When the host has not yet consumed all nents buffers on the first check, virtqueue_enable_cb_delayed_max(vq) clears VRING_AVAIL_F_NO_INTERRUPT in vq->split.avail_flags_shadow, sets vq->event_triggered = false, writes vring_used_event = vq->last_used_idx + bufs, and issues virtio_store_mb(). Immediately afterwards, before sleeping in schedule(), the second condition check in ___wait_event() runs virtqueue_disable_cb(vq): drivers/virtio/virtio_ring.c:virtqueue_disable_cb_split() { if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) { vq->split.avail_flags_shadow |= VRING_AVAIL_F_NO_INTERRUPT; if (vq->event_triggered) return; if (vq->event) /* TODO: this is a hack. Figure out a cleaner value to write. */ vring_used_event(&vq->split.vring) = 0x0; ... } Because the first check enabled callbacks and vq->event_triggered is still false, virtqueue_disable_cb_split() writes vring_used_event = 0x0 into shared ring memory while the kicked device is actively processing buffers, right before virtqueue_enable_cb_delayed_max(vq) overwrites it again. When vq->last_used_idx == 0 (for example, on the first reporting batch after probe or on 16-bit wrap), transiently writing vring_used_event = 0x0 while the device processes the first buffer (used->idx advancing from 0 to 1) satisfies vring_need_event(0x0, 1, 0) on the host and triggers an immediate interrupt on the first page. The same double evaluation also happens on any wakeup of the shared vb->acked waitqueue from inflate_vq or deflate_vq. In addition, on a packed virtqueue, when vring_interrupt() fires and sets vq->event_triggered = true, waking wait_event() runs virtqueue_disable_cb_packed(vq): drivers/virtio/virtio_ring.c:virtqueue_disable_cb_packed() { if (vq->packed.event_flags_shadow != VRING_PACKED_EVENT_FLAG_DISABLE) { vq->packed.event_flags_shadow = VRING_PACKED_EVENT_FLAG_DISABLE; if (vq->event_triggered) return; vq->packed.vring.driver->flags = cpu_to_le16(vq->packed.event_flags_shadow); } } Because vq->event_triggered is true, virtqueue_disable_cb_packed() updates event_flags_shadow to VRING_PACKED_EVENT_FLAG_DISABLE and returns early without updating vq->packed.vring.driver->flags in shared memory. When i >= nents then breaks out of the loop without calling virtqueue_enable_cb_delayed_max(vq), driver->flags remains VRING_PACKED_EVENT_FLAG_DESC after virtballoon_free_page_report() returns, and subsequent virtqueue_disable_cb(vq) calls become no-ops because event_flags_shadow is already VRING_PACKED_EVENT_FLAG_DISABLE. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/32e21059adb6cb1fd08c3f4426a313c77cf450ec.1793057483.git....@redhat.com?part=2
