LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed
@ 2008-10-30  7:23 Li Zefan
  2008-10-30  7:32 ` Andrew Morton
  2008-10-30  7:41 ` Paul Menage
  0 siblings, 2 replies; 5+ messages in thread
From: Li Zefan @ 2008-10-30  7:23 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Paul Menage, Peter Zijlstra, Ingo Molnar, LKML, Linux Containers

This fixes oops when reading /proc/sched_debug.

A cgroup won't be removed completely until finishing cgroup_diput(), so we
shouldn't invalidate cgrp->dentry in cgroup_rmdir(). Otherwise, when a
group is being removed while cgroup_path() gets called, we may trigger NULL
dereference BUG.

The bug can be reproduced:

 # cat test.sh
 #!/bin/sh
 mount -t cgroup -o cpu xxx /mnt
 for (( ; ; ))
 {
	mkdir /mnt/sub
	rmdir /mnt/sub
 }
 # ./test.sh &
 # cat /proc/sched_debug

BUG: unable to handle kernel NULL pointer dereference at 00000038
IP: [<c045a47f>] cgroup_path+0x39/0x90
...
Call Trace:
 [<c0420344>] ? print_cfs_rq+0x6e/0x75d
 [<c0421160>] ? sched_debug_show+0x72d/0xc1e
...

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 35eebd5..358e775 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2497,7 +2497,6 @@ static int cgroup_rmdir(struct inode *unused_dir, struct dentry *dentry)
 	list_del(&cgrp->sibling);
 	spin_lock(&cgrp->dentry->d_lock);
 	d = dget(cgrp->dentry);
-	cgrp->dentry = NULL;
 	spin_unlock(&d->d_lock);
 
 	cgroup_d_remove_dir(d);
-- 
1.5.4.rc3


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed
  2008-10-30  7:23 [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed Li Zefan
@ 2008-10-30  7:32 ` Andrew Morton
  2008-10-30  7:43   ` Li Zefan
  2008-10-30  7:41 ` Paul Menage
  1 sibling, 1 reply; 5+ messages in thread
From: Andrew Morton @ 2008-10-30  7:32 UTC (permalink / raw)
  To: Li Zefan; +Cc: Paul Menage, Peter Zijlstra, Ingo Molnar, LKML, Linux Containers

On Thu, 30 Oct 2008 15:23:27 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:

> This fixes oops when reading /proc/sched_debug.

In which kernel versions?  The patch applies to 2.6.27 - should it be
aplied to 2.6.27.x?  Earlier?

Thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed
  2008-10-30  7:23 [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed Li Zefan
  2008-10-30  7:32 ` Andrew Morton
@ 2008-10-30  7:41 ` Paul Menage
  2008-10-30  8:07   ` Li Zefan
  1 sibling, 1 reply; 5+ messages in thread
From: Paul Menage @ 2008-10-30  7:41 UTC (permalink / raw)
  To: Li Zefan
  Cc: Andrew Morton, Peter Zijlstra, Ingo Molnar, LKML, Linux Containers

On Thu, Oct 30, 2008 at 12:23 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> This fixes oops when reading /proc/sched_debug.
>
> A cgroup won't be removed completely until finishing cgroup_diput(), so we
> shouldn't invalidate cgrp->dentry in cgroup_rmdir(). Otherwise, when a
> group is being removed while cgroup_path() gets called, we may trigger NULL
> dereference BUG.

Clearly a bug if it can hit a NULL dereference. But clearing the
dentry to NULL is something that cgroups inherited from cpusets - it
looks OK to remove it, but I'm mildly nervous.

Directly after the code in your patch, we dput() the dentry. So
theoretically it could be released any time after that. But I guess
that as soon as it *is* released, cgroup_diput() will be called as
part of that cleanup, at which point any subsystems should drop any
pointers they have to the cgroup or the dentry. So I guess it should
be OK.

Thanks.

Acked-by: Paul Menage <menage@google.com>



>
> The bug can be reproduced:
>
>  # cat test.sh
>  #!/bin/sh
>  mount -t cgroup -o cpu xxx /mnt
>  for (( ; ; ))
>  {
>        mkdir /mnt/sub
>        rmdir /mnt/sub
>  }
>  # ./test.sh &
>  # cat /proc/sched_debug
>
> BUG: unable to handle kernel NULL pointer dereference at 00000038
> IP: [<c045a47f>] cgroup_path+0x39/0x90
> ...
> Call Trace:
>  [<c0420344>] ? print_cfs_rq+0x6e/0x75d
>  [<c0421160>] ? sched_debug_show+0x72d/0xc1e
> ...
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 35eebd5..358e775 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2497,7 +2497,6 @@ static int cgroup_rmdir(struct inode *unused_dir, struct dentry *dentry)
>        list_del(&cgrp->sibling);
>        spin_lock(&cgrp->dentry->d_lock);
>        d = dget(cgrp->dentry);
> -       cgrp->dentry = NULL;
>        spin_unlock(&d->d_lock);
>
>        cgroup_d_remove_dir(d);
> --
> 1.5.4.rc3
>
>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed
  2008-10-30  7:32 ` Andrew Morton
@ 2008-10-30  7:43   ` Li Zefan
  0 siblings, 0 replies; 5+ messages in thread
From: Li Zefan @ 2008-10-30  7:43 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Paul Menage, Peter Zijlstra, Ingo Molnar, LKML, Linux Containers

Andrew Morton wrote:
> On Thu, 30 Oct 2008 15:23:27 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
>> This fixes oops when reading /proc/sched_debug.
> 
> In which kernel versions?  The patch applies to 2.6.27 - should it be
> aplied to 2.6.27.x?  Earlier?
> 

I think the bug exists both in 2.6.26 and 2.6.27, but I don't know if we should
apply the patch to these 2 versions. sched_debug is a debug feature.



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed
  2008-10-30  7:41 ` Paul Menage
@ 2008-10-30  8:07   ` Li Zefan
  0 siblings, 0 replies; 5+ messages in thread
From: Li Zefan @ 2008-10-30  8:07 UTC (permalink / raw)
  To: Paul Menage
  Cc: Andrew Morton, Peter Zijlstra, Ingo Molnar, LKML, Linux Containers

Paul Menage wrote:
> On Thu, Oct 30, 2008 at 12:23 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> This fixes oops when reading /proc/sched_debug.
>>
>> A cgroup won't be removed completely until finishing cgroup_diput(), so we
>> shouldn't invalidate cgrp->dentry in cgroup_rmdir(). Otherwise, when a
>> group is being removed while cgroup_path() gets called, we may trigger NULL
>> dereference BUG.
> 
> Clearly a bug if it can hit a NULL dereference. But clearing the
> dentry to NULL is something that cgroups inherited from cpusets - it
> looks OK to remove it, but I'm mildly nervous.
> 
> Directly after the code in your patch, we dput() the dentry. So
> theoretically it could be released any time after that. But I guess
> that as soon as it *is* released, cgroup_diput() will be called as
> part of that cleanup, at which point any subsystems should drop any
> pointers they have to the cgroup or the dentry. So I guess it should
> be OK.
> 

The bug is:

cgroup           cpu_subsystem
------------------------------------

cgroup_remove()
                 print_cfs_stats()
                 print_cfs_rq()
cgroup_diput()
                 cpu_cgroup_destroy()

I think a different fix is to add pre_destroy() method to cpu_subsystem,
and move some code from sched_destroy_group() to that method.

But I didn't try it out.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2008-10-30  8:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-30  7:23 [PATCH] cgroup: fix invalid cgrp->dentry before cgroup has been completely removed Li Zefan
2008-10-30  7:32 ` Andrew Morton
2008-10-30  7:43   ` Li Zefan
2008-10-30  7:41 ` Paul Menage
2008-10-30  8:07   ` Li Zefan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).