[PATCH] mac80211: fix todo lock

[PATCH] mac80211: fix todo lock

[Johannes Berg]

The key todo lock can be taken from different locks
that require it to be _bh to avoid lock inversion
due to (soft)irqs.

This should fix the two problems reported by Bob and
Gabor:
http://mid.gmane.org/20090619113049.GB18956@hash.localnet
http://mid.gmane.org/4A3FA376.8020307@openwrt.org

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Bob Copeland <me@bobcopeland.com>
Cc: Gabor Juhos <juhosg@openwrt.org>
---
 net/mac80211/key.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

--- wireless-testing.orig/net/mac80211/key.c	2009-06-23 11:22:42.000000000 +0200
+++ wireless-testing/net/mac80211/key.c	2009-06-23 11:36:17.000000000 +0200
@@ -73,7 +73,7 @@ static void add_todo(struct ieee80211_ke
 	if (!key)
 		return;
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	key->flags |= flag;
 	/*
 	 * Remove again if already on the list so that we move it to the end.
@@ -82,7 +82,7 @@ static void add_todo(struct ieee80211_ke
 		list_del(&key->todo);
 	list_add_tail(&key->todo, &todo_list);
 	schedule_work(&todo_work);
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 }
 
 /**
@@ -140,9 +140,9 @@ static void ieee80211_key_enable_hw_acce
 	ret = drv_set_key(key->local, SET_KEY, &sdata->vif, sta, &key->conf);
 
 	if (!ret) {
-		spin_lock(&todo_lock);
+		spin_lock_bh(&todo_lock);
 		key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
-		spin_unlock(&todo_lock);
+		spin_unlock_bh(&todo_lock);
 	}
 
 	if (ret && ret != -ENOSPC && ret != -EOPNOTSUPP)
@@ -164,12 +164,12 @@ static void ieee80211_key_disable_hw_acc
 	if (!key || !key->local->ops->set_key)
 		return;
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) {
 		spin_unlock(&todo_lock);
 		return;
 	}
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 
 	sta = get_sta_for_key(key);
 	sdata = key->sdata;
@@ -188,9 +188,9 @@ static void ieee80211_key_disable_hw_acc
 		       wiphy_name(key->local->hw.wiphy),
 		       key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 }
 
 static void __ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata,
@@ -547,7 +547,7 @@ static void __ieee80211_key_todo(void)
 	 */
 	synchronize_rcu();
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	while (!list_empty(&todo_list)) {
 		key = list_first_entry(&todo_list, struct ieee80211_key, todo);
 		list_del_init(&key->todo);
@@ -558,7 +558,7 @@ static void __ieee80211_key_todo(void)
 					  KEY_FLAG_TODO_HWACCEL_REMOVE |
 					  KEY_FLAG_TODO_DELETE);
 		key->flags &= ~todoflags;
-		spin_unlock(&todo_lock);
+		spin_unlock_bh(&todo_lock);
 
 		work_done = false;
 
@@ -591,9 +591,9 @@ static void __ieee80211_key_todo(void)
 
 		WARN_ON(!work_done);
 
-		spin_lock(&todo_lock);
+		spin_lock_bh(&todo_lock);
 	}
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 }
 
 void ieee80211_key_todo(void)

[PATCH v2] mac80211: fix todo lock

[Johannes Berg]

The key todo lock can be taken from different locks
that require it to be _bh to avoid lock inversion
due to (soft)irqs.

This should fix the two problems reported by Bob and
Gabor:
http://mid.gmane.org/20090619113049.GB18956@hash.localnet
http://mid.gmane.org/4A3FA376.8020307@openwrt.org

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Bob Copeland <me@bobcopeland.com>
Cc: Gabor Juhos <juhosg@openwrt.org>
---
 net/mac80211/key.c |   26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

--- wireless-testing.orig/net/mac80211/key.c	2009-06-23 11:48:12.000000000 +0200
+++ wireless-testing/net/mac80211/key.c	2009-06-23 16:06:07.000000000 +0200
@@ -67,6 +67,8 @@ static DECLARE_WORK(todo_work, key_todo)
  *
  * @key: key to add to do item for
  * @flag: todo flag(s)
+ *
+ * Must be called with IRQs or softirqs disabled.
  */
 static void add_todo(struct ieee80211_key *key, u32 flag)
 {
@@ -140,9 +142,9 @@ static void ieee80211_key_enable_hw_acce
 	ret = drv_set_key(key->local, SET_KEY, &sdata->vif, sta, &key->conf);
 
 	if (!ret) {
-		spin_lock(&todo_lock);
+		spin_lock_bh(&todo_lock);
 		key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
-		spin_unlock(&todo_lock);
+		spin_unlock_bh(&todo_lock);
 	}
 
 	if (ret && ret != -ENOSPC && ret != -EOPNOTSUPP)
@@ -164,12 +166,12 @@ static void ieee80211_key_disable_hw_acc
 	if (!key || !key->local->ops->set_key)
 		return;
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) {
 		spin_unlock(&todo_lock);
 		return;
 	}
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 
 	sta = get_sta_for_key(key);
 	sdata = key->sdata;
@@ -188,9 +190,9 @@ static void ieee80211_key_disable_hw_acc
 		       wiphy_name(key->local->hw.wiphy),
 		       key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 }
 
 static void __ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata,
@@ -437,14 +439,14 @@ void ieee80211_key_link(struct ieee80211
 
 	__ieee80211_key_replace(sdata, sta, old_key, key);
 
-	spin_unlock_irqrestore(&sdata->local->key_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))
 		add_todo(key, KEY_FLAG_TODO_HWACCEL_ADD);
+
+	spin_unlock_irqrestore(&sdata->local->key_lock, flags);
 }
 
 static void __ieee80211_key_free(struct ieee80211_key *key)
@@ -547,7 +549,7 @@ static void __ieee80211_key_todo(void)
 	 */
 	synchronize_rcu();
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	while (!list_empty(&todo_list)) {
 		key = list_first_entry(&todo_list, struct ieee80211_key, todo);
 		list_del_init(&key->todo);
@@ -558,7 +560,7 @@ static void __ieee80211_key_todo(void)
 					  KEY_FLAG_TODO_HWACCEL_REMOVE |
 					  KEY_FLAG_TODO_DELETE);
 		key->flags &= ~todoflags;
-		spin_unlock(&todo_lock);
+		spin_unlock_bh(&todo_lock);
 
 		work_done = false;
 
@@ -591,9 +593,9 @@ static void __ieee80211_key_todo(void)
 
 		WARN_ON(!work_done);
 
-		spin_lock(&todo_lock);
+		spin_lock_bh(&todo_lock);
 	}
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 }
 
 void ieee80211_key_todo(void)

Re: [PATCH v2] mac80211: fix todo lock

[Gabor Juhos]

Johannes Berg írta:
> The key todo lock can be taken from different locks
> that require it to be _bh to avoid lock inversion
> due to (soft)irqs.
> 
> This should fix the two problems reported by Bob and
> Gabor:
> http://mid.gmane.org/20090619113049.GB18956@hash.localnet
> http://mid.gmane.org/4A3FA376.8020307@openwrt.org
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> Cc: Bob Copeland <me@bobcopeland.com>
> Cc: Gabor Juhos <juhosg@openwrt.org>
> ---
>  net/mac80211/key.c |   26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)

I will try it ASAP. Thanks!

Gabor

Re: [PATCH v2] mac80211: fix todo lock

[Bob Copeland]

2009/6/23 Gabor Juhos <juhosg@openwrt.org>:
> Johannes Berg írta:
>> The key todo lock can be taken from different locks
>> that require it to be _bh to avoid lock inversion
>> due to (soft)irqs.
>>
>> This should fix the two problems reported by Bob and
>> Gabor:
>> http://mid.gmane.org/20090619113049.GB18956@hash.localnet
>> http://mid.gmane.org/4A3FA376.8020307@openwrt.org

Hmm, I applied the patch, started hostapd, hit ctrl-c to
kill it, and got instant lockup.

I'll try to capture the panic/whatever next.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH v2] mac80211: fix todo lock

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 734 bytes --]

On Tue, 2009-06-23 at 12:51 -0400, Bob Copeland wrote:
> 2009/6/23 Gabor Juhos <juhosg@openwrt.org>:
> > Johannes Berg írta:
> >> The key todo lock can be taken from different locks
> >> that require it to be _bh to avoid lock inversion
> >> due to (soft)irqs.
> >>
> >> This should fix the two problems reported by Bob and
> >> Gabor:
> >> http://mid.gmane.org/20090619113049.GB18956@hash.localnet
> >> http://mid.gmane.org/4A3FA376.8020307@openwrt.org
> 
> Hmm, I applied the patch, started hostapd, hit ctrl-c to
> kill it, and got instant lockup.

Strange, but I can't imagine it's due to this patch since it just
changes locking in one place, which must be harmless (and I also tested
this patch).

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2] mac80211: fix todo lock

[Bob Copeland]

On Tue, Jun 23, 2009 at 06:55:14PM +0200, Johannes Berg wrote:
> Strange, but I can't imagine it's due to this patch since it just
> changes locking in one place, which must be harmless (and I also tested
> this patch).

Here's the trace, after the initial warning it proceeded to continually
dump stack with the "workqueue leaked lock" thing.

[  108.984131] ------------[ cut here ]------------
[  108.984200] WARNING: at kernel/mutex.c:312 __mutex_unlock_slowpath+0x47/0xfe()
[  108.984275] Hardware name: MacBook1,1
[  108.984313] Modules linked in: aes_i586 aes_generic ipt_MASQUERADE iptable_nat nf_nat i915 drm af_packet ipt_REJECT xt_tcpudp nf_conntrack_ipv4 nf_defrag_ipv4 xt_state iptable_filter ip_tables x_tables acpi_cpufreq binfmt_misc dm_mirror dm_region_hash dm_log dm_multipath dm_mod kvm_intel kvm uinput arc4 ecb ath5k hid_apple mac80211 snd_hda_codec_idt usbhid appletouch snd_hda_intel snd_hda_codec ath cfg80211 snd_pcm snd_timer snd sg ohci1394 snd_page_alloc uhci_hcd joydev rfkill ehci_hcd sky2 battery ieee1394 processor video thermal button backlight ac output sr_mod applesmc evdev cdrom input_polldev unix [last unloaded: scsi_wait_scan]
[  108.984944] Pid: 9, comm: events/0 Not tainted 2.6.30-wl #140
[  108.984972] Call Trace:
[  108.984991]  [<c0124238>] warn_slowpath_common+0x6a/0x81
[  108.985035]  [<c036ebdc>] ? __mutex_unlock_slowpath+0x47/0xfe
[  108.985098]  [<c0124261>] warn_slowpath_null+0x12/0x15
[  108.985174]  [<c036ebdc>] __mutex_unlock_slowpath+0x47/0xfe
[  108.985221]  [<c036eca0>] mutex_unlock+0xd/0xf
[  108.985269]  [<fd65c1d8>] ieee80211_key_todo+0x23/0x25 [mac80211]
[  108.985343]  [<fd65c1e7>] key_todo+0xd/0xf [mac80211]
[  108.985425]  [<c0132267>] worker_thread+0x1aa/0x287
[  108.985474]  [<c0132225>] ? worker_thread+0x168/0x287
[  108.985518]  [<fd65c1da>] ? key_todo+0x0/0xf [mac80211]
[  108.985581]  [<c0135acc>] ? autoremove_wake_function+0x0/0x34
[  108.985658]  [<c01320bd>] ? worker_thread+0x0/0x287
[  108.985750]  [<c01357a7>] kthread+0x4b/0x6f
[  108.985776]  [<c013575c>] ? kthread+0x0/0x6f
[  108.985803]  [<c0103467>] kernel_thread_helper+0x7/0x10
[  108.985833] ---[ end trace cbbb1a87dac383bd ]---
[  108.985862] BUG: workqueue leaked lock or atomic: events/0/0x00000100/9
[  108.985896]     last function: key_todo+0x0/0xf [mac80211]
[  108.985946] INFO: lockdep is turned off.
[  108.985971] Pid: 9, comm: events/0 Tainted: G        W  2.6.30-wl #140
[  108.986014] Call Trace:
[  108.987186]  [<c013230c>] worker_thread+0x24f/0x287
[  108.987206]  [<fd65c1da>] ? key_todo+0x0/0xf [mac80211]
[  108.987212]  [<c0135acc>] ? autoremove_wake_function+0x0/0x34
[  108.987216]  [<c01320bd>] ? worker_thread+0x0/0x287
[  108.987219]  [<c01357a7>] kthread+0x4b/0x6f



-- 
Bob Copeland %% www.bobcopeland.com

[PATCH v3] mac80211: fix todo lock

[Johannes Berg]

Subject: mac80211: fix todo lock

The key todo lock can be taken from different locks
that require it to be _bh to avoid lock inversion
due to (soft)irqs.

This should fix the two problems reported by Bob and
Gabor:
http://mid.gmane.org/20090619113049.GB18956@hash.localnet
http://mid.gmane.org/4A3FA376.8020307@openwrt.org

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Bob Copeland <me@bobcopeland.com>
Cc: Gabor Juhos <juhosg@openwrt.org>
---
 net/mac80211/key.c |   28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

--- wireless-testing.orig/net/mac80211/key.c	2009-06-23 11:48:12.000000000 +0200
+++ wireless-testing/net/mac80211/key.c	2009-06-23 20:56:02.000000000 +0200
@@ -67,6 +67,8 @@ static DECLARE_WORK(todo_work, key_todo)
  *
  * @key: key to add to do item for
  * @flag: todo flag(s)
+ *
+ * Must be called with IRQs or softirqs disabled.
  */
 static void add_todo(struct ieee80211_key *key, u32 flag)
 {
@@ -140,9 +142,9 @@ static void ieee80211_key_enable_hw_acce
 	ret = drv_set_key(key->local, SET_KEY, &sdata->vif, sta, &key->conf);
 
 	if (!ret) {
-		spin_lock(&todo_lock);
+		spin_lock_bh(&todo_lock);
 		key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
-		spin_unlock(&todo_lock);
+		spin_unlock_bh(&todo_lock);
 	}
 
 	if (ret && ret != -ENOSPC && ret != -EOPNOTSUPP)
@@ -164,12 +166,12 @@ static void ieee80211_key_disable_hw_acc
 	if (!key || !key->local->ops->set_key)
 		return;
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE)) {
-		spin_unlock(&todo_lock);
+		spin_unlock_bh(&todo_lock);
 		return;
 	}
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 
 	sta = get_sta_for_key(key);
 	sdata = key->sdata;
@@ -188,9 +190,9 @@ static void ieee80211_key_disable_hw_acc
 		       wiphy_name(key->local->hw.wiphy),
 		       key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 }
 
 static void __ieee80211_set_default_key(struct ieee80211_sub_if_data *sdata,
@@ -437,14 +439,14 @@ void ieee80211_key_link(struct ieee80211
 
 	__ieee80211_key_replace(sdata, sta, old_key, key);
 
-	spin_unlock_irqrestore(&sdata->local->key_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))
 		add_todo(key, KEY_FLAG_TODO_HWACCEL_ADD);
+
+	spin_unlock_irqrestore(&sdata->local->key_lock, flags);
 }
 
 static void __ieee80211_key_free(struct ieee80211_key *key)
@@ -547,7 +549,7 @@ static void __ieee80211_key_todo(void)
 	 */
 	synchronize_rcu();
 
-	spin_lock(&todo_lock);
+	spin_lock_bh(&todo_lock);
 	while (!list_empty(&todo_list)) {
 		key = list_first_entry(&todo_list, struct ieee80211_key, todo);
 		list_del_init(&key->todo);
@@ -558,7 +560,7 @@ static void __ieee80211_key_todo(void)
 					  KEY_FLAG_TODO_HWACCEL_REMOVE |
 					  KEY_FLAG_TODO_DELETE);
 		key->flags &= ~todoflags;
-		spin_unlock(&todo_lock);
+		spin_unlock_bh(&todo_lock);
 
 		work_done = false;
 
@@ -591,9 +593,9 @@ static void __ieee80211_key_todo(void)
 
 		WARN_ON(!work_done);
 
-		spin_lock(&todo_lock);
+		spin_lock_bh(&todo_lock);
 	}
-	spin_unlock(&todo_lock);
+	spin_unlock_bh(&todo_lock);
 }
 
 void ieee80211_key_todo(void)

Re: [PATCH v3] mac80211: fix todo lock

[Bob Copeland]

On Tue, Jun 23, 2009 at 08:56:47PM +0200, Johannes Berg wrote:
> Subject: mac80211: fix todo lock
> 
> The key todo lock can be taken from different locks
> that require it to be _bh to avoid lock inversion
> due to (soft)irqs.

v3 works great!  Feel free to add

Tested-by: Bob Copeland <me@bobcopeland.com>

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH v3] mac80211: fix todo lock

[Gabor Juhos]

Johannes Berg írta:
> Subject: mac80211: fix todo lock
> 
> The key todo lock can be taken from different locks
> that require it to be _bh to avoid lock inversion
> due to (soft)irqs.
> 
> This should fix the two problems reported by Bob and
> Gabor:
> http://mid.gmane.org/20090619113049.GB18956@hash.localnet
> http://mid.gmane.org/4A3FA376.8020307@openwrt.org
> 
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> Cc: Bob Copeland <me@bobcopeland.com>
> Cc: Gabor Juhos <juhosg@openwrt.org>
> ---
>  net/mac80211/key.c |   28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)

Yeah, this works. Thanks!

-Gabor