| Message ID | 20231009083906.1212900-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register() | expand |
On Mon, Oct 09, 2023 at 04:39:06PM +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. > > Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> Hi Jinjie Ruan, I agree with your analysis here, but I wonder if it would be nicer to move bcm_sf2_mdio_register() to a more idiomatic way of unwinding from errors. Something like this (compile tested only!) diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 0b62bd78ac50..037ce118ee00 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -617,8 +617,8 @@ 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); @@ -626,8 +626,8 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds) 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_device; } priv->slave_mii_bus->priv = priv; @@ -684,12 +684,18 @@ 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 err; + +err_free_slave_mii_bus: + mdiobus_free(priv->slave_mii_bus); +err_put_master_mii_bus_device: + put_device(&priv->master_mii_bus->dev); +err_of_node_put: + of_node_put(dn); + return err; } static void bcm_sf2_mdio_unregister(struct bcm_sf2_priv *priv)
On 10/10/23 11:05, Simon Horman wrote: > On Mon, Oct 09, 2023 at 04:39:06PM +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. >> >> Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus") >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > > Hi Jinjie Ruan, > > I agree with your analysis here, but I wonder if it would be nicer > to move bcm_sf2_mdio_register() to a more idiomatic way of unwinding > from errors. That would appear a tad cleaner, yes! > > Something like this (compile tested only!) > > diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c > index 0b62bd78ac50..037ce118ee00 100644 > --- a/drivers/net/dsa/bcm_sf2.c > +++ b/drivers/net/dsa/bcm_sf2.c > @@ -617,8 +617,8 @@ 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); > @@ -626,8 +626,8 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds) > > 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_device; > } > > priv->slave_mii_bus->priv = priv; > @@ -684,12 +684,18 @@ 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 err; > + > +err_free_slave_mii_bus: > + mdiobus_free(priv->slave_mii_bus); > +err_put_master_mii_bus_device: > + put_device(&priv->master_mii_bus->dev); > +err_of_node_put: > + of_node_put(dn); > + return err; > } > > static void bcm_sf2_mdio_unregister(struct bcm_sf2_priv *priv)
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c index 72374b066f64..f2b14bd7c38b 100644 --- a/drivers/net/dsa/bcm_sf2.c +++ b/drivers/net/dsa/bcm_sf2.c @@ -621,11 +621,11 @@ static int bcm_sf2_mdio_register(struct dsa_switch *ds) return -EPROBE_DEFER; } - get_device(&priv->master_mii_bus->dev); priv->master_mii_dn = dn; priv->slave_mii_bus = mdiobus_alloc(); if (!priv->slave_mii_bus) { + put_device(&priv->master_mii_bus->dev); of_node_put(dn); return -ENOMEM; } @@ -686,6 +686,7 @@ 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); + put_device(&priv->master_mii_bus->dev); of_node_put(dn); } @@ -696,6 +697,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. Fixes: 461cd1b03e32 ("net: dsa: bcm_sf2: Register our slave MDIO bus") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- v2: - Update the commit message. --- drivers/net/dsa/bcm_sf2.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)