* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
@ 2017-03-23 5:13 ` Jason Wang
0 siblings, 0 replies; 11+ messages in thread
From: Jason Wang @ 2017-03-23 5:13 UTC (permalink / raw)
To: Laura Abbott, Christoph Hellwig, Michael S. Tsirkin
Cc: Linux Kernel Mailing List, virtualization
[-- Attachment #1: Type: text/plain, Size: 2268 bytes --]
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 <hch@lst.de>
> 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 <hch@lst.de>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> :040000 040000 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).
Thanks
[-- Attachment #2: 0001-virtio_pci-fix-out-of-bound-access-for-msix_names.patch --]
[-- Type: text/x-patch, Size: 2040 bytes --]
From 312859b596e83a2164a8430343d31fce2a5ad808 Mon Sep 17 00:00:00 2001
From: Jason Wang <jasowang@redhat.com>
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 <jasowang@redhat.com>
---
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(&vp_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(&vp_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
[-- Attachment #3: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
2017-03-23 5:13 ` Jason Wang
@ 2017-03-23 14:22 ` Christoph Hellwig
-1 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2017-03-23 14:22 UTC (permalink / raw)
To: Jason Wang
Cc: Laura Abbott, Christoph Hellwig, Michael S. Tsirkin,
Linux Kernel Mailing List, virtualization
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 <hch@lst.de>
>> 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 <hch@lst.de>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> :040000 040000 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 <hch@lst.de>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
@ 2017-03-23 14:22 ` Christoph Hellwig
0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2017-03-23 14:22 UTC (permalink / raw)
To: Jason Wang
Cc: Linux Kernel Mailing List, Laura Abbott, virtualization,
Christoph Hellwig, Michael S. Tsirkin
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 <hch@lst.de>
>> 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 <hch@lst.de>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> :040000 040000 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 <hch@lst.de>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
2017-03-23 5:13 ` Jason Wang
@ 2017-03-23 15:19 ` Richard W.M. Jones
-1 siblings, 0 replies; 11+ messages in thread
From: Richard W.M. Jones @ 2017-03-23 15:19 UTC (permalink / raw)
To: Jason Wang
Cc: Laura Abbott, Christoph Hellwig, Michael S. Tsirkin,
Linux Kernel Mailing List, virtualization, Thorsten Leemhuis
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 <jasowang@redhat.com>
> 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 <jasowang@redhat.com>
I tested this, and it does appear to fix the crashes in
vp_modern_find_vqs. Therefore:
Tested-by: Richard W.M. Jones <rjones@redhat.com>
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(&vp_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(&vp_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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
@ 2017-03-23 15:19 ` Richard W.M. Jones
0 siblings, 0 replies; 11+ messages in thread
From: Richard W.M. Jones @ 2017-03-23 15:19 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, Thorsten Leemhuis, Linux Kernel Mailing List,
Laura Abbott, virtualization, Christoph Hellwig
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 <jasowang@redhat.com>
> 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 <jasowang@redhat.com>
I tested this, and it does appear to fix the crashes in
vp_modern_find_vqs. Therefore:
Tested-by: Richard W.M. Jones <rjones@redhat.com>
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(&vp_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(&vp_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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
2017-03-23 15:19 ` Richard W.M. Jones
@ 2017-03-23 16:41 ` Michael S. Tsirkin
-1 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2017-03-23 16:41 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Jason Wang, Laura Abbott, Christoph Hellwig,
Linux Kernel Mailing List, virtualization, Thorsten Leemhuis
On Thu, Mar 23, 2017 at 03:19:07PM +0000, 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 <jasowang@redhat.com>
> > 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 <jasowang@redhat.com>
>
> I tested this, and it does appear to fix the crashes in
> vp_modern_find_vqs. Therefore:
>
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
>
> 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(&vp_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(&vp_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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
@ 2017-03-23 16:41 ` Michael S. Tsirkin
0 siblings, 0 replies; 11+ messages in thread
From: Michael S. Tsirkin @ 2017-03-23 16:41 UTC (permalink / raw)
To: Richard W.M. Jones
Cc: Linux Kernel Mailing List, Thorsten Leemhuis, virtualization,
Laura Abbott, Christoph Hellwig
On Thu, Mar 23, 2017 at 03:19:07PM +0000, 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 <jasowang@redhat.com>
> > 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 <jasowang@redhat.com>
>
> I tested this, and it does appear to fix the crashes in
> vp_modern_find_vqs. Therefore:
>
> Tested-by: Richard W.M. Jones <rjones@redhat.com>
>
> 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(&vp_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(&vp_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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
2017-03-23 16:41 ` Michael S. Tsirkin
(?)
@ 2017-03-23 17:15 ` Thorsten Leemhuis
-1 siblings, 0 replies; 11+ messages in thread
From: Thorsten Leemhuis @ 2017-03-23 17:15 UTC (permalink / raw)
To: Michael S. Tsirkin, Richard W.M. Jones
Cc: Jason Wang, Laura Abbott, Christoph Hellwig,
Linux Kernel Mailing List, virtualization
On 23.03.2017 17:41, Michael S. Tsirkin wrote:
> On Thu, Mar 23, 2017 at 03:19:07PM +0000, 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 <jasowang@redhat.com>
>>> 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 <jasowang@redhat.com>
>> I tested this, and it does appear to fix the crashes in
>> vp_modern_find_vqs. Therefore:
>> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> I've queued the fix, thanks everyone!
Thx. Feel free to add
Tested-by: Thorsten Leemhuis <linux@leemhuis.info>
as a kernel with that patch works well in my reboot tests so far.
Ciao, Thorsten
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [REGRESSION] 07ec51480b5e ("virtio_pci: use shared interrupts for virtqueues") causes crashes in guest
2017-03-23 16:41 ` Michael S. Tsirkin
(?)
(?)
@ 2017-03-23 17:15 ` Thorsten Leemhuis
-1 siblings, 0 replies; 11+ messages in thread
From: Thorsten Leemhuis @ 2017-03-23 17:15 UTC (permalink / raw)
To: Michael S. Tsirkin, Richard W.M. Jones
Cc: Laura Abbott, virtualization, Christoph Hellwig,
Linux Kernel Mailing List
On 23.03.2017 17:41, Michael S. Tsirkin wrote:
> On Thu, Mar 23, 2017 at 03:19:07PM +0000, 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 <jasowang@redhat.com>
>>> 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 <jasowang@redhat.com>
>> I tested this, and it does appear to fix the crashes in
>> vp_modern_find_vqs. Therefore:
>> Tested-by: Richard W.M. Jones <rjones@redhat.com>
> I've queued the fix, thanks everyone!
Thx. Feel free to add
Tested-by: Thorsten Leemhuis <linux@leemhuis.info>
as a kernel with that patch works well in my reboot tests so far.
Ciao, Thorsten
^ permalink raw reply [flat|nested] 11+ messages in thread