If kzalloc() for TYPE_DATA failed on a given cpu, previous chunk
will be leaked. Fix it.
Signed-off-by: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
> ---
> 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;
> + }
> }
> }
>
* Peter Zijlstra <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ rearranged the code to have a clearer flow ]
Signed-off-by: Ingo Molnar <[email protected]>
---
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)
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;
* Peter Zijlstra <[email protected]> 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
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
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.
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
2012-02-27 (월), 12:56 +0100, Ingo Molnar:
> * Peter Zijlstra <[email protected]> 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
* Namhyung Kim <[email protected]> wrote:
> 2012-02-27 (월), 12:56 +0100, Ingo Molnar:
> > * Peter Zijlstra <[email protected]> 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
On Mon, Feb 27, 2012 at 08:40:38PM +0100, Ingo Molnar wrote:
>
> * Namhyung Kim <[email protected]> wrote:
>
> > 2012-02-27 (월), 12:56 +0100, Ingo Molnar:
> > > * Peter Zijlstra <[email protected]> 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 <[email protected]> :)
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 <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
---
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
Commit-ID: 30ce2f7eef095d1b8d070740f1948629814fe3c7
Gitweb: http://git.kernel.org/tip/30ce2f7eef095d1b8d070740f1948629814fe3c7
Author: Namhyung Kim <[email protected]>
AuthorDate: Tue, 28 Feb 2012 10:19:38 +0900
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Acked-by: Frederic Weisbecker <[email protected]>
Acked-by: Peter Zijlstra <[email protected]>
Cc: Paul Mackerras <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;