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

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 23 May 2012, 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?
>
> 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.

It's fixing the problem.

But this move_cleanup stuff could be made less stupid.

The check for irq_desc is superflous. irq_cfg() calls
irq_get_chip_data() which will return NULL if the irq descriptor is
not there.

To avoid the lookup business completely we should really store
irq_desc instead of the irq number in the per cpu vector array, that
would also get rid of the lookup in the irq delivery path.

Now that still needs to iterate over all vectors, but this could be
optimized in a second step.

In complete_move() we send the IPI to all cpus in the old mask. We
really should set the corresponding vector bit in a per cpu bitfield
on those cpus in the mask. The cleanup can rely on the bits and avoid
looking at 200+ vectors to find a single one. 

Thoughts?

	tglx