[PATCH] fsl/fman: Fix fm_node reference leak in read_dts_node()

[PATCH] fsl/fman: Fix fm_node reference leak in read_dts_node()

[Wentao Liang]

of_node_get() takes a reference on the FMan device node. That reference
is released neither on the success path, which returns straight after
of_platform_populate(), nor on the failure paths that jump to fman_free
without going through fman_node_put(). Neither read_dts_node() nor its
caller keeps fm_node around to release it later.

Release the reference on the success path, and route the remaining
failure paths through fman_node_put().

Fixes: ecb239d96d36 ("ethernet: fman: fix wrong of_node_put() in probe function")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/net/ethernet/freescale/fman/fman.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 0f342b1a845b..403278aa357e 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2758,7 +2758,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 fman_node_put;
 	}
 
 	err = of_address_to_resource(muram_node, 0,
@@ -2767,7 +2767,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 fman_node_put;
 	}
 
 	of_node_put(muram_node);
@@ -2777,7 +2777,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 fman_node_put;
 	}
 
 	if (fman->dts_params.err_irq != 0) {
@@ -2787,7 +2787,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 fman_node_put;
 		}
 	}
 
@@ -2795,7 +2795,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 fman_node_put;
 	}
 
 	fman->dts_params.base_addr = base_addr;
@@ -2807,7 +2807,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 fman_node_put;
 	}
 
 #ifdef CONFIG_DPAA_ERRATUM_A050385
@@ -2815,6 +2815,8 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 		of_property_read_bool(fm_node, "fsl,erratum-a050385");
 #endif
 
+	of_node_put(fm_node);
+
 	return fman;
 
 fman_node_put:
-- 
2.34.1

Re: [PATCH] fsl/fman: Fix fm_node reference leak in read_dts_node()

[Simon Horman]

On Thu, Sep 17, 2026 at 11:03:42AM +0000, Wentao Liang wrote:
> of_node_get() takes a reference on the FMan device node. That reference
> is released neither on the success path, which returns straight after
> of_platform_populate(), nor on the failure paths that jump to fman_free
> without going through fman_node_put(). Neither read_dts_node() nor its
> caller keeps fm_node around to release it later.
> 
> Release the reference on the success path, and route the remaining
> failure paths through fman_node_put().
> 
> Fixes: ecb239d96d36 ("ethernet: fman: fix wrong of_node_put() in probe function")
> Cc: stable@vger.kernel.org
> Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>

Hi Wentao,

As flagged by W=1 builds, with this patch applied the fman_free tag is no
longer used. It should be removed.

Please be sure to check that your patches do not introduce any
new warnings when compiled with W=1.

Link: https://docs.kernel.org/process/maintainer-netdev.html#testing

-- 
pw-bot: changes-requested

[PATCH v2] fsl/fman: Fix fm_node reference leak in read_dts_node()

[Wentao Liang]

of_node_get() takes a reference on the FMan device node. That reference
is released neither on the success path, which returns straight after
of_platform_populate(), nor on the failure paths that jump to fman_free
without going through fman_node_put(). Neither read_dts_node() nor its
caller keeps fm_node around to release it later.

Release the reference on the success path, and route the remaining
failure paths through fman_node_put(). The latter leaves the fman_free
label without any user, so drop it as well: fman_node_put already falls
through to kfree(fman).

The candidate was found with static analysis tools; DeepSeek and GPT-6.1
Sol were used to audit it and to help prepare this fix.

Fixes: ecb239d96d36 ("ethernet: fman: fix wrong of_node_put() in probe function")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
Changes in v2:
- Remove the fman_free label, which no longer has a user once the failure
  paths go through fman_node_put(). With W=1 the leftover label is an
  -Werror=unused-label build failure, as Simon reported.
- Rebuilt with W=1 on v7.4-rc1 (x86_64 defconfig + COMPILE_TEST,
  CONFIG_FSL_FMAN=m); drivers/net/ethernet/freescale/fman/ now builds
  without warnings.
- Link to v1: https://lore.kernel.org/all/20260917110342.2148155-1-vulab@iscas.ac.cn/
- Sorry for the late reply, and thanks for catching the build failure.

 drivers/net/ethernet/freescale/fman/fman.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 46cc28895e56..5507002097fd 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2760,7 +2760,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 fman_node_put;
 	}
 
 	err = of_address_to_resource(muram_node, 0,
@@ -2769,7 +2769,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 fman_node_put;
 	}
 
 	of_node_put(muram_node);
@@ -2779,7 +2779,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 fman_node_put;
 	}
 
 	if (fman->dts_params.err_irq != 0) {
@@ -2789,7 +2789,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 fman_node_put;
 		}
 	}
 
@@ -2797,7 +2797,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 fman_node_put;
 	}
 
 	fman->dts_params.base_addr = base_addr;
@@ -2809,7 +2809,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 fman_node_put;
 	}
 
 #ifdef CONFIG_DPAA_ERRATUM_A050385
@@ -2817,11 +2817,12 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 		of_property_read_bool(fm_node, "fsl,erratum-a050385");
 #endif
 
+	of_node_put(fm_node);
+
 	return fman;
 
 fman_node_put:
 	of_node_put(fm_node);
-fman_free:
 	kfree(fman);
 	return ERR_PTR(err);
 }
-- 
2.43.0

Re: [PATCH v2] fsl/fman: Fix fm_node reference leak in read_dts_node()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] This patch adds a double of_node_put(fm_node), so the
  device_node refcount underflows.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits on the
  error paths the patch edits.
- [High] This problem wasn't introduced by this patch, but the FMan IRQ
  handlers are live before driver state is set up, and the init check…

--

fsl/fman: Fix fm_node reference leak in read_dts_node()

The patch adds an of_node_put(fm_node) on the success path of
read_dts_node(). It also sends the failure paths that used to jump to
fman_free through fman_node_put instead, and removes the now unused
fman_free label.

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 46cc28895e565..5507002097fd4 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c
> @@ -2760,7 +2760,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 fman_node_put;

[Severity: High]
Does this drop a reference that read_dts_node() no longer holds?

Just above this hunk, read_dts_node() does:

	muram_node = of_find_matching_node(fm_node, fman_muram_match);

of_find_matching_node() wraps of_find_matching_node_and_match(from,
matches, NULL), and that function releases from on every path:

drivers/of/base.c:of_find_matching_node_and_match() {
    ...
	for_each_of_allnodes_from(from, np) {
		...
	}
	of_node_put(from);
	raw_spin_unlock_irqrestore(&devtree_lock, flags);
	return np;
}

The MURAM lookup therefore consumes the reference taken by
of_node_get(of_dev->dev.of_node). That means the pre-patch code was
already balanced. Early failures went through fman_node_put, while the
later failures and the success path skipped the put.

After this patch, all six error gotos after the lookup, plus the new
of_node_put(fm_node) before return fman, drop one extra reference on the
FMan device_node. That happens on every successful probe.

The commit named in the Fixes: tag, ecb239d96d36, describes this exact
situation:

  "the probe function ends up calling of_find_matching_node() using the
  node as an argument. The function takes care of decreasing the
  refcount on it. We are then incorrectly decreasing the refcount on that
  node again."

With CONFIG_OF_DYNAMIC, of_node_put() becomes kobject_put(). fman_driver
does not set suppress_bind_attrs, so repeated unbind/bind through sysfs
can drive the count to zero. Static nodes would then hit "ERROR: Bad
of_node_put()". Overlay-created nodes could be freed while
of_dev->dev.of_node and the children created by of_platform_populate()
still point at them.

Given that the leak described in the commit message doesn't seem to
exist, should this patch be dropped rather than respun? Without that,
the Cc: stable tag would also carry the imbalance into stable trees.

>  	}
>  
>  	err = of_address_to_resource(muram_node, 0,

[ ... ]

> @@ -2779,7 +2779,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 fman_node_put;
>  	}

[Severity: High]
This is a pre-existing issue, not one introduced by this patch, but can
fman_irq() run before the driver state it reads has been set up?

fman is allocated with kzalloc_obj(), so fman->cfg and fman->fpm_regs
start out NULL. Both are only set in fman_config(), which fman_probe()
calls after read_dts_node() returns. The shared handlers are registered
here, before that.

is_init_done() treats a NULL cfg as "init done":

static bool is_init_done(struct fman_cfg *cfg)
{
	/* Checks if FMan driver parameters were initialized */
	if (!cfg)
		return true;

	return false;
}

If the line fires in that window, fman_irq() does:

	if (!is_init_done(fman->cfg))
		return IRQ_NONE;

	fpm_rg = fman->fpm_regs;

	/* normal interrupts */
	pending = ioread32be(&fpm_rg->fm_npi);

That is a NULL pointer dereference in hardirq context. fman_err_irq()
has the same window.

The interrupt could come from another device sharing the line. It could
also be an FMan interrupt left enabled by a previous kernel across kexec
or kdump, since fman_driver has no .shutdown or .remove callback.

[ ... ]

> @@ -2817,11 +2817,12 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
>  		of_property_read_bool(fm_node, "fsl,erratum-a050385");
>  #endif
>  
> +	of_node_put(fm_node);
> +
>  	return fman;
>  
>  fman_node_put:
>  	of_node_put(fm_node);
> -fman_free:
>  	kfree(fman);

[Severity: High]
This is also a pre-existing issue, not one introduced by this patch, but
can kfree(fman) here leave live shared IRQ handlers pointing at freed
memory?

read_dts_node() passes fman as dev_id to devm_request_irq() with
IRQF_SHARED for both fman_irq() and fman_err_irq(). fman itself is plain
kzalloc memory and devres does not manage it.

Three failures after the IRQ requests reach this kfree(fman) while the
handlers are still installed:

  - a failed err_irq request
  - a failed devm_platform_get_and_ioremap_resource()
  - a failed of_platform_populate()

fman_config() has the same pattern:

err_fm_state:
	kfree(fman);
	return -EINVAL;

free_irq() only runs later, from devres_release_all(), after
fman_probe() returns the error. A shared interrupt in that window would
call fman_irq() on the freed object.

With CONFIG_DEBUG_SHIRQ, __free_irq() always calls the shared handler
once during teardown, so this would happen on every such failure.

On the read_dts_node() failure paths, the freed object probably still
holds cfg == NULL. is_init_done() would then return true, and fman_irq()
would read fman->fpm_regs and do MMIO through a stale pointer. If the
read returns nonzero, call_mac_isr() makes indirect calls through
function pointers in the freed object.

>  	return ERR_PTR(err);
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107072941.314160-1-vulab%40iscas.ac.cn