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

[sashiko-bot@kernel.org]

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