lkml.org 
[lkml]   [2010]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] x86: keep chip_data in create_irq_nr and destroy_irq
    On 02/05/2010 01:09 PM, Brandon Philips wrote:
    > 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>
    > Signed-off-by: Brandon Phiilps <bphilips@suse.de>
    > Cc: stable@kernel.org
    >
    > ---
    > arch/x86/kernel/apic/io_apic.c | 14 +++--------
    > include/linux/irq.h | 2 +
    > kernel/irq/chip.c | 52 +++++++++++++++++++++++++++++++++--------
    > 3 files changed, 49 insertions(+), 19 deletions(-)
    >
    > Index: linux-2.6/arch/x86/kernel/apic/io_apic.c
    > ===================================================================
    > --- linux-2.6.orig/arch/x86/kernel/apic/io_apic.c
    > +++ linux-2.6/arch/x86/kernel/apic/io_apic.c
    > @@ -3228,12 +3228,9 @@ unsigned int create_irq_nr(unsigned int
    > }
    > 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;
    > }
    >
    > @@ -3260,10 +3257,7 @@ void destroy_irq(unsigned int irq)
    >
    > /* 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);
    > Index: linux-2.6/include/linux/irq.h
    > ===================================================================
    > --- linux-2.6.orig/include/linux/irq.h
    > +++ linux-2.6/include/linux/irq.h
    > @@ -400,7 +400,9 @@ static inline int irq_has_action(unsigne
    >
    > /* Dynamic irq helper functions */
    > extern void dynamic_irq_init(unsigned int irq);
    > +extern void dynamic_irq_init_keep_chip_data(unsigned int irq);
    > extern void dynamic_irq_cleanup(unsigned int irq);
    > +extern 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);
    > Index: linux-2.6/kernel/irq/chip.c
    > ===================================================================
    > --- linux-2.6.orig/kernel/irq/chip.c
    > +++ linux-2.6/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 ir
    > }
    > 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 ir
    > 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_init_x(irq, false);

    should be dynamic_irq_cleanup_x here.



    > +}
    > +
    > +/**
    > + * 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_init_x(irq, true);

    should be dynamic_irq_cleanup_x

    > +}
    > +
    >
    > /**
    > * set_irq_chip - set the irq chip for an irq

    YH


    \
     
     \ /
      Last update: 2010-02-05 23:49    [W:0.048 / U:30.580 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site