* Re: [PATCH] sched: fix double kfree
2009-01-06 9:39 [PATCH] sched: fix double kfree Li Zefan
@ 2009-01-06 9:50 ` Ingo Molnar
2009-01-06 9:50 ` Pekka Enberg
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2009-01-06 9:50 UTC (permalink / raw)
To: Li Zefan; +Cc: Rusty Russell, Peter Zijlstra, LKML
* 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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] sched: fix double kfree
2009-01-06 9:39 [PATCH] sched: fix double kfree Li Zefan
2009-01-06 9:50 ` Ingo Molnar
@ 2009-01-06 9:50 ` Pekka Enberg
2009-01-06 10:07 ` Ingo Molnar
2009-01-06 10:08 ` Rusty Russell
2009-01-06 17:19 ` Mike Travis
3 siblings, 1 reply; 7+ messages in thread
From: Pekka Enberg @ 2009-01-06 9:50 UTC (permalink / raw)
To: Li Zefan; +Cc: Ingo Molnar, Rusty Russell, Peter Zijlstra, LKML
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/
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sched: fix double kfree
2009-01-06 9:50 ` Pekka Enberg
@ 2009-01-06 10:07 ` Ingo Molnar
0 siblings, 0 replies; 7+ messages in thread
From: Ingo Molnar @ 2009-01-06 10:07 UTC (permalink / raw)
To: Pekka Enberg; +Cc: Li Zefan, Rusty Russell, Peter Zijlstra, LKML
* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sched: fix double kfree
2009-01-06 9:39 [PATCH] sched: fix double kfree Li Zefan
2009-01-06 9:50 ` Ingo Molnar
2009-01-06 9:50 ` Pekka Enberg
@ 2009-01-06 10:08 ` Rusty Russell
2009-01-06 17:19 ` Mike Travis
3 siblings, 0 replies; 7+ messages in thread
From: Rusty Russell @ 2009-01-06 10:08 UTC (permalink / raw)
To: Li Zefan; +Cc: Ingo Molnar, Peter Zijlstra, LKML
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] sched: fix double kfree
2009-01-06 9:39 [PATCH] sched: fix double kfree Li Zefan
` (2 preceding siblings ...)
2009-01-06 10:08 ` Rusty Russell
@ 2009-01-06 17:19 ` Mike Travis
2009-01-07 1:19 ` Li Zefan
3 siblings, 1 reply; 7+ messages in thread
From: Mike Travis @ 2009-01-06 17:19 UTC (permalink / raw)
To: Li Zefan; +Cc: Ingo Molnar, Rusty Russell, Peter Zijlstra, LKML
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
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] sched: fix double kfree
2009-01-06 17:19 ` Mike Travis
@ 2009-01-07 1:19 ` Li Zefan
0 siblings, 0 replies; 7+ messages in thread
From: Li Zefan @ 2009-01-07 1:19 UTC (permalink / raw)
To: Mike Travis; +Cc: Ingo Molnar, Rusty Russell, Peter Zijlstra, LKML
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread