[PATCH] hw breakpoint: Fix possible memory leak

[PATCH] hw breakpoint: Fix possible memory leak

[Namhyung Kim]

If kzalloc() for TYPE_DATA failed on a given cpu, previous chunk
will be leaked. Fix it.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
---
 kernel/events/hw_breakpoint.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index b0309f76d777..58298b0d0e92 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -645,8 +645,12 @@ int __init init_hw_breakpoint(void)
 			task_bp_pinned = &per_cpu(nr_task_bp_pinned[i], cpu);
 			*task_bp_pinned = kzalloc(sizeof(int) * nr_slots[i],
 						  GFP_KERNEL);
-			if (!*task_bp_pinned)
+			if (!*task_bp_pinned) {
+				while (--i >= 0)
+					kfree(per_cpu(nr_task_bp_pinned[i],
+						      cpu));
 				goto err_alloc;
+			}
 		}
 	}
 
-- 
1.7.9

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Peter Zijlstra]

On Mon, 2012-02-27 at 12:02 +0900, Namhyung Kim wrote:
> If kzalloc() for TYPE_DATA failed on a given cpu, previous chunk
> will be leaked. Fix it.

so why not fix the error loop? wouldn't putting that err_cpu == cpu
break after the kfree sort it?

> Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
> ---
>  kernel/events/hw_breakpoint.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index b0309f76d777..58298b0d0e92 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -645,8 +645,12 @@ int __init init_hw_breakpoint(void)
>  			task_bp_pinned = &per_cpu(nr_task_bp_pinned[i], cpu);
>  			*task_bp_pinned = kzalloc(sizeof(int) * nr_slots[i],
>  						  GFP_KERNEL);
> -			if (!*task_bp_pinned)
> +			if (!*task_bp_pinned) {
> +				while (--i >= 0)
> +					kfree(per_cpu(nr_task_bp_pinned[i],
> +						      cpu));
>  				goto err_alloc;
> +			}
>  		}
>  	}
>  

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Mon, 2012-02-27 at 12:02 +0900, Namhyung Kim wrote:
> > If kzalloc() for TYPE_DATA failed on a given cpu, previous chunk
> > will be leaked. Fix it.
> 
> so why not fix the error loop? wouldn't putting that err_cpu == cpu
> break after the kfree sort it?

I edited that code earlier today - is the form below OK, or can 
you see a simpler method? It's not yet pushed out so can still 
edit it.

Thanks,

	Ingo

>From f019669c93da6c2094326d07735320d3bf223ffe Mon Sep 17 00:00:00 2001
From: Namhyung Kim <namhyung.kim@lge.com>
Date: Mon, 27 Feb 2012 12:02:19 +0900
Subject: [PATCH] hw breakpoints: Fix possible memory leak

If kzalloc() for TYPE_DATA failed on a given cpu, previous chunk
will be leaked. Fix it.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
Cc: Namhyung Kim <namhyung@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Link: http://lkml.kernel.org/r/1330311739-24302-1-git-send-email-namhyung.kim@lge.com
[ rearranged the code to have a clearer flow ]
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/events/hw_breakpoint.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index b7971d6..867032d 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -636,10 +636,9 @@ int __init init_hw_breakpoint(void)
 	for_each_possible_cpu(cpu) {
 		for (i = 0; i < TYPE_MAX; i++) {
 			task_bp_pinned = &per_cpu(nr_task_bp_pinned[i], cpu);
-			*task_bp_pinned = kzalloc(sizeof(int) * nr_slots[i],
-						  GFP_KERNEL);
+			*task_bp_pinned = kzalloc(sizeof(int) * nr_slots[i], GFP_KERNEL);
 			if (!*task_bp_pinned)
-				goto err_alloc;
+				goto err_alloc_pinned;
 		}
 	}
 
@@ -649,6 +648,9 @@ int __init init_hw_breakpoint(void)
 
 	return register_die_notifier(&hw_breakpoint_exceptions_nb);
 
+ err_alloc_pinned:
+	while (--i >= 0)
+		kfree(per_cpu(nr_task_bp_pinned[i], cpu));
  err_alloc:
 	for_each_possible_cpu(err_cpu) {
 		if (err_cpu == cpu)

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Peter Zijlstra]

On Mon, 2012-02-27 at 11:44 +0100, Ingo Molnar wrote:
> I edited that code earlier today - is the form below OK, or can 
> you see a simpler method? It's not yet pushed out so can still 
> edit it. 

I think something like the below should do, but then I didn't really
think much about it, my thoughts went like:

... *shees* that's ugly
  ... that error path already does a loop
    ... what the problem is!? -- reread changelog
      ... err_cpu == cpu is placed wrong!


So I replied and marked read.. waiting to either hear if there's a good
reason to do ugly or find a new (tested) patch in my inbox.. :-)

---
 kernel/events/hw_breakpoint.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index b0309f7..3330022 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -658,10 +658,10 @@ int __init init_hw_breakpoint(void)
 
  err_alloc:
 	for_each_possible_cpu(err_cpu) {
-		if (err_cpu == cpu)
-			break;
 		for (i = 0; i < TYPE_MAX; i++)
 			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
+		if (err_cpu == cpu)
+			break;
 	}
 
 	return -ENOMEM;

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Ingo Molnar]

* Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:

> On Mon, 2012-02-27 at 11:44 +0100, Ingo Molnar wrote:
> > I edited that code earlier today - is the form below OK, or can 
> > you see a simpler method? It's not yet pushed out so can still 
> > edit it. 
> 
> I think something like the below should do, but then I didn't really
> think much about it, my thoughts went like:
> 
> ... *shees* that's ugly
>   ... that error path already does a loop
>     ... what the problem is!? -- reread changelog
>       ... err_cpu == cpu is placed wrong!
> 
> 
> So I replied and marked read.. waiting to either hear if there's a good
> reason to do ugly or find a new (tested) patch in my inbox.. :-)
> 
> ---
>  kernel/events/hw_breakpoint.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index b0309f7..3330022 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -658,10 +658,10 @@ int __init init_hw_breakpoint(void)
>  
>   err_alloc:
>  	for_each_possible_cpu(err_cpu) {
> -		if (err_cpu == cpu)
> -			break;
>  		for (i = 0; i < TYPE_MAX; i++)
>  			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
> +		if (err_cpu == cpu)
> +			break;
>  	}

Looks a lot nicer - I'll wait for an updated patch.

Thanks,

	Ingo

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Namhyung Kim]

2012-02-27 (Mon), 12:04 +0100, Peter Zijlstra wrote:
> On Mon, 2012-02-27 at 11:44 +0100, Ingo Molnar wrote:
> > I edited that code earlier today - is the form below OK, or can 
> > you see a simpler method? It's not yet pushed out so can still 
> > edit it. 
> 
> I think something like the below should do, but then I didn't really
> think much about it, my thoughts went like:
> 
> ... *shees* that's ugly
>   ... that error path already does a loop
>     ... what the problem is!? -- reread changelog
>       ... err_cpu == cpu is placed wrong!
> 
> 
> So I replied and marked read.. waiting to either hear if there's a good
> reason to do ugly or find a new (tested) patch in my inbox.. :-)
> 
> ---
>  kernel/events/hw_breakpoint.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index b0309f7..3330022 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -658,10 +658,10 @@ int __init init_hw_breakpoint(void)
>  
>   err_alloc:
>  	for_each_possible_cpu(err_cpu) {
> -		if (err_cpu == cpu)
> -			break;
>  		for (i = 0; i < TYPE_MAX; i++)
>  			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
> +		if (err_cpu == cpu)
> +			break;
>  	}
>  
>  	return -ENOMEM;
> 

This would depend on the initial value of the percpu memory, and thus
have no problem in this case. It looks better to me, too :)


-- 
Regards,
Namhyung Kim

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Peter Zijlstra]

On Mon, 2012-02-27 at 21:45 +0900, Namhyung Kim wrote:
> This would depend on the initial value of the percpu memory

that should be all 0s iirc.

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Namhyung Kim]

2012-02-27 (Mon), 13:46 +0100, Peter Zijlstra wrote:
> On Mon, 2012-02-27 at 21:45 +0900, Namhyung Kim wrote:
> > This would depend on the initial value of the percpu memory
> 
> that should be all 0s iirc.

Yep, that's why I said there's no problem here.


-- 
Regards,
Namhyung Kim

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Namhyung Kim]

2012-02-27 (월), 12:56 +0100, Ingo Molnar:
> * Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> 
> > On Mon, 2012-02-27 at 11:44 +0100, Ingo Molnar wrote:
> > > I edited that code earlier today - is the form below OK, or can 
> > > you see a simpler method? It's not yet pushed out so can still 
> > > edit it. 
> > 
> > I think something like the below should do, but then I didn't really
> > think much about it, my thoughts went like:
> > 
> > ... *shees* that's ugly
> >   ... that error path already does a loop
> >     ... what the problem is!? -- reread changelog
> >       ... err_cpu == cpu is placed wrong!
> > 
> > 
> > So I replied and marked read.. waiting to either hear if there's a good
> > reason to do ugly or find a new (tested) patch in my inbox.. :-)
> > 
> > ---
> >  kernel/events/hw_breakpoint.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index b0309f7..3330022 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> > @@ -658,10 +658,10 @@ int __init init_hw_breakpoint(void)
> >  
> >   err_alloc:
> >  	for_each_possible_cpu(err_cpu) {
> > -		if (err_cpu == cpu)
> > -			break;
> >  		for (i = 0; i < TYPE_MAX; i++)
> >  			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
> > +		if (err_cpu == cpu)
> > +			break;
> >  	}
> 
> Looks a lot nicer - I'll wait for an updated patch.
> 
> Thanks,
> 
> 	Ingo

Ingo, do you want me to resend? If so, I really don't know how to give
the credit to Peter in this case.

Or will you take the patch from him directly?


-- 
Regards,
Namhyung Kim

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Ingo Molnar]

* Namhyung Kim <namhyung@gmail.com> wrote:

> 2012-02-27 (월), 12:56 +0100, Ingo Molnar:
> > * Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > 
> > > On Mon, 2012-02-27 at 11:44 +0100, Ingo Molnar wrote:
> > > > I edited that code earlier today - is the form below OK, or can 
> > > > you see a simpler method? It's not yet pushed out so can still 
> > > > edit it. 
> > > 
> > > I think something like the below should do, but then I didn't really
> > > think much about it, my thoughts went like:
> > > 
> > > ... *shees* that's ugly
> > >   ... that error path already does a loop
> > >     ... what the problem is!? -- reread changelog
> > >       ... err_cpu == cpu is placed wrong!
> > > 
> > > 
> > > So I replied and marked read.. waiting to either hear if there's a good
> > > reason to do ugly or find a new (tested) patch in my inbox.. :-)
> > > 
> > > ---
> > >  kernel/events/hw_breakpoint.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > > index b0309f7..3330022 100644
> > > --- a/kernel/events/hw_breakpoint.c
> > > +++ b/kernel/events/hw_breakpoint.c
> > > @@ -658,10 +658,10 @@ int __init init_hw_breakpoint(void)
> > >  
> > >   err_alloc:
> > >  	for_each_possible_cpu(err_cpu) {
> > > -		if (err_cpu == cpu)
> > > -			break;
> > >  		for (i = 0; i < TYPE_MAX; i++)
> > >  			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
> > > +		if (err_cpu == cpu)
> > > +			break;
> > >  	}
> > 
> > Looks a lot nicer - I'll wait for an updated patch.
> > 
> > Thanks,
> > 
> > 	Ingo
> 
> Ingo, do you want me to resend? If so, I really don't know how 
> to give the credit to Peter in this case.

Just mention it in the changelog that this solution was his 
idea. It was you who did most of the work: found the bug, wrote 
the patch, wrote the changelog and tested the final patch ;-)

Thanks,

	Ingo

Re: [PATCH] hw breakpoint: Fix possible memory leak

[Frederic Weisbecker]

On Mon, Feb 27, 2012 at 08:40:38PM +0100, Ingo Molnar wrote:
> 
> * Namhyung Kim <namhyung@gmail.com> wrote:
> 
> > 2012-02-27 (월), 12:56 +0100, Ingo Molnar:
> > > * Peter Zijlstra <a.p.zijlstra@chello.nl> wrote:
> > > 
> > > > On Mon, 2012-02-27 at 11:44 +0100, Ingo Molnar wrote:
> > > > > I edited that code earlier today - is the form below OK, or can 
> > > > > you see a simpler method? It's not yet pushed out so can still 
> > > > > edit it. 
> > > > 
> > > > I think something like the below should do, but then I didn't really
> > > > think much about it, my thoughts went like:
> > > > 
> > > > ... *shees* that's ugly
> > > >   ... that error path already does a loop
> > > >     ... what the problem is!? -- reread changelog
> > > >       ... err_cpu == cpu is placed wrong!
> > > > 
> > > > 
> > > > So I replied and marked read.. waiting to either hear if there's a good
> > > > reason to do ugly or find a new (tested) patch in my inbox.. :-)
> > > > 
> > > > ---
> > > >  kernel/events/hw_breakpoint.c |    4 ++--
> > > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > > > index b0309f7..3330022 100644
> > > > --- a/kernel/events/hw_breakpoint.c
> > > > +++ b/kernel/events/hw_breakpoint.c
> > > > @@ -658,10 +658,10 @@ int __init init_hw_breakpoint(void)
> > > >  
> > > >   err_alloc:
> > > >  	for_each_possible_cpu(err_cpu) {
> > > > -		if (err_cpu == cpu)
> > > > -			break;
> > > >  		for (i = 0; i < TYPE_MAX; i++)
> > > >  			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
> > > > +		if (err_cpu == cpu)
> > > > +			break;
> > > >  	}
> > > 
> > > Looks a lot nicer - I'll wait for an updated patch.
> > > 
> > > Thanks,
> > > 
> > > 	Ingo
> > 
> > Ingo, do you want me to resend? If so, I really don't know how 
> > to give the credit to Peter in this case.
> 
> Just mention it in the changelog that this solution was his 
> idea. It was you who did most of the work: found the bug, wrote 
> the patch, wrote the changelog and tested the final patch ;-)

And please add my Acked-by: Frederic Weisbecker <fweisbec@gmail.com> :)

[PATCH v2] hw breakpoint: Fix a possible memory leak

[Namhyung Kim]

If kzalloc() for TYPE_DATA failed on a given cpu, previous chunk
of TYPE_INST will be leaked. Fix it.

Thanks to Peter Zijlstra for suggesting this better solution. It
should work as long as the initial value of the region is all 0's
and that's the case of static (per-cpu) memory allocation.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 kernel/events/hw_breakpoint.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index b0309f76d777..3330022a7ac1 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -658,10 +658,10 @@ int __init init_hw_breakpoint(void)
 
  err_alloc:
 	for_each_possible_cpu(err_cpu) {
-		if (err_cpu == cpu)
-			break;
 		for (i = 0; i < TYPE_MAX; i++)
 			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
+		if (err_cpu == cpu)
+			break;
 	}
 
 	return -ENOMEM;
-- 
1.7.9

[tip:perf/urgent] perf/hwbp: Fix a possible memory leak

[tip-bot for Namhyung Kim]

Commit-ID:  30ce2f7eef095d1b8d070740f1948629814fe3c7
Gitweb:     http://git.kernel.org/tip/30ce2f7eef095d1b8d070740f1948629814fe3c7
Author:     Namhyung Kim <namhyung.kim@lge.com>
AuthorDate: Tue, 28 Feb 2012 10:19:38 +0900
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 28 Feb 2012 09:52:54 +0100

perf/hwbp: Fix a possible memory leak

If kzalloc() for TYPE_DATA failed on a given cpu, previous chunk
of TYPE_INST will be leaked. Fix it.

Thanks to Peter Zijlstra for suggesting this better solution. It
should work as long as the initial value of the region is all
0's and that's the case of static (per-cpu) memory allocation.

Signed-off-by: Namhyung Kim <namhyung.kim@lge.com>
Acked-by: Frederic Weisbecker <fweisbec@gmail.com>
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Arnaldo Carvalho de Melo <acme@ghostprotocols.net>
Link: http://lkml.kernel.org/r/1330391978-28070-1-git-send-email-namhyung.kim@lge.com
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/events/hw_breakpoint.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index b7971d6..ee706ce 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -651,10 +651,10 @@ int __init init_hw_breakpoint(void)
 
  err_alloc:
 	for_each_possible_cpu(err_cpu) {
-		if (err_cpu == cpu)
-			break;
 		for (i = 0; i < TYPE_MAX; i++)
 			kfree(per_cpu(nr_task_bp_pinned[i], cpu));
+		if (err_cpu == cpu)
+			break;
 	}
 
 	return -ENOMEM;