[PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register()

[PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register()

[Jinjie Ruan]

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

Re: [PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register()

[Simon Horman]

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)

Re: [PATCH v2] net: dsa: bcm_sf2: Fix possible memory leak in bcm_sf2_mdio_register()

[Florian Fainelli]

[-- 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 --]