Re: [PATCH 1/4] sparse irq: protect irq_to_desc against irq_free_descs

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 25 May 2011, Milton Miller wrote:
> The radix-tree code uses call_rcu to delay freeing internal data
> elements when removing when deleting an entry.  We must protect
> against the elements being freed while we traverse the tree.
> 
> While preparing a patch to expand the contexts in which the radix
> tree optionally used by powerpc for mapping hardware irq numbers to
> linux numbers would be called, I realized that the radix tree was
> not locked when radix_tree_lookup was called.  I then realized the
> same issue applies to the generic irq code when sparse irqs are in use.
> 
> While the powerpc radix tree was only referenced from one callsite
> that was irqs_disabled and irq_enter, irq_to_desc is called from
> many more contexts including threaded irq handlers and other
> process contexts.
> 
> This does not show up in the rcu lockdep because in 2.6.34 commit
> 2676a58c98 (radix-tree: Disable RCU lockdep checking in radix tree)
> deemed it too hard to pass the condition of the protecting lock
> to the library.
> 
> Signed-off-by: Milton Miller <miltonm@bga.com>
> Cc: <stable@kernel.org>
> ---
> I expect the relatively infrequent calls to irq_free_descs, combined
> with most calls to irq_to_desc being irqs_disabled and the fact
> merged to mainline implemntations of call_rcu requiring a cpu to
> respond to a hard irq or schedule has hidden this error to date.

The reason why nobody ever noticed is that the free happens in the
teardown path of PCI devices and at this point nothing accesses that
irq anymore.
 
> Index: work.git/kernel/irq/irqdesc.c
> ===================================================================
> --- work.git.orig/kernel/irq/irqdesc.c	2011-05-23 13:34:08.728585785 -0500
> +++ work.git/kernel/irq/irqdesc.c	2011-05-23 13:46:09.197635762 -0500
> @@ -108,7 +108,13 @@ static void irq_insert_desc(unsigned int
>  
>  struct irq_desc *irq_to_desc(unsigned int irq)
>  {
> -	return radix_tree_lookup(&irq_desc_tree, irq);
> +	struct irq_desc *desc;
> +
> +	rcu_read_lock();
> +	desc = radix_tree_lookup(&irq_desc_tree, irq);
> +	rcu_read_unlock();
> +
> +	return desc

That does not really compile :)

And it does not help at all because we unconditionally free the irq
descriptor and do not use rcu based kfree. Further you protect only
the lookup and not the complete section which uses the descriptor, so
it could go away after the rcu_read_unlock() in theory.

Thanks,

	tglx