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

[Dimitri Sivanich <sivanich@sgi.com>]

On Wed, May 23, 2012 at 04:49:29PM -0700, Suresh Siddha wrote:
> 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?


And speaking of possible holes in destroy_irq()..

What happens if we're running __assign_irq_vector() (say we're changing irq
affinity), and on another cpu we had just run through __clear_irq_vector()
via destroy_irq().  Now destroy_irq() is going to call
free_irq_at()->free_irq_cfg, which will clear irq_cfg.  Then
__assign_irq_vector goes to access irq_cfg (cfg->vector or
cfg->move_in_progress, for instance), which was already freed.

I'm not sure if this can happen, but just eyeballing it, it does look that
that way.

> 
> 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;
>  }
>  
>