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

[sashiko-bot@kernel.org]

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