[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

[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

[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