Re: [PATCH v2] virtio_balloon: don't push uninitialized buffers to stats virtqueue

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 23, 2017 at 08:04:18AM +0100, Ladi Prosek wrote:
> When init_vqs runs, virtio_balloon.stats is either uninitialized or
> contains stale values. The host updates its state with garbage data
> because it has no way of knowing that this is just a marker buffer
> used for signaling.
> 
> This patch updates the stats before pushing the initial buffer.
> An assertion is also added to update_balloon_stats to make sure
> that all stats fields are initialized.
> 
> Alternative fixes:
> * Push an empty buffer in init_vqs. Not easily done with the current
>   virtio implementation and violates the spec "Driver MUST supply the
>   same subset of statistics in all buffers submitted to the statsq".
> * Push a buffer with invalid tags in init_vqs. Violates the same
>   spec clause, plus "invalid tag" is not really defined.
> 
> Signed-off-by: Ladi Prosek <lprosek@redhat.com>
> ---
> 
> v1->v2:
> 
> * Call update_balloon_stats instead of filling the buffer with
>   invalid tags.
> * Add BUG_ON to update_balloon_stats to formalize the invariant that
>   all slots in the buffer must be initialized.
> 
> 
> Also, I just noticed this paragraph in the spec:
> 
> "When using the legacy interface, the device SHOULD ignore all values in
> the first buffer in the statsq supplied by the driver after device
> initialization. Note: Historically, drivers supplied an uninitialized
> buffer in the first buffer."
> 
> So someone knew about this bug but didn't fix the Linux driver and also
> didn't implement the SHOULD in QEMU. Makes me wonder if I'm missing
> something here.
> 
> Thanks!

I would say let's fix QEMU as well.

> 
>  drivers/virtio/virtio_balloon.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e11915..42dc35f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -266,6 +266,8 @@ static void update_balloon_stats(struct virtio_balloon *vb)
>  				pages_to_bytes(i.totalram));
>  	update_stat(vb, idx++, VIRTIO_BALLOON_S_AVAIL,
>  				pages_to_bytes(available));
> +
> +	BUG_ON(idx < VIRTIO_BALLOON_S_NR);
>  }
>  
>  /*
> @@ -429,6 +431,8 @@ static int init_vqs(struct virtio_balloon *vb)
>  		 * Prime this virtqueue with one buffer so the hypervisor can
>  		 * use it to signal us later (it can't be broken yet!).
>  		 */
> +		update_balloon_stats(vb);
> +
>  		sg_init_one(&sg, vb->stats, sizeof vb->stats);
>  		if (virtqueue_add_outbuf(vb->stats_vq, &sg, 1, vb, GFP_KERNEL)
>  		    < 0)
> -- 
> 2.7.4