Re: [PATCH] x86: check for valid irq_cfg pointer in smp_irq_move_cleanup_interrupt

[Suresh Siddha <suresh.b.siddha@intel.com>]

On Wed, 2012-05-23 at 15:02 -0500, Dimitri Sivanich wrote:
> OK.  Hopefully this covers it.

Sorry No. Now you will understand why Thomas wanted detailed changelog.
I found one more issue with the help of your new modification to the
changelog.

> A NULL pointer dereference can occur in smp_irq_move_cleanup_interrupt() if
> we haven't yet setup the irq_cfg pointer in the irq_desc.irq_data.chip_data.
> 
> In create_irq_nr() there is a window where we have set vector_irq in
> __assign_irq_vector(), but not yet called irq_set_chip_data() to set the
> irq_cfg pointer.
> 
> Should an IRQ_MOVE_CLEANUP_VECTOR hit the cpu in question during this time,
> smp_irq_move_cleanup_interrupt() will attempt to process the aforementioned
> irq, but panic when accessing irq_cfg.
> 
> There is also a window in destroy_irq() where we've cleared the irq_cfg
> pointer in free_irq_cfg(), but have not yet called irq_free_desc().  Note
> that we have cleared vector_irq in __clear_irq_vector() prior to free_irq_cfg(),
> but smp_irq_move_cleanup_interrupt() might've already referenced the irq_desc.

So, what happens if the irq_desc gets freed by the destroy_irq() in the
sparse irq case? smp_irq_move_cleanup_interrupt() will refer to freed
irq desc memory! Right?

May we should really do something like the appended (untested patch)?
Can you please review and give this a try? Let me review a bit more to
see if this really fixes the issue.

Thanks.
---
 arch/x86/kernel/apic/io_apic.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index ffdc152..81f4cab 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2295,6 +2295,8 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 	exit_idle();
 
 	me = smp_processor_id();
+
+	raw_spin_lock(&vector_lock);
 	for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS; vector++) {
 		unsigned int irq;
 		unsigned int irr;
@@ -2310,17 +2312,16 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 			continue;
 
 		cfg = irq_cfg(irq);
-		raw_spin_lock(&desc->lock);
 
 		/*
 		 * Check if the irq migration is in progress. If so, we
 		 * haven't received the cleanup request yet for this irq.
 		 */
 		if (cfg->move_in_progress)
-			goto unlock;
+			continue;
 
 		if (vector == cfg->vector && cpumask_test_cpu(me, cfg->domain))
-			goto unlock;
+			continue;
 
 		irr = apic_read(APIC_IRR + (vector / 32 * 0x10));
 		/*
@@ -2332,12 +2333,11 @@ asmlinkage void smp_irq_move_cleanup_interrupt(void)
 		 */
 		if (irr  & (1 << (vector % 32))) {
 			apic->send_IPI_self(IRQ_MOVE_CLEANUP_VECTOR);
-			goto unlock;
+			continue;
 		}
 		__this_cpu_write(vector_irq[vector], -1);
-unlock:
-		raw_spin_unlock(&desc->lock);
 	}
+	raw_spin_unlock(&vector_lock);
 
 	irq_exit();
 }
@@ -2986,17 +2986,16 @@ unsigned int create_irq_nr(unsigned int from, int node)
 		return 0;
 	}
 
+	irq_set_chip_data(irq, cfg);
 	raw_spin_lock_irqsave(&vector_lock, flags);
 	if (!__assign_irq_vector(irq, cfg, apic->target_cpus()))
 		ret = irq;
 	raw_spin_unlock_irqrestore(&vector_lock, flags);
 
-	if (ret) {
-		irq_set_chip_data(irq, cfg);
+	if (ret)
 		irq_clear_status_flags(irq, IRQ_NOREQUEST);
-	} else {
+	else
 		free_irq_at(irq, cfg);
-	}
 	return ret;
 }