[PATCH] net: fman: fix clock and device node leak in probe error paths

[PATCH] net: fman: fix clock and device node leak in probe error paths

[ZhaoJinming]

In read_dts_node(), fm_node is acquired via of_node_get() and clk is
acquired via of_clk_get(). After a successful of_clk_get() call, the
error paths for clk_get_rate failure and of_property_read_u32_array
failure correctly goto fman_node_put (releasing fm_node) but miss
clk_put(clk).

Worse, all error paths from the MURAM node lookup onward goto
fman_free directly, skipping both fman_node_put and clk_put, leaking
both the fm_node and clk references.

of_clk_get() eventually calls __of_clk_get() -> clk_hw_create_clk() ->
alloc_clk() -> kzalloc_obj() to allocate the clk struct, so clk_put()
must be called to release this memory. Without it, the allocated clk
struct is leaked on every probe failure after of_clk_get() succeeds.

Introduce a clk_put label between the success return and fman_node_put
in the unwind chain, and redirect all error paths after of_clk_get()
to this new label. Since no goto target remains for fman_free, fold
it into fman_node_put and remove the now-unused label.

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 drivers/net/ethernet/freescale/fman/fman.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 013273a2de32..734cbe8efd7e 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2736,7 +2736,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 		err = -EINVAL;
 		dev_err(&of_dev->dev, "%s: Failed to determine FM%d clock rate\n",
 			__func__, fman->dts_params.id);
-		goto fman_node_put;
+		goto clk_put;
 	}
 	/* Rounding to MHz */
 	fman->dts_params.clk_freq = DIV_ROUND_UP(clk_rate, 1000000);
@@ -2746,7 +2746,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	if (err) {
 		dev_err(&of_dev->dev, "%s: failed to read fsl,qman-channel-range for %pOF\n",
 			__func__, fm_node);
-		goto fman_node_put;
+		goto clk_put;
 	}
 	fman->dts_params.qman_channel_base = range[0];
 	fman->dts_params.num_of_qman_channels = range[1];
@@ -2757,7 +2757,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 		err = -EINVAL;
 		dev_err(&of_dev->dev, "%s: could not find MURAM node\n",
 			__func__);
-		goto fman_free;
+		goto clk_put;
 	}
 
 	err = of_address_to_resource(muram_node, 0,
@@ -2766,7 +2766,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 		of_node_put(muram_node);
 		dev_err(&of_dev->dev, "%s: of_address_to_resource() = %d\n",
 			__func__, err);
-		goto fman_free;
+		goto clk_put;
 	}
 
 	of_node_put(muram_node);
@@ -2776,7 +2776,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	if (err < 0) {
 		dev_err(&of_dev->dev, "%s: irq %d allocation failed (error = %d)\n",
 			__func__, irq, err);
-		goto fman_free;
+		goto clk_put;
 	}
 
 	if (fman->dts_params.err_irq != 0) {
@@ -2786,7 +2786,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 		if (err < 0) {
 			dev_err(&of_dev->dev, "%s: irq %d allocation failed (error = %d)\n",
 				__func__, fman->dts_params.err_irq, err);
-			goto fman_free;
+			goto clk_put;
 		}
 	}
 
@@ -2794,7 +2794,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	if (IS_ERR(base_addr)) {
 		err = PTR_ERR(base_addr);
 		dev_err(&of_dev->dev, "%s: devm_ioremap() failed\n", __func__);
-		goto fman_free;
+		goto clk_put;
 	}
 
 	fman->dts_params.base_addr = base_addr;
@@ -2806,7 +2806,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	if (err) {
 		dev_err(&of_dev->dev, "%s: of_platform_populate() failed\n",
 			__func__);
-		goto fman_free;
+		goto clk_put;
 	}
 
 #ifdef CONFIG_DPAA_ERRATUM_A050385
@@ -2816,9 +2816,10 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 
 	return fman;
 
+clk_put:
+	clk_put(clk);
 fman_node_put:
 	of_node_put(fm_node);
-fman_free:
 	kfree(fman);
 	return ERR_PTR(err);
 }
-- 
2.20.1

RE: [PATCH] net: fman: fix clock and device node leak in probe error paths

[Madalin Bucur]

> -----Original Message-----
> From: ZhaoJinming <zhaojinming@uniontech.com>
> Sent: Thursday, June 18, 2026 10:55 AM
> To: Madalin Bucur <madalin.bucur@nxp.com>
> Cc: Sean Anderson <sean.anderson@linux.dev>; Andrew Lunn
> <andrew+netdev@lunn.ch>; David S. Miller <davem@davemloft.net>; Eric
> Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
> Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; ZhaoJinming <zhaojinming@uniontech.com>
> Subject: [PATCH] net: fman: fix clock and device node leak in probe error paths
> 
> In read_dts_node(), fm_node is acquired via of_node_get() and clk is
> acquired via of_clk_get(). After a successful of_clk_get() call, the
> error paths for clk_get_rate failure and of_property_read_u32_array
> failure correctly goto fman_node_put (releasing fm_node) but miss
> clk_put(clk).
> 
> Worse, all error paths from the MURAM node lookup onward goto
> fman_free directly, skipping both fman_node_put and clk_put, leaking
> both the fm_node and clk references.
> 
> of_clk_get() eventually calls __of_clk_get() -> clk_hw_create_clk() ->
> alloc_clk() -> kzalloc_obj() to allocate the clk struct, so clk_put()
> must be called to release this memory. Without it, the allocated clk
> struct is leaked on every probe failure after of_clk_get() succeeds.
> 
> Introduce a clk_put label between the success return and fman_node_put
> in the unwind chain, and redirect all error paths after of_clk_get()
> to this new label. Since no goto target remains for fman_free, fold
> it into fman_node_put and remove the now-unused label.
> 
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
> ---
>  drivers/net/ethernet/freescale/fman/fman.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/fman.c
> b/drivers/net/ethernet/freescale/fman/fman.c
> index 013273a2de32..734cbe8efd7e 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c
> @@ -2736,7 +2736,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>                 err = -EINVAL;
>                 dev_err(&of_dev->dev, "%s: Failed to determine FM%d clock rate\n",
>                         __func__, fman->dts_params.id);
> -               goto fman_node_put;
> +               goto clk_put;
>         }
>         /* Rounding to MHz */
>         fman->dts_params.clk_freq = DIV_ROUND_UP(clk_rate, 1000000);
> @@ -2746,7 +2746,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>         if (err) {
>                 dev_err(&of_dev->dev, "%s: failed to read fsl,qman-channel-range
> for %pOF\n",
>                         __func__, fm_node);
> -               goto fman_node_put;
> +               goto clk_put;
>         }
>         fman->dts_params.qman_channel_base = range[0];
>         fman->dts_params.num_of_qman_channels = range[1];
> @@ -2757,7 +2757,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>                 err = -EINVAL;
>                 dev_err(&of_dev->dev, "%s: could not find MURAM node\n",
>                         __func__);
> -               goto fman_free;
> +               goto clk_put;
>         }
> 
>         err = of_address_to_resource(muram_node, 0,
> @@ -2766,7 +2766,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>                 of_node_put(muram_node);
>                 dev_err(&of_dev->dev, "%s: of_address_to_resource() = %d\n",
>                         __func__, err);
> -               goto fman_free;
> +               goto clk_put;
>         }
> 
>         of_node_put(muram_node);
> @@ -2776,7 +2776,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>         if (err < 0) {
>                 dev_err(&of_dev->dev, "%s: irq %d allocation failed (error = %d)\n",
>                         __func__, irq, err);
> -               goto fman_free;
> +               goto clk_put;
>         }
> 
>         if (fman->dts_params.err_irq != 0) {
> @@ -2786,7 +2786,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>                 if (err < 0) {
>                         dev_err(&of_dev->dev, "%s: irq %d allocation failed (error
> = %d)\n",
>                                 __func__, fman->dts_params.err_irq, err);
> -                       goto fman_free;
> +                       goto clk_put;
>                 }
>         }
> 
> @@ -2794,7 +2794,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>         if (IS_ERR(base_addr)) {
>                 err = PTR_ERR(base_addr);
>                 dev_err(&of_dev->dev, "%s: devm_ioremap() failed\n", __func__);
> -               goto fman_free;
> +               goto clk_put;
>         }
> 
>         fman->dts_params.base_addr = base_addr;
> @@ -2806,7 +2806,7 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
>         if (err) {
>                 dev_err(&of_dev->dev, "%s: of_platform_populate() failed\n",
>                         __func__);
> -               goto fman_free;
> +               goto clk_put;
>         }
> 
>  #ifdef CONFIG_DPAA_ERRATUM_A050385
> @@ -2816,9 +2816,10 @@ static struct fman *read_dts_node(struct
> platform_device *of_dev)
> 
>         return fman;
> 
> +clk_put:
> +       clk_put(clk);
>  fman_node_put:
>         of_node_put(fm_node);
> -fman_free:
>         kfree(fman);
>         return ERR_PTR(err);
>  }
> --
> 2.20.1

Acked-by: Madalin Bucur <madalin.bucur@nxp.com>

Thanks

Re: [PATCH] net: fman: fix clock and device node leak in probe error paths

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says: I think the first issue below does warrant being investigated
            within the context of this patch.

	    The second one seems related to the intent of this patch,
	    but I don't feel strongly about it needing to be fixed
	    as part of this work.

	    And the last one seems yet more weakly related.
---
net: fman: fix clock and device node leak in probe error paths

This patch aims to fix resource leaks in the error paths of
read_dts_node(). It introduces a clk_put label to ensure the clock
reference is properly released on probe failures.

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 013273a2de32f..734cbe8efd7e3 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c
[ ... ]
> @@ -2757,7 +2757,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
>  		err = -EINVAL;
>  		dev_err(&of_dev->dev, "%s: could not find MURAM node\n",
>  			__func__);
> -		goto fman_free;
> +		goto clk_put;
>  	}

[Severity: High]
Will this cause a double-free of the device node on error paths?

In read_dts_node(), the call to of_find_matching_node() drops the
reference on its "from" argument, which is fm_node:

muram_node = of_find_matching_node(fm_node, fman_muram_match);

By redirecting the error paths after this point to the clk_put label,
the code falls through to fman_node_put and calls of_node_put(fm_node).
Since of_find_matching_node() already consumed the reference, does this
second put lead to a reference count underflow and use-after-free?

[ ... ]
> @@ -2816,9 +2816,10 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
>  
>  	return fman;

[Severity: High]
This isn't a bug introduced by this patch, but does this function leak the
clock reference when returning successfully?

While the patch correctly adds clk_put(clk) to the error paths, the
success path shown above returns without releasing the reference acquired
via of_clk_get(). Does the driver need to hold this reference throughout
its lifetime, or should it be released here to prevent a memory leak
during repeated driver binds and unbinds?

> +clk_put:
> +	clk_put(clk);
>  fman_node_put:
>  	of_node_put(fm_node);
> -fman_free:
>  	kfree(fman);
>  	return ERR_PTR(err);
>  }

[Severity: High]
This is a pre-existing issue, but is it safe to manually free the fman
structure here when devm-managed shared interrupts are still registered?

The driver allocates fman using kzalloc_obj() and registers shared
interrupt handlers using devm_request_irq(), passing fman as the dev_id.

If initialization fails later in the function, the error path calls
kfree(fman). However, the devm-managed interrupts are not unregistered
until the probe function returns and devres cleanup runs.

Because the interrupt is IRQF_SHARED, could another device trigger an
interrupt during this window? If so, would the still-registered fman_irq
handler attempt to access the already-freed fman structure?