cfg80211: fix management registrations deadlock

Lockdep reports that we may deadlock because we take the RTNL on
the work struct, but flush it under RTNL. Clearly, it's correct.
In practice, this can happen when doing rfkill on an active device.

Fix this by moving the work struct to the wiphy (registered dev)
layer, and iterate over all the wdevs inside there. This then
means we need to track which one of them has work to do, so we
don't update to the driver for all wdevs all the time.

Also fix a locking bug I noticed while working on this - the
registrations list is iterated as if it was an RCU list, but it
isn't handle that way - and we need to lock now for the update
flag anyway, so remove the RCU.

Fixes: 6cd536fe62ef ("cfg80211: change internal management frame registration API")
Reported-by: Markus Theil <markus.theil@tu-ilmenau.de>
Reported-and-tested-by: Kenneth R. Crudup <kenny@panix.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
Link: https://lore.kernel.org/r/20200604120420.b1dc540a7e26.I55dcca56bb5bdc5d7ad66a36a0b42afd7034d8be@changeid
Signed-off-by: Johannes Berg <johannes.berg@intel.com>

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index b58ad1a3f6956eb02fbd15673efd806d19a9420b..fc7e8807838d9d24cdc6141a4e0c0be83eec135f 100644 (file)
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -5075,7 +5075,8 @@ struct cfg80211_cqm_config;
  *     by cfg80211 on change_interface
  * @mgmt_registrations: list of registrations for management frames
  * @mgmt_registrations_lock: lock for the list
- * @mgmt_registrations_update_wk: update work to defer from atomic context
+ * @mgmt_registrations_need_update: mgmt registrations were updated,
+ *     need to propagate the update to the driver
  * @mtx: mutex used to lock data in this struct, may be used by drivers
  *     and some API functions require it held
  * @beacon_interval: beacon interval used on this device for transmitting
@@ -5121,7 +5122,7 @@ struct wireless_dev {
 
        struct list_head mgmt_registrations;
        spinlock_t mgmt_registrations_lock;
-       struct work_struct mgmt_registrations_update_wk;
+       u8 mgmt_registrations_need_update:1;
 
        struct mutex mtx;
 

diff --git a/net/wireless/core.c b/net/wireless/core.c
index f0226ae9561caa68b5d53fee432a79f021273fcb..c623d9bf5096bbc796dba1dbe5953e639fe9a546 100644 (file)
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -497,6 +497,8 @@ use_default_name:
        INIT_WORK(&rdev->propagate_radar_detect_wk,
                  cfg80211_propagate_radar_detect_wk);
        INIT_WORK(&rdev->propagate_cac_done_wk, cfg80211_propagate_cac_done_wk);
+       INIT_WORK(&rdev->mgmt_registrations_update_wk,
+                 cfg80211_mgmt_registrations_update_wk);
 
 #ifdef CONFIG_CFG80211_DEFAULT_PS
        rdev->wiphy.flags |= WIPHY_FLAG_PS_ON_BY_DEFAULT;
@@ -1047,6 +1049,7 @@ void wiphy_unregister(struct wiphy *wiphy)
        flush_work(&rdev->sched_scan_stop_wk);
        flush_work(&rdev->propagate_radar_detect_wk);
        flush_work(&rdev->propagate_cac_done_wk);
+       flush_work(&rdev->mgmt_registrations_update_wk);
 
 #ifdef CONFIG_PM
        if (rdev->wiphy.wowlan_config && rdev->ops->set_wakeup)
@@ -1108,7 +1111,6 @@ static void __cfg80211_unregister_wdev(struct wireless_dev *wdev, bool sync)
        rdev->devlist_generation++;
 
        cfg80211_mlme_purge_registrations(wdev);
-       flush_work(&wdev->mgmt_registrations_update_wk);
 
        switch (wdev->iftype) {
        case NL80211_IFTYPE_P2P_DEVICE:
@@ -1253,8 +1255,6 @@ void cfg80211_init_wdev(struct cfg80211_registered_device *rdev,
        spin_lock_init(&wdev->event_lock);
        INIT_LIST_HEAD(&wdev->mgmt_registrations);
        spin_lock_init(&wdev->mgmt_registrations_lock);
-       INIT_WORK(&wdev->mgmt_registrations_update_wk,
-                 cfg80211_mgmt_registrations_update_wk);
        INIT_LIST_HEAD(&wdev->pmsr_list);
        spin_lock_init(&wdev->pmsr_lock);
        INIT_WORK(&wdev->pmsr_free_wk, cfg80211_pmsr_free_wk);

diff --git a/net/wireless/core.h b/net/wireless/core.h
index e0e5b3ee96998bc5370d35caaf10986f406e30b7..67b0389fca4dce021daf9ea6cdc7248b80854ede 100644 (file)
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -99,6 +99,8 @@ struct cfg80211_registered_device {
        struct cfg80211_chan_def cac_done_chandef;
        struct work_struct propagate_cac_done_wk;
 
+       struct work_struct mgmt_registrations_update_wk;
+
        /* must be last because of the way we do wiphy_priv(),
         * and it should at least be aligned to NETDEV_ALIGN */
        struct wiphy wiphy __aligned(NETDEV_ALIGN);

diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index 189334314cbaed946f2889ecd370b4760ab170e7..a6c61a2e6569195088509b102156184b0dd092bb 100644 (file)
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -440,9 +440,15 @@ static void cfg80211_mgmt_registrations_update(struct wireless_dev *wdev)
 
        ASSERT_RTNL();
 
+       spin_lock_bh(&wdev->mgmt_registrations_lock);
+       if (!wdev->mgmt_registrations_need_update) {
+               spin_unlock_bh(&wdev->mgmt_registrations_lock);
+               return;
+       }
+
        rcu_read_lock();
        list_for_each_entry_rcu(tmp, &rdev->wiphy.wdev_list, list) {
-               list_for_each_entry_rcu(reg, &tmp->mgmt_registrations, list) {
+               list_for_each_entry(reg, &tmp->mgmt_registrations, list) {
                        u32 mask = BIT(le16_to_cpu(reg->frame_type) >> 4);
                        u32 mcast_mask = 0;
 
@@ -460,16 +466,23 @@ static void cfg80211_mgmt_registrations_update(struct wireless_dev *wdev)
        }
        rcu_read_unlock();
 
+       wdev->mgmt_registrations_need_update = 0;
+       spin_unlock_bh(&wdev->mgmt_registrations_lock);
+
        rdev_update_mgmt_frame_registrations(rdev, wdev, &upd);
 }
 
 void cfg80211_mgmt_registrations_update_wk(struct work_struct *wk)
 {
-       struct wireless_dev *wdev = container_of(wk, struct wireless_dev,
-                                                mgmt_registrations_update_wk);
+       struct cfg80211_registered_device *rdev;
+       struct wireless_dev *wdev;
+
+       rdev = container_of(wk, struct cfg80211_registered_device,
+                           mgmt_registrations_update_wk);
 
        rtnl_lock();
-       cfg80211_mgmt_registrations_update(wdev);
+       list_for_each_entry(wdev, &rdev->wiphy.wdev_list, list)
+               cfg80211_mgmt_registrations_update(wdev);
        rtnl_unlock();
 }
 
@@ -557,6 +570,7 @@ int cfg80211_mlme_register_mgmt(struct wireless_dev *wdev, u32 snd_portid,
                nreg->multicast_rx = multicast_rx;
                list_add(&nreg->list, &wdev->mgmt_registrations);
        }
+       wdev->mgmt_registrations_need_update = 1;
        spin_unlock_bh(&wdev->mgmt_registrations_lock);
 
        cfg80211_mgmt_registrations_update(wdev);
@@ -585,7 +599,8 @@ void cfg80211_mlme_unregister_socket(struct wireless_dev *wdev, u32 nlportid)
                list_del(&reg->list);
                kfree(reg);
 
-               schedule_work(&wdev->mgmt_registrations_update_wk);
+               wdev->mgmt_registrations_need_update = 1;
+               schedule_work(&rdev->mgmt_registrations_update_wk);
        }
 
        spin_unlock_bh(&wdev->mgmt_registrations_lock);
@@ -608,6 +623,7 @@ void cfg80211_mlme_purge_registrations(struct wireless_dev *wdev)
                list_del(&reg->list);
                kfree(reg);
        }
+       wdev->mgmt_registrations_need_update = 1;
        spin_unlock_bh(&wdev->mgmt_registrations_lock);
 
        cfg80211_mgmt_registrations_update(wdev);