[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?

[PATCH v2 1/2] net: fman: fix clk reference leak in read_dts_node()

[ZhaoJinming]

of_clk_get() returns a reference that must be released with clk_put()
when the clock is no longer needed. The current code never calls
clk_put(clk), leaking the reference on both the success path and the
clk_rate == 0 error path.

Add clk_put(clk) after the clock rate is consumed on the success path,
and jump to a new clk_put label on the error path to properly release
the clock reference.

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

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 013273a2de32..31b0081bdf91 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2736,11 +2736,13 @@ 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);
 
+	clk_put(clk);
+
 	err = of_property_read_u32_array(fm_node, "fsl,qman-channel-range",
 					 &range[0], 2);
 	if (err) {
@@ -2818,6 +2820,8 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 
 fman_node_put:
 	of_node_put(fm_node);
+clk_put:
+	clk_put(clk);
 fman_free:
 	kfree(fman);
 	return ERR_PTR(err);
-- 
2.20.1

[PATCH v2 2/2] net: fman: use devm_kzalloc() for fman and rely on devres

[ZhaoJinming]

The driver now allocates the top-level struct fman with devm_kzalloc()
so that its lifetime is bound to the device and resources are released
automatically by the driver core on probe failure or device removal.

Remove the explicit kfree(fman) from the error paths in fman_config()
and read_dts_node() to avoid double-free/use-after-free and to follow
the devm_ allocation convention.

After of_find_matching_node() consumes fm_node's reference via
of_node_put(from), the post-muram error paths no longer need to clean
up fm_node, so replace goto fman_free with direct return ERR_PTR(err).

This change complements the existing use of devm_* resources (irq,
ioremap, etc.) and simplifies the error handling paths.

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

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 31b0081bdf91..23b938afe17a 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -1793,8 +1793,6 @@ static int fman_config(struct fman *fman)
 	kfree(fman->cfg);
 err_fm_drv:
 	kfree(fman->state);
-err_fm_state:
-	kfree(fman);
 	return -EINVAL;
 }
 
@@ -2697,7 +2695,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	struct clk *clk;
 	u32 clk_rate;
 
-	fman = kzalloc_obj(*fman);
+	fman = devm_kzalloc(&of_dev->dev, sizeof(*fman), GFP_KERNEL);
 	if (!fman)
 		return ERR_PTR(-ENOMEM);
 
@@ -2759,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;
+		return ERR_PTR(err);
 	}
 
 	err = of_address_to_resource(muram_node, 0,
@@ -2768,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;
+		return ERR_PTR(err);
 	}
 
 	of_node_put(muram_node);
@@ -2778,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;
+		return ERR_PTR(err);
 	}
 
 	if (fman->dts_params.err_irq != 0) {
@@ -2788,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;
+			return ERR_PTR(err);
 		}
 	}
 
@@ -2796,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;
+		return ERR_PTR(err);
 	}
 
 	fman->dts_params.base_addr = base_addr;
@@ -2808,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;
+		return ERR_PTR(err);
 	}
 
 #ifdef CONFIG_DPAA_ERRATUM_A050385
@@ -2822,8 +2820,6 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	of_node_put(fm_node);
 clk_put:
 	clk_put(clk);
-fman_free:
-	kfree(fman);
 	return ERR_PTR(err);
 }
 
-- 
2.20.1

Re: [PATCH v2 1/2] net: fman: fix clk reference leak in read_dts_node()

[Andrew Lunn]

On Mon, Jun 22, 2026 at 05:05:04PM +0800, ZhaoJinming wrote:
> of_clk_get() returns a reference that must be released with clk_put()
> when the clock is no longer needed. The current code never calls
> clk_put(clk), leaking the reference on both the success path and the
> clk_rate == 0 error path.
> 
> Add clk_put(clk) after the clock rate is consumed on the success path,
> and jump to a new clk_put label on the error path to properly release
> the clock reference.

"When the clock is no longer needed": So once you know the rate the
clock ticks at, you no longer need the clock? It is O.K. for it to
disappear, since there is no reference to it?

    Andrew

---
pw-bot: cr

Re: [PATCH v2 2/2] net: fman: use devm_kzalloc() for fman and rely on devres

[Andrew Lunn]

On Mon, Jun 22, 2026 at 05:05:05PM +0800, ZhaoJinming wrote:
> The driver now allocates the top-level struct fman with devm_kzalloc()
> so that its lifetime is bound to the device and resources are released
> automatically by the driver core on probe failure or device removal.
> 
> Remove the explicit kfree(fman) from the error paths in fman_config()
> and read_dts_node() to avoid double-free/use-after-free and to follow
> the devm_ allocation convention.
> 
> After of_find_matching_node() consumes fm_node's reference via
> of_node_put(from), the post-muram error paths no longer need to clean
> up fm_node, so replace goto fman_free with direct return ERR_PTR(err).
> 
> This change complements the existing use of devm_* resources (irq,
> ioremap, etc.) and simplifies the error handling paths.
> 
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>

Please take a read of:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Please read it all, but see section 1.7.4.

    Andrew

---
pw-bot: cr

Re: [PATCH v2 2/2] net: fman: use devm_kzalloc() for fman and rely on devres

[赵金明]

Hi Andrew,

Thank you for pointing me to the netdev maintainer documentation. I have
read section 1.7.4 and I understand the concern about standalone
cleanup conversions.

I would like to clarify the actual motivation behind the
devm_kzalloc() change. While it may appear to be a simple devm_
conversion on the surface, it is in fact fixing a use-after-free race
condition in the IRQF_SHARED error paths. Let me explain the problem
in detail.

PROBLEM
=======

In read_dts_node(), the driver allocates fman with kzalloc_obj() and
then registers shared interrupt handlers via devm_request_irq(), with
fman passed as the dev_id:

	L2700: fman = kzalloc_obj(*fman);
	...
	L2774: devm_request_irq(&of_dev->dev, irq, fman_irq,
	                         IRQF_SHARED, "fman", fman);
	L2783: devm_request_irq(&of_dev->dev, err_irq, fman_err_irq,
	                         IRQF_SHARED, "fman-err", fman);

The devres list after successful registration of these resources is:

	[fman (manual)] -> [main_irq handler] -> [err_irq handler] -> [ioremap]

There are three error paths after main_irq has been registered that
hit the fman_free label and call kfree(fman):

	- devm_request_irq(err_irq) fails         (L2779)  -- main_irq still registered
	- devm_platform_get_and_ioremap_resource() fails (L2797)  -- both IRQs registered
	- of_platform_populate() fails            (L2809)  -- both IRQs registered

Note that on the err_irq failure path (L2779), err_irq was not
successfully registered, so only main_irq remains in the devres list.
On the other two paths, both IRQ handlers are registered.

Taking of_platform_populate() failure as an example, the full error
cleanup sequence is:

  1. of_platform_populate() fails
  2. goto fman_free
  3. kfree(fman)                          -- fman is freed, but dev_id
                                            still points to this memory
  4. read_dts_node() returns ERR_PTR(err)
  5. fman_probe() returns error code
  6. Driver core calls device_unbind_cleanup() -> devres_release_all(),
     which releases devres resources in LIFO order:
     - Step 6a: free ioremap
     - Step 6b: devm_free_irq(err_irq)       -- err_irq handler unregistered
     - Step 6c: devm_free_irq(main_irq)      -- main_irq handler unregistered

     Until step 6c completes, at least one IRQ handler remains registered
     with the interrupt subsystem. During the window between step 3
     (kfree) and step 6c, a shared IRQ may fire and the handler will
     dereference the already-freed fman.

Because the handlers are registered with IRQF_SHARED, the kernel will
call fman_irq() and fman_err_irq() even when the interrupt is triggered
by another device sharing the same IRQ line.

Both handlers immediately dereference the fman pointer:

	static irqreturn_t fman_irq(int irq, void *handle)
	{
	    struct fman *fman = (struct fman *)handle;
	    if (!is_init_done(fman->cfg))      -- dereferences freed fman
	        return IRQ_NONE;
	    ...
	}

The same issue exists in fman_config(). Its err_fm_state error path
calls kfree(fman) while the devm IRQ handlers registered earlier in
read_dts_node() are still active:

	err_fm_state:
	    kfree(fman);                        -- free fman
	    return -EINVAL;                     -- devres cleanup runs after return

When fman_config() fails, fman_probe() returns -EINVAL at L2841 without
any cleanup, and the driver core then calls devres_release_all() which
releases the IRQ handlers -- again after fman has already been freed.

HOW devm_kzalloc() FIXES IT
============================

By allocating fman with devm_kzalloc(), it becomes the first entry in
the devres list:

	devres list: [fman] -> [main_irq] -> [err_irq] -> [ioremap]

Devres releases resources in LIFO order:

	1. free ioremap
	2. devm_free_irq(err_irq)               -- handlers unregistered
	3. devm_free_irq(main_irq)              -- handlers unregistered
	4. devm_kfree(fman)                     -- fman freed last, no UAF

Since fman is freed only after all IRQ handlers have been unregistered,
the use-after-free window is completely eliminated.

The manual kfree(fman) calls must also be removed to avoid double-free,
which is why they are dropped in this patch along with the allocation
change.

I will rework the patch with a commit message that clearly describes
this as a UAF fix rather than a cleanup conversion. Please let me know
if this explanation addresses the concern.

Best regards,
ZhaoJinming




>On Mon, Jun 22, 2026 at 05:05:05PM +0800, ZhaoJinming wrote:



>> The driver now allocates the top-level struct fman with devm_kzalloc()



>> so that its lifetime is bound to the device and resources are released



>> automatically by the driver core on probe failure or device removal.



>> 



>> Remove the explicit kfree(fman) from the error paths in fman_config()



>> and read_dts_node() to avoid double-free/use-after-free and to follow



>> the devm_ allocation convention.



>> 



>> After of_find_matching_node() consumes fm_node's reference via



>> of_node_put(from), the post-muram error paths no longer need to clean



>> up fm_node, so replace goto fman_free with direct return ERR_PTR(err).



>> 



>> This change complements the existing use of devm_* resources (irq,



>> ioremap, etc.) and simplifies the error handling paths.



>> 



>> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>



>



>Please take a read of:



>



>https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html



>



>Please read it all, but see section 1.7.4.



>



>??? Andrew



>



>---



>pw-bot: cr



>

Re: [PATCH v2 2/2] net: fman: use devm_kzalloc() for fman and rely on devres

[Andrew Lunn]

On Tue, Jun 23, 2026 at 02:16:25PM +0800, 赵金明 wrote:
> Hi Andrew,
> 
> Thank you for pointing me to the netdev maintainer documentation. I have
> read section 1.7.4 and I understand the concern about standalone
> cleanup conversions.
> 
> I would like to clarify the actual motivation behind the
> devm_kzalloc() change. While it may appear to be a simple devm_
> conversion on the surface, it is in fact fixing a use-after-free race
> condition in the IRQF_SHARED error paths. Let me explain the problem
> in detail.

Please make the commit message explain what the fix is, rather then
saying converting to devm_.

But i also hope you also see why we don't like devm_ conversions,
because developers get them wrong like this. And all too often, they
do the conversion without actual hardware to test it with. So it
results in more bugs, not less.

	   Andrew

Re: [PATCH v2 2/2] net: fman: use devm_kzalloc() for fman and rely on devres

[ZhaoJinming]

Thank you for the feedback. I fully understand the concern about
devm_ conversions. I have reworked the patch to avoid the devm_kzalloc()
approach entirely. The new version explicitly calls devm_free_irq() on
the error paths before kfree(fman), keeping the original kzalloc_obj()
allocation unchanged. The fix targets the same UAF problem but without
introducing any devm_ allocation conversion.

v3:
  - Drop devm_kzalloc() approach
  - Fix by explicitly calling devm_free_irq() before kfree(fman)
    on all post-IRQ-registration error paths
  - Add conditional check for err_irq before devm_free_irq() in
    read_dts_node() to handle the case where err_irq is not registered

Best regards,
ZhaoJinming

[PATCH v3] net: fman: fix use-after-free on IRQF_SHARED handler after probe failure

[ZhaoJinming]

In read_dts_node(), the fman structure is allocated with kzalloc_obj()
and then passed as dev_id to devm_request_irq() when registering
shared interrupt handlers:

    devm_request_irq(&of_dev->dev, irq, fman_irq, IRQF_SHARED, fman, fman);
    devm_request_irq(&of_dev->dev, err_irq, fman_err_irq, IRQF_SHARED,
                       fman-err, fman);

On error paths after IRQ registration (err_irq request failure,
ioremap failure, of_platform_populate failure), the error handling
jumps to the fman_free label which calls kfree(fman) and returns
ERR_PTR(err) from read_dts_node().

fman_probe() then returns the error to the driver core, which invokes
device_unbind_cleanup() -> devres_release_all() to clean up devres
resources in LIFO order. Since fman was allocated with kzalloc_obj()
(not devm), it was already freed at this point. However, the devm IRQ
handlers are still registered and will only be released by the
subsequent devres_release_all() call:

    kfree(fman)              <- fman freed, dev_id points to freed memory
    fman_probe() returns error
    devres_release_all():
      - free ioremap
      - devm_free_irq(err_irq)    <- handler still registered
      - devm_free_irq(main_irq)   <- handler still registered

During the window between kfree(fman) and devm_free_irq(main_irq),
the still-registered IRQF_SHARED handler may fire on behalf of another
device sharing the same IRQ line. The handler will dereference the
already-freed fman pointer:

    static irqreturn_t fman_irq(int irq, void *handle)
    {
        struct fman *fman = (struct fman *)handle;
        if (!is_init_done(fman->cfg))    <- accesses freed memory
            return IRQ_NONE;

The same problem exists in fman_config(). When fman_config() fails
at the err_fm_state error path, it calls kfree(fman) and returns
-EINVAL. fman_probe() returns this error without any cleanup, and
the driver core releases the IRQ handlers after fman has already
been freed.

Fix by explicitly calling devm_free_irq() before kfree(fman) on
all post-IRQ-registration error paths. devm_free_irq() removes the
IRQ from both the interrupt subsystem and the devres list, so
devres_release_all() will not attempt to free it again. This ensures
the IRQ handlers are fully unregistered before fman is freed,
eliminating the UAF window.

Store the main IRQ number in struct fman_dts_params so that
fman_config() can also access it for cleanup.

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 drivers/net/ethernet/freescale/fman/fman.c | 17 ++++++++++++++---
 drivers/net/ethernet/freescale/fman/fman.h |  1 +
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 013273a2de32..ba2338da0cea 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -1794,6 +1794,9 @@ static int fman_config(struct fman *fman)
 err_fm_drv:
 	kfree(fman->state);
 err_fm_state:
+	if (fman->dts_params.err_irq != 0)
+		devm_free_irq(fman->dev, fman->dts_params.err_irq, fman);
+	devm_free_irq(fman->dev, fman->dts_params.irq, fman);
 	kfree(fman);
 	return -EINVAL;
 }
@@ -2716,6 +2719,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	if (err < 0)
 		goto fman_node_put;
 	irq = err;
+	fman->dts_params.irq = irq;
 
 	/* Get the FM error interrupt */
 	err = platform_get_irq(of_dev, 1);
@@ -2786,7 +2790,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 free_main_irq;
 		}
 	}
 
@@ -2794,7 +2798,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 free_irqs;
 	}
 
 	fman->dts_params.base_addr = base_addr;
@@ -2806,7 +2810,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 free_irqs;
 	}
 
 #ifdef CONFIG_DPAA_ERRATUM_A050385
@@ -2816,6 +2820,13 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 
 	return fman;
 
+free_irqs:
+	if (fman->dts_params.err_irq != 0)
+		devm_free_irq(&of_dev->dev, fman->dts_params.err_irq, fman);
+free_main_irq:
+	devm_free_irq(&of_dev->dev, irq, fman);
+	goto fman_free;
+
 fman_node_put:
 	of_node_put(fm_node);
 fman_free:
diff --git a/drivers/net/ethernet/freescale/fman/fman.h b/drivers/net/ethernet/freescale/fman/fman.h
index 74eb62eba0d7..d05f857c1c16 100644
--- a/drivers/net/ethernet/freescale/fman/fman.h
+++ b/drivers/net/ethernet/freescale/fman/fman.h
@@ -286,6 +286,7 @@ struct fman_dts_params {
 	struct resource *res;                   /* FMan memory resource */
 	u8 id;                                  /* FMan ID */
 
+	int irq;                                /* FMan main IRQ */
 	int err_irq;                            /* FMan Error IRQ */
 
 	u16 clk_freq;                           /* FMan clock freq (In Mhz) */
-- 
2.20.1

Re: [PATCH v3] net: fman: fix use-after-free on IRQF_SHARED handler after probe failure

[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
---
net: fman: fix use-after-free on IRQF_SHARED handler after probe failure

This patch fixes a use-after-free in the error path of read_dts_node(). It
manually releases the registered IRQ handlers before freeing the fman
structure, preventing the devres cleanup from leaving a registered IRQ
pointing to freed memory.

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 013273a2de32f..ba2338da0cea1 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c
[ ... ]
> @@ -2786,7 +2790,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 free_main_irq;

[Severity: High]
This is a pre-existing issue, but since we are modifying the IRQ error
handling here, could the shared interrupt handler crash if it fires during
this setup phase?

Just before this snippet in read_dts_node(), devm_request_irq() is called
with IRQF_SHARED while fman is still partially initialized. Since fman was
allocated with kzalloc_obj(), fman->cfg and fman->fpm_regs are NULL.

If a shared interrupt fires immediately, fman_irq() is called:

drivers/net/ethernet/freescale/fman/fman.c:fman_irq() {
        ...
        if (!is_init_done(fman->cfg))
                return IRQ_NONE;

        fpm_rg = fman->fpm_regs;
        pending = ioread32be(&fpm_rg->fm_npi);
        ...
}

Since fman->cfg is NULL, is_init_done() behaves like this:

drivers/net/ethernet/freescale/fman/fman.c:is_init_done() {
        if (!cfg)
                return true;
        ...
}

Would this cause fman_irq() to bypass the early return, proceed with a NULL
fpm_rg pointer, and dereference it during the ioread32be() call?

>  		}
>  	}