x86, irq: Keep chip_data in create_irq_nr and destroy_irq

Version 4: use get_irq_chip_data() in destroy_irq() to get rid of some
local vars.

When two drivers are setting up MSI-X at the same time via
pci_enable_msix() there is a race.  See this dmesg excerpt:

[   85.170610] ixgbe 0000:02:00.1: irq 97 for MSI/MSI-X
[   85.170611]   alloc irq_desc for 99 on node -1
[   85.170613] igb 0000:08:00.1: irq 98 for MSI/MSI-X
[   85.170614]   alloc kstat_irqs on node -1
[   85.170616] alloc irq_2_iommu on node -1
[   85.170617]   alloc irq_desc for 100 on node -1
[   85.170619]   alloc kstat_irqs on node -1
[   85.170621] alloc irq_2_iommu on node -1
[   85.170625] ixgbe 0000:02:00.1: irq 99 for MSI/MSI-X
[   85.170626]   alloc irq_desc for 101 on node -1
[   85.170628] igb 0000:08:00.1: irq 100 for MSI/MSI-X
[   85.170630]   alloc kstat_irqs on node -1
[   85.170631] alloc irq_2_iommu on node -1
[   85.170635]   alloc irq_desc for 102 on node -1
[   85.170636]   alloc kstat_irqs on node -1
[   85.170639] alloc irq_2_iommu on node -1
[   85.170646] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000088

As you can see igb and ixgbe are both alternating on create_irq_nr()
via pci_enable_msix() in their probe function.

ixgbe: While looping through irq_desc_ptrs[] via create_irq_nr() ixgbe
choses irq_desc_ptrs[102] and exits the loop, drops vector_lock and
calls dynamic_irq_init. Then it sets irq_desc_ptrs[102]->chip_data =
NULL via dynamic_irq_init().

igb: Grabs the vector_lock now and starts looping over irq_desc_ptrs[]
via create_irq_nr(). It gets to irq_desc_ptrs[102] and does this:

	cfg_new = irq_desc_ptrs[102]->chip_data;
	if (cfg_new->vector != 0)
		continue;

This hits the NULL deref.

Another possible race exists via pci_disable_msix() in a driver or in
the number of error paths that call free_msi_irqs():

destroy_irq()
dynamic_irq_cleanup() which sets desc->chip_data = NULL
...race window...
desc->chip_data = cfg;

Remove the save and restore code for cfg in create_irq_nr() and
destroy_irq() and take the desc->lock when checking the irq_cfg.

Reported-and-analyzed-by: Brandon Philips <bphilips@suse.de>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>
LKML-Reference: <20100207210250.GB8256@jenkins.home.ifup.org>
Signed-off-by: Brandon Phiilps <bphilips@suse.de>
Cc: stable@kernel.org
Signed-off-by: H. Peter Anvin <hpa@zytor.com>

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 5e4cce254e438e479d5002319cfda3a804e46c93..e93a76bc8670afa86628cbf0f177f404744fa592 100644 (file)
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3278,12 +3278,9 @@ unsigned int create_irq_nr(unsigned int irq_want, int node)
        }
        spin_unlock_irqrestore(&vector_lock, flags);
 
-       if (irq > 0) {
-               dynamic_irq_init(irq);
-               /* restore it, in case dynamic_irq_init clear it */
-               if (desc_new)
-                       desc_new->chip_data = cfg_new;
-       }
+       if (irq > 0)
+               dynamic_irq_init_keep_chip_data(irq);
+
        return irq;
 }
 
@@ -3305,19 +3302,12 @@ int create_irq(void)
 void destroy_irq(unsigned int irq)
 {
        unsigned long flags;
-       struct irq_cfg *cfg;
-       struct irq_desc *desc;
 
-       /* store it, in case dynamic_irq_cleanup clear it */
-       desc = irq_to_desc(irq);
-       cfg = desc->chip_data;
-       dynamic_irq_cleanup(irq);
-       /* connect back irq_cfg */
-       desc->chip_data = cfg;
+       dynamic_irq_cleanup_keep_chip_data(irq);
 
        free_irte(irq);
        spin_lock_irqsave(&vector_lock, flags);
-       __clear_irq_vector(irq, cfg);
+       __clear_irq_vector(irq, get_irq_chip_data(irq));
        spin_unlock_irqrestore(&vector_lock, flags);
 }
 

diff --git a/include/linux/irq.h b/include/linux/irq.h
index 451481c082b55c3f571e4dadc1d2b7b386bdfa9a..4d9b26e044bcb7ee02a3f2e4b4aed70e9c88d7c0 100644 (file)
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -400,7 +400,9 @@ static inline int irq_has_action(unsigned int irq)
 
 /* Dynamic irq helper functions */
 extern void dynamic_irq_init(unsigned int irq);
+void dynamic_irq_init_keep_chip_data(unsigned int irq);
 extern void dynamic_irq_cleanup(unsigned int irq);
+void dynamic_irq_cleanup_keep_chip_data(unsigned int irq);
 
 /* Set/get chip/data for an IRQ: */
 extern int set_irq_chip(unsigned int irq, struct irq_chip *chip);

diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index ecc3fa28f66640ddc662e40340c634323f2196e9..d70394f12ee914e313057e2e560f96088a136af0 100644 (file)
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -18,11 +18,7 @@
 
 #include "internals.h"
 
-/**
- *     dynamic_irq_init - initialize a dynamically allocated irq
- *     @irq:   irq number to initialize
- */
-void dynamic_irq_init(unsigned int irq)
+static void dynamic_irq_init_x(unsigned int irq, bool keep_chip_data)
 {
        struct irq_desc *desc;
        unsigned long flags;
@@ -41,7 +37,8 @@ void dynamic_irq_init(unsigned int irq)
        desc->depth = 1;
        desc->msi_desc = NULL;
        desc->handler_data = NULL;
-       desc->chip_data = NULL;
+       if (!keep_chip_data)
+               desc->chip_data = NULL;
        desc->action = NULL;
        desc->irq_count = 0;
        desc->irqs_unhandled = 0;
@@ -55,10 +52,26 @@ void dynamic_irq_init(unsigned int irq)
 }
 
 /**
- *     dynamic_irq_cleanup - cleanup a dynamically allocated irq
+ *     dynamic_irq_init - initialize a dynamically allocated irq
  *     @irq:   irq number to initialize
  */
-void dynamic_irq_cleanup(unsigned int irq)
+void dynamic_irq_init(unsigned int irq)
+{
+       dynamic_irq_init_x(irq, false);
+}
+
+/**
+ *     dynamic_irq_init_keep_chip_data - initialize a dynamically allocated irq
+ *     @irq:   irq number to initialize
+ *
+ *     does not set irq_to_desc(irq)->chip_data to NULL
+ */
+void dynamic_irq_init_keep_chip_data(unsigned int irq)
+{
+       dynamic_irq_init_x(irq, true);
+}
+
+static void dynamic_irq_cleanup_x(unsigned int irq, bool keep_chip_data)
 {
        struct irq_desc *desc = irq_to_desc(irq);
        unsigned long flags;
@@ -77,7 +90,8 @@ void dynamic_irq_cleanup(unsigned int irq)
        }
        desc->msi_desc = NULL;
        desc->handler_data = NULL;
-       desc->chip_data = NULL;
+       if (!keep_chip_data)
+               desc->chip_data = NULL;
        desc->handle_irq = handle_bad_irq;
        desc->chip = &no_irq_chip;
        desc->name = NULL;
@@ -85,6 +99,26 @@ void dynamic_irq_cleanup(unsigned int irq)
        raw_spin_unlock_irqrestore(&desc->lock, flags);
 }
 
+/**
+ *     dynamic_irq_cleanup - cleanup a dynamically allocated irq
+ *     @irq:   irq number to initialize
+ */
+void dynamic_irq_cleanup(unsigned int irq)
+{
+       dynamic_irq_cleanup_x(irq, false);
+}
+
+/**
+ *     dynamic_irq_cleanup_keep_chip_data - cleanup a dynamically allocated irq
+ *     @irq:   irq number to initialize
+ *
+ *     does not set irq_to_desc(irq)->chip_data to NULL
+ */
+void dynamic_irq_cleanup_keep_chip_data(unsigned int irq)
+{
+       dynamic_irq_cleanup_x(irq, true);
+}
+
 
 /**
  *     set_irq_chip - set the irq chip for an irq