[PATCH] sched: fix double kfree

[PATCH] sched: fix double kfree

[Li Zefan]

Impact: fix double kfree in failure path

It's not the responsibility of init_rootdomain() to free root_domain
allocated by alloc_rootdomain().

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/sched.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 545c6fc..2bad712 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -6970,7 +6970,7 @@ static int init_rootdomain(struct root_domain *rd, bool bootmem)
 	}
 
 	if (!alloc_cpumask_var(&rd->span, GFP_KERNEL))
-		goto free_rd;
+		goto out;
 	if (!alloc_cpumask_var(&rd->online, GFP_KERNEL))
 		goto free_span;
 	if (!alloc_cpumask_var(&rd->rto_mask, GFP_KERNEL))
@@ -6986,8 +6986,7 @@ free_online:
 	free_cpumask_var(rd->online);
 free_span:
 	free_cpumask_var(rd->span);
-free_rd:
-	kfree(rd);
+out:
 	return -ENOMEM;
 }
 
-- 
1.5.4.rc3

Re: [PATCH] sched: fix double kfree

[Ingo Molnar]

* Li Zefan <lizf@cn.fujitsu.com> wrote:

> Impact: fix double kfree in failure path
> 
> It's not the responsibility of init_rootdomain() to free root_domain
> allocated by alloc_rootdomain().
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/sched.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)

applied to tip/sched/urgent, thanks!

	Ingo

Re: [PATCH] sched: fix double kfree

[Pekka Enberg]

On Tue, Jan 6, 2009 at 11:39 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> Impact: fix double kfree in failure path
>
> It's not the responsibility of init_rootdomain() to free root_domain
> allocated by alloc_rootdomain().
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>

FWIW

Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

> ---
>  kernel/sched.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 545c6fc..2bad712 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -6970,7 +6970,7 @@ static int init_rootdomain(struct root_domain *rd, bool bootmem)
>        }
>
>        if (!alloc_cpumask_var(&rd->span, GFP_KERNEL))
> -               goto free_rd;
> +               goto out;
>        if (!alloc_cpumask_var(&rd->online, GFP_KERNEL))
>                goto free_span;
>        if (!alloc_cpumask_var(&rd->rto_mask, GFP_KERNEL))
> @@ -6986,8 +6986,7 @@ free_online:
>        free_cpumask_var(rd->online);
>  free_span:
>        free_cpumask_var(rd->span);
> -free_rd:
> -       kfree(rd);
> +out:
>        return -ENOMEM;
>  }
>
> --
> 1.5.4.rc3
>
> --
> 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/
>

Re: [PATCH] sched: fix double kfree

[Ingo Molnar]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On Tue, Jan 6, 2009 at 11:39 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> > Impact: fix double kfree in failure path
> >
> > It's not the responsibility of init_rootdomain() to free root_domain
> > allocated by alloc_rootdomain().
> >
> > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> 
> FWIW
> 
> Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

thanks - added it to the changelog.

	Ingo

Re: [PATCH] sched: fix double kfree

[Rusty Russell]

On Tuesday 06 January 2009 20:09:06 Li Zefan wrote:
> Impact: fix double kfree in failure path
> 
> It's not the responsibility of init_rootdomain() to free root_domain
> allocated by alloc_rootdomain().
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>

Thanks, you're right.

Good catch!
Rusty.

Re: [PATCH] sched: fix double kfree

[Mike Travis]

Li Zefan wrote:
> Impact: fix double kfree in failure path
> 
> It's not the responsibility of init_rootdomain() to free root_domain
> allocated by alloc_rootdomain().
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/sched.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 545c6fc..2bad712 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -6970,7 +6970,7 @@ static int init_rootdomain(struct root_domain *rd, bool bootmem)
>  	}
>  
>  	if (!alloc_cpumask_var(&rd->span, GFP_KERNEL))
> -		goto free_rd;
> +		goto out;
>  	if (!alloc_cpumask_var(&rd->online, GFP_KERNEL))
>  		goto free_span;
>  	if (!alloc_cpumask_var(&rd->rto_mask, GFP_KERNEL))
> @@ -6986,8 +6986,7 @@ free_online:
>  	free_cpumask_var(rd->online);
>  free_span:
>  	free_cpumask_var(rd->span);
> -free_rd:
> -	kfree(rd);
> +out:
>  	return -ENOMEM;
>  }
>  

Thanks Li!

Another thing I noticed but didn't deal with is:

static int init_rootdomain(struct root_domain *rd, bool bootmem)
{
        memset(rd, 0, sizeof(*rd));

        if (bootmem) {
                alloc_bootmem_cpumask_var(&def_root_domain.span);
                alloc_bootmem_cpumask_var(&def_root_domain.online);
                alloc_bootmem_cpumask_var(&def_root_domain.rto_mask);
                cpupri_init(&rd->cpupri, true);
                return 0;
        }
        if (!alloc_cpumask_var(&rd->span, GFP_KERNEL))
		...

Note that under the if (bootmem) case, it specifies the addresses
directly instead of using (&rd->span) as the other alloc's do.

Not a big deal, just an inconsistency.

Thanks,
Mike

Re: [PATCH] sched: fix double kfree

[Li Zefan]

> Another thing I noticed but didn't deal with is:
> 
> static int init_rootdomain(struct root_domain *rd, bool bootmem)
> {
>         memset(rd, 0, sizeof(*rd));
> 
>         if (bootmem) {
>                 alloc_bootmem_cpumask_var(&def_root_domain.span);
>                 alloc_bootmem_cpumask_var(&def_root_domain.online);
>                 alloc_bootmem_cpumask_var(&def_root_domain.rto_mask);
>                 cpupri_init(&rd->cpupri, true);
>                 return 0;
>         }
>         if (!alloc_cpumask_var(&rd->span, GFP_KERNEL))
> 		...
> 
> Note that under the if (bootmem) case, it specifies the addresses
> directly instead of using (&rd->span) as the other alloc's do.
> 
> Not a big deal, just an inconsistency.
> 

I didn't notice this. :)

What makes it a bit worse is another inconsistency in if(bootmem), that rd
is used in cpupri_init() but def_root_domain is used in alloc()s.