From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from crystal.sipsolutions.net ([195.210.38.204]:37989 "EHLO sipsolutions.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964827AbXBLJdM (ORCPT ); Mon, 12 Feb 2007 04:33:12 -0500 Subject: Re: [PATCH 1/4] create cfg80211 From: Johannes Berg To: linux-wireless@vger.kernel.org Cc: Michael Buesch , Broadcom Linux In-Reply-To: <1171222767.3576.2.camel@johannes.berg> References: <20070209162711.375886000@sipsolutions.net> <20070209162826.930724000@sipsolutions.net> <1171222767.3576.2.camel@johannes.berg> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="=-gwcLAd3ClQuBfw8mskjT" Date: Mon, 12 Feb 2007 09:12:08 +0100 Message-Id: <1171267928.4881.9.camel@johannes.berg> Mime-Version: 1.0 Sender: linux-wireless-owner@vger.kernel.org List-ID: --=-gwcLAd3ClQuBfw8mskjT Content-Type: text/plain Content-Transfer-Encoding: quoted-printable On Sun, 2007-02-11 at 20:39 +0100, Johannes Berg wrote: > On Fri, 2007-02-09 at 17:27 +0100, johannes@sipsolutions.net 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/n= et/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; =20 + printk(KERN_INFO "bcm43xx_interrupt_handler\n"); + spin_lock(&dev->wl->irq_lock); =20 assert(bcm43xx_status(dev) =3D=3D 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); } =20 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 =3D wl->hw; =20 + 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); } =20 Now remember the oops I got which was a use-after-free in the interrupt han= dler. 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 --=-gwcLAd3ClQuBfw8mskjT Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part -----BEGIN PGP SIGNATURE----- Comment: Johannes Berg (powerbook) iD4DBQBF0CFY/ETPhpq3jKURApouAJdO/wSuXABqdNZm9d6FeYOzdgHhAJ45VQzA tzjzYSBT3bfq+ocy/qzODA== =Kk6O -----END PGP SIGNATURE----- --=-gwcLAd3ClQuBfw8mskjT--