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

["David Hildenbrand (Arm)" <david@kernel.org>]

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