Patchwork phylib: fix premature freeing of struct mii_bus

login
register
mail settings
Submitter Lennert Buytenhek
Date Nov. 9, 2008, 4:34 a.m.
Message ID <20081109043447.GB17630@xi.wantstofly.org>
Download mbox | patch
Permalink /patch/7902/
State Accepted
Delegated to: Jeff Garzik
Headers show

Comments

Lennert Buytenhek - Nov. 9, 2008, 4:34 a.m.
On Sun, Nov 09, 2008 at 02:59:50AM +0100, Lennert Buytenhek wrote:

> > From: Lennert Buytenhek <buytenh@wantstofly.org>
> > 
> > Commit 46abc02175b3c246dd5141d878f565a8725060c9 ("phylib: give mdio
> > buses a device tree presence") added a call to device_unregister() in
> > a situation where the caller did not expect the reference count on
> > the embedded kobject to be dropped, but device_unregister() does drop
> > that reference count.  The right function to use in this situation
> > is device_del(), which unregisters the device from the system like
> > device_unregister() does, but without dropping the reference count.
> 
> Wait a minute -- the original code _is_ correct.  mdiobus_register()
> bumps the refcount to two (once because of device_initialize(), once
> because of device_add()), mdiobus_unregister() drops it back to one,
> and mdiobus_free() drops it to zero.

I was thrown off by a different bug here.  The original code is not
correct after all, and mdiobus_unregister() does have a bug where it
drops the refcount by one too many.  So the patch in the original
mail was correct, but the commit message was not.

Can you please use this message+patch instead?



commit 19e92973fc187125af02cefabc043359a962e5be
Author: Lennert Buytenhek <buytenh@wantstofly.org>
Date:   Sun Nov 9 05:02:11 2008 +0100

    phylib: fix premature freeing of struct mii_bus
    
    Commit 46abc02175b3c246dd5141d878f565a8725060c9 ("phylib: give mdio
    buses a device tree presence") added a call to device_unregister() in
    a situation where the caller did not intend for the device to be
    freed yet, but apart from just unregistering the device from the
    system, device_unregister() does an additional put_device() that is
    intended to free it.
    
    The right function to use in this situation is device_del(), which
    unregisters the device from the system like device_unregister() does,
    but without dropping the reference count an additional time.
    
    Bug report from Bryan Wu <cooloney@kernel.org>.
    
    Signed-off-by: Lennert Buytenhek <buytenh@marvell.com>
    Tested-by: Bryan Wu <cooloney@kernel.org>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 6671e2d..df06301 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -135,7 +135,7 @@  void mdiobus_unregister(struct mii_bus *bus)
 	BUG_ON(bus->state != MDIOBUS_REGISTERED);
 	bus->state = MDIOBUS_UNREGISTERED;
 
-	device_unregister(&bus->dev);
+	device_del(&bus->dev);
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if (bus->phy_map[i])
 			device_unregister(&bus->phy_map[i]->dev);