From 79010420cc3f78eab911598bfdd29c4b06a83e1f Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Tue, 18 Sep 2007 17:29:21 -0400 Subject: [PATCH] [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 Cc: Florian Lohoff Cc: Herbert Xu Cc: Michal Piotrowski Cc: Satyam Sharma Signed-off-by: Michael Wu Signed-off-by: John W. Linville --- net/mac80211/ieee80211.c | 56 +++++++++++++++------------------- net/mac80211/ieee80211_i.h | 5 ++- net/mac80211/ieee80211_iface.c | 31 +++++++++---------- net/mac80211/ieee80211_sta.c | 12 ++++---- net/mac80211/rx.c | 9 +++--- net/mac80211/tx.c | 10 ++++-- 6 files changed, 58 insertions(+), 65 deletions(-) diff --git a/net/mac80211/ieee80211.c b/net/mac80211/ieee80211.c index 4e345f82f0..ccf84634f1 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 1a43f3e9b6..a5961f16f2 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 4590205fdf..2ba24ef319 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 17455c6a52..651aaba1ad 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 2535d8d4ce..cb44a9db0e 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 38394c40f6..244c80d0c8 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) { -- 2.39.5