[PATCH] fsl/fman: Fix fm_pdev reference leak in fman_port_probe()

[PATCH] fsl/fman: Fix fm_pdev reference leak in fman_port_probe()

[Wentao Liang]

of_find_device_by_node() takes a reference on the FMan device. It is
released on the error paths, but the successful path returns without
dropping it, leaking the reference.

Release the reference when it is not needed anymore.

Fixes: bf2b09fedc17 ("fsl/fman: Fix missing put_device() call in fman_port_probe")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/net/ethernet/freescale/fman/fman_port.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c b/drivers/net/ethernet/freescale/fman/fman_port.c
index 1b8fef69114e..f44a89016512 100644
--- a/drivers/net/ethernet/freescale/fman/fman_port.c
+++ b/drivers/net/ethernet/freescale/fman/fman_port.c
@@ -1868,6 +1868,8 @@ static int fman_port_probe(struct platform_device *of_dev)
 
 	dev_set_drvdata(&of_dev->dev, port);
 
+	put_device(&fm_pdev->dev);
+
 	return 0;
 
 put_device:
-- 
2.34.1

Re: [PATCH] fsl/fman: Fix fm_pdev reference leak in fman_port_probe()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · 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] Incomplete fix plus inaccurate commit message in
  fman_port_probe(): the commit claims the of_find_device_by_node()…

Pre-existing issues:
- [High] fman_port_probe()
  (drivers/net/ethernet/freescale/fman/fman_port.c:1864-1873) treats a…

--

fsl/fman: Fix fm_pdev reference leak in fman_port_probe()

This adds a put_device(&fm_pdev->dev) on the success path of
fman_port_probe(), so the reference taken by of_find_device_by_node() is
dropped before returning 0.

> diff --git a/drivers/net/ethernet/freescale/fman/fman_port.c b/drivers/net/ethernet/freescale/fman/fman_port.c
> index 1b8fef69114ed..f44a890165127 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_port.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_port.c
> @@ -1868,6 +1868,8 @@ static int fman_port_probe(struct platform_device *of_dev)
>  
>  	dev_set_drvdata(&of_dev->dev, port);
>  
> +	put_device(&fm_pdev->dev);
> +
>  	return 0;
>  
>  put_device:

[Severity: High]
The commit message says the fm_pdev reference "is released on the error
paths, but the successful path returns without dropping it".  Is that
accurate for all error paths in fman_port_probe()?

The __devm_request_region() failure branch jumps to free_port:, which sits
below the put_device: label:

	dev_res = __devm_request_region(port->dev, &res, res.start,
					resource_size(&res), "fman-port");
	if (!dev_res) {
		dev_err(port->dev, "%s: __devm_request_region() failed\n",
			__func__);
		err = -EINVAL;
		goto free_port;
	}
	...
put_device:
	put_device(&fm_pdev->dev);
return_err:
	of_node_put(port_node);
free_port:
	kfree(port);
	return err;

Does that branch still leak the fm_pdev device reference after this patch?

Would a single put_device(&fm_pdev->dev) right after the last use of
fm_pdev/fman cover both returns instead?

	port->dts_params.fman = fman;
	put_device(&fm_pdev->dev);

	of_node_put(port_node);

Note that redirecting free_port: to the existing put_device: label does not
look workable, since return_err: would then run of_node_put(port_node) a
second time after the of_node_put(port_node) already done just before
__devm_request_region().

[Severity: High]
This isn't a bug introduced by this patch, but right above the newly added
put_device() the devm_ioremap() failure is only logged and probe still
returns 0:

	port->dts_params.base_addr = devm_ioremap(port->dev, res.start,
						  resource_size(&res));
	if (!port->dts_params.base_addr)
		dev_err(port->dev, "%s: devm_ioremap() failed\n", __func__);

	dev_set_drvdata(&of_dev->dev, port);

Can this publish a port with a NULL register base?  fman_port_bind() hands
the drvdata to mac_probe() in fman/mac.c, and dpaa_eth_init_tx_port() /
dpaa_eth_init_rx_port() then call fman_port_config(), which derives the
register windows from that base without checking it:

drivers/net/ethernet/freescale/fman/fman_port.c:fman_port_config() {
	void __iomem *base_addr = port->dts_params.base_addr;
	...
	port->bmi_regs = base_addr + BMI_PORT_REGS_OFFSET;
	port->qmi_regs = base_addr + QMI_PORT_REGS_OFFSET;
	port->hwp_regs = base_addr + HWP_PORT_REGS_OFFSET;
	...
		reg = 0x00001013;
		iowrite32be(reg, &port->bmi_regs->tx.fmbm_tfp);
}

With base_addr NULL, does that iowrite32be() for the A005127 errata (rev >=
6, 1G TX) dereference a near-NULL __iomem pointer, followed by the
ioread32be()/iowrite32be() calls in fman_port_init() and
fman_port_enable()?

Should the devm_ioremap() failure set err = -ENOMEM and take the cleanup
path like the other fatal probe errors?

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