Thank you for your contribution! Sashiko AI review found 3 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 - [High] virtio_balloon: race between init_vqs() and stats_handle_request() --
commit e212fced31f70fa27727822aaf449978f76f3a04 Author: Mingbing Zhai <[email protected]> virtio_balloon: drain update works on probe error paths This commit factors out virtballoon_stop_updates() from virtballoon_quiesce() and calls it on the virtballoon_probe() and init_vqs() error paths to stop and drain pending balloon update works before deleting virtqueues and freeing the virtio_balloon structure. > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index ab3e3d887e00f..fe99ab0fea227 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] Does draining vb->update_balloon_stats_work via virtballoon_stop_updates() on the init_vqs() and virtballoon_probe() error paths acquire an uninitialized vb->wakeup_lock? In virtballoon_probe(), init_vqs(vb) is called near the beginning of probe, while spin_lock_init(&vb->wakeup_lock) is not called until near the end of virtballoon_probe(), after init_vqs() and after all error jumps to out_del_vqs, out_del_balloon_wq, out_unregister_shrinker, and out_unregister_oom: 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); ... } Once init_vqs() calls virtio_find_vqs(), the stats_vq callback stats_request() is armed and calls start_wakeup_event(), which executes spin_lock_irqsave(&vb->wakeup_lock, flags) on the uninitialized lock. If init_vqs() fails at virtqueue_add_outbuf() or a later step in virtballoon_probe() fails and jumps to out_del_vqs, virtballoon_stop_updates(vb) drains update_balloon_stats_func(), which calls process_wakeup_event() and finish_wakeup_event() and executes spin_lock_irq(&vb->wakeup_lock) without spin_lock_init(&vb->wakeup_lock) ever having been called. Furthermore, if stats_request() fires during or after init_vqs() and probe succeeds, spin_lock_init(&vb->wakeup_lock) will re-initialize the lock while stats_request() or update_balloon_stats_func() may already be holding or contending on it. Although the commit message notes a dependency on "virtio_balloon: initialize wakeup_lock before virtqueues are set up", that change is not present in the tree or included in this commit. Should spin_lock_init(&vb->wakeup_lock) be moved before init_vqs(vb) here so that vb->wakeup_lock is initialized before the virtqueue callbacks are armed and before virtballoon_stop_updates() can drain the work? > > static void update_balloon_size_func(struct work_struct *work) > { > struct virtio_balloon *vb; > @@ -633,6 +644,12 @@ static int init_vqs(struct virtio_balloon *vb) > if (err) { > dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n", > __func__); > + /* > + * The virtqueues are up and the stats callback is > + * armed: stop the works and unwind the queues. > + */ > + virtballoon_stop_updates(vb); > + vb->vdev->config->del_vqs(vb->vdev); > return err; > } > virtqueue_kick(vb->stats_vq); [Severity: High] Can stats_request() and stats_handle_request() race with init_vqs() as soon as virtio_find_vqs() returns? In init_vqs(), virtio_find_vqs() sets up the virtqueues and arms the stats_vq callback stats_request() while vb->stop_update is false, before vb->stats_vq is assigned and before initial priming finishes: drivers/virtio/virtio_balloon.c:init_vqs() { ... err = virtio_find_vqs(vb->vdev, VIRTIO_BALLOON_VQ_MAX, vqs, vqs_info, NULL); if (err) return err; vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE]; vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE]; if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) { struct scatterlist sg; unsigned int num_stats; vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS]; num_stats = update_balloon_stats(vb); sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats); err = virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL); ... } If the device triggers stats_request() and update_balloon_stats_func() -> stats_handle_request() runs on another CPU before vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS] is assigned, stats_handle_request() will read vq = vb->stats_vq as NULL and pass it to virtqueue_get_buf(vq, &len): drivers/virtio/virtio_balloon.c:stats_handle_request() { ... num_stats = update_balloon_stats(vb); vq = vb->stats_vq; if (!virtqueue_get_buf(vq, &len)) return; sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats); err = virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL); if (WARN_ON_ONCE(err)) return; virtqueue_kick(vq); } Even after vb->stats_vq is assigned, if stats_handle_request() runs concurrently with the priming in init_vqs(), both paths will concurrently write to vb->stats[] in update_balloon_stats(vb) and invoke unserialized virtqueue operations (virtqueue_get_buf(), virtqueue_add_outbuf(), virtqueue_kick()) on vb->stats_vq without any lock. > @@ -1076,6 +1093,12 @@ 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: > + /* > + * init_vqs() arms the stats virtqueue callback and primes the > + * queue, so an update work may already be pending: stop queueing > + * and drain it before the virtqueues and vb go away. > + */ > + virtballoon_stop_updates(vb); > vdev->config->del_vqs(vdev); > out_free_vb: > mutex_destroy(&vb->balloon_lock); [Severity: High] Should virtio_reset_device(vdev) be called before vdev->config->del_vqs(vdev) here at out_del_vqs (and on the error path in init_vqs())? In init_vqs(), virtio_find_vqs() enables the virtqueues and arms IRQ callbacks, and when VIRTIO_BALLOON_F_STATS_VQ is negotiated, init_vqs() adds vb->stats as an outbuf to vb->stats_vq via virtqueue_add_outbuf() and kicks vb->stats_vq. In addition, if update_balloon_stats_func() was running when virtballoon_stop_updates(vb) drained it, stats_handle_request() will have re-added vb->stats to vb->stats_vq and kicked the queue again. Unlike remove_common(), which 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); } neither out_del_vqs nor init_vqs() calls virtio_reset_device() before calling del_vqs() and kfree(vb) (and virtio_dev_probe() only sets VIRTIO_CONFIG_S_FAILED on error without resetting the device). Without virtio_reset_device(vdev), on modern virtio-pci (vp_modern_del_vqs()) enabled queues are not disabled without a reset, leaving the device active with DMA mappings to the freed vring pages and to vb->stats after kfree(vb). On virtio-mmio (vm_del_vqs()), vring_del_virtqueue() deletes and frees each virtqueue from vdev->vqs before free_irq() is called, so without virtio_reset_device() to stop the device and synchronize callbacks, vm_interrupt() can concurrently iterate vdev->vqs and dereference freed virtqueue structures. [ ... ] > > > [] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
