| Message ID | 20231011032419.2423290-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 61b40cefe51af005c72dbdcf975a3d166c6e6406 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register() | expand |
On Wed, Oct 11, 2023 at 11:24:19AM +0800, Jinjie Ruan wrote: > In bcm_sf2_mdio_register(), the class_find_device() will call get_device() > to increment reference count for priv->master_mii_bus->dev if > of_mdio_find_bus() succeeds. If mdiobus_alloc() or mdiobus_register() > fails, it will call get_device() twice without decrement reference count > for the device. And it is the same if bcm_sf2_mdio_register() succeeds but > fails in bcm_sf2_sw_probe(), or if bcm_sf2_sw_probe() succeeds. If the > reference count has not decremented to zero, the dev related resource will > not be freed. > > So remove the get_device() in bcm_sf2_mdio_register(), and call > put_device() if mdiobus_alloc() or mdiobus_register() fails and in > bcm_sf2_mdio_unregister() to solve the issue. > > And as Simon suggested, unwind from errors for bcm_sf2_mdio_register() and > just return 0 if it succeeds to make it cleaner. > > Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Suggested-by: Simon Horman <horms@kernel.org> Thanks, this addresses my concerns around v2. Reviewed-by: Simon Horman <horms@kernel.org>
On 10/10/23 20:24, Jinjie Ruan wrote: > In bcm_sf2_mdio_register(), the class_find_device() will call get_device() > to increment reference count for priv->master_mii_bus->dev if > of_mdio_find_bus() succeeds. If mdiobus_alloc() or mdiobus_register() > fails, it will call get_device() twice without decrement reference count > for the device. And it is the same if bcm_sf2_mdio_register() succeeds but > fails in bcm_sf2_sw_probe(), or if bcm_sf2_sw_probe() succeeds. If the > reference count has not decremented to zero, the dev related resource will > not be freed. > > So remove the get_device() in bcm_sf2_mdio_register(), and call > put_device() if mdiobus_alloc() or mdiobus_register() fails and in > bcm_sf2_mdio_unregister() to solve the issue. > > And as Simon suggested, unwind from errors for bcm_sf2_mdio_register() and > just return 0 if it succeeds to make it cleaner. > > Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > Suggested-by: Simon Horman <horms@kernel.org> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Wed, 11 Oct 2023 11:24:19 +0800 you wrote: > In bcm_sf2_mdio_register(), the class_find_device() will call get_device() > to increment reference count for priv->master_mii_bus->dev if > of_mdio_find_bus() succeeds. If mdiobus_alloc() or mdiobus_register() > fails, it will call get_device() twice without decrement reference count > for the device. And it is the same if bcm_sf2_mdio_register() succeeds but > fails in bcm_sf2_sw_probe(), or if bcm_sf2_sw_probe() succeeds. If the > reference count has not decremented to zero, the dev related resource will > not be freed. > > [...] Here is the summary with links: - [v3] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register() https://git.kernel.org/netdev/net/c/61b40cefe51a You are awesome, thank you!
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 72374b066f64..cd1f240c90f3 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -617,17 +617,16 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds) dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio"); priv->master_mii_bus = of_mdio_find_bus(dn); if (!priv->master_mii_bus) { - of_node_put(dn); - return -EPROBE_DEFER; + err = -EPROBE_DEFER; + goto err_of_node_put; } - get_device(&priv->master_mii_bus->dev); priv->master_mii_dn = dn; priv->slave_mii_bus = mdiobus_alloc(); if (!priv->slave_mii_bus) { - of_node_put(dn); - return -ENOMEM; + err = -ENOMEM; + goto err_put_master_mii_bus_dev; } priv->slave_mii_bus->priv = priv; @@ -684,11 +683,17 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds) } err = mdiobus_register(priv->slave_mii_bus); - if (err && dn) { - mdiobus_free(priv->slave_mii_bus); - of_node_put(dn); - } + if (err && dn) + goto err_free_slave_mii_bus; + return 0; + +err_free_slave_mii_bus: + mdiobus_free(priv->slave_mii_bus); +err_put_master_mii_bus_dev: + put_device(&priv->master_mii_bus->dev); +err_of_node_put: + of_node_put(dn); return err; } @@ -696,6 +701,7 @@ static void bcm_sf2_mdio_unregister(struct bcm_sf2_priv *priv) { mdiobus_unregister(priv->slave_mii_bus); mdiobus_free(priv->slave_mii_bus); + put_device(&priv->master_mii_bus->dev); of_node_put(priv->master_mii_dn); }
In bcm_sf2_mdio_register(), the class_find_device() will call get_device() to increment reference count for priv->master_mii_bus->dev if of_mdio_find_bus() succeeds. If mdiobus_alloc() or mdiobus_register() fails, it will call get_device() twice without decrement reference count for the device. And it is the same if bcm_sf2_mdio_register() succeeds but fails in bcm_sf2_sw_probe(), or if bcm_sf2_sw_probe() succeeds. If the reference count has not decremented to zero, the dev related resource will not be freed. So remove the get_device() in bcm_sf2_mdio_register(), and call put_device() if mdiobus_alloc() or mdiobus_register() fails and in bcm_sf2_mdio_unregister() to solve the issue. And as Simon suggested, unwind from errors for bcm_sf2_mdio_register() and just return 0 if it succeeds to make it cleaner. Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> Suggested-by: Simon Horman <horms@kernel.org> --- v3: - Unwind from errors for bcm_sf2_mdio_register(). - Update the commit message. - Add Suggested-by. v2: - Update the commit message. --- drivers/net/dsa/bcm_sf2.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)