mac80211: fix key vs. sta locking problems

Up to now, key manipulation is supposed to run under RTNL to
avoid concurrent manipulations and also allow the set_key()
hardware callback to sleep. This is not feasible because STA
structs are rcu-protected and thus a lot of operations there
cannot take the RTNL. Also, key references are rcu-protected
so we cannot do things atomically.

This patch changes key locking completely:
 * key operations are now atomic
 * hardware crypto offload is enabled and disabled from
   a workqueue, due to that key freeing is also delayed
 * debugfs code is also run from a workqueue
 * keys reference STAs (and vice versa!) so during STA
   unlink the STAs key reference is removed but not the
   keys STA reference, to avoid races key todo work is
   run before STA destruction.
 * fewer STA operations now need the RTNL which was
   required due to key operations

This fixes the locking problems lockdep pointed out and also
makes things more light-weight because the rtnl isn't required
as much.

Note that the key todo lock/key mutex are global locks, this
is not required, of course, they could be per-hardware instead.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: John W. Linville <linville@tuxdriver.com>

net/mac80211/cfg.c

@@ -135,6 +135,7 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
 	struct sta_info *sta = NULL;
 	enum ieee80211_key_alg alg;
 	struct ieee80211_key *key;
+	int err;
 
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
@@ -157,17 +158,24 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
 	if (!key)
 		return -ENOMEM;
 
+	rcu_read_lock();
+
 	if (mac_addr) {
 		sta = sta_info_get(sdata->local, mac_addr);
 		if (!sta) {
 			ieee80211_key_free(key);
-			return -ENOENT;
+			err = -ENOENT;
+			goto out_unlock;
 		}
 	}
 
 	ieee80211_key_link(key, sdata, sta);
 
-	return 0;
+	err = 0;
+ out_unlock:
+	rcu_read_unlock();
+
+	return err;
 }
 
 static int ieee80211_del_key(struct wiphy *wiphy, struct net_device *dev,
@@ -179,28 +187,37 @@ static int ieee80211_del_key(struct wiphy *wiphy, struct net_device *dev,
 
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
+	rcu_read_lock();
+
 	if (mac_addr) {
+		ret = -ENOENT;
+
 		sta = sta_info_get(sdata->local, mac_addr);
 		if (!sta)
-			return -ENOENT;
+			goto out_unlock;
 
-		ret = 0;
 		if (sta->key) {
 			ieee80211_key_free(sta->key);
 			WARN_ON(sta->key);
-		} else
-			ret = -ENOENT;
+			ret = 0;
+		}
 
-		return ret;
+		goto out_unlock;
 	}
 
-	if (!sdata->keys[key_idx])
-		return -ENOENT;
+	if (!sdata->keys[key_idx]) {
+		ret = -ENOENT;
+		goto out_unlock;
+	}
 
 	ieee80211_key_free(sdata->keys[key_idx]);
 	WARN_ON(sdata->keys[key_idx]);
 
-	return 0;
+	ret = 0;
+ out_unlock:
+	rcu_read_unlock();
+
+	return ret;
 }
 
 static int ieee80211_get_key(struct wiphy *wiphy, struct net_device *dev,
@@ -217,6 +234,8 @@ static int ieee80211_get_key(struct wiphy *wiphy, struct net_device *dev,
 	u16 iv16;
 	int err = -ENOENT;
 
+	rcu_read_lock();
+
 	if (mac_addr) {
 		sta = sta_info_get(sdata->local, mac_addr);
 		if (!sta)
@@ -280,6 +299,7 @@ static int ieee80211_get_key(struct wiphy *wiphy, struct net_device *dev,
 	err = 0;
 
  out:
+	rcu_read_unlock();
 	return err;
 }
 
@@ -289,9 +309,13 @@ static int ieee80211_config_default_key(struct wiphy *wiphy,
 {
 	struct ieee80211_sub_if_data *sdata;
 
+	rcu_read_lock();
+
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	ieee80211_set_default_key(sdata, key_idx);
 
+	rcu_read_unlock();
+
 	return 0;
 }
 

net/mac80211/debugfs_key.c

@@ -184,23 +184,35 @@ KEY_OPS(key);
 	key->debugfs.name = debugfs_create_file(#name, 0400,\
 				key->debugfs.dir, key, &key_##name##_ops);
 
-void ieee80211_debugfs_key_add(struct ieee80211_local *local,
-			       struct ieee80211_key *key)
-{
+void ieee80211_debugfs_key_add(struct ieee80211_key *key)
+  {
 	static int keycount;
-	char buf[20];
+	char buf[50];
+	DECLARE_MAC_BUF(mac);
+	struct sta_info *sta;
 
-	if (!local->debugfs.keys)
+	if (!key->local->debugfs.keys)
 		return;
 
 	sprintf(buf, "%d", keycount);
 	keycount++;
 	key->debugfs.dir = debugfs_create_dir(buf,
-					local->debugfs.keys);
+					key->local->debugfs.keys);
 
 	if (!key->debugfs.dir)
 		return;
 
+	rcu_read_lock();
+	sta = rcu_dereference(key->sta);
+	if (sta)
+		sprintf(buf, "../../stations/%s", print_mac(mac, sta->addr));
+	rcu_read_unlock();
+
+	/* using sta as a boolean is fine outside RCU lock */
+	if (sta)
+		key->debugfs.stalink =
+			debugfs_create_symlink("station", key->debugfs.dir, buf);
+
 	DEBUGFS_ADD(keylen);
 	DEBUGFS_ADD(flags);
 	DEBUGFS_ADD(keyidx);
@@ -258,19 +270,6 @@ void ieee80211_debugfs_key_remove_default(struct ieee80211_sub_if_data *sdata)
 	debugfs_remove(sdata->debugfs.default_key);
 	sdata->debugfs.default_key = NULL;
 }
-void ieee80211_debugfs_key_sta_link(struct ieee80211_key *key,
-				    struct sta_info *sta)
-{
-	char buf[50];
-	DECLARE_MAC_BUF(mac);
-
-	if (!key->debugfs.dir)
-		return;
-
-	sprintf(buf, "../../stations/%s", print_mac(mac, sta->addr));
-	key->debugfs.stalink =
-		debugfs_create_symlink("station", key->debugfs.dir, buf);
-}
 
 void ieee80211_debugfs_key_sta_del(struct ieee80211_key *key,
 				   struct sta_info *sta)

net/mac80211/debugfs_key.h

@@ -2,18 +2,14 @@
 #define __MAC80211_DEBUGFS_KEY_H
 
 #ifdef CONFIG_MAC80211_DEBUGFS
-void ieee80211_debugfs_key_add(struct ieee80211_local *local,
-			       struct ieee80211_key *key);
+void ieee80211_debugfs_key_add(struct ieee80211_key *key);
 void ieee80211_debugfs_key_remove(struct ieee80211_key *key);
 void ieee80211_debugfs_key_add_default(struct ieee80211_sub_if_data *sdata);
 void ieee80211_debugfs_key_remove_default(struct ieee80211_sub_if_data *sdata);
-void ieee80211_debugfs_key_sta_link(struct ieee80211_key *key,
-				    struct sta_info *sta);
 void ieee80211_debugfs_key_sta_del(struct ieee80211_key *key,
 				   struct sta_info *sta);
 #else
-static inline void ieee80211_debugfs_key_add(struct ieee80211_local *local,
-					     struct ieee80211_key *key)
+static inline void ieee80211_debugfs_key_add(struct ieee80211_key *key)
 {}
 static inline void ieee80211_debugfs_key_remove(struct ieee80211_key *key)
 {}
@@ -23,9 +19,6 @@ static inline void ieee80211_debugfs_key_add_default(
 static inline void ieee80211_debugfs_key_remove_default(
 	struct ieee80211_sub_if_data *sdata)
 {}
-static inline void ieee80211_debugfs_key_sta_link(
-	struct ieee80211_key *key, struct sta_info *sta)
-{}
 static inline void ieee80211_debugfs_key_sta_del(struct ieee80211_key *key,
 						 struct sta_info *sta)
 {}

net/mac80211/ieee80211.c

@@ -1868,6 +1868,12 @@ static void __exit ieee80211_exit(void)
 {
 	rc80211_pid_exit();
 
+	/*
+	 * For key todo, it'll be empty by now but the work
+	 * might still be scheduled.
+	 */
+	flush_scheduled_work();
+
 	if (mesh_allocated)
 		ieee80211s_stop();
 

net/mac80211/ieee80211_i.h

@@ -600,8 +600,8 @@ struct ieee80211_local {
 	/*
 	 * The lock only protects the list, hash, timer and counter
 	 * against manipulation, reads are done in RCU. Additionally,
-	 * the lock protects each BSS's TIM bitmap and a few items
-	 * in a STA info structure.
+	 * the lock protects each BSS's TIM bitmap, a few items in
+	 * STA info structures and various key pointers.
 	 */
 	spinlock_t sta_lock;
 	unsigned long num_sta;

net/mac80211/ieee80211_ioctl.c

@@ -36,6 +36,7 @@ static int ieee80211_set_encryption(struct net_device *dev, u8 *sta_addr,
 	struct sta_info *sta;
 	struct ieee80211_key *key;
 	struct ieee80211_sub_if_data *sdata;
+	int err;
 
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
@@ -46,23 +47,31 @@ static int ieee80211_set_encryption(struct net_device *dev, u8 *sta_addr,
 	}
 
 	if (remove) {
+		rcu_read_lock();
+
+		err = 0;
+
 		if (is_broadcast_ether_addr(sta_addr)) {
 			key = sdata->keys[idx];
 		} else {
 			sta = sta_info_get(local, sta_addr);
-			if (!sta)
-				return -ENOENT;
+			if (!sta) {
+				err = -ENOENT;
+				goto out_unlock;
+			}
 			key = sta->key;
 		}
 
 		ieee80211_key_free(key);
-		return 0;
 	} else {
 		key = ieee80211_key_alloc(alg, idx, key_len, _key);
 		if (!key)
 			return -ENOMEM;
 
 		sta = NULL;
+		err = 0;
+
+		rcu_read_lock();
 
 		if (!is_broadcast_ether_addr(sta_addr)) {
 			set_tx_key = 0;
@@ -74,13 +83,15 @@ static int ieee80211_set_encryption(struct net_device *dev, u8 *sta_addr,
 			 */
 			if (idx != 0 && alg != ALG_WEP) {
 				ieee80211_key_free(key);
-				return -EINVAL;
+				err = -EINVAL;
+				goto out_unlock;
 			}
 
 			sta = sta_info_get(local, sta_addr);
 			if (!sta) {
 				ieee80211_key_free(key);
-				return -ENOENT;
+				err = -ENOENT;
+				goto out_unlock;
 			}
 		}
 
@@ -90,7 +101,10 @@ static int ieee80211_set_encryption(struct net_device *dev, u8 *sta_addr,
 			ieee80211_set_default_key(sdata, idx);
 	}
 
-	return 0;
+ out_unlock:
+	rcu_read_unlock();
+
+	return err;
 }
 
 static int ieee80211_ioctl_siwgenie(struct net_device *dev,

net/mac80211/ieee80211_key.h

@@ -51,22 +51,32 @@ struct sta_info;
  *
  * @KEY_FLAG_UPLOADED_TO_HARDWARE: Indicates that this key is present
  *	in the hardware for TX crypto hardware acceleration.
- * @KEY_FLAG_REMOVE_FROM_HARDWARE: Indicates to the key code that this
- *	key is present in the hardware (but it cannot be used for
- *	hardware acceleration any more!)
+ * @KEY_FLAG_TODO_DELETE: Key is marked for deletion and will, after an
+ *	RCU grace period, no longer be reachable other than from the
+ *	todo list.
+ * @KEY_FLAG_TODO_HWACCEL: Key needs to be added to hardware acceleration.
+ * @KEY_FLAG_TODO_DEFKEY: Key is default key and debugfs needs to be updated.
+ * @KEY_FLAG_TODO_ADD_DEBUGFS: Key needs to be added to debugfs.
  */
 enum ieee80211_internal_key_flags {
 	KEY_FLAG_UPLOADED_TO_HARDWARE	= BIT(0),
-	KEY_FLAG_REMOVE_FROM_HARDWARE	= BIT(1),
+	KEY_FLAG_TODO_DELETE		= BIT(1),
+	KEY_FLAG_TODO_HWACCEL		= BIT(2),
+	KEY_FLAG_TODO_DEFKEY		= BIT(3),
+	KEY_FLAG_TODO_ADD_DEBUGFS	= BIT(4),
 };
 
 struct ieee80211_key {
 	struct ieee80211_local *local;
 	struct ieee80211_sub_if_data *sdata;
 	struct sta_info *sta;
 
+	/* for sdata list */
 	struct list_head list;
+	/* for todo list */
+	struct list_head todo;
 
+	/* protected by todo lock! */
 	unsigned int flags;
 
 	union {
@@ -142,4 +152,6 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata);
 void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata);
 void ieee80211_disable_keys(struct ieee80211_sub_if_data *sdata);
 
+void ieee80211_key_todo(void);
+
 #endif /* IEEE80211_KEY_H */

net/mac80211/ieee80211_sta.c

@@ -952,11 +952,8 @@ static void ieee80211_associated(struct net_device *dev,
 
 	rcu_read_unlock();
 
-	if (disassoc && sta) {
-		rtnl_lock();
+	if (disassoc && sta)
 		sta_info_destroy(sta);
-		rtnl_unlock();
-	}
 
 	if (disassoc) {
 		ifsta->state = IEEE80211_DISABLED;
@@ -3104,12 +3101,8 @@ static void ieee80211_sta_expire(struct net_device *dev, unsigned long exp_time)
 		}
 	spin_unlock_irqrestore(&local->sta_lock, flags);
 
-	synchronize_rcu();
-
-	rtnl_lock();
 	list_for_each_entry_safe(sta, tmp, &tmp_list, list)
 		sta_info_destroy(sta);
-	rtnl_unlock();
 }