cgroup: fix invalid cgrp->dentry before cgroup has been completely removed

PATCH In review Li Zefan 6 participants 9 messages Oct 30, 2008 3 messages not in the archive

lore

Share & tools

lore

Li Zefan

author

2008-10-30 07:26 UTC #0 lore raw

PATCH 1/1 cgroup: fix invalid cgrp->dentry before cgroup has been completely removed

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 <[email protected]>

---
 kernel/cgroup.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

kernel/cgroup.c +0 -1

index 35eebd5..358e775 100644
@@ -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);

Signature · 1 line

1.5.4.rc3

A message that is not in the archive (sent off-list or not yet received): [email protected]

Andrew Morton

2008-10-30 07:32 UTC #2 lore raw

Quoted 1 line · On Thu, 30 Oct 2008 15:23:27 +0800 Li Zefan…

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.

Paul Menage

Acked-by

2008-10-30 07:41 UTC #3 lore raw

In reply to a message not in the archive

Quoted 6 lines · Li Zefan

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 <[email protected]>

Quoted 43 lines

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 <[email protected]>

---
 kernel/cgroup.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

kernel/cgroup.c +0 -2

index 35eebd5..358e775 100644
@@ -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

Andrew Morton

2008-10-30 07:33 UTC #4 lore raw

In reply to Li Zefan

Quoted 1 line · On Thu, 30 Oct 2008 15:23:27 +0800 Li Zefan <[email protected]> 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.

A message that is not in the archive (sent off-list or not yet received): 20081030003226.33bad22a.akpm-de/[email protected]

Li Zefan

2008-10-30 07:43 UTC #6 lore raw

Quoted 7 lines · Andrew Morton

Quoted 1 line · On Thu, 30 Oct 2008 15:23:27 +0800 Li Zefan…
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.

Li Zefan

author

2008-10-30 07:46 UTC #7 lore raw

In reply to Andrew Morton

Quoted 7 lines · Andrew Morton

Quoted 1 line · On Thu, 30 Oct 2008 15:23:27 +0800 Li Zefan <[email protected]> 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.

Paul Menage

Acked-by on v1 1/1

2008-10-30 07:41 UTC #8 lore raw

In reply to Li Zefan

Quoted 6 lines · Li Zefan

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.

Thanks.

Acked-by Paul Menage <[email protected]>

Quoted 43 lines

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 <[email protected]>

---
 kernel/cgroup.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

kernel/cgroup.c +0 -2

index 35eebd5..358e775 100644
@@ -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

A message that is not in the archive (sent off-list or not yet received): 6599ad830810300041o1f97999em7b64426414bae380-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Li Zefan

2008-10-30 08:07 UTC #10 lore raw

Quoted 12 lines · Paul Menage

Quoted 6 lines · Li Zefan
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.

Quoted 7 lines

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.

Li Zefan

author

2008-10-30 08:10 UTC #11 lore raw

In reply to Paul Menage

Quoted 12 lines · Paul Menage

Quoted 6 lines · Li Zefan
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.

Quoted 7 lines

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.