Message ID | 20200826095141.5156-1-s.hauer@pengutronix.de |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | net: mdiobus: fix device unregistering in mdiobus_register | expand |
On 26.08.2020 11:51, Sascha Hauer wrote: > __mdiobus_register() can fail between calling device_register() and > setting bus->state to MDIOBUS_REGISTERED. When this happens the caller > will call mdiobus_free() which then frees the mdio bus structure. This > is not allowed as the embedded struct device is already registered, thus > must be freed dropping the reference count using put_device(). To > accomplish this set bus->state to MDIOBUS_UNREGISTERED after having > registered the device. With this mdiobus_free() correctly calls > put_device() instead of freeing the mdio bus structure directly. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > 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..85cbaab4a591 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -540,6 +540,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) > return -EINVAL; > } > > + bus->state = MDIOBUS_UNREGISTERED; > + > mutex_init(&bus->mdio_lock); > mutex_init(&bus->shared_lock); > > I see the point. If we bail out after having called device_register() then put_device() has to be called. This however isn't done by mdiobus_free() if state is MDIOBUS_ALLOCATED. So I think the idea is right. However we have to call put_device() even if device_register() fails, therefore setting state to MDIOBUS_UNREGISTERED should be moved to before calling device_register().
On Wed, Aug 26, 2020 at 06:26:36PM +0200, Heiner Kallweit wrote: > On 26.08.2020 11:51, Sascha Hauer wrote: > > __mdiobus_register() can fail between calling device_register() and > > setting bus->state to MDIOBUS_REGISTERED. When this happens the caller > > will call mdiobus_free() which then frees the mdio bus structure. This > > is not allowed as the embedded struct device is already registered, thus > > must be freed dropping the reference count using put_device(). To > > accomplish this set bus->state to MDIOBUS_UNREGISTERED after having > > registered the device. With this mdiobus_free() correctly calls > > put_device() instead of freeing the mdio bus structure directly. > > > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > > --- > > 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..85cbaab4a591 100644 > > --- a/drivers/net/phy/mdio_bus.c > > +++ b/drivers/net/phy/mdio_bus.c > > @@ -540,6 +540,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) > > return -EINVAL; > > } > > > > + bus->state = MDIOBUS_UNREGISTERED; > > + > > mutex_init(&bus->mdio_lock); > > mutex_init(&bus->shared_lock); > > > > > I see the point. If we bail out after having called device_register() > then put_device() has to be called. This however isn't done by > mdiobus_free() if state is MDIOBUS_ALLOCATED. So I think the idea is > right. However we have to call put_device() even if device_register() > fails, therefore setting state to MDIOBUS_UNREGISTERED should be > moved to before calling device_register(). You're right, the comment above device_register clearly states: /* * NOTE: _Never_ directly free @dev after calling this function, even * if it returned an error! Always use put_device() to give up the * reference initialized in this function instead. */ And I read this just yesterday while preparing this patch. Will send a v2. Sascha
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c index 0af20faad69d..85cbaab4a591 100644 --- a/drivers/net/phy/mdio_bus.c +++ b/drivers/net/phy/mdio_bus.c @@ -540,6 +540,8 @@ int __mdiobus_register(struct mii_bus *bus, struct module *owner) return -EINVAL; } + bus->state = MDIOBUS_UNREGISTERED; + mutex_init(&bus->mdio_lock); mutex_init(&bus->shared_lock);
__mdiobus_register() can fail between calling device_register() and setting bus->state to MDIOBUS_REGISTERED. When this happens the caller will call mdiobus_free() which then frees the mdio bus structure. This is not allowed as the embedded struct device is already registered, thus must be freed dropping the reference count using put_device(). To accomplish this set bus->state to MDIOBUS_UNREGISTERED after having registered the device. With this mdiobus_free() correctly calls put_device() instead of freeing the mdio bus structure directly. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/net/phy/mdio_bus.c | 2 ++ 1 file changed, 2 insertions(+)