| Message ID | 20221201033838.1938765-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] net: mdiobus: fix double put fwnode in the error path | expand |
On Thu, Dec 01, 2022 at 11:38:38AM +0800, Yang Yingliang wrote: > If phy_device_register() or fwnode_mdiobus_phy_device_register() > fail, phy_device_free() is called, the device refcount is decreased > to 0, then fwnode_handle_put() will be called in phy_device_release(), > but in the error path, fwnode_handle_put() has already been called, > so set fwnode to NULL after fwnode_handle_put() in the error path to > avoid double put. > > Fixes: cdde1560118f ("net: mdiobus: fix unbalanced node reference count") > Reported-by: Zeng Heng <zengheng4@huawei.com> > Tested-by: Zeng Heng <zengheng4@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > v1 -> v2: > Don't remove fwnode_handle_put() in the error path, > set fwnode to NULL avoid double put. > --- > drivers/net/mdio/fwnode_mdio.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c > index eb344f6d4a7b..9df618577712 100644 > --- a/drivers/net/mdio/fwnode_mdio.c > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -99,6 +99,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, > rc = phy_device_register(phy); > if (rc) { > fwnode_handle_put(child); > + device_set_node(&phy->mdio.dev, NULL); > return rc; > } This looks better, it is balanced. But i would argue the order is wrong. fwnode_handle_get(child); device_set_node(&phy->mdio.dev, child); /* All data is now stored in the phy struct; * register it */ rc = phy_device_register(phy); if (rc) { fwnode_handle_put(child); return rc; } In general you undo stuff in the opposite order to which you did it. So device_set_node() first, then fwnode_handle_put(). Otherwise you have a potential race condition. Andrew
On 2022/12/1 23:33, Andrew Lunn wrote: > On Thu, Dec 01, 2022 at 11:38:38AM +0800, Yang Yingliang wrote: >> If phy_device_register() or fwnode_mdiobus_phy_device_register() >> fail, phy_device_free() is called, the device refcount is decreased >> to 0, then fwnode_handle_put() will be called in phy_device_release(), >> but in the error path, fwnode_handle_put() has already been called, >> so set fwnode to NULL after fwnode_handle_put() in the error path to >> avoid double put. >> >> Fixes: cdde1560118f ("net: mdiobus: fix unbalanced node reference count") >> Reported-by: Zeng Heng <zengheng4@huawei.com> >> Tested-by: Zeng Heng <zengheng4@huawei.com> >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> v1 -> v2: >> Don't remove fwnode_handle_put() in the error path, >> set fwnode to NULL avoid double put. >> --- >> drivers/net/mdio/fwnode_mdio.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c >> index eb344f6d4a7b..9df618577712 100644 >> --- a/drivers/net/mdio/fwnode_mdio.c >> +++ b/drivers/net/mdio/fwnode_mdio.c >> @@ -99,6 +99,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, >> rc = phy_device_register(phy); >> if (rc) { >> fwnode_handle_put(child); >> + device_set_node(&phy->mdio.dev, NULL); >> return rc; >> } > This looks better, it is balanced. But i would argue the order is > wrong. > > fwnode_handle_get(child); > device_set_node(&phy->mdio.dev, child); > > /* All data is now stored in the phy struct; > * register it > */ > rc = phy_device_register(phy); > if (rc) { > fwnode_handle_put(child); > return rc; > } > > In general you undo stuff in the opposite order to which you did > it. So device_set_node() first, then fwnode_handle_put(). Otherwise > you have a potential race condition. OK, I will change the order in v3. Thanks, Yang > > Andrew > .
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index eb344f6d4a7b..9df618577712 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -99,6 +99,7 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, rc = phy_device_register(phy); if (rc) { fwnode_handle_put(child); + device_set_node(&phy->mdio.dev, NULL); return rc; } @@ -154,6 +155,7 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, rc = phy_device_register(phy); if (rc) { fwnode_handle_put(phy->mdio.dev.fwnode); + phy->mdio.dev.fwnode = NULL; goto clean_phy; } } else if (is_of_node(child)) {
If phy_device_register() or fwnode_mdiobus_phy_device_register() fail, phy_device_free() is called, the device refcount is decreased to 0, then fwnode_handle_put() will be called in phy_device_release(), but in the error path, fwnode_handle_put() has already been called, so set fwnode to NULL after fwnode_handle_put() in the error path to avoid double put. Fixes: cdde1560118f ("net: mdiobus: fix unbalanced node reference count") Reported-by: Zeng Heng <zengheng4@huawei.com> Tested-by: Zeng Heng <zengheng4@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- v1 -> v2: Don't remove fwnode_handle_put() in the error path, set fwnode to NULL avoid double put. --- drivers/net/mdio/fwnode_mdio.c | 2 ++ 1 file changed, 2 insertions(+)