[PATCH v4] clocksource/timer-imx-gpt: Prevent resource leake in error path.

[PATCH v4] clocksource/timer-imx-gpt: Prevent resource leake in error path.

[Arvind Yadav]

-Free previously allocated memory.
-Unmap I/O memory from kernel address space.

Signed-off-by: Arvind Yadav <arvind.yadav.cs@gmail.com>
---
 drivers/clocksource/timer-imx-gpt.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/clocksource/timer-imx-gpt.c b/drivers/clocksource/timer-imx-gpt.c
index f595460..022dfdf 100644
--- a/drivers/clocksource/timer-imx-gpt.c
+++ b/drivers/clocksource/timer-imx-gpt.c
@@ -489,12 +489,16 @@ static int __init mxc_timer_init_dt(struct device_node *np,  enum imx_gpt_type t
 		return -ENOMEM;
 
 	imxtm->base = of_iomap(np, 0);
-	if (!imxtm->base)
-		return -ENXIO;
+	if (!imxtm->base) {
+		ret = -ENXIO;
+		goto error_free;
+	}
 
 	imxtm->irq = irq_of_parse_and_map(np, 0);
-	if (imxtm->irq <= 0)
-		return -EINVAL;
+	if (imxtm->irq <= 0) {
+		ret = -EINVAL;
+		goto error_iounmap;
+	}
 
 	imxtm->clk_ipg = of_clk_get_by_name(np, "ipg");
 
@@ -507,11 +511,17 @@ static int __init mxc_timer_init_dt(struct device_node *np,  enum imx_gpt_type t
 
 	ret = _mxc_timer_init(imxtm);
 	if (ret)
-		return ret;
+		goto error_iounmap;
 
 	initialized = 1;
 
 	return 0;
+
+error_iounmap:
+	iounmap(imxtm->base);
+error_free:
+	kfree(imxtm);
+	return ret;
 }
 
 static int __init imx1_timer_init_dt(struct device_node *np)
-- 
2.7.4

Re: [PATCH v4] clocksource/timer-imx-gpt: Prevent resource leake in error path.

[Thomas Gleixner]

On Wed, 28 Sep 2016, Arvind Yadav wrote:

> -Free previously allocated memory.
> -Unmap I/O memory from kernel address space.

Ok. So we have proper looking patch with a proper looking changelog
now. Though
 
> @@ -507,11 +511,17 @@ static int __init mxc_timer_init_dt(struct device_node *np,  enum imx_gpt_type t
>  
>  	ret = _mxc_timer_init(imxtm);
>  	if (ret)
> -		return ret;
> +		goto error_iounmap;

Have you actually thought about the implications of this?

Assume the last function call() which gets invoked via _mxc_timer_init()
fails and then figure out what the iounmap() and the kfree() are going to
cause.

Hint: You just turned a resource leak into a fatal crash.

Just slapping unmap/kfree blindly at everything which can return a failure
is not a good idea if you do not check what the functions actually do and
what the error pathes there are.

While I agree that the error handling in this and in other drivers is
lousy, fixing it just mechanicaly and thereby introducing harder to debug
wreckage is certainly not the right approach.

Thanks,

	tglx