2010-01-09 14:46:47

by Alexander Beregalov

Subject: [PATCH] kernel/resource.c: fix memory leak

Free res before exit.
Found by cppcheck.

Signed-off-by: Alexander Beregalov <[email protected]>
---
kernel/resource.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index af96c1e..ae2e177 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -545,11 +545,12 @@ static void __init __reserve_region_with_split(struct resource *root,
res->flags = IORESOURCE_BUSY;

conflict = __request_resource(parent, res);
+ kfree(res);
+
if (!conflict)
return;

/* failed, split and try again */
- kfree(res);

/* conflict covered whole area */
if (conflict->start <= start && conflict->end >= end)
--
1.6.6

2010-01-09 16:55:59

by Cong Wang

Subject: Re: [PATCH] kernel/resource.c: fix memory leak

On Sat, Jan 09, 2010 at 05:46:35PM +0300, Alexander Beregalov wrote:
>Free res before exit.
>Found by cppcheck.
>
>Signed-off-by: Alexander Beregalov <[email protected]>
>---
> kernel/resource.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
>diff --git a/kernel/resource.c b/kernel/resource.c
>index af96c1e..ae2e177 100644
>--- a/kernel/resource.c
>+++ b/kernel/resource.c
>@@ -545,11 +545,12 @@ static void __init __reserve_region_with_split(struct resource *root,
> res->flags = IORESOURCE_BUSY;
>
> conflict = __request_resource(parent, res);
>+ kfree(res);
>+
> if (!conflict)
> return;
>
> /* failed, split and try again */
>- kfree(res);
>
> /* conflict covered whole area */
> if (conflict->start <= start && conflict->end >= end)

NAK.

If I read it correctly, 'res' will be used if !conflict, we
should't kfree it.


--
Live like a child, think like the god.

2010-01-09 17:34:49

by Linus Torvalds

Subject: Re: [PATCH] kernel/resource.c: fix memory leak

On Sat, 9 Jan 2010, Alexander Beregalov wrote:
>
> Free res before exit.
> Found by cppcheck.
>
> Signed-off-by: Alexander Beregalov <[email protected]>
> ---
> kernel/resource.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index af96c1e..ae2e177 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -545,11 +545,12 @@ static void __init __reserve_region_with_split(struct resource *root,
> res->flags = IORESOURCE_BUSY;
>
> conflict = __request_resource(parent, res);
> + kfree(res);
> +
> if (!conflict)
> return;
>
> /* failed, split and try again */
> - kfree(res);

As Amerigo pointed out, this is very wrong indeed.

If we don't have a conflict, then __request_resource() will have inserted
the resource into the resource tree, and we absolutely must _not_ free it.

So that cppcheck tool is way too simplistic, and wrong.

Linus