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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <184e2953-c5db-4d53-9d64-e81bd433a02a@pf.is.s.u-tokyo.ac.jp>
Date: Thu, 19 Dec 2024 11:45:09 +0900
From: Joe Hattori <joe@...is.s.u-tokyo.ac.jp>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: alexandre.torgue@...s.st.com, joabreu@...opsys.com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, mcoquelin.stm32@...il.com,
 krzk@...nel.org, netdev@...r.kernel.org
Subject: Re: [PATCH v2 1/2] net: stmmac: call of_node_put() and
 stmmac_remove_config_dt() in error paths in stmmac_probe_config_dt()

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