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] [thread-next>] [day] [month] [year] [list]

Message-ID: <50d126f4-e87a-4502-8a9b-7291d0143ed6@stanley.mountain>
Date: Wed, 18 Dec 2024 11:43:53 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Joe Hattori <joe@...is.s.u-tokyo.ac.jp>
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()

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