[PATCH 1/3] mac80211: clean up key freeing a bit

[PATCH 1/3] mac80211: clean up key freeing a bit

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When a key is allocated but not really added, there's no
need to go through the entire teardown process. Also, if
adding a key fails, ieee80211_key_link() can take care of
freeing it instead of the (only) caller.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/cfg.c |  4 +---
 net/mac80211/key.c | 32 +++++++++++++++++++-------------
 net/mac80211/key.h | 11 +++++------
 3 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 61fc911..c2d4bf2 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -175,7 +175,7 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
 		 *       add it to the device after the station.
 		 */
 		if (!sta || !test_sta_flag(sta, WLAN_STA_ASSOC)) {
-			ieee80211_key_free(sdata->local, key);
+			ieee80211_key_free_unused(key);
 			err = -ENOENT;
 			goto out_unlock;
 		}
@@ -214,8 +214,6 @@ static int ieee80211_add_key(struct wiphy *wiphy, struct net_device *dev,
 	}
 
 	err = ieee80211_key_link(key, sdata, sta);
-	if (err)
-		ieee80211_key_free(sdata->local, key);
 
  out_unlock:
 	mutex_unlock(&sdata->local->sta_mtx);
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 99e9f6a..d86be64 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -397,6 +397,15 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
 	return key;
 }
 
+static void ieee80211_key_free_common(struct ieee80211_key *key)
+{
+	if (key->conf.cipher == WLAN_CIPHER_SUITE_CCMP)
+		ieee80211_aes_key_free(key->u.ccmp.tfm);
+	if (key->conf.cipher == WLAN_CIPHER_SUITE_AES_CMAC)
+		ieee80211_aes_cmac_key_free(key->u.aes_cmac.tfm);
+	kfree(key);
+}
+
 static void __ieee80211_key_destroy(struct ieee80211_key *key,
 				    bool delay_tailroom)
 {
@@ -412,10 +421,6 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key,
 	if (key->local)
 		ieee80211_key_disable_hw_accel(key);
 
-	if (key->conf.cipher == WLAN_CIPHER_SUITE_CCMP)
-		ieee80211_aes_key_free(key->u.ccmp.tfm);
-	if (key->conf.cipher == WLAN_CIPHER_SUITE_AES_CMAC)
-		ieee80211_aes_cmac_key_free(key->u.aes_cmac.tfm);
 	if (key->local) {
 		struct ieee80211_sub_if_data *sdata = key->sdata;
 
@@ -431,7 +436,13 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key,
 		}
 	}
 
-	kfree(key);
+	ieee80211_key_free_common(key);
+}
+
+void ieee80211_key_free_unused(struct ieee80211_key *key)
+{
+	WARN_ON(key->sdata || key->local);
+	ieee80211_key_free_common(key);
 }
 
 int ieee80211_key_link(struct ieee80211_key *key,
@@ -469,6 +480,9 @@ int ieee80211_key_link(struct ieee80211_key *key,
 
 	ret = ieee80211_key_enable_hw_accel(key);
 
+	if (ret)
+		__ieee80211_key_free(key, true);
+
 	mutex_unlock(&sdata->local->key_mtx);
 
 	return ret;
@@ -489,14 +503,6 @@ void __ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom)
 	__ieee80211_key_destroy(key, delay_tailroom);
 }
 
-void ieee80211_key_free(struct ieee80211_local *local,
-			struct ieee80211_key *key)
-{
-	mutex_lock(&local->key_mtx);
-	__ieee80211_key_free(key, true);
-	mutex_unlock(&local->key_mtx);
-}
-
 void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_key *key;
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index 2a682d8..8ef56cd 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -129,14 +129,13 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
 					  size_t seq_len, const u8 *seq);
 /*
  * Insert a key into data structures (sdata, sta if necessary)
- * to make it used, free old key.
+ * to make it used, free old key. On failure, also free the new key.
  */
-int __must_check ieee80211_key_link(struct ieee80211_key *key,
-				    struct ieee80211_sub_if_data *sdata,
-				    struct sta_info *sta);
+int ieee80211_key_link(struct ieee80211_key *key,
+		       struct ieee80211_sub_if_data *sdata,
+		       struct sta_info *sta);
 void __ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom);
-void ieee80211_key_free(struct ieee80211_local *local,
-			struct ieee80211_key *key);
+void ieee80211_key_free_unused(struct ieee80211_key *key);
 void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx,
 			       bool uni, bool multi);
 void ieee80211_set_default_mgmt_key(struct ieee80211_sub_if_data *sdata,
-- 
1.8.0

[PATCH 2/3] mac80211: remove underscores from some key functions

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Some key function don't exist without underscores, so
remove the underscores from those.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/cfg.c      |  2 +-
 net/mac80211/key.c      | 28 ++++++++++++++--------------
 net/mac80211/key.h      |  2 +-
 net/mac80211/sta_info.c |  8 ++++----
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index c2d4bf2..e5c1441 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -252,7 +252,7 @@ static int ieee80211_del_key(struct wiphy *wiphy, struct net_device *dev,
 		goto out_unlock;
 	}
 
-	__ieee80211_key_free(key, true);
+	ieee80211_key_free(key, true);
 
 	ret = 0;
  out_unlock:
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index d86be64..953887b 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -248,11 +248,11 @@ void ieee80211_set_default_mgmt_key(struct ieee80211_sub_if_data *sdata,
 }
 
 
-static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
-				    struct sta_info *sta,
-				    bool pairwise,
-				    struct ieee80211_key *old,
-				    struct ieee80211_key *new)
+static void ieee80211_key_replace(struct ieee80211_sub_if_data *sdata,
+				  struct sta_info *sta,
+				  bool pairwise,
+				  struct ieee80211_key *old,
+				  struct ieee80211_key *new)
 {
 	int idx;
 	bool defunikey, defmultikey, defmgmtkey;
@@ -406,8 +406,8 @@ static void ieee80211_key_free_common(struct ieee80211_key *key)
 	kfree(key);
 }
 
-static void __ieee80211_key_destroy(struct ieee80211_key *key,
-				    bool delay_tailroom)
+static void ieee80211_key_destroy(struct ieee80211_key *key,
+				  bool delay_tailroom)
 {
 	if (!key)
 		return;
@@ -473,22 +473,22 @@ int ieee80211_key_link(struct ieee80211_key *key,
 
 	increment_tailroom_need_count(sdata);
 
-	__ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
-	__ieee80211_key_destroy(old_key, true);
+	ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
+	ieee80211_key_destroy(old_key, true);
 
 	ieee80211_debugfs_key_add(key);
 
 	ret = ieee80211_key_enable_hw_accel(key);
 
 	if (ret)
-		__ieee80211_key_free(key, true);
+		ieee80211_key_free(key, true);
 
 	mutex_unlock(&sdata->local->key_mtx);
 
 	return ret;
 }
 
-void __ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom)
+void ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom)
 {
 	if (!key)
 		return;
@@ -497,10 +497,10 @@ void __ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom)
 	 * Replace key with nothingness if it was ever used.
 	 */
 	if (key->sdata)
-		__ieee80211_key_replace(key->sdata, key->sta,
+		ieee80211_key_replace(key->sdata, key->sta,
 				key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE,
 				key, NULL);
-	__ieee80211_key_destroy(key, delay_tailroom);
+	ieee80211_key_destroy(key, delay_tailroom);
 }
 
 void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
@@ -572,7 +572,7 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata)
 	ieee80211_debugfs_key_remove_mgmt_default(sdata);
 
 	list_for_each_entry_safe(key, tmp, &sdata->key_list, list)
-		__ieee80211_key_free(key, false);
+		ieee80211_key_free(key, false);
 
 	ieee80211_debugfs_key_update_default(sdata);
 
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index 8ef56cd..a353ddd6 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -134,7 +134,7 @@ struct ieee80211_key *ieee80211_key_alloc(u32 cipher, int idx, size_t key_len,
 int ieee80211_key_link(struct ieee80211_key *key,
 		       struct ieee80211_sub_if_data *sdata,
 		       struct sta_info *sta);
-void __ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom);
+void ieee80211_key_free(struct ieee80211_key *key, bool delay_tailroom);
 void ieee80211_key_free_unused(struct ieee80211_key *key);
 void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx,
 			       bool uni, bool multi);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index a36ceed..2961f3d 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -812,11 +812,11 @@ int __must_check __sta_info_destroy(struct sta_info *sta)
 
 	mutex_lock(&local->key_mtx);
 	for (i = 0; i < NUM_DEFAULT_KEYS; i++)
-		__ieee80211_key_free(key_mtx_dereference(local, sta->gtk[i]),
-				     true);
+		ieee80211_key_free(key_mtx_dereference(local, sta->gtk[i]),
+				   true);
 	if (sta->ptk)
-		__ieee80211_key_free(key_mtx_dereference(local, sta->ptk),
-				     true);
+		ieee80211_key_free(key_mtx_dereference(local, sta->ptk),
+				   true);
 	mutex_unlock(&local->key_mtx);
 
 	sta->dead = true;
-- 
1.8.0

[PATCH 3/3] mac80211: batch key free synchronize_net()

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Instead of calling synchronize_net() for every key
on an interface or when a station is removed, do it
only once for all keys in both of these cases.

As a side-effect, removing station keys now always
calls synchronize_net() even if there are no keys,
which fixes an issue with station removal happening
in the driver while the station could still be used
for TX.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/key.c      | 77 ++++++++++++++++++++++++++++++++++++++++---------
 net/mac80211/key.h      |  2 ++
 net/mac80211/sta_info.c | 12 ++------
 3 files changed, 69 insertions(+), 22 deletions(-)

diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 953887b..989f773 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -406,18 +406,9 @@ static void ieee80211_key_free_common(struct ieee80211_key *key)
 	kfree(key);
 }
 
-static void ieee80211_key_destroy(struct ieee80211_key *key,
-				  bool delay_tailroom)
+static void __ieee80211_key_destroy(struct ieee80211_key *key,
+				    bool delay_tailroom)
 {
-	if (!key)
-		return;
-
-	/*
-	 * Synchronize so the TX path can no longer be using
-	 * this key before we free/remove it.
-	 */
-	synchronize_net();
-
 	if (key->local)
 		ieee80211_key_disable_hw_accel(key);
 
@@ -439,6 +430,21 @@ static void ieee80211_key_destroy(struct ieee80211_key *key,
 	ieee80211_key_free_common(key);
 }
 
+static void ieee80211_key_destroy(struct ieee80211_key *key,
+				  bool delay_tailroom)
+{
+	if (!key)
+		return;
+
+	/*
+	 * Synchronize so the TX path can no longer be using
+	 * this key before we free/remove it.
+	 */
+	synchronize_net();
+
+	__ieee80211_key_destroy(key, delay_tailroom);
+}
+
 void ieee80211_key_free_unused(struct ieee80211_key *key)
 {
 	WARN_ON(key->sdata || key->local);
@@ -560,6 +566,7 @@ EXPORT_SYMBOL(ieee80211_iter_keys);
 void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata)
 {
 	struct ieee80211_key *key, *tmp;
+	LIST_HEAD(keys);
 
 	cancel_delayed_work_sync(&sdata->dec_tailroom_needed_wk);
 
@@ -571,17 +578,61 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata)
 
 	ieee80211_debugfs_key_remove_mgmt_default(sdata);
 
-	list_for_each_entry_safe(key, tmp, &sdata->key_list, list)
-		ieee80211_key_free(key, false);
+	list_for_each_entry_safe(key, tmp, &sdata->key_list, list) {
+		ieee80211_key_replace(key->sdata, key->sta,
+				key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE,
+				key, NULL);
+		list_add_tail(&key->list, &keys);
+	}
 
 	ieee80211_debugfs_key_update_default(sdata);
 
+	synchronize_net();
+	list_for_each_entry_safe(key, tmp, &keys, list)
+		__ieee80211_key_destroy(key, false);
+
 	WARN_ON_ONCE(sdata->crypto_tx_tailroom_needed_cnt ||
 		     sdata->crypto_tx_tailroom_pending_dec);
 
 	mutex_unlock(&sdata->local->key_mtx);
 }
 
+void ieee80211_free_sta_keys(struct ieee80211_local *local,
+			     struct sta_info *sta)
+{
+	struct ieee80211_key *key, *tmp;
+	LIST_HEAD(keys);
+	int i;
+
+	mutex_lock(&local->key_mtx);
+	for (i = 0; i < NUM_DEFAULT_KEYS; i++) {
+		key = key_mtx_dereference(local, sta->gtk[i]);
+		ieee80211_key_replace(key->sdata, key->sta,
+				key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE,
+				key, NULL);
+		list_add(&key->list, &keys);
+	}
+
+	key = key_mtx_dereference(local, sta->ptk);
+	if (key) {
+		ieee80211_key_replace(key->sdata, key->sta,
+				key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE,
+				key, NULL);
+		list_add(&key->list, &keys);
+	}
+
+	/*
+	 * NB: the station code relies on this being
+	 * done even if there aren't any keys
+	 */
+	synchronize_net();
+
+	list_for_each_entry_safe(key, tmp, &keys, list)
+		__ieee80211_key_destroy(key, true);
+
+	mutex_unlock(&local->key_mtx);
+}
+
 void ieee80211_delayed_tailroom_dec(struct work_struct *wk)
 {
 	struct ieee80211_sub_if_data *sdata;
diff --git a/net/mac80211/key.h b/net/mac80211/key.h
index a353ddd6..e8de3e6 100644
--- a/net/mac80211/key.h
+++ b/net/mac80211/key.h
@@ -141,6 +141,8 @@ void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx,
 void ieee80211_set_default_mgmt_key(struct ieee80211_sub_if_data *sdata,
 				    int idx);
 void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata);
+void ieee80211_free_sta_keys(struct ieee80211_local *local,
+			     struct sta_info *sta);
 void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata);
 
 #define key_mtx_dereference(local, ref) \
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 2961f3d..11216bc 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -783,7 +783,7 @@ int __must_check __sta_info_destroy(struct sta_info *sta)
 {
 	struct ieee80211_local *local;
 	struct ieee80211_sub_if_data *sdata;
-	int ret, i;
+	int ret;
 
 	might_sleep();
 
@@ -810,14 +810,8 @@ int __must_check __sta_info_destroy(struct sta_info *sta)
 
 	list_del_rcu(&sta->list);
 
-	mutex_lock(&local->key_mtx);
-	for (i = 0; i < NUM_DEFAULT_KEYS; i++)
-		ieee80211_key_free(key_mtx_dereference(local, sta->gtk[i]),
-				   true);
-	if (sta->ptk)
-		ieee80211_key_free(key_mtx_dereference(local, sta->ptk),
-				   true);
-	mutex_unlock(&local->key_mtx);
+	/* this always calls synchronize_net() */
+	ieee80211_free_sta_keys(local, sta);
 
 	sta->dead = true;
 
-- 
1.8.0

Re: [PATCH 3/3] mac80211: batch key free synchronize_net()

[Johannes Berg]

On Thu, 2013-03-07 at 11:27 +0100, Johannes Berg wrote:

>  void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata)
>  {
>  	struct ieee80211_key *key, *tmp;
> +	LIST_HEAD(keys);
>  
>  	cancel_delayed_work_sync(&sdata->dec_tailroom_needed_wk);
>  
> @@ -571,17 +578,61 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata)
>  
>  	ieee80211_debugfs_key_remove_mgmt_default(sdata);
>  
> -	list_for_each_entry_safe(key, tmp, &sdata->key_list, list)
> -		ieee80211_key_free(key, false);
> +	list_for_each_entry_safe(key, tmp, &sdata->key_list, list) {
> +		ieee80211_key_replace(key->sdata, key->sta,
> +				key->conf.flags & IEEE80211_KEY_FLAG_PAIRWISE,
> +				key, NULL);
> +		list_add_tail(&key->list, &keys);
> +	}
>  
>  	ieee80211_debugfs_key_update_default(sdata);
>  
> +	synchronize_net();
> +	list_for_each_entry_safe(key, tmp, &keys, list)
> +		__ieee80211_key_destroy(key, false);

Since it's likely that there are no keys, I'll change this:

 	ieee80211_debugfs_key_update_default(sdata);
 
-	synchronize_net();
-	list_for_each_entry_safe(key, tmp, &keys, list)
-		__ieee80211_key_destroy(key, false);
+	if (!list_empty(&keys)) {
+		synchronize_net();
+		list_for_each_entry_safe(key, tmp, &keys, list)
+			__ieee80211_key_destroy(key, false);
+	}
 
 	WARN_ON_ONCE(sdata->crypto_tx_tailroom_needed_cnt ||
 		     sdata->crypto_tx_tailroom_pending_dec);


johannes

Re: [PATCH 1/3] mac80211: clean up key freeing a bit

[Johannes Berg]

Applied all.

johannes