Re: [PATCH v2] virtio_balloon: prime stats vq after virtio_device_ready()

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

On 7/5/26 21:12, Michael S. Tsirkin wrote:
> The virtio spec requires the driver not to kick the device before
> DRIVER_OK is set.  init_vqs() primes the stats virtqueue with a buffer
> and kicks the device before virtio_device_ready() is called in
> virtballoon_probe(), violating this requirement.
> 
> Further, if the device responds to the early kick by processing the
> buffer before DRIVER_OK, stats_request() fires and queues
> update_balloon_stats_work.  Should probe then fail and free vb, the work
> runs against freed memory.
> 
> To fix, move buffer setup to after DRIVER_OK. Be careful to
> disable update_balloon_stats_work while this is going on,
> to make sure it does not race with the setup.
> 
> setup_vqs() warns but does not fail probe or restore if
> virtqueue_add_outbuf() fails; the call never actually fails in these
> contexts since the queue is freshly initialized and empty.
> 
> Testing: tested that stats still work after the change.
> 
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon driver (V4)")
> Reported-by: Sashiko:gemini-3.1-pro-preview
> Cc: David Hildenbrand <david@kernel.org>
> Assisted-by: Claude:claude-sonnet-4-6
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> changes from v1:
> 	check that work enable/disable is balanced
> 	explain how add buf never fails in probe/restore
> 
>  drivers/virtio/virtio_balloon.c | 50 +++++++++++++++++++++------------
>  1 file changed, 32 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 088b3a0e6ce6..bc0a2b19ca7d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -611,25 +611,8 @@ static int init_vqs(struct virtio_balloon *vb)
>  	vb->inflate_vq = vqs[VIRTIO_BALLOON_VQ_INFLATE];
>  	vb->deflate_vq = vqs[VIRTIO_BALLOON_VQ_DEFLATE];
>  	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
> -		struct scatterlist sg;
> -		unsigned int num_stats;
>  		vb->stats_vq = vqs[VIRTIO_BALLOON_VQ_STATS];
> -
> -		/*
> -		 * Prime this virtqueue with one buffer so the hypervisor can
> -		 * use it to signal us later (it can't be broken yet!).
> -		 */
> -		num_stats = update_balloon_stats(vb);
> -
> -		sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> -		err = virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb,
> -					   GFP_KERNEL);
> -		if (err) {
> -			dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n",
> -				 __func__);
> -			return err;
> -		}
> -		virtqueue_kick(vb->stats_vq);
> +		disable_work(&vb->update_balloon_stats_work);

That's to stop the stats queue triggering stats_request() I assume? Is that
valid before we actually added+kicked ourselves?

Also, can't we simply handle that in stats_request(), just ignoring it there?
Then  we wouldn't have to go through the siable + enable.

>  	}
>  
>  	if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_FREE_PAGE_HINT))
> @@ -916,6 +899,33 @@ static int virtio_balloon_register_shrinker(struct virtio_balloon *vb)
>  	return 0;
>  }
>  
> +static void setup_vqs(struct virtio_balloon *vb)
> +{
> +	struct scatterlist sg;
> +	unsigned int num_stats;
> +	bool ret;
> +
> +	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ))
> +		return;
> +
> +	/*
> +	 * Prime this virtqueue with one buffer so the hypervisor can
> +	 * use it to signal us later (it can't be broken yet!).
> +	 */
> +	num_stats = update_balloon_stats(vb);
> +	sg_init_one(&sg, vb->stats, sizeof(vb->stats[0]) * num_stats);
> +	if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)) {
> +		dev_warn(&vb->vdev->dev, "%s: add stat_vq failed\n", __func__);
> +		return;
> +	}
> +	virtqueue_kick(vb->stats_vq);
> +
> +	ret = enable_and_queue_work(system_freezable_wq,
> +				    &vb->update_balloon_stats_work);
> +	/* Make sure we balanced enable/disable, or we won't report stats. */
> +	BUG_ON(!ret);

A WARN_ON_ONCE() should be good enough here, no need to crash the kernel (no new
BUG_ON's).

-- 
Cheers,

David