[PATCH] x86, irq: Check if irq is remapped before freeing irte

[PATCH] x86, irq: Check if irq is remapped before freeing irte

[Yinghai Lu]

On one system that support intr-rempping when boot with "intremap=off"

got:
[  177.824202] calling  alsa_card_azx_init+0x0/0x1b @ 1
[  177.843968] HDA Intel 0000:00:1b.0: PCI INT A -> GSI 22 (level, low) -> IRQ 22
[  177.848210] HDA Intel 0000:00:1b.0: irq 1435 for MSI/MSI-X
[  177.863797] HDA Intel 0000:00:1b.0: setting latency timer to 64
[  177.895084] hda-intel: no codecs found!
[  177.895501] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8
[  177.913316] IP: [<ffffffff8145fc18>] free_irte+0x47/0xc0
[  177.913859] PGD 0 
[  177.914037] Oops: 0000 [#1] SMP 
[  177.933240] last sysfs file: 
[  177.933501] CPU 0 
[  177.933655] Modules linked in:
[  177.933937] 
[  177.934078] Pid: 15044, comm: work_for_cpu Not tainted 2.6.36-rc8-tip-yh-01994-g95100d0-dirty #198      /Sun Fire X4800
[  177.953986] RIP: 0010:[<ffffffff8145fc18>]  [<ffffffff8145fc18>] free_irte+0x47/0xc0
...
[  178.173326] Call Trace:
[  178.173574]  [<ffffffff810515b4>] destroy_irq+0x3a/0x75
[  178.192934]  [<ffffffff81051834>] arch_teardown_msi_irq+0xe/0x10
[  178.193418]  [<ffffffff81458dc3>] arch_teardown_msi_irqs+0x56/0x7f
[  178.213021]  [<ffffffff81458e79>] free_msi_irqs+0x8d/0xeb
[  178.213490]  [<ffffffff81459673>] pci_disable_msi+0x35/0x3a
[  178.232956]  [<ffffffff81b68917>] azx_free+0x83/0x11c
[  178.233301]  [<ffffffff81cb1ec7>] azx_probe+0x7b1/0xab4
[  178.252885]  [<ffffffff810a59ef>] ? trace_hardirqs_on+0xd/0xf
[  178.253303]  [<ffffffff81442a50>] local_pci_probe+0x4d/0x96
[  178.272801]  [<ffffffff8108e72c>] ? do_work_for_cpu+0x0/0x2b
[  178.273270]  [<ffffffff8108e744>] do_work_for_cpu+0x18/0x2b
[  178.292785]  [<ffffffff8108e72c>] ? do_work_for_cpu+0x0/0x2b
[  178.293220]  [<ffffffff81094135>] kthread+0x9d/0xa5
[  178.312742]  [<ffffffff81034954>] kernel_thread_helper+0x4/0x10
[  178.313222]  [<ffffffff81cc96fc>] ? restore_args+0x0/0x30
[  178.332746]  [<ffffffff81094098>] ? kthread+0x0/0xa5
[  178.333207]  [<ffffffff81034950>] ? kernel_thread_helper+0x0/0x10

Root cause is that irq_2_iommu is embedded into irq_cfg now...

Need to check if that irq is really mapped, before free irte.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/intr_remapping.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/pci/intr_remapping.c
===================================================================
--- linux-2.6.orig/drivers/pci/intr_remapping.c
+++ linux-2.6/drivers/pci/intr_remapping.c
@@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry
 	unsigned long flags;
 	int index;
 
-	if (!entry || !irq_iommu)
+	if (!entry || !irq_iommu || !irq_iommu->iommu)
 		return -1;
 
 	spin_lock_irqsave(&irq_2_ir_lock, flags);
@@ -268,7 +268,7 @@ int free_irte(int irq)
 	unsigned long flags;
 	int rc;
 
-	if (!irq_iommu)
+	if (!irq_iommu || !irq_iommu->iommu)
 		return -1;
 
 	spin_lock_irqsave(&irq_2_ir_lock, flags);

Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte

[Thomas Gleixner]

On Mon, 18 Oct 2010, Yinghai Lu wrote:
> 
> Index: linux-2.6/drivers/pci/intr_remapping.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/intr_remapping.c
> +++ linux-2.6/drivers/pci/intr_remapping.c
> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry
>  	unsigned long flags;
>  	int index;
>  
> -	if (!entry || !irq_iommu)
> +	if (!entry || !irq_iommu || !irq_iommu->iommu)
>  		return -1;

Hmm, why do we need this? This is only called from
ir_ioapic_set_affinity() and ir_msi_set_affinity().

We should never end up there when intr_remapping=off, right ?
  
>  	spin_lock_irqsave(&irq_2_ir_lock, flags);
> @@ -268,7 +268,7 @@ int free_irte(int irq)
>  	unsigned long flags;
>  	int rc;
>  
> -	if (!irq_iommu)
> +	if (!irq_iommu || !irq_iommu->iommu)
>  		return -1;

That one makes sense

Thanks,

	tglx

Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte

[Thomas Gleixner]

On Mon, 18 Oct 2010, Thomas Gleixner wrote:

> On Mon, 18 Oct 2010, Yinghai Lu wrote:
> > 
> > Index: linux-2.6/drivers/pci/intr_remapping.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/intr_remapping.c
> > +++ linux-2.6/drivers/pci/intr_remapping.c
> > @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry
> >  	unsigned long flags;
> >  	int index;
> >  
> > -	if (!entry || !irq_iommu)
> > +	if (!entry || !irq_iommu || !irq_iommu->iommu)
> >  		return -1;
> 
> Hmm, why do we need this? This is only called from
> ir_ioapic_set_affinity() and ir_msi_set_affinity().
> 
> We should never end up there when intr_remapping=off, right ?

Thinking more about it, this check is actively bogus. The call sites do:

        struct irte irte;

        if (get_irte(irq, &irte))
                return -1;

So entry _CANNOT_ be NULL.

And in fact we should change get_irte() to

get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry)

The call site already knows about it. No need to lookup irq_iommu
based on the irq number.

Thanks,

	tglx

Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte

[Yinghai]

On Oct 18, 2010, at 2:17 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> 
> 
> On Mon, 18 Oct 2010, Thomas Gleixner wrote:
> 
>> On Mon, 18 Oct 2010, Yinghai Lu wrote:
>>> 
>>> Index: linux-2.6/drivers/pci/intr_remapping.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/pci/intr_remapping.c
>>> +++ linux-2.6/drivers/pci/intr_remapping.c
>>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry
>>>    unsigned long flags;
>>>    int index;
>>> 
>>> -    if (!entry || !irq_iommu)
>>> +    if (!entry || !irq_iommu || !irq_iommu->iommu)
>>>        return -1;
>> 
>> Hmm, why do we need this? This is only called from
>> ir_ioapic_set_affinity() and ir_msi_set_affinity().
>> 
>> We should never end up there when intr_remapping=off, right ?
> 
> Thinking more about it, this check is actively bogus. The call sites do:
> 
>        struct irte irte;
> 
>        if (get_irte(irq, &irte))
>                return -1;
> 
> So entry _CANNOT_ be NULL.
> 
> And in fact we should change get_irte() to
> 
> get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry)
> 
> The call site already knows about it. No need to lookup irq_iommu
> based on the irq number.

Yes

Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte

[Yinghai Lu]

On 10/18/2010 02:17 PM, Thomas Gleixner wrote:
> 
> 
> On Mon, 18 Oct 2010, Thomas Gleixner wrote:
> 
>> On Mon, 18 Oct 2010, Yinghai Lu wrote:
>>>
>>> Index: linux-2.6/drivers/pci/intr_remapping.c
>>> ===================================================================
>>> --- linux-2.6.orig/drivers/pci/intr_remapping.c
>>> +++ linux-2.6/drivers/pci/intr_remapping.c
>>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry
>>>  	unsigned long flags;
>>>  	int index;
>>>  
>>> -	if (!entry || !irq_iommu)
>>> +	if (!entry || !irq_iommu || !irq_iommu->iommu)
>>>  		return -1;
>>
>> Hmm, why do we need this? This is only called from
>> ir_ioapic_set_affinity() and ir_msi_set_affinity().
>>
>> We should never end up there when intr_remapping=off, right ?
> 
> Thinking more about it, this check is actively bogus. The call sites do:
> 
>         struct irte irte;
> 
>         if (get_irte(irq, &irte))
>                 return -1;
> 
> So entry _CANNOT_ be NULL.
> 
> And in fact we should change get_irte() to
> 
> get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry)
> 
> The call site already knows about it. No need to lookup irq_iommu
> based on the irq number.

looks like all irq-irte related API could replace "int irq" to "struct irq_2_iommu *irq_iommu"

extern int get_irte(int irq, struct irte *entry);
extern int modify_irte(int irq, struct irte *irte_modified);
extern int alloc_irte(struct intel_iommu *iommu, int irq, u16 count);
extern int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index,
                        u16 sub_handle);
extern int map_irq_to_irte_handle(int irq, u16 *sub_handle);
extern int free_irte(int irq);

Thanks

Yinghai

Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte

[Thomas Gleixner]

On Mon, 18 Oct 2010, Yinghai Lu wrote:

> On 10/18/2010 02:17 PM, Thomas Gleixner wrote:
> > 
> > 
> > On Mon, 18 Oct 2010, Thomas Gleixner wrote:
> > 
> >> On Mon, 18 Oct 2010, Yinghai Lu wrote:
> >>>
> >>> Index: linux-2.6/drivers/pci/intr_remapping.c
> >>> ===================================================================
> >>> --- linux-2.6.orig/drivers/pci/intr_remapping.c
> >>> +++ linux-2.6/drivers/pci/intr_remapping.c
> >>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry
> >>>  	unsigned long flags;
> >>>  	int index;
> >>>  
> >>> -	if (!entry || !irq_iommu)
> >>> +	if (!entry || !irq_iommu || !irq_iommu->iommu)
> >>>  		return -1;
> >>
> >> Hmm, why do we need this? This is only called from
> >> ir_ioapic_set_affinity() and ir_msi_set_affinity().

That does not answer that question !

> >> We should never end up there when intr_remapping=off, right ?
> > 
> > Thinking more about it, this check is actively bogus. The call sites do:
> > 
> >         struct irte irte;
> > 
> >         if (get_irte(irq, &irte))
> >                 return -1;
> > 
> > So entry _CANNOT_ be NULL.
> > 
> > And in fact we should change get_irte() to
> > 
> > get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry)
> > 
> > The call site already knows about it. No need to lookup irq_iommu
> > based on the irq number.
> 
> looks like all irq-irte related API could replace "int irq" to "struct irq_2_iommu *irq_iommu"
> 
> extern int get_irte(int irq, struct irte *entry);
> extern int modify_irte(int irq, struct irte *irte_modified);
> extern int alloc_irte(struct intel_iommu *iommu, int irq, u16 count);
> extern int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index,
>                         u16 sub_handle);
> extern int map_irq_to_irte_handle(int irq, u16 *sub_handle);
> extern int free_irte(int irq);

Probably, but we need to figure out which functions need which checks
instead of having either redundant or superflous ones there.

Thanks,

	tglx

Re: [PATCH] x86, irq: Check if irq is remapped before freeing irte

[Yinghai Lu]

On 10/18/2010 03:31 PM, Thomas Gleixner wrote:
> On Mon, 18 Oct 2010, Yinghai Lu wrote:
> 
>> On 10/18/2010 02:17 PM, Thomas Gleixner wrote:
>>>
>>>
>>> On Mon, 18 Oct 2010, Thomas Gleixner wrote:
>>>
>>>> On Mon, 18 Oct 2010, Yinghai Lu wrote:
>>>>>
>>>>> Index: linux-2.6/drivers/pci/intr_remapping.c
>>>>> ===================================================================
>>>>> --- linux-2.6.orig/drivers/pci/intr_remapping.c
>>>>> +++ linux-2.6/drivers/pci/intr_remapping.c
>>>>> @@ -60,7 +60,7 @@ int get_irte(int irq, struct irte *entry
>>>>>  	unsigned long flags;
>>>>>  	int index;
>>>>>  
>>>>> -	if (!entry || !irq_iommu)
>>>>> +	if (!entry || !irq_iommu || !irq_iommu->iommu)
>>>>>  		return -1;
>>>>
>>>> Hmm, why do we need this? This is only called from
>>>> ir_ioapic_set_affinity() and ir_msi_set_affinity().
> 
> That does not answer that question !
we don't need that checking there.
> 
>>>> We should never end up there when intr_remapping=off, right ?
>>>
>>> Thinking more about it, this check is actively bogus. The call sites do:
>>>
>>>         struct irte irte;
>>>
>>>         if (get_irte(irq, &irte))
>>>                 return -1;
>>>
>>> So entry _CANNOT_ be NULL.
>>>
>>> And in fact we should change get_irte() to
>>>
>>> get_irte(struct irq_2_iommu *irq_iommu, struct irte *entry)
>>>
>>> The call site already knows about it. No need to lookup irq_iommu
>>> based on the irq number.
>>
>> looks like all irq-irte related API could replace "int irq" to "struct irq_2_iommu *irq_iommu"
>>
>> extern int get_irte(int irq, struct irte *entry);
>> extern int modify_irte(int irq, struct irte *irte_modified);
>> extern int alloc_irte(struct intel_iommu *iommu, int irq, u16 count);
>> extern int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index,
>>                         u16 sub_handle);
>> extern int map_irq_to_irte_handle(int irq, u16 *sub_handle);
>> extern int free_irte(int irq);
> 
> Probably, but we need to figure out which functions need which checks
> instead of having either redundant or superflous ones there.
> 
only free_irte().

because other have have protection from intr_remapping_enabled or irq_remapped(get_irq_chip_data(irq)) or with ir related chip.

this one works too.

---
 arch/x86/kernel/apic/io_apic.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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
@@ -3109,7 +3109,8 @@ void destroy_irq(unsigned int irq)
 
 	irq_set_status_flags(irq, IRQ_NOREQUEST|IRQ_NOPROBE);
 
-	free_irte(irq);
+	if (intr_remapping_enabled)
+		free_irte(irq);
 	raw_spin_lock_irqsave(&vector_lock, flags);
 	__clear_irq_vector(irq, cfg);
 	raw_spin_unlock_irqrestore(&vector_lock, flags);

[tip:irq/core] x86: ioapic: Call free_irte only if interrupt remapping enabled

[tip-bot for Yinghai Lu]

Commit-ID:  9717967c4b704ce344c954afb5bb160aa9c01c34
Gitweb:     http://git.kernel.org/tip/9717967c4b704ce344c954afb5bb160aa9c01c34
Author:     Yinghai Lu <yinghai@kernel.org>
AuthorDate: Mon, 18 Oct 2010 13:47:48 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 19 Oct 2010 09:25:33 +0200

x86: ioapic: Call free_irte only if interrupt remapping enabled

On a system that support intr-rempping when booting with "intremap=off"

[  177.895501] BUG: unable to handle kernel NULL pointer dereference at 00000000000000f8
[  177.913316] IP: [<ffffffff8145fc18>] free_irte+0x47/0xc0
...
[  178.173326] Call Trace:
[  178.173574]  [<ffffffff810515b4>] destroy_irq+0x3a/0x75
[  178.192934]  [<ffffffff81051834>] arch_teardown_msi_irq+0xe/0x10
[  178.193418]  [<ffffffff81458dc3>] arch_teardown_msi_irqs+0x56/0x7f
[  178.213021]  [<ffffffff81458e79>] free_msi_irqs+0x8d/0xeb

Call free_irte only when interrupt remapping is enabled.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
LKML-Reference: <4CBCB274.7010108@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/x86/kernel/apic/io_apic.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 20e47e0..8ae808d 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3109,7 +3109,8 @@ void destroy_irq(unsigned int irq)
 
 	irq_set_status_flags(irq, IRQ_NOREQUEST|IRQ_NOPROBE);
 
-	free_irte(irq);
+	if (intr_remapping_enabled)
+		free_irte(irq);
 	raw_spin_lock_irqsave(&vector_lock, flags);
 	__clear_irq_vector(irq, cfg);
 	raw_spin_unlock_irqrestore(&vector_lock, flags);