* [PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register()
@ 2023-10-09 8:39 Jinjie Ruan
2023-10-10 18:05 ` Simon Horman
0 siblings, 1 reply; 3+ messages in thread
From: Jinjie Ruan @ 2023-10-09 8:39 UTC (permalink / raw)
To: netdev, Florian Fainelli, Andrew Lunn, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vivien Didelot
Cc: ruanjinjie
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(-)
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);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register()
2023-10-09 8:39 [PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register() Jinjie Ruan
@ 2023-10-10 18:05 ` Simon Horman
2023-10-10 18:18 ` Florian Fainelli
0 siblings, 1 reply; 3+ messages in thread
From: Simon Horman @ 2023-10-10 18:05 UTC (permalink / raw)
To: Jinjie Ruan
Cc: netdev, Florian Fainelli, Andrew Lunn, Vladimir Oltean,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Vivien Didelot
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)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register()
2023-10-10 18:05 ` Simon Horman
@ 2023-10-10 18:18 ` Florian Fainelli
0 siblings, 0 replies; 3+ messages in thread
From: Florian Fainelli @ 2023-10-10 18:18 UTC (permalink / raw)
To: Simon Horman, Jinjie Ruan
Cc: netdev, Andrew Lunn, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Vivien Didelot
[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]
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)
--
Florian
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-10-10 18:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-09 8:39 [PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register() Jinjie Ruan
2023-10-10 18:05 ` Simon Horman
2023-10-10 18:18 ` Florian Fainelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox