[PATCH] pinctrl: stm32: Fix reset control leak in stm32_pctl_probe() error paths

[PATCH] pinctrl: stm32: Fix reset control leak in stm32_pctl_probe() error paths

[Xiangwu Lai]

stm32_pctl_probe() acquires an exclusive reset control for every GPIO
bank with of_reset_control_get_exclusive(), but none of the error
paths release them again: the bank loop returns directly on
-EPROBE_DEFER and on a failed clock lookup, and the
clk_bulk_prepare_enable() failure and the err_register path also
leave the already acquired reset controls behind.

On success the reset controls are kept for the lifetime of the device
and are never released either (the driver has no .remove), but that
is harmless.  On a failed probe, and in particular when the probe is
retried after -EPROBE_DEFER, every attempt leaks another set of
reset control references.

Release the acquired reset controls on all error paths.

Fixes: bb949ed9b16b ("pinctrl: stm32: Switch to use for_each_gpiochip_node() helper")
Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
---
 drivers/pinctrl/stm32/pinctrl-stm32.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 98cf9c6f4..49a8dcc9e 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -1966,15 +1966,17 @@ int stm32_pctl_probe(struct platform_device *pdev)
 		bank->rstc = of_reset_control_get_exclusive(np, NULL);
 		if (PTR_ERR(bank->rstc) == -EPROBE_DEFER) {
 			fwnode_handle_put(child);
-			return -EPROBE_DEFER;
+			ret = -EPROBE_DEFER;
+			goto err_put_rstcs;
 		}
 
 		pctl->clks[i].clk = of_clk_get_by_name(np, NULL);
 		if (IS_ERR(pctl->clks[i].clk)) {
 			fwnode_handle_put(child);
 			clk_bulk_put(i, pctl->clks);
-			return dev_err_probe(dev, PTR_ERR(pctl->clks[i].clk),
-					     "failed to get clk\n");
+			ret = dev_err_probe(dev, PTR_ERR(pctl->clks[i].clk),
+					    "failed to get clk\n");
+			goto err_put_rstcs;
 		}
 		pctl->clks[i].id = "pctl";
 		i++;
@@ -1984,7 +1986,7 @@ int stm32_pctl_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "failed to prepare_enable clk (%d)\n", ret);
 		clk_bulk_put(banks, pctl->clks);
-		return ret;
+		goto err_put_rstcs;
 	}
 
 	for_each_gpiochip_node(dev, child) {
@@ -2009,6 +2011,10 @@ int stm32_pctl_probe(struct platform_device *pdev)
 
 	clk_bulk_disable_unprepare(banks, pctl->clks);
 	clk_bulk_put(banks, pctl->clks);
+err_put_rstcs:
+	for (i = 0; i < banks; i++)
+		if (!IS_ERR_OR_NULL(pctl->banks[i].rstc))
+			reset_control_put(pctl->banks[i].rstc);
 	return ret;
 }
 EXPORT_SYMBOL(stm32_pctl_probe);

Re: [PATCH] pinctrl: stm32: Fix reset control leak in stm32_pctl_probe() error paths

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] pinctrl: stm32: incorrect Fixes trailer in commit message
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] pinctrl: stm32: clock leak in stm32_pctl_probe() [1]
--

commit 7844cefd15f00547f16870f8594d1153fe131bff
Author: Xiangwu Lai <laixiangwu@cmss.chinamobile.com>

> pinctrl: stm32: Fix reset control leak in stm32_pctl_probe() error paths
>
> stm32_pctl_probe() acquires an exclusive reset control for every GPIO
> bank with of_reset_control_get_exclusive(), but none of the error
> paths release them again: the bank loop returns directly on
> -EPROBE_DEFER and on a failed clock lookup, and the
> clk_bulk_prepare_enable() failure and the err_register path also
> leave the already acquired reset controls behind.
>
> On success the reset controls are kept for the lifetime of the device
> and are never released either (the driver has no .remove), but that
> is harmless.  On a failed probe, and in particular when the probe is
> retried after -EPROBE_DEFER, every attempt leaks another set of
> reset control references.
>
> Release the acquired reset controls on all error paths.
>
> Fixes: bb949ed9b16b ("pinctrl: stm32: Switch to use for_each_gpiochip_node() helper")
> Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>

[Severity: Low]
Commit bb949ed9b16b ("pinctrl: stm32: Switch to use for_each_gpiochip_node()
helper") only replaced the child-node iteration macro without changing how
bank->rstc was acquired or cleaned up.  Should the Fixes tag instead point
to 2254e77665d5 ("pinctrl: stm32: defer probe if reset resource is not yet
ready"), which moved the of_reset_control_get_exclusive() loop into
stm32_pctl_probe() and added the -EPROBE_DEFER return path?

> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 405ebb22c9eaf..02410e57f97d7 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -1963,15 +1963,17 @@ int stm32_pctl_probe(struct platform_device *pdev)
>  		bank->rstc = of_reset_control_get_exclusive(np, NULL);
>  		if (PTR_ERR(bank->rstc) == -EPROBE_DEFER) {
>  			fwnode_handle_put(child);
> -			return -EPROBE_DEFER;
> +			ret = -EPROBE_DEFER;
> +			goto err_put_rstcs;
>  		}

[Severity: Medium]
This problem wasn't introduced by this patch, but does jumping to
err_put_rstcs without calling clk_bulk_put(i, pctl->clks) leak the clock
references [1] acquired in earlier iterations of the loop when
of_reset_control_get_exclusive() returns -EPROBE_DEFER on a subsequent bank
(i > 0)?

[1] https://sashiko.dev/#/bug/linux-022b050a-8471-43dd-a330-d8c0a082f0ff

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110065722.1340-1-laixiangwu@cmss.chinamobile.com?part=1

Re: [PATCH] pinctrl: stm32: Fix reset control leak in stm32_pctl_probe() error paths

[Xiangwu Lai]

From: laixiangwu <laixiangwu@cmss.chinamobile.com>

Hi,

thanks for the review.  You are right on both points.

The Fixes tag indeed points at the wrong commit: bb949ed9b16b only
replaced the child node iteration helper, while 2254e77665d5
("pinctrl: stm32: defer probe if reset resource is not yet ready") is
the commit that moved the of_reset_control_get_exclusive() calls into
stm32_pctl_probe() and added the -EPROBE_DEFER return path.  The tag
is corrected in v2.

The clock leak on the -EPROBE_DEFER path is real as well: the banks
processed before the deferred one have already taken their clock
references, and the deferral return does not release them.  This path
was missed by dfd8259507c5 ("pinctrl: stm32: Fix clock reference leak
in stm32_pctl_probe()"), which added clk_bulk_put() to the other
error paths.  Since the deferral path is restructured by this patch
anyway, v2 also releases the clocks there.

v2 follows shortly.

Best regards,
laixiangwu