[PATCH] msi: Avoid uninitialized msi descriptors

[PATCH] msi: Avoid uninitialized msi descriptors

[Wei Wang2]

[-- Attachment #1: Type: text/plain, Size: 764 bytes --]

Hi,
When __pci_enable_msix() returns early, output parameter (struct msi_desc 
**desc) will not be initialized.  On my machine, a Broadcom BCM5709 nic has 
both MSI and MSIX capability blocks and when guest tries to enable msix 
interrupts but __pci_enable_msix() returns early for encountering a msi 
block,  the whole system will crash for fatal page fault immediately.

Thanks, 
Wei

Signed-off-by: Wei Wang <wei.wang2@amd.com>
-- 
AMD GmbH, Germany
Operating System Research Center

Legal Information:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34
85609 Dornach b. München

Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis München
Registergericht München, HRB Nr. 43632

[-- Attachment #2: fix_msi.patch --]
[-- Type: text/x-diff, Size: 2716 bytes --]

diff -r ef5f25de00da xen/arch/x86/msi.c
--- a/xen/arch/x86/msi.c	Mon Aug 02 17:19:06 2010 +0100
+++ b/xen/arch/x86/msi.c	Wed Aug 11 14:07:02 2010 +0200
@@ -607,30 +607,35 @@ static int msix_capability_init(struct p
  * indicates the successful setup of an entry zero with the new MSI
  * irq or non-zero for otherwise.
  **/
+
 static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
 {
     int status;
     struct pci_dev *pdev;
+    struct msi_desc *old_desc;
 
     ASSERT(spin_is_locked(&pcidevs_lock));
     pdev = pci_get_pdev(msi->bus, msi->devfn);
     if ( !pdev )
         return -ENODEV;
 
-    if ( find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI) )
+    old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI);
+    if ( old_desc )
     {
         dprintk(XENLOG_WARNING, "irq %d has already mapped to MSI on "
                 "device %02x:%02x.%01x.\n", msi->irq, msi->bus,
                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
+        *desc = old_desc;
         return 0;
     }
 
-    if ( find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX) )
+    old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
+    if ( old_desc )
     {
         dprintk(XENLOG_WARNING, "MSI-X is already in use on "
                 "device %02x:%02x.%01x\n", msi->bus,
                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
-        return 0;
+        pci_disable_msi(old_desc);
     }
 
     status = msi_capability_init(pdev, msi->irq, desc);
@@ -679,6 +684,7 @@ static int __pci_enable_msix(struct msi_
     u16 control;
     u8 slot = PCI_SLOT(msi->devfn);
     u8 func = PCI_FUNC(msi->devfn);
+    struct msi_desc *old_desc;
 
     ASSERT(spin_is_locked(&pcidevs_lock));
     pdev = pci_get_pdev(msi->bus, msi->devfn);
@@ -691,20 +697,24 @@ static int __pci_enable_msix(struct msi_
     if (msi->entry_nr >= nr_entries)
         return -EINVAL;
 
-    if ( find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSIX) )
+    old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSIX);
+    if ( old_desc )
     {
         dprintk(XENLOG_WARNING, "irq %d has already mapped to MSIX on "
                 "device %02x:%02x.%01x.\n", msi->irq, msi->bus,
                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
+        *desc = old_desc;
         return 0;
     }
 
-    if ( find_msi_entry(pdev, -1, PCI_CAP_ID_MSI) )
+    old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSI);
+    if ( old_desc )
     {
         dprintk(XENLOG_WARNING, "MSI is already in use on "
                 "device %02x:%02x.%01x\n", msi->bus,
                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
-        return 0;
+        pci_disable_msi(old_desc);
+
     }
 
     status = msix_capability_init(pdev, msi, desc);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH] msi: Avoid uninitialized msi descriptors

[Jan Beulich]

>>> On 11.08.10 at 15:43, Wei Wang2 <wei.wang2@amd.com> wrote:
> static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)
> {
>     int status;
>     struct pci_dev *pdev;
>+    struct msi_desc *old_desc;
> 
>     ASSERT(spin_is_locked(&pcidevs_lock));
>     pdev = pci_get_pdev(msi->bus, msi->devfn);
>     if ( !pdev )
>         return -ENODEV;
> 
>-    if ( find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI) )
>+    old_desc = find_msi_entry(pdev, msi->irq, PCI_CAP_ID_MSI);
>+    if ( old_desc )
>     {
>         dprintk(XENLOG_WARNING, "irq %d has already mapped to MSI on "
>                 "device %02x:%02x.%01x.\n", msi->irq, msi->bus,
>                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
>+        *desc = old_desc;

While I agree to this part, ...

>         return 0;
>     }
> 
>-    if ( find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX) )
>+    old_desc = find_msi_entry(pdev, -1, PCI_CAP_ID_MSIX);
>+    if ( old_desc )
>     {
>         dprintk(XENLOG_WARNING, "MSI-X is already in use on "
>                 "device %02x:%02x.%01x\n", msi->bus,
>                 PCI_SLOT(msi->devfn), PCI_FUNC(msi->devfn));
>-        return 0;
>+        pci_disable_msi(old_desc);

... I don't think this one's right: Admittedly I should have changed
the return value from 0 to an actual error (e.g. -EBUSY) - I simply
overlooked that after doing the copy-and-paste operation.

Whether disabling and switching to the alternative mechanism
is the right thing to do here I don't know. But I'm pretty certain
that old_desc may now be leaked, as msi_free_irq() isn't being
called on it and set_irq_msi() also doesn't check whether
irq_desc[].msi_desc is already non-NULL.

Same thing (obviously) for the second part of the changes.

Jan

RE: [PATCH] msi: Avoid uninitialized msi descriptors

[Dante Cinco]

I'm glad that somebody who is familiar with MSI has resolved this
problem although I haven't applied and verified the proposed patch yet
on my system. I posted the Xen crash I experienced related to this
issue on xen-devel a couple of weeks ago (see link below) but wasn't
familiar enough with the MSI/MSI-X code to propose a solution other
than to back out changelist 21778.

http://old.nabble.com/Re:-(XEN)-RIP:-e008:-%3Cffff82c48015564b%3E---write_msi_msg%2B0x2b-0x150-p29301832.html

Dante


-----Original Message-----
From: xen-devel-bounces@lists.xensource.com
[mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2
Sent: Wednesday, August 11, 2010 6:43 AM
To: xen-devel@lists.xensource.com
Subject: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors

Hi,
When __pci_enable_msix() returns early, output parameter (struct msi_desc
**desc) will not be initialized.  On my machine, a Broadcom BCM5709
nic has both MSI and MSIX capability blocks and when guest tries to
enable msix interrupts but __pci_enable_msix() returns early for
encountering a msi block,  the whole system will crash for fatal page
fault immediately.

Thanks,
Wei

Signed-off-by: Wei Wang <wei.wang2@amd.com>
--
AMD GmbH, Germany
Operating System Research Center

Legal Information:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34
85609 Dornach b. München

Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht
München, HRB Nr. 43632

Re: [PATCH] msi: Avoid uninitialized msi descriptors

[Keir Fraser]

On 11/08/2010 18:09, "Dante Cinco" <dantecinco@gmail.com> wrote:

> I'm glad that somebody who is familiar with MSI has resolved this
> problem although I haven't applied and verified the proposed patch yet
> on my system. I posted the Xen crash I experienced related to this
> issue on xen-devel a couple of weeks ago (see link below) but wasn't
> familiar enough with the MSI/MSI-X code to propose a solution other
> than to back out changelist 21778.

Let me know when you confirm. I backported 21778 for 4.0.1, perhaps in too
much haste. If this is a good fix then I should backport it as well.

 Thanks,
 Keir

> http://old.nabble.com/Re:-(XEN)-RIP:-e008:-%3Cffff82c48015564b%3E---write_msi_
> msg%2B0x2b-0x150-p29301832.html
> 
> Dante
> 
> 
> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2
> Sent: Wednesday, August 11, 2010 6:43 AM
> To: xen-devel@lists.xensource.com
> Subject: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors
> 
> Hi,
> When __pci_enable_msix() returns early, output parameter (struct msi_desc
> **desc) will not be initialized.  On my machine, a Broadcom BCM5709
> nic has both MSI and MSIX capability blocks and when guest tries to
> enable msix interrupts but __pci_enable_msix() returns early for
> encountering a msi block,  the whole system will crash for fatal page
> fault immediately.
> 
> Thanks,
> Wei
> 
> Signed-off-by: Wei Wang <wei.wang2@amd.com>
> --
> AMD GmbH, Germany
> Operating System Research Center
> 
> Legal Information:
> Advanced Micro Devices GmbH
> Karl-Hammerschmidt-Str. 34
> 85609 Dornach b. München
> 
> Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
> Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht
> München, HRB Nr. 43632
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] msi: Avoid uninitialized msi descriptors

[Wei Wang2]

Keir,
I would suggest to backport this patch. I saw the same issue on both Xen-4.0.1 
RC6 and xen-unstable. Actually, some MSI-X capable devices will also offer 
MSI capabilities. For such device, MSI will be enabled first by OS. When guest 
driver (like Broadcom driver) detects multiple vcpus, it will disable MSI and 
enable MSI-X block for better cpu-interrupt affinity. In this case, the whole 
system crashes.  But on UP system, only MSI block  will be enabled, it might 
still work even without the fix.
Thanks,
Wei


On Wednesday 11 August 2010 19:36:47 Keir Fraser wrote:
> On 11/08/2010 18:09, "Dante Cinco" <dantecinco@gmail.com> wrote:
> > I'm glad that somebody who is familiar with MSI has resolved this
> > problem although I haven't applied and verified the proposed patch yet
> > on my system. I posted the Xen crash I experienced related to this
> > issue on xen-devel a couple of weeks ago (see link below) but wasn't
> > familiar enough with the MSI/MSI-X code to propose a solution other
> > than to back out changelist 21778.
>
> Let me know when you confirm. I backported 21778 for 4.0.1, perhaps in too
> much haste. If this is a good fix then I should backport it as well.
>
>  Thanks,
>  Keir
>
> > http://old.nabble.com/Re:-(XEN)-RIP:-e008:-%3Cffff82c48015564b%3E---write
> >_msi_ msg%2B0x2b-0x150-p29301832.html
> >
> > Dante
> >
> >
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xensource.com
> > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Wei Wang2
> > Sent: Wednesday, August 11, 2010 6:43 AM
> > To: xen-devel@lists.xensource.com
> > Subject: [Xen-devel] [PATCH] msi: Avoid uninitialized msi descriptors
> >
> > Hi,
> > When __pci_enable_msix() returns early, output parameter (struct msi_desc
> > **desc) will not be initialized.  On my machine, a Broadcom BCM5709
> > nic has both MSI and MSIX capability blocks and when guest tries to
> > enable msix interrupts but __pci_enable_msix() returns early for
> > encountering a msi block,  the whole system will crash for fatal page
> > fault immediately.
> >
> > Thanks,
> > Wei
> >
> > Signed-off-by: Wei Wang <wei.wang2@amd.com>
> > --
> > AMD GmbH, Germany
> > Operating System Research Center
> >
> > Legal Information:
> > Advanced Micro Devices GmbH
> > Karl-Hammerschmidt-Str. 34
> > 85609 Dornach b. München
> >
> > Geschäftsführer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
> > Sitz: Dornach, Gemeinde Aschheim, Landkreis München Registergericht
> > München, HRB Nr. 43632
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xensource.com
> > http://lists.xensource.com/xen-devel