diff mbox series

rhashtable: use irq-safe spinlock in rhashtable_rehash_table()

Message ID 20190205143706.28371-1-johannes@sipsolutions.net (mailing list archive)
State Not Applicable
Delegated to: Johannes Berg
Headers show
Series rhashtable: use irq-safe spinlock in rhashtable_rehash_table() | expand

Commit Message

Johannes Berg Feb. 5, 2019, 2:37 p.m. UTC 
From: Johannes Berg <johannes.berg@intel.com>

When an rhashtabl walk is done from irq/bh context, we rightfully
get a lockdep complaint saying that we could get a (soft-)IRQ in
the middle of a rehash. This happened e.g. in mac80211 as it does
a walk in soft-irq context.

Fix this by using irq-safe locking here. We don't need _irqsave()
as we know this will be called only in process context from the
workqueue. We could get away with _bh() but that seems a bit less
generic, though I'm not sure anyone would want to do a walk from
a real IRQ handler.

Reported-by: Jouni Malinen <j@w1.fi>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 lib/rhashtable.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Johannes Berg Feb. 6, 2019, 8:15 a.m. UTC | #1 
On Tue, 2019-02-05 at 15:37 +0100, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> When an rhashtabl walk is done from irq/bh context, we rightfully
> get a lockdep complaint saying that we could get a (soft-)IRQ in
> the middle of a rehash. This happened e.g. in mac80211 as it does
> a walk in soft-irq context.
> 
> Fix this by using irq-safe locking here. We don't need _irqsave()
> as we know this will be called only in process context from the
> workqueue. We could get away with _bh() but that seems a bit less
> generic, though I'm not sure anyone would want to do a walk from
> a real IRQ handler.

Please drop this, it doesn't make sense.

I'll resend with all the spinlock usage changed to either _bh or
_irqsave(), since it makes no sense to enforce any kind of outside
BH/irq disabling for purposes of the inner lock.

johannes
David Miller Feb. 6, 2019, 5:40 p.m. UTC | #2 
From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 06 Feb 2019 09:15:15 +0100

> On Tue, 2019-02-05 at 15:37 +0100, Johannes Berg wrote:
>> From: Johannes Berg <johannes.berg@intel.com>
>> 
>> When an rhashtabl walk is done from irq/bh context, we rightfully
>> get a lockdep complaint saying that we could get a (soft-)IRQ in
>> the middle of a rehash. This happened e.g. in mac80211 as it does
>> a walk in soft-irq context.
>> 
>> Fix this by using irq-safe locking here. We don't need _irqsave()
>> as we know this will be called only in process context from the
>> workqueue. We could get away with _bh() but that seems a bit less
>> generic, though I'm not sure anyone would want to do a walk from
>> a real IRQ handler.
> 
> Please drop this, it doesn't make sense.
> 
> I'll resend with all the spinlock usage changed to either _bh or
> _irqsave(), since it makes no sense to enforce any kind of outside
> BH/irq disabling for purposes of the inner lock.

Ok.
diff mbox series

Patch

diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 852ffa5160f1..ad3c1da15475 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -327,10 +327,10 @@  static int rhashtable_rehash_table(struct rhashtable *ht)
 	/* Publish the new table pointer. */
 	rcu_assign_pointer(ht->tbl, new_tbl);
 
-	spin_lock(&ht->lock);
+	spin_lock_irq(&ht->lock);
 	list_for_each_entry(walker, &old_tbl->walkers, list)
 		walker->tbl = NULL;
-	spin_unlock(&ht->lock);
+	spin_unlock_irq(&ht->lock);
 
 	/* Wait for readers. All new readers will see the new
 	 * table, and thus no references to the old table will