Crash in cfg80211_unlink_bss

DISCUSSION Ben Greear 2 participants 8 messages Oct 6, 2010 802.11 (including cfg80211/NL80211)

Share & tools

Ben Greear

author

2010-10-06 17:28 UTC #0 lore raw

This test scenario has 72 stations on ath5k trying to connect to a cisco AP that supposedly only supports 63 stations.

The 72 STA were created without ssid's configured, then we re-configured all 72 'at once' to give them the proper SSID (ifdown, ifup, iwconfig to set values).

The system crashed and rebooted.

Kernel is wireless-testing as of later yesterday, with a few additional patches mostly dealing with counters in /proc/net/wireless and some lockdep fixes pulled in from lkml etc.

We have seen this before, but this is the first good stacktrace we got.

Likely we can reproduce this if extra information is needed.

------------[ cut here ]------------
WARNING: at /home/greearb/git/linux.wireless-testing/lib/list_debug.c:48 list_del+0x24/0xab()
Hardware name: PDSM4+
list_del corruption, next is LIST_POISON1 (00100100)
Modules linked in: xt_CT iptable_raw ipt_addrtype xt_DSCP xt_dscp xt_string xt_owner xt_NFQUEUE xt_multiport xt_mark xt_ipra]
Pid: 27077, comm: kworker/u:1 Not tainted 2.6.36-rc6-wl+ #6
Call Trace:
 [<c04345cb>] warn_slowpath_common+0x65/0x7a
 [<c058f3c8>] ? list_del+0x24/0xab
 [<c0434644>] warn_slowpath_fmt+0x26/0x2a
 [<c058f3c8>] list_del+0x24/0xab
 [<fac52fed>] cfg80211_unlink_bss+0x4e/0x7d [cfg80211]
 [<fb8aa464>] ieee80211_work_work+0x562/0xcfa [mac80211]
 [<c0443cea>] ? process_one_work+0x145/0x295
 [<c0443d34>] process_one_work+0x18f/0x295
 [<c0443cea>] ? process_one_work+0x145/0x295
 [<fb8a9f02>] ? ieee80211_work_work+0x0/0xcfa [mac80211]
 [<c044535d>] worker_thread+0xf9/0x1b8
 [<c0445264>] ? worker_thread+0x0/0x1b8
 [<c0447ca7>] kthread+0x62/0x67
 [<c0447c45>] ? kthread+0x0/0x67
 [<c0403506>] kernel_thread_helper+0x6/0x1a
---[ end trace 378943e5dc829f28 ]---
BUG: unable to handle kernel paging request at 00200200
IP: [<c058f3f8>] list_del+0x54/0xab
*pde = 00000000
Oops: 0000 [#1] SMP DEBUG_PAGEALLOC
last sysfs file: /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.2/0000:03:01.0/ieee80211/phy0/macaddress
Modules linked in: xt_CT iptable_raw ipt_addrtype xt_DSCP xt_dscp xt_string xt_owner xt_NFQUEUE xt_multiport xt_mark xt_ipra]

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

maintainer

2010-10-06 18:04 UTC #1 lore raw

Quoted 5 lines · Ben Greear

This test scenario has 72 stations on ath5k trying to connect to a cisco AP that supposedly only supports 63 stations.
The 72 STA were created without ssid's configured, then we re-configured all 72 'at once' to give them the proper SSID (ifdown, ifup, iwconfig to set values).

Eww, iwconfig ;-)

Quoted 11 lines

The system crashed and rebooted.
Kernel is wireless-testing as of later yesterday, with a few additional patches mostly dealing with counters in /proc/net/wireless and some lockdep fixes pulled in from lkml etc.
We have seen this before, but this is the first good stacktrace we got.
Likely we can reproduce this if extra information is needed.
list_del corruption, next is LIST_POISON1 (00100100)

This one's interesting.

But anyway, now that I look at it in more detail, it seems fairly obvious. You should be able to trigger it with two stations, but it's probably harder ...

I need to analyse the refcounting here again and in more detail, but in the meantime can you try below patch?

johannes

---
 net/wireless/scan.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

net/wireless/scan.c +5 -5

@@ -668,11 +668,11 @@ void cfg80211_unlink_bss(struct wiphy *w
 	bss = container_of(pub, struct cfg80211_internal_bss, pub);
 
 	spin_lock_bh(&dev->bss_lock);
-
-	list_del(&bss->list);
-	dev->bss_generation++;
-	rb_erase(&bss->rbn, &dev->bss_tree);
-
+	if (!list_empty(&bss->list)) {
+		list_del_init(&bss->list);
+		dev->bss_generation++;
+		rb_erase(&bss->rbn, &dev->bss_tree);
+	}
 	spin_unlock_bh(&dev->bss_lock);
 
 	kref_put(&bss->ref, bss_release);

Ben Greear

author

2010-10-06 18:08 UTC #2 lore raw

Quoted 8 lines · Johannes Berg

Quoted 5 lines · Ben Greear
This test scenario has 72 stations on ath5k trying to connect to a cisco AP that supposedly only supports 63 stations.
The 72 STA were created without ssid's configured, then we re-configured all 72 'at once' to give them the proper SSID (ifdown, ifup, iwconfig to set values).
Eww, iwconfig ;-)

Heh, one thing at a time :)

Quoted 12 lines

Quoted 11 lines
The system crashed and rebooted.
Kernel is wireless-testing as of later yesterday, with a few additional patches mostly dealing with counters in /proc/net/wireless and some lockdep fixes pulled in from lkml etc.
We have seen this before, but this is the first good stacktrace we got.
Likely we can reproduce this if extra information is needed.
list_del corruption, next is LIST_POISON1 (00100100)

Quoted 8 lines

This one's interesting.
But anyway, now that I look at it in more detail, it seems fairly obvious. You should be able to trigger it with two stations, but it's probably harder ...
I need to analyse the refcounting here again and in more detail, but in the meantime can you try below patch?

Yes, will do so and let you know the results.

Thanks,
Ben

Quoted 27 lines

johannes

---
  net/wireless/scan.c |   10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)

net/wireless/scan.c +5 -5

@@ -668,11 +668,11 @@ void cfg80211_unlink_bss(struct wiphy *w
  	bss = container_of(pub, struct cfg80211_internal_bss, pub);
 
  	spin_lock_bh(&dev->bss_lock);
-
-	list_del(&bss->list);
-	dev->bss_generation++;
-	rb_erase(&bss->rbn,&dev->bss_tree);
-
+	if (!list_empty(&bss->list)) {
+		list_del_init(&bss->list);
+		dev->bss_generation++;
+		rb_erase(&bss->rbn,&dev->bss_tree);
+	}
  	spin_unlock_bh(&dev->bss_lock);
 
  	kref_put(&bss->ref, bss_release);

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

maintainer

2010-10-06 18:16 UTC #3 lore raw

Quoted 4 lines · Ben Greear

Quoted 2 lines
I need to analyse the refcounting here again and in more detail, but in the meantime can you try below patch?
Yes, will do so and let you know the results.

Please try the new version, and let me know. I should submit this ASAP.

johannes

Ben Greear

author

2010-10-06 18:20 UTC #4 lore raw

Quoted 8 lines · Johannes Berg

Quoted 4 lines · Ben Greear
Quoted 2 lines
I need to analyse the refcounting here again and in more detail, but in the meantime can you try below patch?
Yes, will do so and let you know the results.
Please try the new version, and let me know. I should submit this ASAP.

I have to stash my slub debug hacking and re-compile...should have it tested within 30 min if all goes well.

Thanks,
Ben

Quoted 2 lines

johannes

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Ben Greear

author

2010-10-06 19:14 UTC #5 lore raw

In reply to Johannes Berg

Quoted 8 lines · Johannes Berg

Quoted 4 lines · Ben Greear
Quoted 2 lines
I need to analyse the refcounting here again and in more detail, but in the meantime can you try below patch?
Yes, will do so and let you know the results.
Please try the new version, and let me know. I should submit this ASAP.

That patch seems to fix things.

Feel free to add: Tested-by: Hun-Kyi Wynn <[email protected]>

Thanks,
Ben

Quoted 2 lines

johannes

Signature · 2 lines

Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com

Johannes Berg

maintainer

2010-10-06 19:19 UTC #6 lore raw

Quoted 6 lines · Ben Greear

Quoted 5 lines · Johannes Berg
Quoted 3 lines · Ben Greear
Quoted 2 lines
I need to analyse the refcounting here again and in more detail, but in the meantime can you try below patch?

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
Yes, will do so and let you know the results.
Please try the new version, and let me know. I should submit this ASAP.
That patch seems to fix things.
Feel free to add: Tested-by: Hun-Kyi Wynn <[email protected]>

Thanks, I've sent out the patch.

johannes

Johannes Berg

maintainer

2010-10-06 18:11 UTC #7 lore raw

In reply to Johannes Berg

Quoted 6 lines · Johannes Berg

But anyway, now that I look at it in more detail, it seems fairly obvious. You should be able to trigger it with two stations, but it's probably harder ...
I need to analyse the refcounting here again and in more detail, but in the meantime can you try below patch?

Ok, I did that, and it's because the BSS list owns a reference, so this'll just crash somewhere else for you now ... Below should fix it completely.

johannes

---
 net/wireless/scan.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

net/wireless/scan.c +6 -6

@@ -668,14 +668,14 @@ void cfg80211_unlink_bss(struct wiphy *w
 	bss = container_of(pub, struct cfg80211_internal_bss, pub);
 
 	spin_lock_bh(&dev->bss_lock);
+	if (!list_empty(&bss->list)) {
+		list_del_init(&bss->list);
+		dev->bss_generation++;
+		rb_erase(&bss->rbn, &dev->bss_tree);
 
-	list_del(&bss->list);
-	dev->bss_generation++;
-	rb_erase(&bss->rbn, &dev->bss_tree);
-
+		kref_put(&bss->ref, bss_release);
+	}
 	spin_unlock_bh(&dev->bss_lock);
-
-	kref_put(&bss->ref, bss_release);
 }
 EXPORT_SYMBOL(cfg80211_unlink_bss);