On Thu, May 24, 2012 at 04:53:06PM +0200, Thomas Gleixner wrote: > 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. So if the irq_desc gets deallocated you would clear all corresponding per cpu vector_irq references before deallocation, protecting accesses by smp_irq_move_cleanup_interrupt? > > 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. This part does sound more efficient at first glance. > > Thoughts? > > tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/