Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] virtio_balloon: uninitialized wakeup_lock used in virtballoon_stop_updates() - [High] virtio_balloon: missing virtio_reset_device() before del_vqs() on probe error --
commit e9743442ee54f8ae6c7693dbd62ca4b60f4254f3 Author: Mingbing Zhai <[email protected]> virtio_balloon: drain update works on probe error paths This commit factors out virtballoon_stop_updates() from virtballoon_quiesce() to stop and drain pending balloon size and stats update work items. It calls virtballoon_stop_updates() on error unwind paths in init_vqs() and virtballoon_probe() before deleting virtqueues and freeing struct virtio_balloon. > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index ab3e3d887e00f..b8b70a0943dd1 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -554,6 +554,17 @@ static void update_balloon_stats_func(struct work_struct > *work) > finish_wakeup_event(vb); > } > > +/* Stop queueing of and drain any pending balloon update works. */ > +static void virtballoon_stop_updates(struct virtio_balloon *vb) > +{ > + spin_lock_irq(&vb->stop_update_lock); > + vb->stop_update = true; > + spin_unlock_irq(&vb->stop_update_lock); > + > + cancel_work_sync(&vb->update_balloon_size_work); > + cancel_work_sync(&vb->update_balloon_stats_work); > +} [Severity: High] When virtballoon_stop_updates() drains update_balloon_stats_work or update_balloon_size_work on the init_vqs() and virtballoon_probe() error paths, will the drained work operate on an uninitialized vb->wakeup_lock? In drivers/virtio/virtio_balloon.c, update_balloon_stats_func() and update_balloon_size_func() both call process_wakeup_event() and finish_wakeup_event(), which acquire vb->wakeup_lock: drivers/virtio/virtio_balloon.c:process_wakeup_event() { spin_lock_irq(&vb->wakeup_lock); vb->wakeup_signal_mask &= ~mask; spin_unlock_irq(&vb->wakeup_lock); } drivers/virtio/virtio_balloon.c:finish_wakeup_event() { spin_lock_irq(&vb->wakeup_lock); if (!vb->wakeup_signal_mask && vb->processing_wakeup_event) { vb->processing_wakeup_event = false; pm_relax(&vb->vdev->dev); } spin_unlock_irq(&vb->wakeup_lock); } Similarly, stats_request() calls start_wakeup_event(), which takes spin_lock_irqsave(&vb->wakeup_lock, flags). However, in virtballoon_probe(), spin_lock_init(&vb->wakeup_lock) is not called until near the end of probe, after init_vqs() and after all of the error jumps that unwind via out_del_vqs: drivers/virtio/virtio_balloon.c:virtballoon_probe() { ... err = init_vqs(vb); if (err) goto out_free_vb; ... err = page_reporting_register(&vb->pr_dev_info); if (err) goto out_unregister_oom; } spin_lock_init(&vb->wakeup_lock); } The commit message notes that this patch depends on "virtio_balloon: initialize wakeup_lock before virtqueues are set up" to move spin_lock_init(&vb->wakeup_lock) to the top of virtballoon_probe(), but that change is not present in this tree. Without it, if stats_request() fires after virtio_find_vqs() in init_vqs(), both the callback and the work drained by virtballoon_stop_updates() will lock an uninitialized spinlock_t (and if probe succeeds, spin_lock_init(&vb->wakeup_lock) can reinitialize the lock while it is in use). Should spin_lock_init(&vb->wakeup_lock) be moved before init_vqs() here or the prerequisite patch be applied first? > + > static void update_balloon_size_func(struct work_struct *work) > { > struct virtio_balloon *vb; > @@ -633,6 +644,8 @@ static int init_vqs(struct virtio_balloon *vb) > if (err) { > dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n", > __func__); > + virtballoon_stop_updates(vb); > + vb->vdev->config->del_vqs(vb->vdev); > return err; > } > virtqueue_kick(vb->stats_vq); > @@ -1076,6 +1089,7 @@ static int virtballoon_probe(struct virtio_device *vdev) > if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT)) > destroy_workqueue(vb->balloon_wq); > out_del_vqs: > + virtballoon_stop_updates(vb); > vdev->config->del_vqs(vdev); [Severity: High] Should the error unwind paths in init_vqs() and virtballoon_probe() call virtio_reset_device() before calling del_vqs(), just as remove_common() does? In drivers/virtio/virtio_balloon.c, remove_common() resets the device before deleting the virtqueues: drivers/virtio/virtio_balloon.c:remove_common() { ... /* Now we reset the device so we can clean up the queues. */ virtio_reset_device(vb->vdev); vb->vdev->config->del_vqs(vb->vdev); } Once virtio_find_vqs() succeeds in init_vqs(), the vring DMA addresses are programmed and the queues are enabled on the transport (for example, vp_modern_find_vqs() sets queue_enable to 1). When VIRTIO_BALLOON_F_STATS_VQ is negotiated, init_vqs() also queues vb->stats into vb->stats_vq via virtqueue_add_outbuf() and calls virtqueue_kick(vb->stats_vq) (and if update_balloon_stats_func() runs while virtballoon_stop_updates() drains it, stats_handle_request() re-queues vb->stats and kicks stats_vq again). If init_vqs() fails after virtio_find_vqs() or a later step in virtballoon_probe() fails and jumps to out_del_vqs, virtio_dev_probe() only adds VIRTIO_CONFIG_S_FAILED without resetting the device. Calling del_vqs() and kfree(vb) without virtio_reset_device() first can leave the device with active queues pointing to freed vring and vb->stats DMA memory, cause subsequent re-probes on modern virtio-pci to fail with -ENOENT in setup_vq() because queue_enable remains set, and allow vring interrupts to race with queue teardown. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
