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: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=0f659760bac1ecb01812
> Signed-off-by: Mingbing Zhai <[email protected]>
> ---
>
> 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
