x86: irq: Check for valid irq descriptor in check_irq_vectors_for_cpu_disable

From 153e8f6cf39c42dd9767eb49a27eacfb69738fb0 Mon Sep 17 00:00:00 2001

Hi,

here is a patch to fix a kernel panic at shutdown we have seen recently. The issue is hard to reproduce, so the patch description about the root cause of the bug comes only from review and my current understanding of x86 irq code.

So if what I wrote in the patch description doesn't make sense, please let me know.

Constructive comments and feedback welcome.

Thanks,

Joerg

From 153e8f6cf39c42dd9767eb49a27eacfb69738fb0 Mon Sep 17 00:00:00 2001
From: Joerg Roedel <[email protected]>
Date: Wed, 4 Feb 2015 13:33:33 +0100
Subject: [PATCH] x86: irq: Check for valid irq descriptor in
check_irq_vectors_for_cpu_disable

When an interrupt is migrated away from a cpu it will stay in its vector_irq array until smp_irq_move_cleanup_interrupt succeeded. The cfg->move_in_progress flag is cleared already when the IPI was sent.

When the interrupt is destroyed after migration its 'struct irq_desc' is freed and the vector_irq arrays are cleaned up. But since cfg->move_in_progress is already 0 the references at cpus before the last migration will not be cleared. So this would leave a reference to an already destroyed irq alive.

When the cpu is taken down at this point, the check_irq_vectors_for_cpu_disable function finds a valid irq number in the vector_irq array, but gets NULL for its descriptor and dereferences it, causing a kernel panic.

This has been observed on real systems at shutdown. Add a check to check_irq_vectors_for_cpu_disable for a valid 'struct irq_desc' to prevent this issue.

Signed-off-by Joerg Roedel <[email protected]>

---
 arch/x86/kernel/irq.c | 3 +++
 1 file changed, 3 insertions(+)

arch/x86/kernel/irq.c +3 -0

index 705ef8d..67b1cbe 100644
@@ -302,6 +302,9 @@ int check_irq_vectors_for_cpu_disable(void)
 		irq = __this_cpu_read(vector_irq[vector]);
 		if (irq >= 0) {
 			desc = irq_to_desc(irq);
+			if (!desc)
+				continue;
+
 			data = irq_desc_get_irq_data(desc);
 			cpumask_copy(&affinity_new, data->affinity);
 			cpu_clear(this_cpu, affinity_new);

Reviewed-by Jiang Liu <[email protected]>

Actually there's another racing pattern.

for (irq = 0; irq < nr_irqs; irq++) {
	desc = irq_to_desc(irq);
	access desc->xxx
}

When sparsing IRQ is enabled, there's no mechanism to protect desc returned by irq_to_desc(). Once I have considered a brute solution of disabling freeing of irq_desc:(