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

2017-03-23 Thread Michael S. Tsirkin 
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 
> ---
> 
> 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(, vb->stats, sizeof vb->stats);
>   if (virtqueue_add_outbuf(vb->stats_vq, , 1, vb, GFP_KERNEL)
>   < 0)
> -- 
> 2.7.4
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest

2017-03-23 Thread Michael S. Tsirkin 
On Thu, Mar 23, 2017 at 03:19:07PM +, Richard W.M. Jones wrote:
> On Thu, Mar 23, 2017 at 01:13:50PM +0800, Jason Wang wrote:
> > >From 312859b596e83a2164a8430343d31fce2a5ad808 Mon Sep 17 00:00:00 2001
> > From: Jason Wang 
> > Date: Thu, 23 Mar 2017 13:07:16 +0800
> > Subject: [PATCH] virtio_pci: fix out of bound access for msix_names
> > 
> > Signed-off-by: Jason Wang 
> 
> I tested this, and it does appear to fix the crashes in
> vp_modern_find_vqs.  Therefore:
> 
> Tested-by: Richard W.M. Jones 
> 
> Thanks,
> 
> Rich.

I've queued the fix, thanks everyone!

> >  drivers/virtio/virtio_pci_common.c | 9 +
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_pci_common.c 
> > b/drivers/virtio/virtio_pci_common.c
> > index df548a6..5905349 100644
> > --- a/drivers/virtio/virtio_pci_common.c
> > +++ b/drivers/virtio/virtio_pci_common.c
> > @@ -147,7 +147,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
> > unsigned nvqs,
> >  {
> > struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> > const char *name = dev_name(_dev->vdev.dev);
> > -   int i, err = -ENOMEM, allocated_vectors, nvectors;
> > +   int i, j, err = -ENOMEM, allocated_vectors, nvectors;
> > unsigned flags = PCI_IRQ_MSIX;
> > bool shared = false;
> > u16 msix_vec;
> > @@ -212,7 +212,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
> > unsigned nvqs,
> > if (!vp_dev->msix_vector_map)
> > goto out_disable_config_irq;
> >  
> > -   allocated_vectors = 1; /* vector 0 is the config interrupt */
> > +   allocated_vectors = j = 1; /* vector 0 is the config interrupt */
> > for (i = 0; i < nvqs; ++i) {
> > if (!names[i]) {
> > vqs[i] = NULL;
> > @@ -236,18 +236,19 @@ static int vp_find_vqs_msix(struct virtio_device 
> > *vdev, unsigned nvqs,
> > continue;
> > }
> >  
> > -   snprintf(vp_dev->msix_names[i + 1],
> > +   snprintf(vp_dev->msix_names[j],
> >  sizeof(*vp_dev->msix_names), "%s-%s",
> >  dev_name(_dev->vdev.dev), names[i]);
> > err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
> >   vring_interrupt, IRQF_SHARED,
> > - vp_dev->msix_names[i + 1], vqs[i]);
> > + vp_dev->msix_names[j], vqs[i]);
> > if (err) {
> > /* don't free this irq on error */
> > vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR;
> > goto out_remove_vqs;
> > }
> > vp_dev->msix_vector_map[i] = msix_vec;
> > +   j++;
> >  
> > /*
> >  * Use a different vector for each queue if they are available,
> > -- 
> > 2.7.4
> > 
> 
> 
> -- 
> Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
> Read my programming and virtualization blog: http://rwmj.wordpress.com
> virt-builder quickly builds VMs from scratch
> http://libguestfs.org/virt-builder.1.html
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio_balloon: prevent uninitialized variable use

2017-03-23 Thread Denis V. Lunev 
On 03/23/2017 06:17 PM, Arnd Bergmann wrote:
> The latest gcc-7.0.1 snapshot reports a new warning:
>
> virtio/virtio_balloon.c: In function 'update_balloon_stats':
> virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in 
> this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in 
> this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in 
> this function [-Werror=uninitialized]
> virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in 
> this function [-Werror=uninitialized]
>
> This seems absolutely right, so we should add an extra check to
> prevent copying uninitialized stack data into the statistics.
> From all I can tell, this has been broken since the statistics code
> was originally added in 2.6.34.
>
> Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon 
> driver (V4)")
> Signed-off-by: Arnd Bergmann 
Reviewed-by: Denis V. Lunev 



> ---
>  drivers/virtio/virtio_balloon.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 4e1191508228..cd5c54e2003d 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon 
> *vb)
>  
>   available = si_mem_available();
>  
> +#ifdef CONFIG_VM_EVENT_COUNTERS
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
>   pages_to_bytes(events[PSWPIN]));
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
>   pages_to_bytes(events[PSWPOUT]));
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
> +#endif
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
>   pages_to_bytes(i.freeram));
>   update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest

2017-03-23 Thread Richard W.M. Jones 
On Thu, Mar 23, 2017 at 01:13:50PM +0800, Jason Wang wrote:
> >From 312859b596e83a2164a8430343d31fce2a5ad808 Mon Sep 17 00:00:00 2001
> From: Jason Wang 
> Date: Thu, 23 Mar 2017 13:07:16 +0800
> Subject: [PATCH] virtio_pci: fix out of bound access for msix_names
> 
> Signed-off-by: Jason Wang 

I tested this, and it does appear to fix the crashes in
vp_modern_find_vqs.  Therefore:

Tested-by: Richard W.M. Jones 

Thanks,

Rich.

>  drivers/virtio/virtio_pci_common.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c 
> b/drivers/virtio/virtio_pci_common.c
> index df548a6..5905349 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -147,7 +147,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
> unsigned nvqs,
>  {
>   struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>   const char *name = dev_name(_dev->vdev.dev);
> - int i, err = -ENOMEM, allocated_vectors, nvectors;
> + int i, j, err = -ENOMEM, allocated_vectors, nvectors;
>   unsigned flags = PCI_IRQ_MSIX;
>   bool shared = false;
>   u16 msix_vec;
> @@ -212,7 +212,7 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
> unsigned nvqs,
>   if (!vp_dev->msix_vector_map)
>   goto out_disable_config_irq;
>  
> - allocated_vectors = 1; /* vector 0 is the config interrupt */
> + allocated_vectors = j = 1; /* vector 0 is the config interrupt */
>   for (i = 0; i < nvqs; ++i) {
>   if (!names[i]) {
>   vqs[i] = NULL;
> @@ -236,18 +236,19 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, 
> unsigned nvqs,
>   continue;
>   }
>  
> - snprintf(vp_dev->msix_names[i + 1],
> + snprintf(vp_dev->msix_names[j],
>sizeof(*vp_dev->msix_names), "%s-%s",
>dev_name(_dev->vdev.dev), names[i]);
>   err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
> vring_interrupt, IRQF_SHARED,
> -   vp_dev->msix_names[i + 1], vqs[i]);
> +   vp_dev->msix_names[j], vqs[i]);
>   if (err) {
>   /* don't free this irq on error */
>   vp_dev->msix_vector_map[i] = VIRTIO_MSI_NO_VECTOR;
>   goto out_remove_vqs;
>   }
>   vp_dev->msix_vector_map[i] = msix_vec;
> + j++;
>  
>   /*
>* Use a different vector for each queue if they are available,
> -- 
> 2.7.4
> 


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] virtio_balloon: prevent uninitialized variable use

2017-03-23 Thread Arnd Bergmann 
The latest gcc-7.0.1 snapshot reports a new warning:

virtio/virtio_balloon.c: In function 'update_balloon_stats':
virtio/virtio_balloon.c:258:26: error: 'events[2]' is used uninitialized in 
this function [-Werror=uninitialized]
virtio/virtio_balloon.c:260:26: error: 'events[3]' is used uninitialized in 
this function [-Werror=uninitialized]
virtio/virtio_balloon.c:261:56: error: 'events[18]' is used uninitialized in 
this function [-Werror=uninitialized]
virtio/virtio_balloon.c:262:56: error: 'events[17]' is used uninitialized in 
this function [-Werror=uninitialized]

This seems absolutely right, so we should add an extra check to
prevent copying uninitialized stack data into the statistics.
>From all I can tell, this has been broken since the statistics code
was originally added in 2.6.34.

Fixes: 9564e138b1f6 ("virtio: Add memory statistics reporting to the balloon 
driver (V4)")
Signed-off-by: Arnd Bergmann 
---
 drivers/virtio/virtio_balloon.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 4e1191508228..cd5c54e2003d 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -254,12 +254,14 @@ static void update_balloon_stats(struct virtio_balloon 
*vb)
 
available = si_mem_available();
 
+#ifdef CONFIG_VM_EVENT_COUNTERS
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_IN,
pages_to_bytes(events[PSWPIN]));
update_stat(vb, idx++, VIRTIO_BALLOON_S_SWAP_OUT,
pages_to_bytes(events[PSWPOUT]));
update_stat(vb, idx++, VIRTIO_BALLOON_S_MAJFLT, events[PGMAJFAULT]);
update_stat(vb, idx++, VIRTIO_BALLOON_S_MINFLT, events[PGFAULT]);
+#endif
update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMFREE,
pages_to_bytes(i.freeram));
update_stat(vb, idx++, VIRTIO_BALLOON_S_MEMTOT,
-- 
2.9.0

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest

2017-03-23 Thread Christoph Hellwig 
On Thu, Mar 23, 2017 at 01:13:50PM +0800, Jason Wang wrote:
>
>
> On 2017年03月23日 08:30, Laura Abbott wrote:
>> Hi,
>>
>> Fedora has received multiple reports of crashes when running
>> 4.11 as a guest
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1430297
>> https://bugzilla.redhat.com/show_bug.cgi?id=1434462
>> https://bugzilla.kernel.org/show_bug.cgi?id=194911
>> https://bugzilla.redhat.com/show_bug.cgi?id=1433899
>>
>> The crashes are not always consistent but they are generally
>> some flavor of oops or GPF in virtio related code. Multiple people
>> have done bisections (Thank you Thorsten Leemhuis and
>> Richard W.M. Jones) and found this commit to be at fault
>>
>> 07ec51480b5eb1233f8c1b0f5d7a7c8d1247c507 is the first bad commit
>> commit 07ec51480b5eb1233f8c1b0f5d7a7c8d1247c507
>> Author: Christoph Hellwig 
>> Date:   Sun Feb 5 18:15:19 2017 +0100
>>
>>  virtio_pci: use shared interrupts for virtqueues
>>   This lets IRQ layer handle dispatching IRQs to separate handlers 
>> for the
>>  case where we don't have per-VQ MSI-X vectors, and allows us to greatly
>>  simplify the code based on the assumption that we always have interrupt
>>  vector 0 (legacy INTx or config interrupt for MSI-X) available, and
>>  any other interrupt is request/freed throught the VQ, even if the
>>  actual interrupt line might be shared in some cases.
>>   This allows removing a great deal of variables keeping track of 
>> the
>>  interrupt state in struct virtio_pci_device, as we can now simply walk 
>> the
>>  list of VQs and deal with per-VQ interrupt handlers there, and only 
>> treat
>>  vector 0 special.
>>   Additionally clean up the VQ allocation code to properly unwind 
>> on error
>>  instead of having a single global cleanup label, which is error prone,
>>  and in this case also leads to more code.
>>   Signed-off-by: Christoph Hellwig 
>>  Signed-off-by: Michael S. Tsirkin 
>>
>> :04 04 79a8267ffb73f9d244267c5f68365305bddd4696 
>> 8832a160b978710bbd24ba6966f462b3faa27fcc M   drivers
>>
>> It doesn't revert cleanly so we haven't been able to do a clean
>> test. Any ideas?
>>
>> Thanks,
>> Laura
>
> Hello:
>
> Can you try the attached patch to see if it solves the problem? (At least 
> it silent KASan warnings for me).

This looks like a correct fix to me, independent of fixing the original
bug or not:

Reviewed-by: Christoph Hellwig 
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

2017-03-23 Thread Ladi Prosek 
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 
---

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!


 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(, vb->stats, sizeof vb->stats);
if (virtqueue_add_outbuf(vb->stats_vq, , 1, vb, GFP_KERNEL)
< 0)
-- 
2.7.4

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization