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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 22, 2017 at 6:15 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
> On Wed, Mar 22, 2017 at 06:05:39PM +0100, Ladi Prosek wrote:
>> On Wed, Mar 22, 2017 at 5:14 PM, Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>> > On Wed, Mar 22, 2017 at 04:10:27PM +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 initializes all tags with U16_MAX which is guaranteed to
>> >> be ignored by the host.
>> >>
>> >> The alternative fix would be to push an empty buffer in init_vqs but
>> >> that's not easily done with the current virtio implementation and
>> >> would still not eliminate the invariant that update_balloon_stats
>> >> has to update all VIRTIO_BALLOON_S_NR fields.
>> >>
>> >> Signed-off-by: Ladi Prosek <lprosek@xxxxxxxxxx>
>> >> ---
>> >>  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..d34f56f 100644
>> >> --- a/drivers/virtio/virtio_balloon.c
>> >> +++ b/drivers/virtio/virtio_balloon.c
>> >> @@ -423,12 +423,16 @@ static int init_vqs(struct virtio_balloon *vb)
>> >>       vb->deflate_vq = vqs[1];
>> >>       if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_STATS_VQ)) {
>> >>               struct scatterlist sg;
>> >> +             int idx;
>> >>               vb->stats_vq = vqs[2];
>> >>
>> >>               /*
>> >>                * Prime this virtqueue with one buffer so the hypervisor can
>> >>                * use it to signal us later (it can't be broken yet!).
>> >>                */
>> >> +             for (idx = 0; idx < VIRTIO_BALLOON_S_NR; idx++)
>> >> +                     update_stat(vb, idx, U16_MAX, 0);
>> >> +
>> >
>> > Can't we fill in actual values?
>>
>> We can if we're fine with the unsolicited update.
>>
>> In QEMU, this would mean that you'll see valid stats in
>> /machine/peripheral/balloon0/guest-stats without setting the
>> guest-stats-polling-interval. And they would be updated on driver
>> (re-)load, so kind of arbitrarily from host pov.
>
> IUC the only issue would be that someone might come to
> depend on this. Doesn't look like a big deal to me.

Ok, I'll post v2 of the patch to use actual values when sending the
initial buffer.

Btw, I checked the Windows driver and it's been sending all FF's in
the first stats buffer since about 2011. Just for completeness,
probably won't change your mind :)

Thanks!
Ladi

>> On the other hand, this patch does not really comply with (5.5.6.3.1
>> Driver Requirements: Memory Statistics):
>>
>> "Driver MUST supply the same subset of statistics in all buffers
>> submitted to the statsq."
>>
>> So going by the letter of the law, we should fill in actual values.
>> Going by the (the purpose of the initial buffer is to wait for the
>> host to request stats), we should push an empty buffer or invalid
>> values.
>> >>               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
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization



[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux