Commit 79010420 authored by Johannes Berg's avatar Johannes Berg Committed by David S. Miller
Browse files

[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: default avatarJohannes 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: default avatarMichael Wu <flamingice@sourmilk.net>
Signed-off-by: default avatarJohn W. Linville <linville@tuxdriver.com>
parent ea49c359

net/mac80211/ieee80211.c

+25 −31
Original line number Diff line number Diff line
@@ -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();

net/mac80211/ieee80211_i.h

+2 −3
Original line number Diff line number Diff line
@@ -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;

net/mac80211/ieee80211_iface.c

+14 −17
Original line number Diff line number Diff line
@@ -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;

}

net/mac80211/ieee80211_sta.c

+6 −6
Original line number Diff line number Diff line
@@ -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;

net/mac80211/rx.c

+4 −5
Original line number Diff line number Diff line
@@ -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();

CRA Git | Maintained and supported by SUSTech CRA and CCSE