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

[Simon Horman <horms@kernel.org>]

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?