[PATCH] mac80211: fix virtual interface locking

Florian Lohoff noticed a bug in mac80211: when bringing the
master interface down while other virtual interfaces are up
we call dev_close() under a spinlock which is not allowed.
This patch removes the sub_if_lock used by mac80211 in favour
of using an RCU list. All list manipulations are already done
under rtnl so are well protected against each other, and the
read-side locks we took in the RX and TX code are already in
RCU read-side critical sections.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Florian Lohoff <flo@rfc822.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: Michal Piotrowski <michal.k.k.piotrowski@gmail.com>
Cc: Satyam Sharma <satyam@infradead.org>
Signed-off-by: Michael Wu <flamingice@sourmilk.net>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

6 files changed, 58 insertions, 65 deletions

diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c
index 4e345f82f044..ccf84634f156 100644
--- a/net/mac80211/ieee80211.c
+++ b/net/mac80211/ieee80211.c
@@ -93,14 +93,13 @@ static int ieee80211_master_open(struct net_device *dev)
         struct ieee80211_sub_if_data *sdata;
         int res = -EOPNOTSUPP;
 
-        read_lock(&local->sub_if_lock);
-        list_for_each_entry(sdata, &local->sub_if_list, list) {
+        /* we hold the RTNL here so can safely walk the list */
+        list_for_each_entry(sdata, &local->interfaces, list) {
                 if (sdata->dev != dev && netif_running(sdata->dev)) {
                         res = 0;
                         break;
                 }
         }
-        read_unlock(&local->sub_if_lock);
         return res;
 }
 
@@ -109,11 +108,10 @@ static int ieee80211_master_stop(struct net_device *dev)
         struct ieee80211_local *local = wdev_priv(dev->ieee80211_ptr);
         struct ieee80211_sub_if_data *sdata;
 
-        read_lock(&local->sub_if_lock);
-        list_for_each_entry(sdata, &local->sub_if_list, list)
+        /* we hold the RTNL here so can safely walk the list */
+        list_for_each_entry(sdata, &local->interfaces, list)
                 if (sdata->dev != dev && netif_running(sdata->dev))
                         dev_close(sdata->dev);
-        read_unlock(&local->sub_if_lock);
 
         return 0;
 }
@@ -315,8 +313,8 @@ static int ieee80211_open(struct net_device *dev)
 
         sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
-        read_lock(&local->sub_if_lock);
-        list_for_each_entry(nsdata, &local->sub_if_list, list) {
+        /* we hold the RTNL here so can safely walk the list */
+        list_for_each_entry(nsdata, &local->interfaces, list) {
                 struct net_device *ndev = nsdata->dev;
 
                 if (ndev != dev && ndev != local->mdev && netif_running(ndev) &&
@@ -325,10 +323,8 @@ static int ieee80211_open(struct net_device *dev)
                          * check whether it may have the same address
                          */
                         if (!identical_mac_addr_allowed(sdata->type,
-                                                        nsdata->type)) {
-                                read_unlock(&local->sub_if_lock);
+                                                        nsdata->type))
                                 return -ENOTUNIQ;
-                        }
 
                         /*
                          * can only add VLANs to enabled APs
@@ -339,7 +335,6 @@ static int ieee80211_open(struct net_device *dev)
                                 sdata->u.vlan.ap = nsdata;
                 }
         }
-        read_unlock(&local->sub_if_lock);
 
         switch (sdata->type) {
         case IEEE80211_IF_TYPE_WDS:
@@ -466,14 +461,13 @@ static int ieee80211_stop(struct net_device *dev)
                 sdata->u.sta.state = IEEE80211_DISABLED;
                 del_timer_sync(&sdata->u.sta.timer);
                 /*
-                 * Holding the sub_if_lock for writing here blocks
-                 * out the receive path and makes sure it's not
-                 * currently processing a packet that may get
-                 * added to the queue.
+                 * When we get here, the interface is marked down.
+                 * Call synchronize_rcu() to wait for the RX path
+                 * should it be using the interface and enqueuing
+                 * frames at this very time on another CPU.
                  */
-                write_lock_bh(&local->sub_if_lock);
+                synchronize_rcu();
                 skb_queue_purge(&sdata->u.sta.skb_queue);
-                write_unlock_bh(&local->sub_if_lock);
 
                 if (!local->ops->hw_scan &&
                     local->scan_dev == sdata->dev) {
@@ -1033,9 +1027,9 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
 
         rthdr->data_retries = status->retry_count;
 
-        read_lock(&local->sub_if_lock);
+        rcu_read_lock();
         monitors = local->monitors;
-        list_for_each_entry(sdata, &local->sub_if_list, list) {
+        list_for_each_entry_rcu(sdata, &local->interfaces, list) {
                 /*
                  * Using the monitors counter is possibly racy, but
                  * if the value is wrong we simply either clone the skb
@@ -1051,7 +1045,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
                                 continue;
                         monitors--;
                         if (monitors)
-                                skb2 = skb_clone(skb, GFP_KERNEL);
+                                skb2 = skb_clone(skb, GFP_ATOMIC);
                         else
                                 skb2 = NULL;
                         skb->dev = sdata->dev;
@@ -1066,7 +1060,7 @@ void ieee80211_tx_status(struct ieee80211_hw *hw, struct sk_buff *skb,
                 }
         }
  out:
-        read_unlock(&local->sub_if_lock);
+        rcu_read_unlock();
         if (skb)
                 dev_kfree_skb(skb);
 }
@@ -1154,8 +1148,7 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
 
         INIT_LIST_HEAD(&local->modes_list);
 
-        rwlock_init(&local->sub_if_lock);
-        INIT_LIST_HEAD(&local->sub_if_list);
+        INIT_LIST_HEAD(&local->interfaces);
 
         INIT_DELAYED_WORK(&local->scan_work, ieee80211_sta_scan_work);
         ieee80211_rx_bss_list_init(mdev);
@@ -1175,7 +1168,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
         sdata->u.ap.force_unicast_rateidx = -1;
         sdata->u.ap.max_ratectrl_rateidx = -1;
         ieee80211_if_sdata_init(sdata);
-        list_add_tail(&sdata->list, &local->sub_if_list);
+        /* no RCU needed since we're still during init phase */
+        list_add_tail(&sdata->list, &local->interfaces);
 
         tasklet_init(&local->tx_pending_tasklet, ieee80211_tx_pending,
                      (unsigned long)local);
@@ -1334,7 +1328,6 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
 {
         struct ieee80211_local *local = hw_to_local(hw);
         struct ieee80211_sub_if_data *sdata, *tmp;
-        struct list_head tmp_list;
         int i;
 
         tasklet_kill(&local->tx_pending_tasklet);
@@ -1348,11 +1341,12 @@ void ieee80211_unregister_hw(struct ieee80211_hw *hw)
         if (local->apdev)
                 ieee80211_if_del_mgmt(local);
 
-        write_lock_bh(&local->sub_if_lock);
-        list_replace_init(&local->sub_if_list, &tmp_list);
-        write_unlock_bh(&local->sub_if_lock);
-
-        list_for_each_entry_safe(sdata, tmp, &tmp_list, list)
+        /*
+         * At this point, interface list manipulations are fine
+         * because the driver cannot be handing us frames any
+         * more and the tasklet is killed.
+         */
+        list_for_each_entry_safe(sdata, tmp, &local->interfaces, list)
                 __ieee80211_if_del(local, sdata);
 
         rtnl_unlock();
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 1a43f3e9b6bd..a5961f16f206 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -475,9 +475,8 @@ struct ieee80211_local {
         ieee80211_rx_handler *rx_handlers;
         ieee80211_tx_handler *tx_handlers;
 
-        rwlock_t sub_if_lock; /* Protects sub_if_list. Cannot be taken under
-                               * sta_bss_lock or sta_lock. */
-        struct list_head sub_if_list;
+        struct list_head interfaces;
+
         int sta_scanning;
         int scan_channel_idx;
         enum { SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state;
diff --git a/net/mac80211/ieee80211_iface.c b/net/mac80211/ieee80211_iface.c
index 4590205fdf4b..2ba24ef319da 100644
--- a/net/mac80211/ieee80211_iface.c
+++ b/net/mac80211/ieee80211_iface.c
@@ -79,16 +79,15 @@ int ieee80211_if_add(struct net_device *dev, const char *name,
         ieee80211_debugfs_add_netdev(sdata);
         ieee80211_if_set_type(ndev, type);
 
-        write_lock_bh(&local->sub_if_lock);
+        /* we're under RTNL so all this is fine */
         if (unlikely(local->reg_state == IEEE80211_DEV_UNREGISTERED)) {
-                write_unlock_bh(&local->sub_if_lock);
                 __ieee80211_if_del(local, sdata);
                 return -ENODEV;
         }
-        list_add(&sdata->list, &local->sub_if_list);
+        list_add_tail_rcu(&sdata->list, &local->interfaces);
+
         if (new_dev)
                 *new_dev = ndev;
-        write_unlock_bh(&local->sub_if_lock);
 
         return 0;
 
@@ -226,22 +225,22 @@ void ieee80211_if_reinit(struct net_device *dev)
                 /* Remove all virtual interfaces that use this BSS
                  * as their sdata->bss */
                 struct ieee80211_sub_if_data *tsdata, *n;
-                LIST_HEAD(tmp_list);
 
-                write_lock_bh(&local->sub_if_lock);
-                list_for_each_entry_safe(tsdata, n, &local->sub_if_list, list) {
+                list_for_each_entry_safe(tsdata, n, &local->interfaces, list) {
                         if (tsdata != sdata && tsdata->bss == &sdata->u.ap) {
                                 printk(KERN_DEBUG "%s: removing virtual "
                                        "interface %s because its BSS interface"
                                        " is being removed\n",
                                        sdata->dev->name, tsdata->dev->name);
-                                list_move_tail(&tsdata->list, &tmp_list);
+                                list_del_rcu(&tsdata->list);
+                                /*
+                                 * We have lots of time and can afford
+                                 * to sync for each interface
+                                 */
+                                synchronize_rcu();
+                                __ieee80211_if_del(local, tsdata);
                         }
                 }
-                write_unlock_bh(&local->sub_if_lock);
-
-                list_for_each_entry_safe(tsdata, n, &tmp_list, list)
-                        __ieee80211_if_del(local, tsdata);
 
                 kfree(sdata->u.ap.beacon_head);
                 kfree(sdata->u.ap.beacon_tail);
@@ -318,18 +317,16 @@ int ieee80211_if_remove(struct net_device *dev, const char *name, int id)
 
         ASSERT_RTNL();
 
-        write_lock_bh(&local->sub_if_lock);
-        list_for_each_entry_safe(sdata, n, &local->sub_if_list, list) {
+        list_for_each_entry_safe(sdata, n, &local->interfaces, list) {
                 if ((sdata->type == id || id == -1) &&
                     strcmp(name, sdata->dev->name) == 0 &&
                     sdata->dev != local->mdev) {
-                        list_del(&sdata->list);
-                        write_unlock_bh(&local->sub_if_lock);
+                        list_del_rcu(&sdata->list);
+                        synchronize_rcu();
                         __ieee80211_if_del(local, sdata);
                         return 0;
                 }
         }
-        write_unlock_bh(&local->sub_if_lock);
         return -ENODEV;
 }
 
diff --git a/net/mac80211/ieee80211_sta.c b/net/mac80211/ieee80211_sta.c
index 17455c6a5229..651aaba1add4 100644
--- a/net/mac80211/ieee80211_sta.c
+++ b/net/mac80211/ieee80211_sta.c
@@ -2676,8 +2676,8 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
         memset(&wrqu, 0, sizeof(wrqu));
         wireless_send_event(dev, SIOCGIWSCAN, &wrqu, NULL);
 
-        read_lock(&local->sub_if_lock);
-        list_for_each_entry(sdata, &local->sub_if_list, list) {
+        rcu_read_lock();
+        list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 
                 /* No need to wake the master device. */
                 if (sdata->dev == local->mdev)
@@ -2691,7 +2691,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw)
 
                 netif_wake_queue(sdata->dev);
         }
-        read_unlock(&local->sub_if_lock);
+        rcu_read_unlock();
 
         sdata = IEEE80211_DEV_TO_SUB_IF(dev);
         if (sdata->type == IEEE80211_IF_TYPE_IBSS) {
@@ -2828,8 +2828,8 @@ static int ieee80211_sta_start_scan(struct net_device *dev,
 
         local->sta_scanning = 1;
 
-        read_lock(&local->sub_if_lock);
-        list_for_each_entry(sdata, &local->sub_if_list, list) {
+        rcu_read_lock();
+        list_for_each_entry_rcu(sdata, &local->interfaces, list) {
 
                 /* Don't stop the master interface, otherwise we can't transmit
                  * probes! */
@@ -2841,7 +2841,7 @@ static int ieee80211_sta_start_scan(struct net_device *dev,
                     (sdata->u.sta.flags & IEEE80211_STA_ASSOCIATED))
                         ieee80211_send_nullfunc(local, sdata, 1);
         }
-        read_unlock(&local->sub_if_lock);
+        rcu_read_unlock();
 
         if (ssid) {
                 local->scan_ssid_len = ssid_len;
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 2535d8d4ce90..cb44a9db0e19 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -1383,8 +1383,9 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
         }
 
         /*
-         * key references are protected using RCU and this requires that
-         * we are in a read-site RCU section during receive processing
+         * key references and virtual interfaces are protected using RCU
+         * and this requires that we are in a read-side RCU section during
+         * receive processing
          */
         rcu_read_lock();
 
@@ -1439,8 +1440,7 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
 
         bssid = ieee80211_get_bssid(hdr, skb->len - radiotap_len);
 
-        read_lock(&local->sub_if_lock);
-        list_for_each_entry(sdata, &local->sub_if_list, list) {
+        list_for_each_entry_rcu(sdata, &local->interfaces, list) {
                 rx.flags |= IEEE80211_TXRXD_RXRA_MATCH;
 
                 if (!netif_running(sdata->dev))
@@ -1493,7 +1493,6 @@ void __ieee80211_rx(struct ieee80211_hw *hw, struct sk_buff *skb,
                                              &rx, sta);
         } else
                 dev_kfree_skb(skb);
-        read_unlock(&local->sub_if_lock);
 
  end:
         rcu_read_unlock();
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 38394c40f6ad..244c80d0c8fb 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -295,8 +295,12 @@ static void purge_old_ps_buffers(struct ieee80211_local *local)
         struct ieee80211_sub_if_data *sdata;
         struct sta_info *sta;
 
-        read_lock(&local->sub_if_lock);
-        list_for_each_entry(sdata, &local->sub_if_list, list) {
+        /*
+         * virtual interfaces are protected by RCU
+         */
+        rcu_read_lock();
+
+        list_for_each_entry_rcu(sdata, &local->interfaces, list) {
                 struct ieee80211_if_ap *ap;
                 if (sdata->dev == local->mdev ||
                     sdata->type != IEEE80211_IF_TYPE_AP)
@@ -309,7 +313,7 @@ static void purge_old_ps_buffers(struct ieee80211_local *local)
                 }
                 total += skb_queue_len(&ap->ps_bc_buf);
         }
-        read_unlock(&local->sub_if_lock);
+        rcu_read_unlock();
 
         read_lock_bh(&local->sta_lock);
         list_for_each_entry(sta, &local->sta_list, list) {