[PATCH] virtio_balloon: drain update works on probe error paths

[PATCH] virtio_balloon: drain update works on probe error paths

[Mingbing Zhai]

init_vqs() arms the stats virtqueue callback, so an update work can
already be queued on the system freezable workqueue while
virtballoon_probe() is still running.  If a later probe step fails, the
error unwind deletes the virtqueues and frees vb without stopping the
works, and the queued work runs after the balloon struct is gone:

  BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
   update_balloon_stats_func+0x121/0x300
   Workqueue: events_freezable update_balloon_stats_func

remove() already handles this via virtballoon_quiesce().  Factor the
stopping of the update works out of it and run it on the probe error
path as well, before the queues and vb go away.

Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
---
 drivers/virtio/virtio_balloon.c | 17 ++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
--- 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);
+}
+
 static void update_balloon_size_func(struct work_struct *work)
 {
 	struct virtio_balloon *vb;
@@ -1076,6 +1087,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);
@@ -1116,11 +1133,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		virtio_balloon_unregister_shrinker(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);
+	virtballoon_stop_updates(vb);

 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		cancel_work_sync(&vb->report_free_page_work);
--
2.47.0

Re: [PATCH] virtio_balloon: drain update works on probe error paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] virtio_balloon: uninitialized wakeup_lock used in virtballoon_probe()
- [High] virtio_balloon: leak vqs and UAF vb when init_vqs() fails after find_vqs
--

commit ead8a642cbbc78b91d17dc13d18353f2a7792fd1
Author: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>

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() error path before deleting the
virtqueues and freeing struct virtio_balloon.

> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index ab3e3d887e00f..caac7f5b3459c 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);
> +}
> +
>  static void update_balloon_size_func(struct work_struct *work)
>  {
>  	struct virtio_balloon *vb;
> @@ -1076,6 +1087,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]
Can this acquire an uninitialized vb->wakeup_lock when stats_request() fires
during virtballoon_probe()?

In virtballoon_probe(), init_vqs(vb) arms stats_request() and kicks stats_vq
early in the function, whereas spin_lock_init(&vb->wakeup_lock) is only called
near the end of virtballoon_probe(), after all of the error jumps that unwind
through out_del_vqs:

drivers/virtio/virtio_balloon.c:virtballoon_probe() {
	...
	spin_lock_init(&vb->stop_update_lock);
	mutex_init(&vb->balloon_lock);
	init_waitqueue_head(&vb->acked);
	vb->vdev = vdev;

	balloon_devinfo_init(&vb->vb_dev_info);

	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);
}

When stats_request() triggers during virtballoon_probe(), it calls
start_wakeup_event(vb, ...) -> spin_lock_irqsave(&vb->wakeup_lock, flags)
before vb->wakeup_lock has been initialized.

If a subsequent probe step fails and jumps to out_del_vqs,
spin_lock_init(&vb->wakeup_lock) is skipped altogether, and draining
update_balloon_stats_work in virtballoon_stop_updates(vb) runs:

update_balloon_stats_func()
  -> process_wakeup_event() -> spin_lock_irq(&vb->wakeup_lock)
  -> finish_wakeup_event()  -> spin_lock_irq(&vb->wakeup_lock)

on an uninitialized spinlock. Even when probe succeeds, if stats_request()
fires before spin_lock_init(&vb->wakeup_lock), spin_lock_init() will
re-initialize vb->wakeup_lock while it may already be in use. Should
spin_lock_init(&vb->wakeup_lock) be moved before init_vqs(vb)?

[Severity: High]
Also in virtballoon_probe(), what happens if init_vqs() fails at
virtqueue_add_outbuf() after virtio_find_vqs() has already succeeded?

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;
	...
	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
		...
		err = virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb,
					   GFP_KERNEL);
		if (err) {
			dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
				 __func__);
			return err;
		}
		virtqueue_kick(vb->stats_vq);
	}
}

If virtqueue_add_outbuf() fails (for example, on a DMA mapping failure),
init_vqs() returns an error while the virtqueues allocated by
virtio_find_vqs() and their callbacks remain active. In virtballoon_probe(),
the error path for init_vqs() jumps directly to out_free_vb, bypassing
out_del_vqs:

	err = init_vqs(vb);
	if (err)
		goto out_free_vb;

Could this leak the virtqueues and leave the armed callbacks and any queued
update_balloon_stats_work referencing vb after kfree(vb)?

[ ... ]

>
>
>

[]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019084406.162-1-zhaimingbing@cmss.chinamobile.com?part=1

[PATCH v2] virtio_balloon: drain update works on probe error paths

[Mingbing Zhai]

init_vqs() arms the stats virtqueue callback, so an update work can
already be queued on the system freezable workqueue while
virtballoon_probe() is still running.  If a later probe step fails, the
error unwind deletes the virtqueues and frees vb without stopping the
works, and the queued work runs after the balloon struct is gone:

  BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
   update_balloon_stats_func+0x121/0x300
   Workqueue: events_freezable update_balloon_stats_func

remove() already handles this via virtballoon_quiesce().  Factor the
stopping of the update works out of it and run it on the probe error
path as well, before the queues and vb go away.

The same hole exists when init_vqs() itself fails after
virtio_find_vqs() succeeded: the stats priming can fail with the queues
up and the callback armed, yet probe jumps straight to out_free_vb and
leaks the virtqueues while the queued work still references vb.  Unwind
inside init_vqs() in that case, stopping the works before the queues
are deleted.

The drained work takes vb->wakeup_lock in process_wakeup_event() and
finish_wakeup_event(), so this depends on "virtio_balloon: initialize
wakeup_lock before virtqueues are set up", which moves that
spin_lock_init() to the top of probe.

Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
---

Changes in v2 (from sashiko review):
- unwind inside init_vqs() when the stats priming fails after
  virtio_find_vqs() succeeded, instead of leaking the virtqueues on the
  out_free_vb path with the callback still armed.
- spell out the dependency on the wakeup_lock initialization patch:
  the drained work takes that lock.

 drivers/virtio/virtio_balloon.c | 29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8c1b47ec1..dd2056882 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);
+}
+
 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);
@@ -1075,6 +1092,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);
@@ -1115,11 +1138,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		virtio_balloon_unregister_shrinker(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);
+	virtballoon_stop_updates(vb);
 
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		cancel_work_sync(&vb->report_free_page_work);
-- 
2.43.0

Re: [PATCH v2] virtio_balloon: drain update works on probe error paths

[sashiko-bot]

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 <zhaimingbing@cmss.chinamobile.com>

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/20261020035744.1131-1-zhaimingbing@cmss.chinamobile.com?part=1

Re: [PATCH v2] virtio_balloon: drain update works on probe error paths

[David Hildenbrand (Arm)]

On 10/20/26 05:57, Mingbing Zhai wrote:
> init_vqs() arms the stats virtqueue callback, so an update work can
> already be queued on the system freezable workqueue while
> virtballoon_probe() is still running.  If a later probe step fails, the
> error unwind deletes the virtqueues and frees vb without stopping the
> works, and the queued work runs after the balloon struct is gone:
> 
>   BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
>    update_balloon_stats_func+0x121/0x300
>    Workqueue: events_freezable update_balloon_stats_func

Did you manage to reproduce this? How? (forcing the probe to fail?)

Does this require the device to trigger us even before virtio_device_ready(vdev)?

> 
> remove() already handles this via virtballoon_quiesce().  Factor the
> stopping of the update works out of it and run it on the probe error
> path as well, before the queues and vb go away.
> 
> The same hole exists when init_vqs() itself fails after
> virtio_find_vqs() succeeded: the stats priming can fail with the queues
> up and the callback armed, yet probe jumps straight to out_free_vb and
> leaks the virtqueues while the queued work still references vb.  Unwind
> inside init_vqs() in that case, stopping the works before the queues
> are deleted.
> 
> The drained work takes vb->wakeup_lock in process_wakeup_event() and
> finish_wakeup_event(), so this depends on "virtio_balloon: initialize
> wakeup_lock before virtqueues are set up", which moves that
> spin_lock_init() to the top of probe.
> 
> Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
> Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
> Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
> ---
> 
> Changes in v2 (from sashiko review):
> - unwind inside init_vqs() when the stats priming fails after
>   virtio_find_vqs() succeeded, instead of leaking the virtqueues on the
>   out_free_vb path with the callback still armed.
> - spell out the dependency on the wakeup_lock initialization patch:
>   the drained work takes that lock.
> 
>  drivers/virtio/virtio_balloon.c | 29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 8c1b47ec1..dd2056882 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);
> +}
> +
>  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.
> +			 */

Do we really need this comment?

> +			virtballoon_stop_updates(vb);
> +			vb->vdev->config->del_vqs(vb->vdev);

I guess this path is very hard to trigger.

>  			return err;
>  		}
>  		virtqueue_kick(vb->stats_vq);
> @@ -1075,6 +1092,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.
> +	 */

Do we really need this comment?

> +	virtballoon_stop_updates(vb);
>  	vdev->config->del_vqs(vdev);
>  out_free_vb:
>  	mutex_destroy(&vb->balloon_lock);
> @@ -1115,11 +1138,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
>  	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
>  		virtio_balloon_unregister_shrinker(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);
> +	virtballoon_stop_updates(vb);
>  
>  	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
>  		cancel_work_sync(&vb->report_free_page_work);


-- 
Cheers,

David

[PATCH v3] virtio_balloon: drain update works on probe error paths

[Mingbing Zhai]

init_vqs() arms the stats virtqueue callback, so an update work can
already be queued on the system freezable workqueue while
virtballoon_probe() is still running.  If a later probe step fails, the
error unwind deletes the virtqueues and frees vb without stopping the
works, and the queued work runs after the balloon struct is gone:

  BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
   update_balloon_stats_func+0x121/0x300
   Workqueue: events_freezable update_balloon_stats_func

remove() already handles this via virtballoon_quiesce().  Factor the
stopping of the update works out of it and run it on the probe error
path as well, before the queues and vb go away.

The same hole exists when init_vqs() itself fails after
virtio_find_vqs() succeeded: the stats priming can fail with the queues
up and the callback armed, yet probe jumps straight to out_free_vb and
leaks the virtqueues while the queued work still references vb.  Unwind
inside init_vqs() in that case, stopping the works before the queues
are deleted.

The drained work takes vb->wakeup_lock in process_wakeup_event() and
finish_wakeup_event(), so this depends on "virtio_balloon: initialize
wakeup_lock before virtqueues are set up", which moves that
spin_lock_init() to the top of probe.

Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
---

Hi David,

> Did you manage to reproduce this? How? (forcing the probe to fail?)

Yes -- KASAN, injection right after init_vqs(): queue both works,
mdelay() to hold the window open, then err = -ENOMEM; goto
out_del_vqs, driven by bind/unbind loops. Unpatched: KASAN
use-after-free with both stacks in virtballoon_probe().

> Does this require the device to trigger us even before
> virtio_device_ready(vdev)?

Yes -- the callback is armed and the stats buffer primed/kicked in
init_vqs(), and ready() is only reached at the end of a successful
probe.

> I guess this path is very hard to trigger.

Agreed. That one came from the sashiko bot's review of v1 for the vq
leak -- it does look at least theoretically reachable, though.

> Do we really need this comment?

Dropped both in v3.

Thanks,
Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>

 drivers/virtio/virtio_balloon.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8c1b47ec1..cb53acdf7 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);
+}
+
 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);
@@ -1075,6 +1088,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);
 out_free_vb:
 	mutex_destroy(&vb->balloon_lock);
@@ -1115,11 +1129,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		virtio_balloon_unregister_shrinker(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);
+	virtballoon_stop_updates(vb);
 
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		cancel_work_sync(&vb->report_free_page_work);
-- 
2.43.0

Re: [PATCH v3] virtio_balloon: drain update works on probe error paths

[sashiko-bot]

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 <zhaimingbing@cmss.chinamobile.com>

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/20261023022311.1577-1-zhaimingbing@cmss.chinamobile.com?part=1

[PATCH v4] virtio_balloon: drain update works on probe error paths

[Mingbing Zhai]

init_vqs() arms the stats virtqueue callback, so an update work can
already be queued on the system freezable workqueue while
virtballoon_probe() is still running.  If a later probe step fails, the
error unwind deletes the virtqueues and frees vb without stopping the
works, and the queued work runs after the balloon struct is gone:

  BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
   update_balloon_stats_func+0x121/0x300
   Workqueue: events_freezable update_balloon_stats_func

remove() already handles this via virtballoon_quiesce().  Factor the
stopping of the update works out of it and run it on the probe error
path as well, before the queues and vb go away.

The same hole exists when init_vqs() itself fails after
virtio_find_vqs() succeeded: the stats priming can fail with the queues
up and the callback armed, yet probe jumps straight to out_free_vb and
leaks the virtqueues while the queued work still references vb.  Unwind
inside init_vqs() in that case, stopping the works before the queues
are deleted.

Also reset the device before del_vqs() on these paths, like
remove_common() and the virtio_net/virtio_scsi error unwinds do:
without the reset the queues stay enabled on the device and the next
probe fails in setup_vq() with -ENOENT.

The drained work takes vb->wakeup_lock in process_wakeup_event() and
finish_wakeup_event(), so this depends on "virtio_balloon: initialize
wakeup_lock before virtqueues are set up", which moves that
spin_lock_init() to the top of probe.

Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
---

The wakeup_lock dependency is now in mainline (161aa708b02c).

Changes in v4 (from sashiko review):
- reset the device before del_vqs() on both error paths, like
  remove_common() and the virtio_net/virtio_scsi unwinds: without the
  reset queue_enable stays set and the next probe fails in setup_vq()
  with -ENOENT.

 drivers/virtio/virtio_balloon.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8c1b47ec1..b30a0ca26 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);
+}
+
 static void update_balloon_size_func(struct work_struct *work)
 {
 	struct virtio_balloon *vb;
@@ -633,6 +644,9 @@ 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);
+			virtio_reset_device(vb->vdev);
+			vb->vdev->config->del_vqs(vb->vdev);
 			return err;
 		}
 		virtqueue_kick(vb->stats_vq);
@@ -1075,6 +1089,8 @@ 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);
+	virtio_reset_device(vdev);
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	mutex_destroy(&vb->balloon_lock);
@@ -1115,11 +1131,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		virtio_balloon_unregister_shrinker(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);
+	virtballoon_stop_updates(vb);
 
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		cancel_work_sync(&vb->report_free_page_work);
-- 
2.43.0

Re: [PATCH v4] virtio_balloon: drain update works on probe error paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] virtio_balloon: inaccurate Fixes trailer commit in commit
  message
--

commit 86ecd6e43dcbe0f92a723582119232b2820f4329
Author: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>

> virtio_balloon: drain update works on probe error paths
>
> init_vqs() arms the stats virtqueue callback, so an update work can
> already be queued on the system freezable workqueue while
> virtballoon_probe() is still running.  If a later probe step fails, the
> error unwind deletes the virtqueues and frees vb without stopping the
> works, and the queued work runs after the balloon struct is gone:
>
>   BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
>    update_balloon_stats_func+0x121/0x300
>    Workqueue: events_freezable update_balloon_stats_func
>
> remove() already handles this via virtballoon_quiesce().  Factor the
> stopping of the update works out of it and run it on the probe error
> path as well, before the queues and vb go away.
>
> The same hole exists when init_vqs() itself fails after
> virtio_find_vqs() succeeded: the stats priming can fail with the queues
> up and the callback armed, yet probe jumps straight to out_free_vb and
> leaks the virtqueues while the queued work still references vb.  Unwind
> inside init_vqs() in that case, stopping the works before the queues
> are deleted.
>
> Also reset the device before del_vqs() on these paths, like
> remove_common() and the virtio_net/virtio_scsi error unwinds do:
> without the reset the queues stay enabled on the device and the next
> probe fails in setup_vq() with -ENOENT.
>
> The drained work takes vb->wakeup_lock in process_wakeup_event() and
> finish_wakeup_event(), so this depends on "virtio_balloon: initialize
> wakeup_lock before virtqueues are set up", which moves that
> spin_lock_init() to the top of probe.
>
> Fixes: 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon stats")
> Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
> Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>

[Severity: Low]
In 1f34c71afe51 ("virtio: Fix scheduling while atomic in virtio_balloon
stats"), stats_request() only set vb->need_stats_update and woke the
vballoon kthread, which was created as the final fallible step in
virtballoon_probe(), and init_vqs() called BUG() if stats_vq priming
failed.

The undrained work on the virtballoon_probe() error path was introduced in
fad7b7b27b6a ("virtio_balloon: Use a workqueue instead of "vballoon"
kthread"), and the early error return in init_vqs() without cleaning up the
virtqueues was introduced in 74cf5b169c4c ("virtio-balloon: remove BUG() in
init_vqs").  Should the Fixes: tags reference those two commits instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109062152.1097-1-zhaimingbing@cmss.chinamobile.com?part=1

[PATCH v5] virtio_balloon: drain update works on probe error paths

[Mingbing Zhai]

init_vqs() arms the stats virtqueue callback, so an update work can
already be queued on the system freezable workqueue while
virtballoon_probe() is still running.  If a later probe step fails, the
error unwind deletes the virtqueues and frees vb without stopping the
works, and the queued work runs after the balloon struct is gone:

  BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
   update_balloon_stats_func+0x121/0x300
   Workqueue: events_freezable update_balloon_stats_func

remove() already handles this via virtballoon_quiesce().  Factor the
stopping of the update works out of it and run it on the probe error
path as well, before the queues and vb go away.

The same hole exists when init_vqs() itself fails after
virtio_find_vqs() succeeded: the stats priming can fail with the queues
up and the callback armed, yet probe jumps straight to out_free_vb and
leaks the virtqueues while the queued work still references vb.  Unwind
inside init_vqs() in that case, stopping the works before the queues
are deleted.

Also reset the device before del_vqs() on these paths, like
remove_common() and the virtio_net/virtio_scsi error unwinds do:
without the reset the queues stay enabled on the device and the next
probe fails in setup_vq() with -ENOENT.

The drained work takes vb->wakeup_lock in process_wakeup_event() and
finish_wakeup_event(), so this depends on "virtio_balloon: initialize
wakeup_lock before virtqueues are set up", which moves that
spin_lock_init() to the top of probe.

Fixes: fad7b7b27b6a ("virtio_balloon: Use a workqueue instead of "vballoon" kthread")
Fixes: 74cf5b169c4c ("virtio-balloon: remove BUG() in init_vqs")
Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>
---

The wakeup_lock dependency is now in mainline (161aa708b02c).

Changes in v5 (from sashiko review):
- point the Fixes tags at the commits that actually introduced the two
  hazards: the workqueue conversion (fad7b7b27b6a) for the undrained
  works, and the BUG() removal (74cf5b169c4c) for the uncleaned early
  return in init_vqs().

 drivers/virtio/virtio_balloon.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 8c1b47ec17a2..b30a0ca26292 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);
+}
+
 static void update_balloon_size_func(struct work_struct *work)
 {
 	struct virtio_balloon *vb;
@@ -633,6 +644,9 @@ 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);
+			virtio_reset_device(vb->vdev);
+			vb->vdev->config->del_vqs(vb->vdev);
 			return err;
 		}
 		virtqueue_kick(vb->stats_vq);
@@ -1075,6 +1089,8 @@ 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);
+	virtio_reset_device(vdev);
 	vdev->config->del_vqs(vdev);
 out_free_vb:
 	mutex_destroy(&vb->balloon_lock);
@@ -1115,11 +1131,7 @@ static void virtballoon_quiesce(struct virtio_balloon *vb)
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		virtio_balloon_unregister_shrinker(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);
+	virtballoon_stop_updates(vb);
 
 	if (virtio_has_feature(vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
 		cancel_work_sync(&vb->report_free_page_work);
-- 
2.53.0.windows.1

Re: [PATCH v5] virtio_balloon: drain update works on probe error paths

[sashiko-bot]

> init_vqs() arms the stats virtqueue callback, so an update work can
> already be queued on the system freezable workqueue while
> virtballoon_probe() is still running.  If a later probe step fails, the
> error unwind deletes the virtqueues and frees vb without stopping the
> works, and the queued work runs after the balloon struct is gone:
> 
>   BUG: KFENCE: use-after-free read in virtqueue_get_buf_ctx
>    update_balloon_stats_func+0x121/0x300
>    Workqueue: events_freezable update_balloon_stats_func
> 
> remove() already handles this via virtballoon_quiesce().  Factor the
> stopping of the update works out of it and run it on the probe error
> path as well, before the queues and vb go away.
> 
> The same hole exists when init_vqs() itself fails after
> [ ... ]
> Fixes: fad7b7b27b6a ("virtio_balloon: Use a workqueue instead of "vballoon" kthread")
> Fixes: 74cf5b169c4c ("virtio-balloon: remove BUG() in init_vqs")
> Reported-by: syzbot+0f659760bac1ecb01812@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
> Signed-off-by: Mingbing Zhai <zhaimingbing@cmss.chinamobile.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109072651.1410-1-zhaimingbing@cmss.chinamobile.com?part=1