| Message ID | 20241218032230.117453-2-joe@pf.is.s.u-tokyo.ac.jp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: stmmac: fix an OF node reference leak in error paths in stmmac_probe_config_dt() | expand |
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
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
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)
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(-)