On Sun, 2007-02-11 at 20:39 +0100, Johannes Berg wrote:
> On Fri, 2007-02-09 at 17:27 +0100, johannes@xxxxxxxxxxxxxxxx wrote:
> [...]
> needs to be changed. wiphy_free() should wait for
> wiphy_class_dev_release (to make sure sysfs is gone) before freeing the
> structure (if it has ever been added to sysfs).
On second thought, no, that's perfectly correct. There's a slight bug
however in the registration, it must not set wiphy_index when sysfs
registration fails.
The real bug I'm chasing (use-after-free leading to oops when rmmod'ing
bcm43xx-d80211 while device is up) is in bcm43xx-d80211 and I found it
too. Consider the following changes I made to debug:
diff --git a/drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c b/drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c
index 9f4d51d..5205859 100644
--- a/drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c
+++ b/drivers/net/wireless/d80211/bcm43xx/bcm43xx_main.c
@@ -1479,6 +1479,8 @@ static irqreturn_t bcm43xx_interrupt_handler(int irq, void *dev_id)
if (!dev)
return IRQ_NONE;
+ printk(KERN_INFO "bcm43xx_interrupt_handler\n");
+
spin_lock(&dev->wl->irq_lock);
assert(bcm43xx_status(dev) == BCM43xx_STAT_INITIALIZED);
@@ -3453,7 +3455,8 @@ static void bcm43xx_one_core_detach(struct ssb_device *dev)
list_del(&wldev->list);
wl->nr_devs--;
ssb_set_drvdata(dev, NULL);
- kfree(wldev);
+ printk(KERN_INFO "kfree(wldev)\n");
+// kfree(wldev);
}
static int bcm43xx_one_core_attach(struct ssb_device *dev,
@@ -3535,8 +3538,10 @@ static void bcm43xx_wireless_exit(struct ssb_device *dev,
{
struct ieee80211_hw *hw = wl->hw;
+ printk(KERN_INFO "bcm43xx_wireless_exit(): unregister_hw()\n");
ieee80211_unregister_hw(hw);
ssb_set_devtypedata(dev, NULL);
+ printk(KERN_INFO "bcm43xx_wireless_exit(): free_hw()\n");
ieee80211_free_hw(hw);
}
Now remember the oops I got which was a use-after-free in the interrupt handler.
Now also consider this message log I got when inserting bcm43xx,
scanning (I removed some interrupt messages), and then removing the
module again:
[ 1443.269289] wlan0: starting scan
[ 1443.316409] bcm43xx_interrupt_handler
...
[ 1444.016378] bcm43xx_interrupt_handler
[ 1444.053623] wlan0: scan completed
[ 1445.317311] kfree(wldev)
[ 1445.317931] bcm43xx_wireless_exit(): unregister_hw()
[ 1445.385872] bcm43xx_d80211: Wireless interface stopped
[ 1445.386554] bcm43xx_d80211: Removing Interface type 2
[ 1445.387219] bcm43xx_d80211: DMA-32 0x0200 (RX) max used slots: 0/64
[ 1445.389551] bcm43xx_d80211: DMA-32 0x02A0 (TX) max used slots: 0/128
[ 1445.390609] bcm43xx_d80211: DMA-32 0x0280 (TX) max used slots: 0/128
[ 1445.391678] bcm43xx_d80211: DMA-32 0x0260 (TX) max used slots: 0/128
[ 1445.392756] bcm43xx_d80211: DMA-32 0x0240 (TX) max used slots: 0/128
[ 1445.393821] bcm43xx_d80211: DMA-32 0x0220 (TX) max used slots: 2/128
[ 1445.394869] bcm43xx_d80211: DMA-32 0x0200 (TX) max used slots: 0/128
[ 1445.395924] bcm43xx_d80211: Radio turned off
[ 1445.516479] bcm43xx_wireless_exit(): free_hw()
[ 1445.517144] wiphy_free()
[ 1445.518029] PM: Removing info for ssb:ssb04:04
[ 1445.518164] PM: Removing info for ssb:ssb04:03
[ 1445.518284] PM: Removing info for ssb:ssb04:02
[ 1445.518405] PM: Removing info for ssb:ssb04:01
[ 1445.518597] PM: Removing info for ssb:ssb04:00
Now let's also take a look at the code that prints "Wireless interface
stopped". That function is bcm43xx_wireless_core_stop(), which is passed
a struct bcm43xx_wldev, precisely the one that two lines before was
passed to that kfree()...
So what happens is that sometimes a whole bunch of things in
bcm43xx_wireless_core_stop will not work properly due to the
use-after-free we have here. Unless you have slab debugging disabled
(like me) where of course it fails every time...
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
