From db4d1169d0b893bfb7923b6526748fe2c5a7373f Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Mon, 25 Feb 2008 16:27:45 +0100 Subject: mac80211: split ieee80211_key_alloc/free In order to RCU-ify sta_info, we need to be able to allocate a key without linking it to an sdata/sta structure (because allocation cannot be done in an rcu critical section). This patch splits up ieee80211_key_alloc() and updates all users appropriately. While at it, this patch fixes a number of race conditions such as finally making key replacement atomic, unfortunately at the expense of more complex code. Note that this patch documents /existing/ bugs with sta info and key interaction, there is currently a race condition when a sta info is freed without holding the RTNL. This will finally be fixed by a followup patch. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 156 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 113 insertions(+), 43 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index ed57fb8e82fc..60aaaf471544 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -13,6 +13,7 @@ #include #include #include +#include #include #include "ieee80211_i.h" #include "debugfs_key.h" @@ -34,6 +35,10 @@ * * All operations here are called under RTNL so no extra locking is * required. + * + * NOTE: This code requires that sta info *destruction* is done under + * RTNL, otherwise it can try to access already freed STA structs + * when a STA key is being freed. */ static const u8 bcast_addr[ETH_ALEN] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF }; @@ -84,16 +89,25 @@ static void ieee80211_key_enable_hw_accel(struct ieee80211_key *key) key->conf.keyidx, print_mac(mac, addr), ret); } +static void ieee80211_key_mark_hw_accel_off(struct ieee80211_key *key) +{ + if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) { + key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE; + key->flags |= KEY_FLAG_REMOVE_FROM_HARDWARE; + } +} + static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key) { const u8 *addr; int ret; DECLARE_MAC_BUF(mac); - if (!key->local->ops->set_key) + if (!key || !key->local->ops->set_key) return; - if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) && + !(key->flags & KEY_FLAG_REMOVE_FROM_HARDWARE)) return; addr = get_mac_for_key(key); @@ -108,12 +122,11 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key) wiphy_name(key->local->hw.wiphy), key->conf.keyidx, print_mac(mac, addr), ret); - key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE; + key->flags &= ~(KEY_FLAG_UPLOADED_TO_HARDWARE | + KEY_FLAG_REMOVE_FROM_HARDWARE); } -struct ieee80211_key *ieee80211_key_alloc(struct ieee80211_sub_if_data *sdata, - struct sta_info *sta, - enum ieee80211_key_alg alg, +struct ieee80211_key *ieee80211_key_alloc(enum ieee80211_key_alg alg, int idx, size_t key_len, const u8 *key_data) @@ -138,10 +151,6 @@ struct ieee80211_key *ieee80211_key_alloc(struct ieee80211_sub_if_data *sdata, key->conf.keylen = key_len; memcpy(key->conf.key, key_data, key_len); - key->local = sdata->local; - key->sdata = sdata; - key->sta = sta; - if (alg == ALG_CCMP) { /* * Initialize AES key state here as an optimization so that @@ -154,13 +163,62 @@ struct ieee80211_key *ieee80211_key_alloc(struct ieee80211_sub_if_data *sdata, } } - ieee80211_debugfs_key_add(key->local, key); + return key; +} - /* remove key first */ - if (sta) - ieee80211_key_free(sta->key); - else - ieee80211_key_free(sdata->keys[idx]); +static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, + struct sta_info *sta, + struct ieee80211_key *key, + struct ieee80211_key *new) +{ + int idx, defkey; + + if (sta) { + rcu_assign_pointer(sta->key, new); + } else { + WARN_ON(new && key && new->conf.keyidx != key->conf.keyidx); + + if (key) + idx = key->conf.keyidx; + else + idx = new->conf.keyidx; + + defkey = key && sdata->default_key == key; + + if (defkey && !new) + ieee80211_set_default_key(sdata, -1); + + rcu_assign_pointer(sdata->keys[idx], new); + + if (defkey && new) + ieee80211_set_default_key(sdata, new->conf.keyidx); + } + + if (key) { + ieee80211_key_mark_hw_accel_off(key); + list_del(&key->list); + } +} + +void ieee80211_key_link(struct ieee80211_key *key, + struct ieee80211_sub_if_data *sdata, + struct sta_info *sta) +{ + struct ieee80211_key *old_key; + int idx; + + ASSERT_RTNL(); + might_sleep(); + + BUG_ON(!sdata); + BUG_ON(!key); + + idx = key->conf.keyidx; + key->local = sdata->local; + key->sdata = sdata; + key->sta = sta; + + ieee80211_debugfs_key_add(key->local, key); if (sta) { ieee80211_debugfs_key_sta_link(key, sta); @@ -186,50 +244,53 @@ struct ieee80211_key *ieee80211_key_alloc(struct ieee80211_sub_if_data *sdata, } } - /* enable hwaccel if appropriate */ - if (netif_running(key->sdata->dev)) - ieee80211_key_enable_hw_accel(key); - if (sta) - rcu_assign_pointer(sta->key, key); + old_key = sta->key; else - rcu_assign_pointer(sdata->keys[idx], key); + old_key = sdata->keys[idx]; + + __ieee80211_key_replace(sdata, sta, old_key, key); list_add(&key->list, &sdata->key_list); - return key; + synchronize_rcu(); + + ieee80211_key_free(old_key); + ieee80211_key_enable_hw_accel(key); } void ieee80211_key_free(struct ieee80211_key *key) { + ASSERT_RTNL(); + might_sleep(); + if (!key) return; - if (key->sta) { - rcu_assign_pointer(key->sta->key, NULL); - } else { - if (key->sdata->default_key == key) - ieee80211_set_default_key(key->sdata, -1); - if (key->conf.keyidx >= 0 && - key->conf.keyidx < NUM_DEFAULT_KEYS) - rcu_assign_pointer(key->sdata->keys[key->conf.keyidx], - NULL); - else - WARN_ON(1); - } + if (key->sdata) { + /* + * Replace key with nothingness. + * + * Because other code may have key reference (RCU protected) + * right now, we then wait for a grace period before freeing + * it. + */ + __ieee80211_key_replace(key->sdata, key->sta, key, NULL); - /* wait for all key users to complete */ - synchronize_rcu(); + synchronize_rcu(); - /* remove from hwaccel if appropriate */ - ieee80211_key_disable_hw_accel(key); + /* + * Remove from hwaccel if appropriate, this will + * only happen when the key is actually unlinked, + * it will already be done when the key was replaced. + */ + ieee80211_key_disable_hw_accel(key); + } if (key->conf.alg == ALG_CCMP) ieee80211_aes_key_free(key->u.ccmp.tfm); ieee80211_debugfs_key_remove(key); - list_del(&key->list); - kfree(key); } @@ -253,6 +314,10 @@ void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx) void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata) { struct ieee80211_key *key, *tmp; + LIST_HEAD(tmp_list); + + ASSERT_RTNL(); + might_sleep(); list_for_each_entry_safe(key, tmp, &sdata->key_list, list) ieee80211_key_free(key); @@ -262,8 +327,10 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) { struct ieee80211_key *key; - WARN_ON(!netif_running(sdata->dev)); - if (!netif_running(sdata->dev)) + ASSERT_RTNL(); + might_sleep(); + + if (WARN_ON(!netif_running(sdata->dev))) return; list_for_each_entry(key, &sdata->key_list, list) @@ -274,6 +341,9 @@ void ieee80211_disable_keys(struct ieee80211_sub_if_data *sdata) { struct ieee80211_key *key; + ASSERT_RTNL(); + might_sleep(); + list_for_each_entry(key, &sdata->key_list, list) ieee80211_key_disable_hw_accel(key); } -- cgit v1.2.3 From e4861829072c61883114c64a3af61f305a789ff0 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 27 Feb 2008 13:39:00 +0100 Subject: mac80211: fix key replacing, hw accel Even though I thought about it a lot and had also tested it, some of my recent changes in the key code broke replacing keys, making the kernel oops because a key is removed from a list while not on it. This patch fixes that using the list as an indication whether or not the key is on it (an empty list means it's not on any list.) Also, this patch fixes hw accel enabling, the check for not doing hw accel when the interface is down was lost and is restored by this. Additionally, move adding the key to the list into the function __ieee80211_key_replace() for more consistency. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index 60aaaf471544..eac9c59dbc4d 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -150,6 +150,7 @@ struct ieee80211_key *ieee80211_key_alloc(enum ieee80211_key_alg alg, key->conf.keyidx = idx; key->conf.keylen = key_len; memcpy(key->conf.key, key_data, key_len); + INIT_LIST_HEAD(&key->list); if (alg == ALG_CCMP) { /* @@ -189,6 +190,8 @@ static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, ieee80211_set_default_key(sdata, -1); rcu_assign_pointer(sdata->keys[idx], new); + if (new) + list_add(&new->list, &sdata->key_list); if (defkey && new) ieee80211_set_default_key(sdata, new->conf.keyidx); @@ -196,7 +199,11 @@ static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, if (key) { ieee80211_key_mark_hw_accel_off(key); - list_del(&key->list); + /* + * We'll use an empty list to indicate that the key + * has already been removed. + */ + list_del_init(&key->list); } } @@ -251,12 +258,13 @@ void ieee80211_key_link(struct ieee80211_key *key, __ieee80211_key_replace(sdata, sta, old_key, key); - list_add(&key->list, &sdata->key_list); - - synchronize_rcu(); + if (old_key) { + synchronize_rcu(); + ieee80211_key_free(old_key); + } - ieee80211_key_free(old_key); - ieee80211_key_enable_hw_accel(key); + if (netif_running(sdata->dev)) + ieee80211_key_enable_hw_accel(key); } void ieee80211_key_free(struct ieee80211_key *key) @@ -274,8 +282,13 @@ void ieee80211_key_free(struct ieee80211_key *key) * Because other code may have key reference (RCU protected) * right now, we then wait for a grace period before freeing * it. + * An empty list indicates it was never added to the key list + * or has been removed already. It may, however, still be in + * hardware for acceleration. */ - __ieee80211_key_replace(key->sdata, key->sta, key, NULL); + if (!list_empty(&key->list)) + __ieee80211_key_replace(key->sdata, key->sta, + key, NULL); synchronize_rcu(); -- cgit v1.2.3 From d0709a65181beb787ef3f58cfe45536a2bb254c8 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Mon, 25 Feb 2008 16:27:46 +0100 Subject: mac80211: RCU-ify STA info structure access This makes access to the STA hash table/list use RCU to protect against freeing of items. However, it's not a true RCU, the copy step is missing: whenever somebody changes a STA item it is simply updated. This is an existing race condition that is now somewhat understandable. This patch also fixes the race key freeing vs. STA destruction by making sure that sta_info_destroy() is always called under RTNL and frees the key. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index eac9c59dbc4d..df0c04cedbe4 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -240,14 +240,17 @@ void ieee80211_key_link(struct ieee80211_key *key, if (sdata->vif.type == IEEE80211_IF_TYPE_STA) { struct sta_info *ap; + rcu_read_lock(); + /* same here, the AP could be using QoS */ ap = sta_info_get(key->local, key->sdata->u.sta.bssid); if (ap) { if (ap->flags & WLAN_STA_WME) key->conf.flags |= IEEE80211_KEY_FLAG_WMM_STA; - sta_info_put(ap); } + + rcu_read_unlock(); } } @@ -290,6 +293,9 @@ void ieee80211_key_free(struct ieee80211_key *key) __ieee80211_key_replace(key->sdata, key->sta, key, NULL); + /* + * Do NOT remove this without looking at sta_info_destroy() + */ synchronize_rcu(); /* -- cgit v1.2.3 From dbbea6713d6096cd1c411cb453a6b71292c78b33 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Tue, 26 Feb 2008 14:34:06 +0100 Subject: mac80211: add documentation book Quite a while ago I started this book. The required kernel-doc patches have since gone into the tree so it is now possible to build the book in mainline. The actual documentation is still rather incomplete and not all things are linked into the book, but this enables us to edit the documentation collaboratively, hopefully driver authors can add documentation based on their experience with mac80211. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index df0c04cedbe4..166d0f00d135 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -20,8 +20,8 @@ #include "aes_ccm.h" -/* - * Key handling basics +/** + * DOC: Key handling basics * * Key handling in mac80211 is done based on per-interface (sub_if_data) * keys and per-station keys. Since each station belongs to an interface, -- cgit v1.2.3 From 96c46546e28282a743b97f26e94c7565350898b7 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Sat, 1 Mar 2008 19:32:18 +0100 Subject: mac80211: always insert key into list Today I hit one of my new WARN_ONs in the mac80211 code because a key wasn't being freed correctly. After wondering for a while I finally tracked it to the fact that STA keys aren't added to the per-sdata key list correctly, they are supposed to always be on that list, not just for default keys. This patch fixes that. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index 166d0f00d135..f91fb4092652 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -174,6 +174,9 @@ static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, { int idx, defkey; + if (new) + list_add(&new->list, &sdata->key_list); + if (sta) { rcu_assign_pointer(sta->key, new); } else { @@ -190,9 +193,6 @@ static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, ieee80211_set_default_key(sdata, -1); rcu_assign_pointer(sdata->keys[idx], new); - if (new) - list_add(&new->list, &sdata->key_list); - if (defkey && new) ieee80211_set_default_key(sdata, new->conf.keyidx); } -- cgit v1.2.3 From dc6676b7f2c2072ec05254aaca32e99f87a8a417 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Mon, 31 Mar 2008 19:23:03 +0200 Subject: mac80211: sta_info_flush() fixes When the IBSS code tries to flush the STA list, it does so in an atomic context. Flushing isn't safe there, however, and requires the RTNL, so we need to defer it to a workqueue. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index f91fb4092652..5df9e0cc009f 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -73,6 +73,15 @@ static void ieee80211_key_enable_hw_accel(struct ieee80211_key *key) if (!key->local->ops->set_key) return; + /* + * This makes sure that all pending flushes have + * actually completed prior to uploading new key + * material to the hardware. That is necessary to + * avoid races between flushing STAs and adding + * new keys for them. + */ + __ieee80211_run_pending_flush(key->local); + addr = get_mac_for_key(key); ret = key->local->ops->set_key(local_to_hw(key->local), SET_KEY, -- cgit v1.2.3 From 3b96766f0e643f52ae19e134664df6730c737e87 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Tue, 8 Apr 2008 17:56:52 +0200 Subject: 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 Signed-off-by: John W. Linville --- net/mac80211/key.c | 408 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 264 insertions(+), 144 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index 5df9e0cc009f..711e36e54ff8 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -2,7 +2,7 @@ * Copyright 2002-2005, Instant802 Networks, Inc. * Copyright 2005-2006, Devicescape Software, Inc. * Copyright 2006-2007 Jiri Benc - * Copyright 2007 Johannes Berg + * Copyright 2007-2008 Johannes Berg * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License version 2 as @@ -33,17 +33,78 @@ * There is currently no way of knowing this except by looking into * debugfs. * - * All operations here are called under RTNL so no extra locking is - * required. + * All key operations are protected internally so you can call them at + * any time. * - * NOTE: This code requires that sta info *destruction* is done under - * RTNL, otherwise it can try to access already freed STA structs - * when a STA key is being freed. + * Within mac80211, key references are, just as STA structure references, + * protected by RCU. Note, however, that some things are unprotected, + * namely the key->sta dereferences within the hardware acceleration + * functions. This means that sta_info_destroy() must flush the key todo + * list. + * + * All the direct key list manipulation functions must not sleep because + * they can operate on STA info structs that are protected by RCU. */ static const u8 bcast_addr[ETH_ALEN] = { 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF }; static const u8 zero_addr[ETH_ALEN]; +/* key mutex: used to synchronise todo runners */ +static DEFINE_MUTEX(key_mutex); +static DEFINE_SPINLOCK(todo_lock); +static LIST_HEAD(todo_list); + +static void key_todo(struct work_struct *work) +{ + ieee80211_key_todo(); +} + +static DECLARE_WORK(todo_work, key_todo); + +/** + * add_todo - add todo item for a key + * + * @key: key to add to do item for + * @flag: todo flag(s) + */ +static void add_todo(struct ieee80211_key *key, u32 flag) +{ + if (!key) + return; + + spin_lock(&todo_lock); + key->flags |= flag; + /* only add if not already added */ + if (list_empty(&key->todo)) + list_add(&key->todo, &todo_list); + schedule_work(&todo_work); + spin_unlock(&todo_lock); +} + +/** + * ieee80211_key_lock - lock the mac80211 key operation lock + * + * This locks the (global) mac80211 key operation lock, all + * key operations must be done under this lock. + */ +static void ieee80211_key_lock(void) +{ + mutex_lock(&key_mutex); +} + +/** + * ieee80211_key_unlock - unlock the mac80211 key operation lock + */ +static void ieee80211_key_unlock(void) +{ + mutex_unlock(&key_mutex); +} + +static void assert_key_lock(void) +{ + WARN_ON(!mutex_is_locked(&key_mutex)); +} + static const u8 *get_mac_for_key(struct ieee80211_key *key) { const u8 *addr = bcast_addr; @@ -70,26 +131,23 @@ static void ieee80211_key_enable_hw_accel(struct ieee80211_key *key) int ret; DECLARE_MAC_BUF(mac); + assert_key_lock(); + might_sleep(); + if (!key->local->ops->set_key) return; - /* - * This makes sure that all pending flushes have - * actually completed prior to uploading new key - * material to the hardware. That is necessary to - * avoid races between flushing STAs and adding - * new keys for them. - */ - __ieee80211_run_pending_flush(key->local); - addr = get_mac_for_key(key); ret = key->local->ops->set_key(local_to_hw(key->local), SET_KEY, key->sdata->dev->dev_addr, addr, &key->conf); - if (!ret) + if (!ret) { + spin_lock(&todo_lock); key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE; + spin_unlock(&todo_lock); + } if (ret && ret != -ENOSPC && ret != -EOPNOTSUPP) printk(KERN_ERR "mac80211-%s: failed to set key " @@ -98,26 +156,24 @@ static void ieee80211_key_enable_hw_accel(struct ieee80211_key *key) key->conf.keyidx, print_mac(mac, addr), ret); } -static void ieee80211_key_mark_hw_accel_off(struct ieee80211_key *key) -{ - if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) { - key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE; - key->flags |= KEY_FLAG_REMOVE_FROM_HARDWARE; - } -} - static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key) { const u8 *addr; int ret; DECLARE_MAC_BUF(mac); + assert_key_lock(); + might_sleep(); + if (!key || !key->local->ops->set_key) return; - if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) && - !(key->flags & KEY_FLAG_REMOVE_FROM_HARDWARE)) + spin_lock(&todo_lock); + if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) { + spin_unlock(&todo_lock); return; + } + spin_unlock(&todo_lock); addr = get_mac_for_key(key); @@ -131,8 +187,72 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key) wiphy_name(key->local->hw.wiphy), key->conf.keyidx, print_mac(mac, addr), ret); - key->flags &= ~(KEY_FLAG_UPLOADED_TO_HARDWARE | - KEY_FLAG_REMOVE_FROM_HARDWARE); + spin_lock(&todo_lock); + key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE; + spin_unlock(&todo_lock); +} + +static void __ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, + int idx) +{ + struct ieee80211_key *key = NULL; + + if (idx >= 0 && idx < NUM_DEFAULT_KEYS) + key = sdata->keys[idx]; + + rcu_assign_pointer(sdata->default_key, key); + + if (key) + add_todo(key, KEY_FLAG_TODO_DEFKEY); +} + +void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx) +{ + unsigned long flags; + + spin_lock_irqsave(&sdata->local->sta_lock, flags); + __ieee80211_set_default_key(sdata, idx); + spin_unlock_irqrestore(&sdata->local->sta_lock, flags); +} + + +static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, + struct sta_info *sta, + struct ieee80211_key *old, + struct ieee80211_key *new) +{ + int idx, defkey; + + if (new) + list_add(&new->list, &sdata->key_list); + + if (sta) { + rcu_assign_pointer(sta->key, new); + } else { + WARN_ON(new && old && new->conf.keyidx != old->conf.keyidx); + + if (old) + idx = old->conf.keyidx; + else + idx = new->conf.keyidx; + + defkey = old && sdata->default_key == old; + + if (defkey && !new) + __ieee80211_set_default_key(sdata, -1); + + rcu_assign_pointer(sdata->keys[idx], new); + if (defkey && new) + __ieee80211_set_default_key(sdata, new->conf.keyidx); + } + + if (old) { + /* + * We'll use an empty list to indicate that the key + * has already been removed. + */ + list_del_init(&old->list); + } } struct ieee80211_key *ieee80211_key_alloc(enum ieee80211_key_alg alg, @@ -160,6 +280,7 @@ struct ieee80211_key *ieee80211_key_alloc(enum ieee80211_key_alg alg, key->conf.keylen = key_len; memcpy(key->conf.key, key_data, key_len); INIT_LIST_HEAD(&key->list); + INIT_LIST_HEAD(&key->todo); if (alg == ALG_CCMP) { /* @@ -168,7 +289,7 @@ struct ieee80211_key *ieee80211_key_alloc(enum ieee80211_key_alg alg, */ key->u.ccmp.tfm = ieee80211_aes_key_setup_encrypt(key_data); if (!key->u.ccmp.tfm) { - ieee80211_key_free(key); + kfree(key); return NULL; } } @@ -176,56 +297,14 @@ struct ieee80211_key *ieee80211_key_alloc(enum ieee80211_key_alg alg, return key; } -static void __ieee80211_key_replace(struct ieee80211_sub_if_data *sdata, - struct sta_info *sta, - struct ieee80211_key *key, - struct ieee80211_key *new) -{ - int idx, defkey; - - if (new) - list_add(&new->list, &sdata->key_list); - - if (sta) { - rcu_assign_pointer(sta->key, new); - } else { - WARN_ON(new && key && new->conf.keyidx != key->conf.keyidx); - - if (key) - idx = key->conf.keyidx; - else - idx = new->conf.keyidx; - - defkey = key && sdata->default_key == key; - - if (defkey && !new) - ieee80211_set_default_key(sdata, -1); - - rcu_assign_pointer(sdata->keys[idx], new); - if (defkey && new) - ieee80211_set_default_key(sdata, new->conf.keyidx); - } - - if (key) { - ieee80211_key_mark_hw_accel_off(key); - /* - * We'll use an empty list to indicate that the key - * has already been removed. - */ - list_del_init(&key->list); - } -} - void ieee80211_key_link(struct ieee80211_key *key, struct ieee80211_sub_if_data *sdata, struct sta_info *sta) { struct ieee80211_key *old_key; + unsigned long flags; int idx; - ASSERT_RTNL(); - might_sleep(); - BUG_ON(!sdata); BUG_ON(!key); @@ -234,11 +313,7 @@ void ieee80211_key_link(struct ieee80211_key *key, key->sdata = sdata; key->sta = sta; - ieee80211_debugfs_key_add(key->local, key); - if (sta) { - ieee80211_debugfs_key_sta_link(key, sta); - /* * some hardware cannot handle TKIP with QoS, so * we indicate whether QoS could be in use. @@ -249,7 +324,10 @@ void ieee80211_key_link(struct ieee80211_key *key, if (sdata->vif.type == IEEE80211_IF_TYPE_STA) { struct sta_info *ap; - rcu_read_lock(); + /* + * We're getting a sta pointer in, + * so must be under RCU read lock. + */ /* same here, the AP could be using QoS */ ap = sta_info_get(key->local, key->sdata->u.sta.bssid); @@ -258,11 +336,11 @@ void ieee80211_key_link(struct ieee80211_key *key, key->conf.flags |= IEEE80211_KEY_FLAG_WMM_STA; } - - rcu_read_unlock(); } } + spin_lock_irqsave(&sdata->local->sta_lock, flags); + if (sta) old_key = sta->key; else @@ -270,108 +348,150 @@ void ieee80211_key_link(struct ieee80211_key *key, __ieee80211_key_replace(sdata, sta, old_key, key); - if (old_key) { - synchronize_rcu(); - ieee80211_key_free(old_key); - } + spin_unlock_irqrestore(&sdata->local->sta_lock, flags); + + /* free old key later */ + add_todo(old_key, KEY_FLAG_TODO_DELETE); + add_todo(key, KEY_FLAG_TODO_ADD_DEBUGFS); if (netif_running(sdata->dev)) - ieee80211_key_enable_hw_accel(key); + add_todo(key, KEY_FLAG_TODO_HWACCEL); } void ieee80211_key_free(struct ieee80211_key *key) { - ASSERT_RTNL(); - might_sleep(); + unsigned long flags; if (!key) return; + /* + * Replace key with nothingness if it was ever used. + */ if (key->sdata) { - /* - * Replace key with nothingness. - * - * Because other code may have key reference (RCU protected) - * right now, we then wait for a grace period before freeing - * it. - * An empty list indicates it was never added to the key list - * or has been removed already. It may, however, still be in - * hardware for acceleration. - */ - if (!list_empty(&key->list)) - __ieee80211_key_replace(key->sdata, key->sta, - key, NULL); + spin_lock_irqsave(&key->sdata->local->sta_lock, flags); + __ieee80211_key_replace(key->sdata, key->sta, + key, NULL); + spin_unlock_irqrestore(&key->sdata->local->sta_lock, flags); + } - /* - * Do NOT remove this without looking at sta_info_destroy() - */ - synchronize_rcu(); + add_todo(key, KEY_FLAG_TODO_DELETE); +} - /* - * Remove from hwaccel if appropriate, this will - * only happen when the key is actually unlinked, - * it will already be done when the key was replaced. - */ - ieee80211_key_disable_hw_accel(key); - } +void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) +{ + struct ieee80211_key *key; - if (key->conf.alg == ALG_CCMP) - ieee80211_aes_key_free(key->u.ccmp.tfm); - ieee80211_debugfs_key_remove(key); + might_sleep(); - kfree(key); + if (WARN_ON(!netif_running(sdata->dev))) + return; + + ieee80211_key_lock(); + + list_for_each_entry(key, &sdata->key_list, list) + ieee80211_key_enable_hw_accel(key); + + ieee80211_key_unlock(); } -void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx) +void ieee80211_disable_keys(struct ieee80211_sub_if_data *sdata) { - struct ieee80211_key *key = NULL; + struct ieee80211_key *key; - if (idx >= 0 && idx < NUM_DEFAULT_KEYS) - key = sdata->keys[idx]; + might_sleep(); - if (sdata->default_key != key) { - ieee80211_debugfs_key_remove_default(sdata); + ieee80211_key_lock(); - rcu_assign_pointer(sdata->default_key, key); + list_for_each_entry(key, &sdata->key_list, list) + ieee80211_key_disable_hw_accel(key); - if (sdata->default_key) - ieee80211_debugfs_key_add_default(sdata); - } + ieee80211_key_unlock(); } -void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata) +static void __ieee80211_key_free(struct ieee80211_key *key) { - struct ieee80211_key *key, *tmp; - LIST_HEAD(tmp_list); + if (!key) + return; - ASSERT_RTNL(); - might_sleep(); + ieee80211_key_disable_hw_accel(key); - list_for_each_entry_safe(key, tmp, &sdata->key_list, list) - ieee80211_key_free(key); + if (key->conf.alg == ALG_CCMP) + ieee80211_aes_key_free(key->u.ccmp.tfm); + ieee80211_debugfs_key_remove(key); + + kfree(key); } -void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) +static void __ieee80211_key_todo(void) { struct ieee80211_key *key; + bool work_done; + u32 todoflags; - ASSERT_RTNL(); - might_sleep(); + /* + * NB: sta_info_destroy relies on this! + */ + synchronize_rcu(); + + spin_lock(&todo_lock); + while (!list_empty(&todo_list)) { + key = list_first_entry(&todo_list, struct ieee80211_key, todo); + list_del_init(&key->todo); + todoflags = key->flags & (KEY_FLAG_TODO_ADD_DEBUGFS | + KEY_FLAG_TODO_DEFKEY | + KEY_FLAG_TODO_HWACCEL | + KEY_FLAG_TODO_DELETE); + key->flags &= ~todoflags; + spin_unlock(&todo_lock); + + work_done = false; + + if (todoflags & KEY_FLAG_TODO_ADD_DEBUGFS) { + ieee80211_debugfs_key_add(key); + work_done = true; + } + if (todoflags & KEY_FLAG_TODO_DEFKEY) { + ieee80211_debugfs_key_remove_default(key->sdata); + ieee80211_debugfs_key_add_default(key->sdata); + work_done = true; + } + if (todoflags & KEY_FLAG_TODO_HWACCEL) { + ieee80211_key_enable_hw_accel(key); + work_done = true; + } + if (todoflags & KEY_FLAG_TODO_DELETE) { + __ieee80211_key_free(key); + work_done = true; + } - if (WARN_ON(!netif_running(sdata->dev))) - return; + WARN_ON(!work_done); - list_for_each_entry(key, &sdata->key_list, list) - ieee80211_key_enable_hw_accel(key); + spin_lock(&todo_lock); + } + spin_unlock(&todo_lock); } -void ieee80211_disable_keys(struct ieee80211_sub_if_data *sdata) +void ieee80211_key_todo(void) { - struct ieee80211_key *key; + ieee80211_key_lock(); + __ieee80211_key_todo(); + ieee80211_key_unlock(); +} - ASSERT_RTNL(); - might_sleep(); +void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata) +{ + struct ieee80211_key *key, *tmp; + LIST_HEAD(tmp_list); - list_for_each_entry(key, &sdata->key_list, list) - ieee80211_key_disable_hw_accel(key); + ieee80211_key_lock(); + + ieee80211_debugfs_key_remove_default(sdata); + + list_for_each_entry_safe(key, tmp, &sdata->key_list, list) + ieee80211_key_free(key); + + __ieee80211_key_todo(); + + ieee80211_key_unlock(); } -- cgit v1.2.3 From 3a245766901a9dfdc3f53457a7954b369b50f281 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Wed, 9 Apr 2008 16:45:37 +0200 Subject: mac80211: fix key hwaccel race The previous key locking patch left a small race: it would be possible to add a key and take the interface down before the key todo is run so that hwaccel for that key is enabled on an interface that is down. Avoid this by running the todo list when an interface is brought up or down. This patch also fixes a small bug: before this change, a few functions used the key list without the lock that protects it. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 84 +++++++++++++++++++++++++++++++++--------------------- 1 file changed, 52 insertions(+), 32 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index 711e36e54ff8..acf8d0370a37 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -355,61 +355,74 @@ void ieee80211_key_link(struct ieee80211_key *key, add_todo(key, KEY_FLAG_TODO_ADD_DEBUGFS); if (netif_running(sdata->dev)) - add_todo(key, KEY_FLAG_TODO_HWACCEL); + add_todo(key, KEY_FLAG_TODO_HWACCEL_ADD); } -void ieee80211_key_free(struct ieee80211_key *key) +static void __ieee80211_key_free(struct ieee80211_key *key) { - unsigned long flags; - - if (!key) - return; - /* * Replace key with nothingness if it was ever used. */ - if (key->sdata) { - spin_lock_irqsave(&key->sdata->local->sta_lock, flags); + if (key->sdata) __ieee80211_key_replace(key->sdata, key->sta, key, NULL); - spin_unlock_irqrestore(&key->sdata->local->sta_lock, flags); - } add_todo(key, KEY_FLAG_TODO_DELETE); } -void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) +void ieee80211_key_free(struct ieee80211_key *key) { - struct ieee80211_key *key; - - might_sleep(); + unsigned long flags; - if (WARN_ON(!netif_running(sdata->dev))) + if (!key) return; - ieee80211_key_lock(); + spin_lock_irqsave(&key->sdata->local->sta_lock, flags); + __ieee80211_key_free(key); + spin_unlock_irqrestore(&key->sdata->local->sta_lock, flags); +} + +/* + * To be safe against concurrent manipulations of the list (which shouldn't + * actually happen) we need to hold the spinlock. But under the spinlock we + * can't actually do much, so we defer processing to the todo list. Then run + * the todo list to be sure the operation and possibly previously pending + * operations are completed. + */ +static void ieee80211_todo_for_each_key(struct ieee80211_sub_if_data *sdata, + u32 todo_flags) +{ + struct ieee80211_key *key; + unsigned long flags; + might_sleep(); + + spin_lock_irqsave(&sdata->local->sta_lock, flags); list_for_each_entry(key, &sdata->key_list, list) - ieee80211_key_enable_hw_accel(key); + add_todo(key, todo_flags); + spin_unlock_irqrestore(&sdata->local->sta_lock, flags); - ieee80211_key_unlock(); + ieee80211_key_todo(); } -void ieee80211_disable_keys(struct ieee80211_sub_if_data *sdata) +void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata) { - struct ieee80211_key *key; + ASSERT_RTNL(); - might_sleep(); + if (WARN_ON(!netif_running(sdata->dev))) + return; - ieee80211_key_lock(); + ieee80211_todo_for_each_key(sdata, KEY_FLAG_TODO_HWACCEL_ADD); +} - list_for_each_entry(key, &sdata->key_list, list) - ieee80211_key_disable_hw_accel(key); +void ieee80211_disable_keys(struct ieee80211_sub_if_data *sdata) +{ + ASSERT_RTNL(); - ieee80211_key_unlock(); + ieee80211_todo_for_each_key(sdata, KEY_FLAG_TODO_HWACCEL_REMOVE); } -static void __ieee80211_key_free(struct ieee80211_key *key) +static void __ieee80211_key_destroy(struct ieee80211_key *key) { if (!key) return; @@ -440,7 +453,8 @@ static void __ieee80211_key_todo(void) list_del_init(&key->todo); todoflags = key->flags & (KEY_FLAG_TODO_ADD_DEBUGFS | KEY_FLAG_TODO_DEFKEY | - KEY_FLAG_TODO_HWACCEL | + KEY_FLAG_TODO_HWACCEL_ADD | + KEY_FLAG_TODO_HWACCEL_REMOVE | KEY_FLAG_TODO_DELETE); key->flags &= ~todoflags; spin_unlock(&todo_lock); @@ -456,12 +470,16 @@ static void __ieee80211_key_todo(void) ieee80211_debugfs_key_add_default(key->sdata); work_done = true; } - if (todoflags & KEY_FLAG_TODO_HWACCEL) { + if (todoflags & KEY_FLAG_TODO_HWACCEL_ADD) { ieee80211_key_enable_hw_accel(key); work_done = true; } + if (todoflags & KEY_FLAG_TODO_HWACCEL_REMOVE) { + ieee80211_key_disable_hw_accel(key); + work_done = true; + } if (todoflags & KEY_FLAG_TODO_DELETE) { - __ieee80211_key_free(key); + __ieee80211_key_destroy(key); work_done = true; } @@ -482,14 +500,16 @@ void ieee80211_key_todo(void) void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata) { struct ieee80211_key *key, *tmp; - LIST_HEAD(tmp_list); + unsigned long flags; ieee80211_key_lock(); ieee80211_debugfs_key_remove_default(sdata); + spin_lock_irqsave(&sdata->local->sta_lock, flags); list_for_each_entry_safe(key, tmp, &sdata->key_list, list) - ieee80211_key_free(key); + __ieee80211_key_free(key); + spin_unlock_irqrestore(&sdata->local->sta_lock, flags); __ieee80211_key_todo(); -- cgit v1.2.3 From b16bd15c379410f2aa47837aa4a0de5712856ad5 Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Fri, 11 Apr 2008 21:40:35 +0200 Subject: mac80211: fix spinlock recursion When STAs are expired, we need to hold the sta_lock. Using the same lock for keys too would then mean we'd need another key free function, and that'll just lead to confusion, so just use a new spinlock for all key lists. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index acf8d0370a37..b98711dcdc54 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -210,9 +210,9 @@ void ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata, int idx) { unsigned long flags; - spin_lock_irqsave(&sdata->local->sta_lock, flags); + spin_lock_irqsave(&sdata->local->key_lock, flags); __ieee80211_set_default_key(sdata, idx); - spin_unlock_irqrestore(&sdata->local->sta_lock, flags); + spin_unlock_irqrestore(&sdata->local->key_lock, flags); } @@ -339,7 +339,7 @@ void ieee80211_key_link(struct ieee80211_key *key, } } - spin_lock_irqsave(&sdata->local->sta_lock, flags); + spin_lock_irqsave(&sdata->local->key_lock, flags); if (sta) old_key = sta->key; @@ -348,7 +348,7 @@ void ieee80211_key_link(struct ieee80211_key *key, __ieee80211_key_replace(sdata, sta, old_key, key); - spin_unlock_irqrestore(&sdata->local->sta_lock, flags); + spin_unlock_irqrestore(&sdata->local->key_lock, flags); /* free old key later */ add_todo(old_key, KEY_FLAG_TODO_DELETE); @@ -377,9 +377,9 @@ void ieee80211_key_free(struct ieee80211_key *key) if (!key) return; - spin_lock_irqsave(&key->sdata->local->sta_lock, flags); + spin_lock_irqsave(&key->sdata->local->key_lock, flags); __ieee80211_key_free(key); - spin_unlock_irqrestore(&key->sdata->local->sta_lock, flags); + spin_unlock_irqrestore(&key->sdata->local->key_lock, flags); } /* @@ -397,10 +397,10 @@ static void ieee80211_todo_for_each_key(struct ieee80211_sub_if_data *sdata, might_sleep(); - spin_lock_irqsave(&sdata->local->sta_lock, flags); + spin_lock_irqsave(&sdata->local->key_lock, flags); list_for_each_entry(key, &sdata->key_list, list) add_todo(key, todo_flags); - spin_unlock_irqrestore(&sdata->local->sta_lock, flags); + spin_unlock_irqrestore(&sdata->local->key_lock, flags); ieee80211_key_todo(); } @@ -506,10 +506,10 @@ void ieee80211_free_keys(struct ieee80211_sub_if_data *sdata) ieee80211_debugfs_key_remove_default(sdata); - spin_lock_irqsave(&sdata->local->sta_lock, flags); + spin_lock_irqsave(&sdata->local->key_lock, flags); list_for_each_entry_safe(key, tmp, &sdata->key_list, list) __ieee80211_key_free(key); - spin_unlock_irqrestore(&sdata->local->sta_lock, flags); + spin_unlock_irqrestore(&sdata->local->key_lock, flags); __ieee80211_key_todo(); -- cgit v1.2.3 From 245cbe7a65f3e17999de276ea1c84538f3a7451e Mon Sep 17 00:00:00 2001 From: Johannes Berg Date: Sun, 13 Apr 2008 10:43:50 +0200 Subject: mac80211: fix key todo list order When we add multiple todo entries, we rely on them being executed mostly in the right order, especially when a key is being replaced. But when a default key is replaced, the todo list order will differ from the order when the key being replaced is not a default key, so problems will happen. Hence, just move each todo item to the end of the list when it is added so we can in the other code ensure that hw accel for a key will be disabled before it is enabled for the replacement. Signed-off-by: Johannes Berg Signed-off-by: John W. Linville --- net/mac80211/key.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'net/mac80211/key.c') diff --git a/net/mac80211/key.c b/net/mac80211/key.c index b98711dcdc54..150d66dbda9d 100644 --- a/net/mac80211/key.c +++ b/net/mac80211/key.c @@ -74,9 +74,12 @@ static void add_todo(struct ieee80211_key *key, u32 flag) spin_lock(&todo_lock); key->flags |= flag; - /* only add if not already added */ - if (list_empty(&key->todo)) - list_add(&key->todo, &todo_list); + /* + * Remove again if already on the list so that we move it to the end. + */ + if (!list_empty(&key->todo)) + list_del(&key->todo); + list_add_tail(&key->todo, &todo_list); schedule_work(&todo_work); spin_unlock(&todo_lock); } -- cgit v1.2.3