[PATCH] clocksource/drivers/davinci: Fix memory leak in davinci_timer_register

[PATCH] clocksource/drivers/davinci: Fix memory leak in davinci_timer_register

[Navid Emamdoost]

In the impelementation of davinci_timer_register() the allocated memory
for clockevent should be released if request_irq() fails.

Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 drivers/clocksource/timer-davinci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
index 62745c962049..910d4d2f0d64 100644
--- a/drivers/clocksource/timer-davinci.c
+++ b/drivers/clocksource/timer-davinci.c
@@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
 			 "clockevent/tim12", clockevent);
 	if (rv) {
 		pr_err("Unable to request the clockevent interrupt");
+		kfree(clockevent);
 		return rv;
 	}
 
-- 
2.17.1

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

[Markus Elfring]

> In the impelementation of davinci_timer_register() the allocated memory
> for clockevent should be released if request_irq() 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-davinci.c
> @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
>  			 "clockevent/tim12", clockevent);
>  	if (rv) {
>  		pr_err("Unable to request the clockevent interrupt");
> +		kfree(clockevent);
>  		return rv;
>  	}

* Should a complete source code analysis point out that a similar fix
  will be needed also in the if branch after a failed call of
  the function “clocksource_register_hz”?

* Can any more exception handling become relevant because of previous
  resource allocations in this function implementation?

Regards,
Markus

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

[Markus Elfring]

> In the impelementation of davinci_timer_register() the allocated memory
> for clockevent should be released if request_irq() 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-davinci.c
> @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
>  			 "clockevent/tim12", clockevent);
>  	if (rv) {
>  		pr_err("Unable to request the clockevent interrupt");
> +		kfree(clockevent);
>  		return rv;
>  	}

* Should a complete source code analysis point out that a similar fix
  will be needed also in the if branch after a failed call of
  the function “clocksource_register_hz”?

* Can any more exception handling become relevant because of previous
  resource allocations in this function implementation?

Regards,
Markus

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

[Bartosz Golaszewski]

pon., 21 paź 2019 o 22:26 Navid Emamdoost <navid.emamdoost@gmail.com>
napisał(a):
>
> In the impelementation of davinci_timer_register() the allocated memory
> for clockevent should be released if request_irq() fails.
>
> Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
> ---
>  drivers/clocksource/timer-davinci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> index 62745c962049..910d4d2f0d64 100644
> --- a/drivers/clocksource/timer-davinci.c
> +++ b/drivers/clocksource/timer-davinci.c
> @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
>                          "clockevent/tim12", clockevent);
>         if (rv) {
>                 pr_err("Unable to request the clockevent interrupt");
> +               kfree(clockevent);
>                 return rv;
>         }
>
> --
> 2.17.1
>

Any failure in this driver means the system is fried. I explicitly
didn't bother freeing any resources. Nack.

Bart

[PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

In the case where request_irq fails, the return path does not kfree
clockevent and hence we have a memory leak.  Fix this by kfree'ing
clockevent before returning.

Addresses-Coverity: ("Resource leak")
Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/clocksource/timer-davinci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
index 62745c962049..910d4d2f0d64 100644
--- a/drivers/clocksource/timer-davinci.c
+++ b/drivers/clocksource/timer-davinci.c
@@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
 			 "clockevent/tim12", clockevent);
 	if (rv) {
 		pr_err("Unable to request the clockevent interrupt");
+		kfree(clockevent);
 		return rv;
 	}
 
-- 
2.20.1

[PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

In the case where request_irq fails, the return path does not kfree
clockevent and hence we have a memory leak.  Fix this by kfree'ing
clockevent before returning.

Addresses-Coverity: ("Resource leak")
Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/clocksource/timer-davinci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
index 62745c962049..910d4d2f0d64 100644
--- a/drivers/clocksource/timer-davinci.c
+++ b/drivers/clocksource/timer-davinci.c
@@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
 			 "clockevent/tim12", clockevent);
 	if (rv) {
 		pr_err("Unable to request the clockevent interrupt");
+		kfree(clockevent);
 		return rv;
 	}
 
-- 
2.20.1

Re: [PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Bartosz Golaszewski]

sob., 9 lis 2019 o 16:58 Colin King <colin.king@canonical.com> napisał(a):
>
> From: Colin Ian King <colin.king@canonical.com>
>
> In the case where request_irq fails, the return path does not kfree
> clockevent and hence we have a memory leak.  Fix this by kfree'ing
> clockevent before returning.
>
> Addresses-Coverity: ("Resource leak")
> Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/clocksource/timer-davinci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> index 62745c962049..910d4d2f0d64 100644
> --- a/drivers/clocksource/timer-davinci.c
> +++ b/drivers/clocksource/timer-davinci.c
> @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
>                          "clockevent/tim12", clockevent);
>         if (rv) {
>                 pr_err("Unable to request the clockevent interrupt");
> +               kfree(clockevent);
>                 return rv;
>         }
>
> --
> 2.20.1
>

Hi Daniel,

this is what I think the third time someone tries to "fix" this
driver's "memory leaks". I'm not sure what the general approach in
clocksource is but it doesn't make sense to free resources on
non-recoverable errors, does it? Should I add a comment about it or
you'll just take those "fixes" to stop further such submissions?

Best regards,
Bartosz Golaszewski

Re: [PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Bartosz Golaszewski]

sob., 9 lis 2019 o 16:58 Colin King <colin.king@canonical.com> napisał(a):
>
> From: Colin Ian King <colin.king@canonical.com>
>
> In the case where request_irq fails, the return path does not kfree
> clockevent and hence we have a memory leak.  Fix this by kfree'ing
> clockevent before returning.
>
> Addresses-Coverity: ("Resource leak")
> Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/clocksource/timer-davinci.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> index 62745c962049..910d4d2f0d64 100644
> --- a/drivers/clocksource/timer-davinci.c
> +++ b/drivers/clocksource/timer-davinci.c
> @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
>                          "clockevent/tim12", clockevent);
>         if (rv) {
>                 pr_err("Unable to request the clockevent interrupt");
> +               kfree(clockevent);
>                 return rv;
>         }
>
> --
> 2.20.1
>

Hi Daniel,

this is what I think the third time someone tries to "fix" this
driver's "memory leaks". I'm not sure what the general approach in
clocksource is but it doesn't make sense to free resources on
non-recoverable errors, does it? Should I add a comment about it or
you'll just take those "fixes" to stop further such submissions?

Best regards,
Bartosz Golaszewski

Re: [PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

Bartosz,

On Sun, 10 Nov 2019, Bartosz Golaszewski wrote:
> sob., 9 lis 2019 o 16:58 Colin King <colin.king@canonical.com> napisał(a):
> >
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > In the case where request_irq fails, the return path does not kfree
> > clockevent and hence we have a memory leak.  Fix this by kfree'ing

s/we have/creates/  or whatever verb you prefer.

> > clockevent before returning.
> >
> > Addresses-Coverity: ("Resource leak")
> > Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/clocksource/timer-davinci.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> > index 62745c962049..910d4d2f0d64 100644
> > --- a/drivers/clocksource/timer-davinci.c
> > +++ b/drivers/clocksource/timer-davinci.c
> > @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
> >                          "clockevent/tim12", clockevent);
> >         if (rv) {
> >                 pr_err("Unable to request the clockevent interrupt");
> > +               kfree(clockevent);
> >                 return rv;
> >         }
> >
> > --
> > 2.20.1
> >
> 
> Hi Daniel,
> 
> this is what I think the third time someone tries to "fix" this
> driver's "memory leaks". I'm not sure what the general approach in
> clocksource is but it doesn't make sense to free resources on
> non-recoverable errors, does it? Should I add a comment about it or
> you'll just take those "fixes" to stop further such submissions?

There are two ways to deal with that:

  1) If the error is really unrecoverable, panic right there. No point
     to continue.

  2) If there is even a minimal chance to survive, free the memory and
     return.

Adding a comment is just a useless non-option.

Thanks,

	tglx

Re: [PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Thomas Gleixner]

[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]

Bartosz,

On Sun, 10 Nov 2019, Bartosz Golaszewski wrote:
> sob., 9 lis 2019 o 16:58 Colin King <colin.king@canonical.com> napisał(a):
> >
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > In the case where request_irq fails, the return path does not kfree
> > clockevent and hence we have a memory leak.  Fix this by kfree'ing

s/we have/creates/  or whatever verb you prefer.

> > clockevent before returning.
> >
> > Addresses-Coverity: ("Resource leak")
> > Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/clocksource/timer-davinci.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> > index 62745c962049..910d4d2f0d64 100644
> > --- a/drivers/clocksource/timer-davinci.c
> > +++ b/drivers/clocksource/timer-davinci.c
> > @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
> >                          "clockevent/tim12", clockevent);
> >         if (rv) {
> >                 pr_err("Unable to request the clockevent interrupt");
> > +               kfree(clockevent);
> >                 return rv;
> >         }
> >
> > --
> > 2.20.1
> >
> 
> Hi Daniel,
> 
> this is what I think the third time someone tries to "fix" this
> driver's "memory leaks". I'm not sure what the general approach in
> clocksource is but it doesn't make sense to free resources on
> non-recoverable errors, does it? Should I add a comment about it or
> you'll just take those "fixes" to stop further such submissions?

There are two ways to deal with that:

  1) If the error is really unrecoverable, panic right there. No point
     to continue.

  2) If there is even a minimal chance to survive, free the memory and
     return.

Adding a comment is just a useless non-option.

Thanks,

	tglx

Re: [PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Bartosz Golaszewski]

wt., 12 lis 2019 o 00:37 Thomas Gleixner <tglx@linutronix.de> napisał(a):
>
> Bartosz,
>
> On Sun, 10 Nov 2019, Bartosz Golaszewski wrote:
> > sob., 9 lis 2019 o 16:58 Colin King <colin.king@canonical.com> napisał(a):
> > >
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > In the case where request_irq fails, the return path does not kfree
> > > clockevent and hence we have a memory leak.  Fix this by kfree'ing
>
> s/we have/creates/  or whatever verb you prefer.
>
> > > clockevent before returning.
> > >
> > > Addresses-Coverity: ("Resource leak")
> > > Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > >  drivers/clocksource/timer-davinci.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> > > index 62745c962049..910d4d2f0d64 100644
> > > --- a/drivers/clocksource/timer-davinci.c
> > > +++ b/drivers/clocksource/timer-davinci.c
> > > @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
> > >                          "clockevent/tim12", clockevent);
> > >         if (rv) {
> > >                 pr_err("Unable to request the clockevent interrupt");
> > > +               kfree(clockevent);
> > >                 return rv;
> > >         }
> > >
> > > --
> > > 2.20.1
> > >
> >
> > Hi Daniel,
> >
> > this is what I think the third time someone tries to "fix" this
> > driver's "memory leaks". I'm not sure what the general approach in
> > clocksource is but it doesn't make sense to free resources on
> > non-recoverable errors, does it? Should I add a comment about it or
> > you'll just take those "fixes" to stop further such submissions?
>
> There are two ways to deal with that:
>
>   1) If the error is really unrecoverable, panic right there. No point
>      to continue.

Fair enough.

Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

>
>   2) If there is even a minimal chance to survive, free the memory and
>      return.
>
> Adding a comment is just a useless non-option.
>
> Thanks,
>
>         tglx

Re: [PATCH] clocksource/drivers/davinci: fix memory leak on clockevent on error return

[Bartosz Golaszewski]

wt., 12 lis 2019 o 00:37 Thomas Gleixner <tglx@linutronix.de> napisał(a):
>
> Bartosz,
>
> On Sun, 10 Nov 2019, Bartosz Golaszewski wrote:
> > sob., 9 lis 2019 o 16:58 Colin King <colin.king@canonical.com> napisał(a):
> > >
> > > From: Colin Ian King <colin.king@canonical.com>
> > >
> > > In the case where request_irq fails, the return path does not kfree
> > > clockevent and hence we have a memory leak.  Fix this by kfree'ing
>
> s/we have/creates/  or whatever verb you prefer.
>
> > > clockevent before returning.
> > >
> > > Addresses-Coverity: ("Resource leak")
> > > Fixes: 721154f972aa ("clocksource/drivers/davinci: Add support for clockevents")
> > > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > >  drivers/clocksource/timer-davinci.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/clocksource/timer-davinci.c b/drivers/clocksource/timer-davinci.c
> > > index 62745c962049..910d4d2f0d64 100644
> > > --- a/drivers/clocksource/timer-davinci.c
> > > +++ b/drivers/clocksource/timer-davinci.c
> > > @@ -299,6 +299,7 @@ int __init davinci_timer_register(struct clk *clk,
> > >                          "clockevent/tim12", clockevent);
> > >         if (rv) {
> > >                 pr_err("Unable to request the clockevent interrupt");
> > > +               kfree(clockevent);
> > >                 return rv;
> > >         }
> > >
> > > --
> > > 2.20.1
> > >
> >
> > Hi Daniel,
> >
> > this is what I think the third time someone tries to "fix" this
> > driver's "memory leaks". I'm not sure what the general approach in
> > clocksource is but it doesn't make sense to free resources on
> > non-recoverable errors, does it? Should I add a comment about it or
> > you'll just take those "fixes" to stop further such submissions?
>
> There are two ways to deal with that:
>
>   1) If the error is really unrecoverable, panic right there. No point
>      to continue.

Fair enough.

Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

>
>   2) If there is even a minimal chance to survive, free the memory and
>      return.
>
> Adding a comment is just a useless non-option.
>
> Thanks,
>
>         tglx