| Message ID | 20200827070618.26754-1-s.hauer@pengutronix.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [PATCHi,v2] net: mdiobus: fix device unregistering in mdiobus_register | expand |
On 27.08.2020 09:06, Sascha Hauer wrote: > After device_register has been called the device structure may not be > freed anymore, put_device() has to be called instead. This gets violated > when device_register() or any of the following steps before the mdio > bus is fully registered fails. In this case the caller will call > mdiobus_free() which then directly frees the mdio bus structure. > > Set bus->state to MDIOBUS_UNREGISTERED right before calling > device_register(). With this mdiobus_free() calls put_device() instead > as it ought to be. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > > Changes since v1: > - set bus->state before calling device_register(), not afterwards > > drivers/net/phy/mdio_bus.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 0af20faad69d..9434b04a11c8 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -534,6 +534,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) > bus->dev.groups = NULL; > dev_set_name(&bus->dev, "%s", bus->id); > > + bus->state = MDIOBUS_UNREGISTERED; > + > err = device_register(&bus->dev); > if (err) { > pr_err("mii_bus %s failed to register\n", bus->id); > LGTM. Just two points: 1. Subject has a typo (PATCHi). And it should be [PATCH net v2], because it's something for the stable branch. 2. A "Fixes" tag is needed.
On Thu, Aug 27, 2020 at 10:48:48AM +0200, Heiner Kallweit wrote: > On 27.08.2020 09:06, Sascha Hauer wrote: > > After device_register has been called the device structure may not be > > freed anymore, put_device() has to be called instead. This gets violated > > when device_register() or any of the following steps before the mdio > > bus is fully registered fails. In this case the caller will call > > mdiobus_free() which then directly frees the mdio bus structure. > > > > Set bus->state to MDIOBUS_UNREGISTERED right before calling > > device_register(). With this mdiobus_free() calls put_device() instead > > as it ought to be. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > > > Changes since v1: > > - set bus->state before calling device_register(), not afterwards > > > > drivers/net/phy/mdio_bus.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > > index 0af20faad69d..9434b04a11c8 100644 > > --- a/drivers/net/phy/mdio_bus.c > > +++ b/drivers/net/phy/mdio_bus.c > > @@ -534,6 +534,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) > > bus->dev.groups = NULL; > > dev_set_name(&bus->dev, "%s", bus->id); > > > > + bus->state = MDIOBUS_UNREGISTERED; > > + > > err = device_register(&bus->dev); > > if (err) { > > pr_err("mii_bus %s failed to register\n", bus->id); > > > LGTM. Just two points: > 1. Subject has a typo (PATCHi). And it should be [PATCH net v2], because it's > something for the stable branch. > 2. A "Fixes" tag is needed. Uh, AFAICT this fixes a patch from 2008, this makes for quite some stable updates :) Sascha | commit 161c8d2f50109b44b664eaf23831ea1587979a61 | Author: Krzysztof Halasa <khc@pm.waw.pl> | Date: Thu Dec 25 16:50:41 2008 -0800 | | net: PHYLIB mdio fixes #2
On 28.08.2020 16:15, Sascha Hauer wrote: > On Thu, Aug 27, 2020 at 10:48:48AM +0200, Heiner Kallweit wrote: >> On 27.08.2020 09:06, Sascha Hauer wrote: >>> After device_register has been called the device structure may not be >>> freed anymore, put_device() has to be called instead. This gets violated >>> when device_register() or any of the following steps before the mdio >>> bus is fully registered fails. In this case the caller will call >>> mdiobus_free() which then directly frees the mdio bus structure. >>> >>> Set bus->state to MDIOBUS_UNREGISTERED right before calling >>> device_register(). With this mdiobus_free() calls put_device() instead >>> as it ought to be. >>> >>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> >>> --- >>> >>> Changes since v1: >>> - set bus->state before calling device_register(), not afterwards >>> >>> drivers/net/phy/mdio_bus.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c >>> index 0af20faad69d..9434b04a11c8 100644 >>> --- a/drivers/net/phy/mdio_bus.c >>> +++ b/drivers/net/phy/mdio_bus.c >>> @@ -534,6 +534,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) >>> bus->dev.groups = NULL; >>> dev_set_name(&bus->dev, "%s", bus->id); >>> >>> + bus->state = MDIOBUS_UNREGISTERED; >>> + >>> err = device_register(&bus->dev); >>> if (err) { >>> pr_err("mii_bus %s failed to register\n", bus->id); >>> >> LGTM. Just two points: >> 1. Subject has a typo (PATCHi). And it should be [PATCH net v2], because it's >> something for the stable branch. >> 2. A "Fixes" tag is needed. > > Uh, AFAICT this fixes a patch from 2008, this makes for quite some > stable updates :) > There's just a handful of LTS kernel versions (oldest is 4.4), therefore it shouldn't be that bad. But right, for things that have always been like they are now, sometimes it's tricky to find a proper Fixes tag. > Sascha > > | commit 161c8d2f50109b44b664eaf23831ea1587979a61 > | Author: Krzysztof Halasa <khc@pm.waw.pl> > | Date: Thu Dec 25 16:50:41 2008 -0800 > | > | net: PHYLIB mdio fixes #2 >
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 0af20faad69d..9434b04a11c8 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -534,6 +534,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) bus->dev.groups = NULL; dev_set_name(&bus->dev, "%s", bus->id); + bus->state = MDIOBUS_UNREGISTERED; + err = device_register(&bus->dev); if (err) { pr_err("mii_bus %s failed to register\n", bus->id);
After device_register has been called the device structure may not be freed anymore, put_device() has to be called instead. This gets violated when device_register() or any of the following steps before the mdio bus is fully registered fails. In this case the caller will call mdiobus_free() which then directly frees the mdio bus structure. Set bus->state to MDIOBUS_UNREGISTERED right before calling device_register(). With this mdiobus_free() calls put_device() instead as it ought to be. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- Changes since v1: - set bus->state before calling device_register(), not afterwards drivers/net/phy/mdio_bus.c | 2 ++ 1 file changed, 2 insertions(+)