Skip to content

Commit

Permalink
powerpc/irq: Protect irq_radix_revmap_lookup against irq_free_virt
Browse files Browse the repository at this point in the history
The radix-tree code uses call_rcu when freeing internal elements.
We must protect against the elements being freed while we traverse
the tree, even if the returned pointer will still be valid.

While preparing a patch to expand the context in which
irq_radix_revmap_lookup will be called, I realized that the
radix tree was not locked.

When asked

    For a normal call_rcu usage, is it allowed to read the structure in
    irq_enter / irq_exit, without additional rcu_read_lock?  Could an
    element freed with call_rcu advance with the cpu still between
    irq_enter/irq_exit (and irq_disabled())?

Paul McKenney replied:

    Absolutely illegal to do so. OK for call_rcu_sched(), but a
    flaming bug for call_rcu().

    And thank you very much for finding this!!!

Further analysis:

In the current CONFIG_TREE_RCU implementation. CONFIG_TREE_PREEMPT_RCU
(and CONFIG_TINY_PREEMPT_RCU) uses explicit counters.

These counters are reflected from per-CPU to global in the
scheduling-clock-interrupt handler, so disabling irq does prevent the
grace period from completing. But there are real-time implementations
(such as the one use by the Concurrent guys) where disabling irq
does -not- prevent the grace period from completing.

While an alternative fix would be to switch radix-tree to rcu_sched, I
don't want to audit the other users of radix trees (nor put alternative
freeing in the library).  The normal overhead for rcu_read_lock and
unlock are a local counter increment and decrement.

This does not show up in the rcu lockdep because in 2.6.34 commit
2676a58 (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>
Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
  • Loading branch information
Milton Miller authored and Benjamin Herrenschmidt committed May 26, 2011
1 parent 2e45525 commit 9b78825
Showing 1 changed file with 5 additions and 2 deletions.
7 changes: 5 additions & 2 deletions arch/powerpc/kernel/irq.c
Original file line number Diff line number Diff line change
Expand Up @@ -893,10 +893,13 @@ unsigned int irq_radix_revmap_lookup(struct irq_host *host,
return irq_find_mapping(host, hwirq);

/*
* No rcu_read_lock(ing) needed, the ptr returned can't go under us
* as it's referencing an entry in the static irq_map table.
* The ptr returned references the static global irq_map.
* but freeing an irq can delete nodes along the path to
* do the lookup via call_rcu.
*/
rcu_read_lock();
ptr = radix_tree_lookup(&host->revmap_data.tree, hwirq);
rcu_read_unlock();

/*
* If found in radix tree, then fine.
Expand Down

0 comments on commit 9b78825

Please sign in to comment.