[PATCH v2 0/2] net: stmmac: fix an OF node reference leak in error paths in stmmac_probe_config_dt()

[PATCH v2 0/2] net: stmmac: fix an OF node reference leak in error paths in stmmac_probe_config_dt()

[Joe Hattori]

This patch series fixes an OF node reference leak in
stmmac_probe_config_dt() and removes the unnecessary argument of
stmmac_remove_config_dt().

---
Changes in v2:
- Call of_node_put() instead of stmmac_remove_config_dt() when
  stmmac_mdio_setup() fails.
- Split the patch into two.
---
Joe Hattori (2):
  net: stmmac: call of_node_put() and stmmac_remove_config_dt() in error
    paths in stmmac_probe_config_dt()
  net: stmmac: remove the unnecessary argument of
    stmmac_remove_config_dt()

 .../ethernet/stmicro/stmmac/stmmac_platform.c | 37 +++++++------------
 1 file changed, 13 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] net: stmmac: call of_node_put() and stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()

[Joe Hattori]

Current implementation of stmmac_probe_config_dt() does not release the
OF node reference obtained by of_parse_phandle() when
stmmac_mdio_setup() fails, thus call of_node_put(). Also, the
error_hw_init and error_pclk_get labels can be removed as just calling
stmmac_remove_config_dt() suffices.

This bug was found by an experimental verification tool that I am
developing.

Fixes: 4838a5405028 ("net: stmmac: Fix wrapper drivers not detecting PHY")
Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 24 +++++++------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 3ac32444e492..669d8eb07044 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -436,7 +436,6 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	struct plat_stmmacenet_data *plat;
 	struct stmmac_dma_cfg *dma_cfg;
 	int phy_mode;
-	void *ret;
 	int rc;
 
 	plat = devm_kzalloc(&pdev->dev, sizeof(*plat), GFP_KERNEL);
@@ -490,8 +489,10 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 		dev_warn(&pdev->dev, "snps,phy-addr property is deprecated\n");
 
 	rc = stmmac_mdio_setup(plat, np, &pdev->dev);
-	if (rc)
+	if (rc) {
+		of_node_put(plat->phy_node);
 		return ERR_PTR(rc);
+	}
 
 	of_property_read_u32(np, "tx-fifo-depth", &plat->tx_fifo_size);
 
@@ -627,8 +628,8 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 
 	plat->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
 	if (IS_ERR(plat->pclk)) {
-		ret = plat->pclk;
-		goto error_pclk_get;
+		stmmac_remove_config_dt(pdev, plat);
+		return ERR_CAST(plat->pclk);
 	}
 	clk_prepare_enable(plat->pclk);
 
@@ -646,25 +647,18 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev,
 							   STMMAC_RESOURCE_NAME);
 	if (IS_ERR(plat->stmmac_rst)) {
-		ret = plat->stmmac_rst;
-		goto error_hw_init;
+		stmmac_remove_config_dt(pdev, plat);
+		return ERR_CAST(plat->stmmac_rst);
 	}
 
 	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
 							&pdev->dev, "ahb");
 	if (IS_ERR(plat->stmmac_ahb_rst)) {
-		ret = plat->stmmac_ahb_rst;
-		goto error_hw_init;
+		stmmac_remove_config_dt(pdev, plat);
+		return ERR_CAST(plat->stmmac_ahb_rst);
 	}
 
 	return plat;
-
-error_hw_init:
-	clk_disable_unprepare(plat->pclk);
-error_pclk_get:
-	clk_disable_unprepare(plat->stmmac_clk);
-
-	return ret;
 }
 
 static void devm_stmmac_remove_config_dt(void *data)
-- 
2.34.1

Re: [PATCH v2 1/2] net: stmmac: call of_node_put() and stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()

[Dan Carpenter]

The subject is too long.

On Wed, Dec 18, 2024 at 12:22:29PM +0900, Joe Hattori wrote:
>  	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
>  							&pdev->dev, "ahb");
>  	if (IS_ERR(plat->stmmac_ahb_rst)) {
> -		ret = plat->stmmac_ahb_rst;
> -		goto error_hw_init;
> +		stmmac_remove_config_dt(pdev, plat);
> +		return ERR_CAST(plat->stmmac_ahb_rst);
>  	}
>  
>  	return plat;
> -
> -error_hw_init:
> -	clk_disable_unprepare(plat->pclk);
> -error_pclk_get:
> -	clk_disable_unprepare(plat->stmmac_clk);
> -
> -	return ret;

Ah...  This is a bug fix, but it's not fixed in the right way.

These labels at the end of the function are called an unwind ladder.
This is where people mostly expect the error handling to be done.  Don't
get rid of the unwind ladder, but instead add the calls to:

error_put_phy:
	of_node_put(plat->phy_node);
error_put_mdio:
	of_node_put(plat->mdio_node);

The original code had some code paths which called
stmmac_remove_config_dt().  Get rid of that.  Everything should use the
unwind ladder.  This business of mixing error handling styles is what led
to this bug.

Calling a function to cleanup "everything" doesn't work because we keep
adding more stuff so it starts out as "everything" today but tomorrow
it's a leak.

This can all be sent as one patch if you describe it in the right way.

    The error handling in stmmac_probe_config_dt() has some
    error paths which don't call of_node_put().  The problem is
    that some error paths call stmmac_remove_config_dt() to
    clean up but others use and unwind ladder.  These two types
    of error handling have not kept in sync and have been a
    recurring source of bugs.  Re-write the error handling in
    stmmac_probe_config_dt() to use an unwind ladder.

regards,
dan carpenter

Re: [PATCH v2 1/2] net: stmmac: call of_node_put() and stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()

[Joe Hattori]

Thank you for your review.

On 12/18/24 17:43, Dan Carpenter wrote:
> The subject is too long.
> 
> On Wed, Dec 18, 2024 at 12:22:29PM +0900, Joe Hattori wrote:
>>   	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
>>   							&pdev->dev, "ahb");
>>   	if (IS_ERR(plat->stmmac_ahb_rst)) {
>> -		ret = plat->stmmac_ahb_rst;
>> -		goto error_hw_init;
>> +		stmmac_remove_config_dt(pdev, plat);
>> +		return ERR_CAST(plat->stmmac_ahb_rst);
>>   	}
>>   
>>   	return plat;
>> -
>> -error_hw_init:
>> -	clk_disable_unprepare(plat->pclk);
>> -error_pclk_get:
>> -	clk_disable_unprepare(plat->stmmac_clk);
>> -
>> -	return ret;
> 
> Ah...  This is a bug fix, but it's not fixed in the right way.
> 
> These labels at the end of the function are called an unwind ladder.
> This is where people mostly expect the error handling to be done.  Don't
> get rid of the unwind ladder, but instead add the calls to:
> 
> error_put_phy:
> 	of_node_put(plat->phy_node);
> error_put_mdio:
> 	of_node_put(plat->mdio_node);
> 
> The original code had some code paths which called
> stmmac_remove_config_dt().  Get rid of that.  Everything should use the
> unwind ladder.  This business of mixing error handling styles is what led
> to this bug.
> 
> Calling a function to cleanup "everything" doesn't work because we keep
> adding more stuff so it starts out as "everything" today but tomorrow
> it's a leak.

Yes, it really makes sense. Switched to the unwind ladder in v3 patch.

> 
> This can all be sent as one patch if you describe it in the right way.
> 
>      The error handling in stmmac_probe_config_dt() has some
>      error paths which don't call of_node_put().  The problem is
>      that some error paths call stmmac_remove_config_dt() to
>      clean up but others use and unwind ladder.  These two types
>      of error handling have not kept in sync and have been a
>      recurring source of bugs.  Re-write the error handling in
>      stmmac_probe_config_dt() to use an unwind ladder.

Thank you for the suggestion. Now the v3 is a single patch, and your 
commit message has been applied to it.

> 
> regards,
> dan carpenter
> 

Best,
Joe

[PATCH v2 2/2] net: stmmac: remove the unnecessary argument of stmmac_remove_config_dt()

[Joe Hattori]

The first argument of stmmac_remove_config_dt() is not used, so drop it.

Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
 .../ethernet/stmicro/stmmac/stmmac_platform.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 669d8eb07044..aadacb1d5939 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -407,13 +407,11 @@ static int stmmac_of_get_mac_mode(struct device_node *np)
 
 /**
  * stmmac_remove_config_dt - undo the effects of stmmac_probe_config_dt()
- * @pdev: platform_device structure
  * @plat: driver data platform structure
  *
  * Release resources claimed by stmmac_probe_config_dt().
  */
-static void stmmac_remove_config_dt(struct platform_device *pdev,
-				    struct plat_stmmacenet_data *plat)
+static void stmmac_remove_config_dt(struct plat_stmmacenet_data *plat)
 {
 	clk_disable_unprepare(plat->stmmac_clk);
 	clk_disable_unprepare(plat->pclk);
@@ -582,7 +580,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg),
 			       GFP_KERNEL);
 	if (!dma_cfg) {
-		stmmac_remove_config_dt(pdev, plat);
+		stmmac_remove_config_dt(plat);
 		return ERR_PTR(-ENOMEM);
 	}
 	plat->dma_cfg = dma_cfg;
@@ -611,7 +609,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 
 	rc = stmmac_mtl_setup(pdev, plat);
 	if (rc) {
-		stmmac_remove_config_dt(pdev, plat);
+		stmmac_remove_config_dt(plat);
 		return ERR_PTR(rc);
 	}
 
@@ -628,7 +626,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 
 	plat->pclk = devm_clk_get_optional(&pdev->dev, "pclk");
 	if (IS_ERR(plat->pclk)) {
-		stmmac_remove_config_dt(pdev, plat);
+		stmmac_remove_config_dt(plat);
 		return ERR_CAST(plat->pclk);
 	}
 	clk_prepare_enable(plat->pclk);
@@ -647,14 +645,14 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 	plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev,
 							   STMMAC_RESOURCE_NAME);
 	if (IS_ERR(plat->stmmac_rst)) {
-		stmmac_remove_config_dt(pdev, plat);
+		stmmac_remove_config_dt(plat);
 		return ERR_CAST(plat->stmmac_rst);
 	}
 
 	plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared(
 							&pdev->dev, "ahb");
 	if (IS_ERR(plat->stmmac_ahb_rst)) {
-		stmmac_remove_config_dt(pdev, plat);
+		stmmac_remove_config_dt(plat);
 		return ERR_CAST(plat->stmmac_ahb_rst);
 	}
 
@@ -663,10 +661,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
 
 static void devm_stmmac_remove_config_dt(void *data)
 {
-	struct plat_stmmacenet_data *plat = data;
-
-	/* Platform data argument is unused */
-	stmmac_remove_config_dt(NULL, plat);
+	stmmac_remove_config_dt(data);
 }
 
 /**
-- 
2.34.1

Re: [PATCH v2 2/2] net: stmmac: remove the unnecessary argument of stmmac_remove_config_dt()

[Dan Carpenter]

On Wed, Dec 18, 2024 at 12:22:30PM +0900, Joe Hattori wrote:
>  static void devm_stmmac_remove_config_dt(void *data)
>  {
> -	struct plat_stmmacenet_data *plat = data;
> -
> -	/* Platform data argument is unused */
> -	stmmac_remove_config_dt(NULL, plat);
> +	stmmac_remove_config_dt(data);

Instead of doing this, move the code from stmmac_remove_config_dt()
to here and delete the stmmac_remove_config_dt() function.  Delete
the comments that mention stmmac_remove_config_dt().

- * Description: Devres variant of stmmac_probe_config_dt(). Does not require
- * the user to call stmmac_remove_config_dt() at driver detach.

This comment no longer makes sense.

regards,
dan carpenter