[PATCH] net: ethernet: bgmac-platform: fix an OF node reference leak

[PATCH] net: ethernet: bgmac-platform: fix an OF node reference leak

[Joe Hattori]

The OF node obtained by of_parse_phandle() is not freed. Define a
device node with __free(device_node) to fix the leak.

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

Fixes: 1676aba5ef7e ("net: ethernet: bgmac: device tree phy enablement")
Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
index ecce23cecbea..ca07a6d26590 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -236,7 +236,10 @@ static int bgmac_probe(struct platform_device *pdev)
 	bgmac->cco_ctl_maskset = platform_bgmac_cco_ctl_maskset;
 	bgmac->get_bus_clock = platform_bgmac_get_bus_clock;
 	bgmac->cmn_maskset32 = platform_bgmac_cmn_maskset32;
-	if (of_parse_phandle(np, "phy-handle", 0)) {
+
+	struct device_node *phy_node __free(device_node) =
+		of_parse_phandle(np, "phy-handle", 0);
+	if (phy_node) {
 		bgmac->phy_connect = platform_phy_connect;
 	} else {
 		bgmac->phy_connect = bgmac_phy_connect_direct;
-- 
2.34.1

Re: [PATCH] net: ethernet: bgmac-platform: fix an OF node reference leak

[Simon Horman]

On Thu, Dec 12, 2024 at 11:32:56AM +0900, Joe Hattori wrote:
> The OF node obtained by of_parse_phandle() is not freed. Define a
> device node with __free(device_node) to fix the leak.
> 
> This bug was found by an experimental static analysis tool that I am
> developing.
> 
> Fixes: 1676aba5ef7e ("net: ethernet: bgmac: device tree phy enablement")
> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
> ---
>  drivers/net/ethernet/broadcom/bgmac-platform.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
> index ecce23cecbea..ca07a6d26590 100644
> --- a/drivers/net/ethernet/broadcom/bgmac-platform.c
> +++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
> @@ -236,7 +236,10 @@ static int bgmac_probe(struct platform_device *pdev)
>  	bgmac->cco_ctl_maskset = platform_bgmac_cco_ctl_maskset;
>  	bgmac->get_bus_clock = platform_bgmac_get_bus_clock;
>  	bgmac->cmn_maskset32 = platform_bgmac_cmn_maskset32;
> -	if (of_parse_phandle(np, "phy-handle", 0)) {
> +
> +	struct device_node *phy_node __free(device_node) =
> +		of_parse_phandle(np, "phy-handle", 0);
> +	if (phy_node) {
>  		bgmac->phy_connect = platform_phy_connect;
>  	} else {
>  		bgmac->phy_connect = bgmac_phy_connect_direct;

Hi Joe,

I agree this is a problem and that it was introduced by the
cited commit. But I wonder if we can consider a different approach.

I would suggest that rather than using __free the node is explicitly
released. Something like this (untested):

	struct device_node *phy_node;

	...

	phy_node = of_parse_phandle(np, "phy-handle", 0);
	if (phy_node) {
		of_node_put(phy_node);
		bgmac->phy_connect = platform_phy_connect;
	} ...

That is, assuming that it is safe to release phy_node so early.
If not, some adjustment should be made to when of_node_put()
is called.

This is for several reasons;

1. I could be wrong, but I believe your patch kfree's phy_node,
   but my understanding is that correct operation is to call
   of_node_put().

2. More importantly, there is a preference in Newkorking code
   not to use __free and similar constructs.

     "Low level cleanup constructs (such as __free()) can be used when
      building APIs and helpers, especially scoped iterators. However,
      direct use of __free() within networking core and drivers is
      discouraged. Similar guidance applies to declaring variables
      mid-function.

     Link: https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

3. As per the end of the quote above, there is a preference to declare all
   local variables at the top of the function (ideally, in reverse xmas
   tree order [*})

   [*] https://docs.kernel.org/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs

-- 
pw-bot: changes-requested

Re: [PATCH] net: ethernet: bgmac-platform: fix an OF node reference leak

[Andrew Lunn]

> Hi Joe,
> 
> I agree this is a problem and that it was introduced by the
> cited commit. But I wonder if we can consider a different approach.
> 
> I would suggest that rather than using __free the node is explicitly
> released. Something like this (untested):
> 
> 	struct device_node *phy_node;
> 
> 	...
> 
> 	phy_node = of_parse_phandle(np, "phy-handle", 0);
> 	if (phy_node) {
> 		of_node_put(phy_node);
> 		bgmac->phy_connect = platform_phy_connect;
> 	} ...
> 
> That is, assuming that it is safe to release phy_node so early.
> If not, some adjustment should be made to when of_node_put()
> is called.
> 
> This is for several reasons;
> 
> 1. I could be wrong, but I believe your patch kfree's phy_node,
>    but my understanding is that correct operation is to call
>    of_node_put().

Hi Simon

I _think_ that is wrong. More of the magic which i don't really
like. The cleanup subsystem has to be taught all the types, and what
operation to perform for each type. Despite the name __free(), i think
it does actually call of_node_put(). The magic would be more readable
if it was actually __put(), not __free().

> 2. More importantly, there is a preference in Newkorking code
>    not to use __free and similar constructs.
> 
>      "Low level cleanup constructs (such as __free()) can be used when
>       building APIs and helpers, especially scoped iterators. However,
>       direct use of __free() within networking core and drivers is
>       discouraged. Similar guidance applies to declaring variables
>       mid-function.

And this is a good example of why.

	Andrew

Re: [PATCH] net: ethernet: bgmac-platform: fix an OF node reference leak

[Simon Horman]

On Fri, Dec 13, 2024 at 01:04:42PM +0100, Andrew Lunn wrote:
> > Hi Joe,
> > 
> > I agree this is a problem and that it was introduced by the
> > cited commit. But I wonder if we can consider a different approach.
> > 
> > I would suggest that rather than using __free the node is explicitly
> > released. Something like this (untested):
> > 
> > 	struct device_node *phy_node;
> > 
> > 	...
> > 
> > 	phy_node = of_parse_phandle(np, "phy-handle", 0);
> > 	if (phy_node) {
> > 		of_node_put(phy_node);
> > 		bgmac->phy_connect = platform_phy_connect;
> > 	} ...
> > 
> > That is, assuming that it is safe to release phy_node so early.
> > If not, some adjustment should be made to when of_node_put()
> > is called.
> > 
> > This is for several reasons;
> > 
> > 1. I could be wrong, but I believe your patch kfree's phy_node,
> >    but my understanding is that correct operation is to call
> >    of_node_put().
> 
> Hi Simon
> 
> I _think_ that is wrong. More of the magic which i don't really
> like. The cleanup subsystem has to be taught all the types, and what
> operation to perform for each type. Despite the name __free(), i think
> it does actually call of_node_put(). The magic would be more readable
> if it was actually __put(), not __free().

Thanks, TIL.

> > 2. More importantly, there is a preference in Newkorking code
> >    not to use __free and similar constructs.
> > 
> >      "Low level cleanup constructs (such as __free()) can be used when
> >       building APIs and helpers, especially scoped iterators. However,
> >       direct use of __free() within networking core and drivers is
> >       discouraged. Similar guidance applies to declaring variables
> >       mid-function.
> 
> And this is a good example of why.
> 
> 	Andrew
>