Re: [PATCH] x86: irq: Check for valid irq descriptor in check_irq_vectors_for_cpu_disable

[Jiang Liu <jiang.liu@linux.intel.com>]

On 2015/2/4 21:27, Joerg Roedel wrote:
> 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 <jroedel@suse.de>
> 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 <jroedel@suse.de>
Reviewed-by: Jiang Liu <jiang.liu@linux.intel.com>

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:(
Regards!
Gerry
> ---
>  arch/x86/kernel/irq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/x86/kernel/irq.c b/arch/x86/kernel/irq.c
> index 705ef8d..67b1cbe 100644
> --- a/arch/x86/kernel/irq.c
> +++ b/arch/x86/kernel/irq.c
> @@ -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);
>