* [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
@ 2010-09-30 19:43 ` Davidlohr Bueso
0 siblings, 0 replies; 11+ messages in thread
From: Davidlohr Bueso @ 2010-09-30 19:43 UTC (permalink / raw)
To: davej, cpufreq; +Cc: LKML
x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()
There is no need to fall through the out_free label thus saving a kfree call.
Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
index 994230d..db7dc35 100644
--- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
@@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
if (!(supported & 0x1))
return -ENODEV;
+ return ret;
+
out_free:
kfree(output.pointer);
return ret;
--
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 19:43 ` Davidlohr Bueso
(?)
@ 2010-09-30 19:59 ` Pekka Enberg
2010-09-30 20:06 ` Dave Jones
2010-09-30 20:06 ` David Rientjes
-1 siblings, 2 replies; 11+ messages in thread
From: Pekka Enberg @ 2010-09-30 19:59 UTC (permalink / raw)
To: dave; +Cc: davej, cpufreq, LKML, x86 maintainers
[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]
On Thu, Sep 30, 2010 at 10:43 PM, Davidlohr Bueso <dave@gnu.org> wrote:
> x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()
>
> There is no need to fall through the out_free label thus saving a kfree call.
>
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
> arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> index 994230d..db7dc35 100644
> --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> if (!(supported & 0x1))
> return -ENODEV;
>
> + return ret;
> +
> out_free:
> kfree(output.pointer);
> return ret;
Where is the double free here? I can't see it. I do see memory leaks
happening in error handling paths of pcc_cpufreq_do_osc() which makes
me think we need something like the attached patch.
[-- Attachment #2: 0001-x86-cpufreq-Fix-pcc_cpufreq_do_osc-memory-leaks.patch --]
[-- Type: application/octet-stream, Size: 1397 bytes --]
From 8b18a51077c9e5b36d54a5e45f9058eb3aae2477 Mon Sep 17 00:00:00 2001
From: Pekka Enberg <penberg@cs.helsinki.fi>
Date: Thu, 30 Sep 2010 22:57:33 +0300
Subject: [PATCH] x86, cpufreq: Fix pcc_cpufreq_do_osc() memory leaks
If acpi_evaluate_object() function call doesn't fail, we must kfree()
output.buffer before returning from pcc_cpufreq_do_osc().
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c | 18 ++++++++++++------
1 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
index 994230d..4f6f679 100644
--- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
@@ -368,16 +368,22 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
return -ENODEV;
out_obj = output.pointer;
- if (out_obj->type != ACPI_TYPE_BUFFER)
- return -ENODEV;
+ if (out_obj->type != ACPI_TYPE_BUFFER) {
+ ret = -ENODEV;
+ goto out_free;
+ }
errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
- if (errors)
- return -ENODEV;
+ if (errors) {
+ ret = -ENODEV;
+ goto out_free;
+ }
supported = *((u32 *)(out_obj->buffer.pointer + 4));
- if (!(supported & 0x1))
- return -ENODEV;
+ if (!(supported & 0x1)) {
+ ret = -ENODEV;
+ goto out_free;
+ }
out_free:
kfree(output.pointer);
--
1.5.6.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 19:59 ` Pekka Enberg
@ 2010-09-30 20:06 ` Dave Jones
2010-10-01 4:47 ` Pekka Enberg
2010-09-30 20:06 ` David Rientjes
1 sibling, 1 reply; 11+ messages in thread
From: Dave Jones @ 2010-09-30 20:06 UTC (permalink / raw)
To: Pekka Enberg; +Cc: dave, cpufreq, LKML, x86 maintainers
On Thu, Sep 30, 2010 at 10:59:51PM +0300, Pekka Enberg wrote:
> > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> > if (!(supported & 0x1))
> > return -ENODEV;
> >
> > + return ret;
> > +
> > out_free:
> > kfree(output.pointer);
> > return ret;
>
> Where is the double free here? I can't see it. I do see memory leaks
> happening in error handling paths of pcc_cpufreq_do_osc() which makes
> me think we need something like the attached patch.
I think Dave's patch is correct. There's a kfree(output.pointer) at line 359.
If we fall all the way through without hitting any of the return -ENODEVs,
we end up doing a 2nd kfree in the out_free:
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 20:06 ` Dave Jones
@ 2010-10-01 4:47 ` Pekka Enberg
2010-10-01 5:25 ` Dave Jones
0 siblings, 1 reply; 11+ messages in thread
From: Pekka Enberg @ 2010-10-01 4:47 UTC (permalink / raw)
To: Dave Jones, dave, cpufreq, LKML, x86 maintainers
Hi Dave,
On 30.9.2010 23.06, Dave Jones wrote:
> On Thu, Sep 30, 2010 at 10:59:51PM +0300, Pekka Enberg wrote:
> > > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> > > if (!(supported& 0x1))
> > > return -ENODEV;
> > >
> > > + return ret;
> > > +
> > > out_free:
> > > kfree(output.pointer);
> > > return ret;
> >
> > Where is the double free here? I can't see it. I do see memory leaks
> > happening in error handling paths of pcc_cpufreq_do_osc() which makes
> > me think we need something like the attached patch.
>
> I think Dave's patch is correct. There's a kfree(output.pointer) at line 359.
> If we fall all the way through without hitting any of the return -ENODEVs,
> we end up doing a 2nd kfree in the out_free:
There's a second call to acpi_evaluate_object() which takes "output" as
its argument and allocates more memory.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-10-01 4:47 ` Pekka Enberg
@ 2010-10-01 5:25 ` Dave Jones
0 siblings, 0 replies; 11+ messages in thread
From: Dave Jones @ 2010-10-01 5:25 UTC (permalink / raw)
To: Pekka Enberg; +Cc: dave, cpufreq, LKML, x86 maintainers
On Fri, Oct 01, 2010 at 07:47:23AM +0300, Pekka Enberg wrote:
> > I think Dave's patch is correct. There's a kfree(output.pointer) at line 359.
> > If we fall all the way through without hitting any of the return -ENODEVs,
> > we end up doing a 2nd kfree in the out_free:
>
> There's a second call to acpi_evaluate_object() which takes "output" as
> its argument and allocates more memory.
yup. I merged your patch. after it's soaked in -next for a day, I'll push it to
Linus. (Looks obviously correct, but just being extra careful given where
we are in the release cycle)
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 19:59 ` Pekka Enberg
2010-09-30 20:06 ` Dave Jones
@ 2010-09-30 20:06 ` David Rientjes
1 sibling, 0 replies; 11+ messages in thread
From: David Rientjes @ 2010-09-30 20:06 UTC (permalink / raw)
To: Pekka Enberg; +Cc: dave, davej, cpufreq, LKML, x86 maintainers
On Thu, 30 Sep 2010, Pekka Enberg wrote:
> From 8b18a51077c9e5b36d54a5e45f9058eb3aae2477 Mon Sep 17 00:00:00 2001
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> Date: Thu, 30 Sep 2010 22:57:33 +0300
> Subject: [PATCH] x86, cpufreq: Fix pcc_cpufreq_do_osc() memory leaks
>
> If acpi_evaluate_object() function call doesn't fail, we must kfree()
> output.buffer before returning from pcc_cpufreq_do_osc().
>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
Acked-by: David Rientjes <rientjes@google.com>
> ---
> arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c | 18 ++++++++++++------
> 1 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> index 994230d..4f6f679 100644
> --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> @@ -368,16 +368,22 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> return -ENODEV;
>
> out_obj = output.pointer;
> - if (out_obj->type != ACPI_TYPE_BUFFER)
> - return -ENODEV;
> + if (out_obj->type != ACPI_TYPE_BUFFER) {
> + ret = -ENODEV;
> + goto out_free;
> + }
>
> errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
> - if (errors)
> - return -ENODEV;
> + if (errors) {
> + ret = -ENODEV;
> + goto out_free;
> + }
>
> supported = *((u32 *)(out_obj->buffer.pointer + 4));
> - if (!(supported & 0x1))
> - return -ENODEV;
> + if (!(supported & 0x1)) {
> + ret = -ENODEV;
> + goto out_free;
> + }
>
> out_free:
> kfree(output.pointer);
> --
> 1.5.6.4
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 19:43 ` Davidlohr Bueso
(?)
(?)
@ 2010-09-30 20:02 ` David Rientjes
2010-09-30 20:08 ` Dave Jones
2010-09-30 20:15 ` Davidlohr Bueso
-1 siblings, 2 replies; 11+ messages in thread
From: David Rientjes @ 2010-09-30 20:02 UTC (permalink / raw)
To: Davidlohr Bueso; +Cc: davej, cpufreq, LKML
On Thu, 30 Sep 2010, Davidlohr Bueso wrote:
> x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()
>
> There is no need to fall through the out_free label thus saving a kfree call.
>
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
> arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> index 994230d..db7dc35 100644
> --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> if (!(supported & 0x1))
> return -ENODEV;
>
> + return ret;
> +
> out_free:
> kfree(output.pointer);
> return ret;
Why is the kfree() unnecessary? acpi_evaluate_object() will allocate a
new output.pointer if it returns 0, so at this point in the code you would
now be leaking the buffer.
Instead, it would probably be better to fix the existing memory leaks in
that function where we return -ENODEV without going to out_free when
output.length is non-zero.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 20:02 ` David Rientjes
@ 2010-09-30 20:08 ` Dave Jones
2010-09-30 20:17 ` Davidlohr Bueso
2010-09-30 20:15 ` Davidlohr Bueso
1 sibling, 1 reply; 11+ messages in thread
From: Dave Jones @ 2010-09-30 20:08 UTC (permalink / raw)
To: David Rientjes; +Cc: Davidlohr Bueso, cpufreq, LKML
On Thu, Sep 30, 2010 at 01:02:54PM -0700, David Rientjes wrote:
> > index 994230d..db7dc35 100644
> > --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> > if (!(supported & 0x1))
> > return -ENODEV;
> >
> > + return ret;
> > +
> > out_free:
> > kfree(output.pointer);
> > return ret;
>
> Why is the kfree() unnecessary? acpi_evaluate_object() will allocate a
> new output.pointer if it returns 0, so at this point in the code you would
> now be leaking the buffer.
>
> Instead, it would probably be better to fix the existing memory leaks in
> that function where we return -ENODEV without going to out_free when
> output.length is non-zero.
Oh, *duh*. That was subtle.
You, and Pekka are of course correct. I'll merge up Pekka's patch.
thanks,
Dave
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 20:08 ` Dave Jones
@ 2010-09-30 20:17 ` Davidlohr Bueso
0 siblings, 0 replies; 11+ messages in thread
From: Davidlohr Bueso @ 2010-09-30 20:17 UTC (permalink / raw)
To: Dave Jones; +Cc: David Rientjes, cpufreq, LKML
On Thu, 2010-09-30 at 16:08 -0400, Dave Jones wrote:
> On Thu, Sep 30, 2010 at 01:02:54PM -0700, David Rientjes wrote:
> > > index 994230d..db7dc35 100644
> > > --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> > > if (!(supported & 0x1))
> > > return -ENODEV;
> > >
> > > + return ret;
> > > +
> > > out_free:
> > > kfree(output.pointer);
> > > return ret;
> >
> > Why is the kfree() unnecessary? acpi_evaluate_object() will allocate a
> > new output.pointer if it returns 0, so at this point in the code you would
> > now be leaking the buffer.
> >
> > Instead, it would probably be better to fix the existing memory leaks in
> > that function where we return -ENODEV without going to out_free when
> > output.length is non-zero.
>
> Oh, *duh*. That was subtle.
Yep, anyways, at least we got the correct "fix".
> You, and Pekka are of course correct. I'll merge up Pekka's patch.
>
> thanks,
>
> Dave
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc
2010-09-30 20:02 ` David Rientjes
2010-09-30 20:08 ` Dave Jones
@ 2010-09-30 20:15 ` Davidlohr Bueso
1 sibling, 0 replies; 11+ messages in thread
From: Davidlohr Bueso @ 2010-09-30 20:15 UTC (permalink / raw)
To: David Rientjes, Pekka Enberg; +Cc: davej, cpufreq, LKML
On Thu, 2010-09-30 at 13:02 -0700, David Rientjes wrote:
> On Thu, 30 Sep 2010, Davidlohr Bueso wrote:
>
> > x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()
> >
> > There is no need to fall through the out_free label thus saving a kfree call.
> >
> > Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> > ---
> > arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > index 994230d..db7dc35 100644
> > --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> > if (!(supported & 0x1))
> > return -ENODEV;
> >
> > + return ret;
> > +
> > out_free:
> > kfree(output.pointer);
> > return ret;
>
> Why is the kfree() unnecessary? acpi_evaluate_object() will allocate a
> new output.pointer if it returns 0, so at this point in the code you would
> now be leaking the buffer.
>
> Instead, it would probably be better to fix the existing memory leaks in
> that function where we return -ENODEV without going to out_free when
> output.length is non-zero.
>
Silly me, I must be asleep still. Pekka's attached patch is just right.
Sorry for the noise.
Davidlohr
^ permalink raw reply [flat|nested] 11+ messages in thread