From 49ec6fa22028054f292c9c290415b88281f7b783 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Thu, 3 Apr 2008 14:31:05 +0200 Subject: [PATCH] mac80211: fix possible sta-debugfs work lockup Because we queue the sta-debugfs-adding work on our mac80211 workqueue (which needs to be flushed under RTNL) and that work needs the RTNL, it can currently deadlock, thanks to Reinette Chatre for pointing out the lockdep warning about this. This patch fixes it by moving this work to the common kernel workqueue (using schedule_work) and canceling it as appropriate. It also fixes a related problem: When a STA is pinned by the debugfs adding work and sta_info_flush() runs concurrently it is not guaranteed that all STAs are removed from the driver before the corresponding interface is removed which may lead to bugs. Signed-off-by: Johannes Berg Cc: Reinette Chatre Signed-off-by: John W. Linville --- net/mac80211/sta_info.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 7e1e872576..bfdaf5c82f 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -351,7 +351,7 @@ int sta_info_insert(struct sta_info *sta) * NOTE: due to auto-freeing semantics this may only be done * if the insertion is successful! */ - queue_work(local->hw.workqueue, &local->sta_debugfs_add); + schedule_work(&local->sta_debugfs_add); #endif if (ieee80211_vif_is_mesh(&sdata->vif)) @@ -476,16 +476,23 @@ void __sta_info_unlink(struct sta_info **sta) * * The rules are not trivial, but not too complex either: * (1) pin_status is only modified under the sta_lock - * (2) sta_info_debugfs_add_work() will set the status + * (2) STAs may only be pinned under the RTNL so that + * sta_info_flush() is guaranteed to actually destroy + * all STAs that are active for a given interface, this + * is required for correctness because otherwise we + * could notify a driver that an interface is going + * away and only after that (!) notify it about a STA + * on that interface going away. + * (3) sta_info_debugfs_add_work() will set the status * to PINNED when it found an item that needs a new * debugfs directory created. In that case, that item * must not be freed although all *RCU* users are done * with it. Hence, we tell the caller of _unlink() * that the item is already gone (as can happen when * two tasks try to unlink/destroy at the same time) - * (3) We set the pin_status to DESTROY here when we + * (4) We set the pin_status to DESTROY here when we * find such an item. - * (4) sta_info_debugfs_add_work() will reset the pin_status + * (5) sta_info_debugfs_add_work() will reset the pin_status * from PINNED to NORMAL when it is done with the item, * but will check for DESTROY before resetting it in * which case it will free the item. @@ -617,6 +624,8 @@ static void sta_info_debugfs_add_work(struct work_struct *work) struct sta_info *sta, *tmp; unsigned long flags; + /* We need to keep the RTNL across the whole pinned status. */ + rtnl_lock(); while (1) { sta = NULL; @@ -637,10 +646,9 @@ static void sta_info_debugfs_add_work(struct work_struct *work) rate_control_add_sta_debugfs(sta); sta = __sta_info_unpin(sta); - rtnl_lock(); sta_info_destroy(sta); - rtnl_unlock(); } + rtnl_unlock(); } #endif @@ -700,6 +708,15 @@ void sta_info_stop(struct ieee80211_local *local) { del_timer(&local->sta_cleanup); cancel_work_sync(&local->sta_flush_work); +#ifdef CONFIG_MAC80211_DEBUGFS + /* + * Make sure the debugfs adding work isn't pending after this + * because we're about to be destroyed. It doesn't matter + * whether it ran or not since we're going to flush all STAs + * anyway. + */ + cancel_work_sync(&local->sta_debugfs_add); +#endif rtnl_lock(); sta_info_flush(local, NULL); -- 2.39.5