Skip to contentSkip to conversation

msi: Avoid uninitialized msi descriptors

PATCH In review Wei Wang2 2 participants 2 messages Aug 11, 2010 – Aug 13, 2010

lore
Share & tools
lore

Wei Wang2

author
#0 lore raw

PATCH 1/1 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 <[email protected]>
Signature · 11 lines
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/arch/x86/msi.c +16 -6
diff -r ef5f25de00da xen/arch/x86/msi.c
@@ -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);

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

#1 lore raw
Quoted 10 lines
Quoted 1 line
Quoted 1 line · On 11.08.10 at 15:43, Wei Wang2 <[email protected]> wrote:

static int __pci_enable_msi(struct msi_info *msi, struct msi_desc **desc)

{
    int status;
    struct pci_dev *pdev;
xen/arch/x86/msi.c +1 -0
+    struct msi_desc *old_desc;
 
    ASSERT(spin_is_locked(&pcidevs_lock));
    pdev = pci_get_pdev(msi->bus, msi->devfn);
    if ( !pdev )

xen/arch/x86/msi.c static int msix_capability_init(struct p · line 628

Quoted 10 lines
xen/arch/x86/msi.c +3 -1
        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, ...

xen/arch/x86/msi.c static int msix_capability_init(struct p · line 638

Quoted 12 lines
        return 0;
    }
xen/arch/x86/msi.c +3 -2
-    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