On Mon, Jul 06, 2026 at 09:28:50AM +0200, David Hildenbrand (Arm) wrote:
> 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 <[email protected]>
> > Assisted-by: Claude:claude-sonnet-4-6
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
> > ---
> >
> > 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?
Yes.
> Is that
> valid before we actually added+kicked ourselves?
Why not?
> Also, can't we simply handle that in stats_request(), just ignoring it there?
Then we'd need to maintain a special flag and I dislike that.
> Then we wouldn't have to go through the siable + enable.
We'd still need to flip the flag on/off.
> > }
> >
> > 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).
Will do, thanks!
> --
> Cheers,
>
> David
