[PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[Navid Emamdoost]

In the impelementation of ttc_setup_clockevent() the allocated memory
for ttcce should be released if clk_notifier_register() fails.

Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/clocksource/timer-cadence-ttc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 88fe2e9ba9a3..b40fc6581389 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
 				    &ttcce->ttc.clk_rate_change_nb);
 	if (err) {
 		pr_warn("Unable to register clock notifier.\n");
+		kfree(ttcce);
 		return err;
 	}
 
-- 
2.17.1

Re: [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[Markus Elfring]

> In the impelementation of ttc_setup_clockevent() the allocated memory
> for ttcce should be released if clk_notifier_register() fails.

* Please avoid the copying of typos from previous change descriptions.

* Under which circumstances will an “imperative mood” matter for you here?
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n151


> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>  				    &ttcce->ttc.clk_rate_change_nb);
>  	if (err) {
>  		pr_warn("Unable to register clock notifier.\n");
> +		kfree(ttcce);
>  		return err;
>  	}

This addition looks correct.
But I would prefer to move such exception handling code to the end of
this function implementation so that duplicate source code will be reduced.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n450

Regards,
Markus

Re: [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[Michal Simek]

On 22. 10. 19 10:26, Markus Elfring wrote:
>> In the impelementation of ttc_setup_clockevent() the allocated memory
>> for ttcce should be released if clk_notifier_register() fails.
> 
> * Please avoid the copying of typos from previous change descriptions.
> 
> * Under which circumstances will an “imperative mood” matter for you here?
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n151
> 
> 
>> +++ b/drivers/clocksource/timer-cadence-ttc.c
>> @@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>>  				    &ttcce->ttc.clk_rate_change_nb);
>>  	if (err) {
>>  		pr_warn("Unable to register clock notifier.\n");
>> +		kfree(ttcce);
>>  		return err;
>>  	}
> 
> This addition looks correct.
> But I would prefer to move such exception handling code to the end of
> this function implementation so that duplicate source code will be reduced.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n450

Just a note. Maybe you should also consider to fix this error path in
ttc_setup_clocksource() when notifier also can fail that there is no
need to continue with code execution.

Thanks,
Michal

[PATCH v2] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[Navid Emamdoost]

In the implementation of ttc_setup_clockevent() release the allocated
memory for ttcce if clk_notifier_register() fails.

Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
Changes in v2:
	- Added goto label for error handling
	- Update description and fix typo

 drivers/clocksource/timer-cadence-ttc.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 88fe2e9ba9a3..0caacbc67102 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -411,10 +411,8 @@ static int __init ttc_setup_clockevent(struct clk *clk,
 	ttcce->ttc.clk = clk;
 
 	err = clk_prepare_enable(ttcce->ttc.clk);
-	if (err) {
-		kfree(ttcce);
-		return err;
-	}
+	if (err)
+		goto release_ttcce;
 
 	ttcce->ttc.clk_rate_change_nb.notifier_call =
 		ttc_rate_change_clockevent_cb;
@@ -424,7 +422,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
 				    &ttcce->ttc.clk_rate_change_nb);
 	if (err) {
 		pr_warn("Unable to register clock notifier.\n");
-		return err;
+		goto release_ttcce;
 	}
 
 	ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
@@ -453,15 +451,18 @@ static int __init ttc_setup_clockevent(struct clk *clk,
 
 	err = request_irq(irq, ttc_clock_event_interrupt,
 			  IRQF_TIMER, ttcce->ce.name, ttcce);
-	if (err) {
-		kfree(ttcce);
-		return err;
-	}
+	if (err)
+		goto release_ttcce;
 
 	clockevents_config_and_register(&ttcce->ce,
 			ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
 
 	return 0;
+
+release_ttcce:
+
+	kfree(ttcce);
+	return err;
 }
 
 /**
-- 
2.17.1

[PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource

[Navid Emamdoost]

In the implementation of ttc_setup_clocksource() when
clk_notifier_register() fails the execution should go to error handling.
Additionally, to avoid memory leak the allocated memory for ttccs should
be released, too. So, goto error handling to release the memory and
return.

Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/clocksource/timer-cadence-ttc.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 88fe2e9ba9a3..035e16bc17d3 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
 	ttccs->ttc.clk = clk;
 
 	err = clk_prepare_enable(ttccs->ttc.clk);
-	if (err) {
-		kfree(ttccs);
-		return err;
-	}
+	if (err)
+		goto release_ttccs;
 
 	ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
 
@@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
 
 	err = clk_notifier_register(ttccs->ttc.clk,
 				    &ttccs->ttc.clk_rate_change_nb);
-	if (err)
+	if (err) {
 		pr_warn("Unable to register clock notifier.\n");
+		goto release_ttccs;
+	}
 
 	ttccs->ttc.base_addr = base;
 	ttccs->cs.name = "ttc_clocksource";
@@ -363,16 +363,18 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
 		     ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
 
 	err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
-	if (err) {
-		kfree(ttccs);
-		return err;
-	}
+	if (err)
+		goto release_ttccs;
 
 	ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
 	sched_clock_register(ttc_sched_clock_read, timer_width,
 			     ttccs->ttc.freq / PRESCALE);
 
 	return 0;
+
+release_ttccs:
+	kfree(ttccs);
+	return err;
 }
 
 static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
-- 
2.17.1

Re: [PATCH] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[Navid Emamdoost]

Thanks for the feedback, I updated this patch and sent v2.
Also, I submitted a patch to fix the error handling path in
ttc_setup_clocksource(). Here is the link to it:
https://lore.kernel.org/patchwork/patch/1143242/

On Tue, Oct 22, 2019 at 3:51 AM Michal Simek <michal.simek@xilinx.com> wrote:
>
> On 22. 10. 19 10:26, Markus Elfring wrote:
> >> In the impelementation of ttc_setup_clockevent() the allocated memory
> >> for ttcce should be released if clk_notifier_register() fails.
> >
> > * Please avoid the copying of typos from previous change descriptions.
> >
> > * Under which circumstances will an “imperative mood” matter for you here?
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n151
> >
> >
> >> +++ b/drivers/clocksource/timer-cadence-ttc.c
> >> @@ -424,6 +424,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
> >>                                  &ttcce->ttc.clk_rate_change_nb);
> >>      if (err) {
> >>              pr_warn("Unable to register clock notifier.\n");
> >> +            kfree(ttcce);
> >>              return err;
> >>      }
> >
> > This addition looks correct.
> > But I would prefer to move such exception handling code to the end of
> > this function implementation so that duplicate source code will be reduced.
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=7d194c2100ad2a6dded545887d02754948ca5241#n450
>
> Just a note. Maybe you should also consider to fix this error path in
> ttc_setup_clocksource() when notifier also can fail that there is no
> need to continue with code execution.
>
> Thanks,
> Michal



-- 
Navid.

Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource

[Markus Elfring]

> In the implementation of ttc_setup_clocksource() when
> clk_notifier_register() fails the execution should go to error handling.
> Additionally, to avoid memory leak the allocated memory for ttccs should
> be released, too.

I got other wording preferences. Thus I imagine that such a change
description can still be improved another bit.

How do you think about to omit the word “should” for describing
the previous software situation?


> So, goto error handling to release the memory and return.

Would you like to express the addition of a jump target (according to
the Linux coding style) for the completion of desired exception handling
in a different way?

Regards,
Markus

Re: [PATCH v2] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[Michal Simek]

On 23. 10. 19 6:31, Navid Emamdoost wrote:
> In the implementation of ttc_setup_clockevent() release the allocated
> memory for ttcce if clk_notifier_register() fails.
> 
> Fixes: 70504f311d4b ("clocksource/drivers/cadence_ttc: Convert init function to return error")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
> Changes in v2:
> 	- Added goto label for error handling
> 	- Update description and fix typo
> 
>  drivers/clocksource/timer-cadence-ttc.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
> index 88fe2e9ba9a3..0caacbc67102 100644
> --- a/drivers/clocksource/timer-cadence-ttc.c
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -411,10 +411,8 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>  	ttcce->ttc.clk = clk;
>  
>  	err = clk_prepare_enable(ttcce->ttc.clk);
> -	if (err) {
> -		kfree(ttcce);
> -		return err;
> -	}
> +	if (err)
> +		goto release_ttcce;
>  
>  	ttcce->ttc.clk_rate_change_nb.notifier_call =
>  		ttc_rate_change_clockevent_cb;
> @@ -424,7 +422,7 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>  				    &ttcce->ttc.clk_rate_change_nb);
>  	if (err) {
>  		pr_warn("Unable to register clock notifier.\n");
> -		return err;
> +		goto release_ttcce;
>  	}
>  
>  	ttcce->ttc.freq = clk_get_rate(ttcce->ttc.clk);
> @@ -453,15 +451,18 @@ static int __init ttc_setup_clockevent(struct clk *clk,
>  
>  	err = request_irq(irq, ttc_clock_event_interrupt,
>  			  IRQF_TIMER, ttcce->ce.name, ttcce);
> -	if (err) {
> -		kfree(ttcce);
> -		return err;
> -	}
> +	if (err)
> +		goto release_ttcce;
>  
>  	clockevents_config_and_register(&ttcce->ce,
>  			ttcce->ttc.freq / PRESCALE, 1, 0xfffe);
>  
>  	return 0;
> +
> +release_ttcce:
> +
> +	kfree(ttcce);
> +	return err;
>  }
>  
>  /**
> 

Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal

Re: [PATCH v2] clocksource/drivers: Fix memory leak in ttc_setup_clockevent

[Markus Elfring]

> In the implementation of ttc_setup_clockevent() release the allocated
> memory for ttcce if clk_notifier_register() fails.

I got other wording preferences. Thus I imagine that such a change
description can still be improved another bit.
Would you like to express the addition of a jump target (according to
the Linux coding style) for the completion of desired exception handling
in a different way?


…
> +++ b/drivers/clocksource/timer-cadence-ttc.c
…
> @@ -453,15 +451,18 @@ static int __init ttc_setup_clockevent(struct clk *clk,
…
> +release_ttcce:
> +
> +	kfree(ttcce);
…

I would prefer that a blank line will not be added directly after such a label.

Regards,
Markus

Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource

[Markus Elfring]

> Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")

How do you think about to add the tag “Reported-by” for Michal Simek?
https://lore.kernel.org/linux-arm-kernel/2a6cdb63-397b-280a-7379-740e8f43ddf6@xilinx.com/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=3b7c59a1950c75f2c0152e5a9cd77675b09233d6#n584

Regards,
Markus

Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource

[Markus Elfring]

> Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")

I find the commit 70504f311d4bd5b6a6d494f50c5ab0bd30fdf75c
("clocksource/drivers/cadence_ttc: Convert init function to return error" from 2016-06-28)
also interesting (besides the contribution from 2013-04-04) for your software update.

Regards,
Markus

Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource

[Navid Emamdoost]

Would you review this patch, please?

On Tue, Oct 22, 2019 at 11:47 PM Navid Emamdoost
<navid.emamdoost@gmail.com> wrote:
>
> In the implementation of ttc_setup_clocksource() when
> clk_notifier_register() fails the execution should go to error handling.
> Additionally, to avoid memory leak the allocated memory for ttccs should
> be released, too. So, goto error handling to release the memory and
> return.
>
> Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
>  drivers/clocksource/timer-cadence-ttc.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
> index 88fe2e9ba9a3..035e16bc17d3 100644
> --- a/drivers/clocksource/timer-cadence-ttc.c
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
>         ttccs->ttc.clk = clk;
>
>         err = clk_prepare_enable(ttccs->ttc.clk);
> -       if (err) {
> -               kfree(ttccs);
> -               return err;
> -       }
> +       if (err)
> +               goto release_ttccs;
>
>         ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
>
> @@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
>
>         err = clk_notifier_register(ttccs->ttc.clk,
>                                     &ttccs->ttc.clk_rate_change_nb);
> -       if (err)
> +       if (err) {
>                 pr_warn("Unable to register clock notifier.\n");
> +               goto release_ttccs;
> +       }
>
>         ttccs->ttc.base_addr = base;
>         ttccs->cs.name = "ttc_clocksource";
> @@ -363,16 +363,18 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
>                      ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
>
>         err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
> -       if (err) {
> -               kfree(ttccs);
> -               return err;
> -       }
> +       if (err)
> +               goto release_ttccs;
>
>         ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
>         sched_clock_register(ttc_sched_clock_read, timer_width,
>                              ttccs->ttc.freq / PRESCALE);
>
>         return 0;
> +
> +release_ttccs:
> +       kfree(ttccs);
> +       return err;
>  }
>
>  static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
> --
> 2.17.1
>


-- 
Navid.

Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource

[Daniel Lezcano]

Hi Navid,

On 14/12/2019 23:54, Navid Emamdoost wrote:
> Would you review this patch, please?

please fix the version number, send without in-reply-to.

Do the same for the other patch:

clocksource/drivers: Fix memory leak in ttc_setup_clockevent

It is a bit confuse what patch is ok or not.

Thanks

  -- Daniel

> On Tue, Oct 22, 2019 at 11:47 PM Navid Emamdoost
> <navid.emamdoost@gmail.com> wrote:
>>
>> In the implementation of ttc_setup_clocksource() when
>> clk_notifier_register() fails the execution should go to error handling.
>> Additionally, to avoid memory leak the allocated memory for ttccs should
>> be released, too. So, goto error handling to release the memory and
>> return.
>>
>> Fixes: e932900a3279 ("arm: zynq: Use standard timer binding")
>> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
>> ---
>>  drivers/clocksource/timer-cadence-ttc.c | 20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
>> index 88fe2e9ba9a3..035e16bc17d3 100644
>> --- a/drivers/clocksource/timer-cadence-ttc.c
>> +++ b/drivers/clocksource/timer-cadence-ttc.c
>> @@ -328,10 +328,8 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
>>         ttccs->ttc.clk = clk;
>>
>>         err = clk_prepare_enable(ttccs->ttc.clk);
>> -       if (err) {
>> -               kfree(ttccs);
>> -               return err;
>> -       }
>> +       if (err)
>> +               goto release_ttccs;
>>
>>         ttccs->ttc.freq = clk_get_rate(ttccs->ttc.clk);
>>
>> @@ -341,8 +339,10 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
>>
>>         err = clk_notifier_register(ttccs->ttc.clk,
>>                                     &ttccs->ttc.clk_rate_change_nb);
>> -       if (err)
>> +       if (err) {
>>                 pr_warn("Unable to register clock notifier.\n");
>> +               goto release_ttccs;
>> +       }
>>
>>         ttccs->ttc.base_addr = base;
>>         ttccs->cs.name = "ttc_clocksource";
>> @@ -363,16 +363,18 @@ static int __init ttc_setup_clocksource(struct clk *clk, void __iomem *base,
>>                      ttccs->ttc.base_addr + TTC_CNT_CNTRL_OFFSET);
>>
>>         err = clocksource_register_hz(&ttccs->cs, ttccs->ttc.freq / PRESCALE);
>> -       if (err) {
>> -               kfree(ttccs);
>> -               return err;
>> -       }
>> +       if (err)
>> +               goto release_ttccs;
>>
>>         ttc_sched_clock_val_reg = base + TTC_COUNT_VAL_OFFSET;
>>         sched_clock_register(ttc_sched_clock_read, timer_width,
>>                              ttccs->ttc.freq / PRESCALE);
>>
>>         return 0;
>> +
>> +release_ttccs:
>> +       kfree(ttccs);
>> +       return err;
>>  }
>>
>>  static int ttc_rate_change_clockevent_cb(struct notifier_block *nb,
>> --
>> 2.17.1
>>
> 
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH] clocksource/drivers: Fix error handling in ttc_setup_clocksource

[Michal Simek]

On 16. 12. 19 14:41, Daniel Lezcano wrote:
> 
> Hi Navid,
> 
> On 14/12/2019 23:54, Navid Emamdoost wrote:
>> Would you review this patch, please?
> 
> please fix the version number, send without in-reply-to.
> 
> Do the same for the other patch:
> 
> clocksource/drivers: Fix memory leak in ttc_setup_clockevent
> 
> It is a bit confuse what patch is ok or not.

+1 on this. And patch looks good to me but as I said before the same
changes should be done also in ttc_setup_clockevent. It means v2 with
these two things together is the best way to go.

Thanks,
Michal