sched: fix another race when reading /proc/sched_debug
PATCH 1/1 sched: fix another race when reading /proc/sched_debug
I fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.
The above commit fixed a race that reading /proc/sched_debug may access NULL cgrp->dentry if a cgroup is being removed.
But I found there's another different race, in that reading sched_debug may access a cgroup which hasn't been completed created, and thus dereference NULL cgrp->dentry!
cgroup_create()
cpu_cgroup_create()
register_fair_sched_group()
list_add_rcu(...)
print_cfs_stats()
for_each_leaf_cfs_rq()
print_cfs_rq()
cgroup_path() cgroup->dentry = dentry;
task_group is added to the global list before the cgroup has been created completely, if at this time print_cfs_stats() is called, it will access the half-created cgroup.
This patch fixes the bug by holding cgroup_lock() to wait the cgroup to be created completely before calling cgroup_path().
---
The patch is based on linus's git tree, and should go into 2.6.28, but it conflicts with the cleanup patch in sched/core:
0a0db8f5c9d4bbb9bbfcc2b6cb6bce2d0ef4d73d
--- kernel/sched_debug.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
kernel/sched_debug.c +8 -2
@@ -127,8 +127,11 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +cgroup_lock ();cgroup_path (cgroup, path,sizeof (path)); +cgroup_unlock (); + }SEQ_printf (m,"\ncfs_rq[%d]:%s\n" , cpu, path);#else
@@ -181,8 +184,11 @@ void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +cgroup_lock ();cgroup_path (cgroup, path,sizeof (path)); +cgroup_unlock (); + }SEQ_printf (m,"\nrt_rq[%d]:%s\n" , cpu, path);#else
Signature · 1 line
1.5.4.rc3
Ingo Molnar
AppliedQuoted 27 lines · Li Zefan
I fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.The above commit fixed a race that reading
/proc/sched_debugmay access NULLcgrp->dentryif a cgroup is being removed.But I found there's another different race, in that reading
sched_debugmay access a cgroup which hasn't been completed created, and thus dereference NULLcgrp->dentry!
cgroup_create()
cpu_cgroup_create()register_fair_sched_group() list_add_rcu(...) print_cfs_stats() for_each_leaf_cfs_rq() print_cfs_rq() cgroup_path()
cgroup->dentry= dentry;
Quoted 8 lines
task_groupis added to the global list before the cgroup has been created completely, if at this timeprint_cfs_stats()is called, it will access the half-created cgroup.This patch fixes the bug by holding
cgroup_lock()to wait the cgroup to be created completely before callingcgroup_path().Signed-off-by Li Zefan <[email protected]>
applied to tip/sched/urgent, thanks!
Quoted 3 lines
The patch is based on linus's git tree, and should go into 2.6.28, but it conflicts with the cleanup patch in sched/core:
0a0db8f5c9d4bbb9bbfcc2b6cb6bce2d0ef4d73d
i merged it up in tip/master, could you please check whether it's ok?
Ingo
Quoted 23 lines · Li Zefan
kernel/sched_debug.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)kernel/sched_debug.c +4
-1@@ -127,8 +127,11 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +cgroup_lock ();cgroup_path (cgroup, path,sizeof (path)); +cgroup_unlock (); + }SEQ_printf (m,"\ncfs_rq[%d]:%s\n" , cpu, path);#else @@ -181,8 +184,11 @@ void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq)if (tg) cgroup = tg->css.cgroup;
kernel/sched_debug.c void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq) · line 194
Quoted 10 lines
kernel/sched_debug.c +4
-1-if (cgroup) +if (cgroup) { +cgroup_lock ();cgroup_path (cgroup, path,sizeof (path)); +cgroup_unlock (); + }SEQ_printf (m,"\nrt_rq[%d]:%s\n" , cpu, path);#else
The comment in cgroup_path() routine says that it needs to be called with cgroup_mutex held. With the above fix, print_task() in sched_debug.c remains the last caller of cgroup_path() which calls it without holding cgroup_mutex. Does this also need a fix ?
Regards,
Bharata.
Signature · 1 line
http://bharata.sulekha.com/blog/posts.htm
Quoted 32 lines · Bharata B Rao
Quoted 31 lines · Li Zefan
kernel/sched_debug.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)kernel/sched_debug.c +8
-2@@ -127,8 +127,11 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +cgroup_lock ();cgroup_path (cgroup, path,sizeof (path)); +cgroup_unlock (); + }SEQ_printf (m,"\ncfs_rq[%d]:%s\n" , cpu, path);#else @@ -181,8 +184,11 @@ void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +cgroup_lock ();cgroup_path (cgroup, path,sizeof (path)); +cgroup_unlock (); + }
Quoted 8 lines
kernel/sched_debug.c void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq) · line 194
Quoted 2 lines
kernel/sched_debug.c +0
-0SEQ_printf (m,"\nrt_rq[%d]:%s\n" , cpu, path);#else The comment in
cgroup_path()routine says that it needs to be called withcgroup_mutexheld. With the above fix,print_task()insched_debug.cremains the last caller ofcgroup_path()which calls it without holdingcgroup_mutex. Does this also need a fix ?
You mean:
print_task(struct seq_file *m, struct rq *rq, struct task_struct *p)
{
...
cgroup_path(task_group(p)->css.cgroup, path, sizeof(path));
...
}Hmm...Normally we have to take task_lock() or rcu_read_lock() to retrieve the cgroup from the task, and as long as we hold either lock, we don't need to take cgroup_lock().
I noticed neither task_lock() nor rcu is held before calling cgroup_path, so I wrote a test program to see if I can trigger a but here, but it didn't happen. I'll dig more.
Quoted 2 lines
i merged it up in tip/master, could you please check whether it's ok?
Sorry, though this patch avoids accessing a half-created cgroup, but I found current code may access a cgroup which has been destroyed.
The simplest fix is to take cgroup_lock() before for_each_leaf_cfs_rq.
Could you revert this patch and apply the following new one? My box has survived for 16 hours with it applied.
==========
From: Li Zefan <[email protected]>
Date: Sun, 14 Dec 2008 09:53:28 +0800
Subject: [PATCH] sched: fix another race when reading /proc/sched_debug
I fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.
The above commit fixed a race that reading /proc/sched_debug may access NULL cgrp->dentry if a cgroup is being removed (via cgroup_rmdir), but hasn't been destroyed (via cgroup_diput).
But I found there's another different race, in that reading sched_debug may access a cgroup which is being created or has been destroyed, and thus dereference NULL cgrp->dentry!
task_group is added to the global list while the cgroup is being created, and is removed from the global list while the cgroup is under destruction. So running through the list should be protected by cgroup_lock(), if cgroup data will be accessed (here by calling cgroup_path).
--- kernel/sched_fair.c | 6 ++++++ kernel/sched_rt.c | 6 ++++++ 2 files changed, 12 insertions(+), 0 deletions(-)
kernel/sched_fair.c +6 -0
@@ -1737,9 +1737,15 @@ static void print_cfs_stats(struct seq_file *m, int cpu) {struct cfs_rq *cfs_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_cfs_rq (cpu_rq (cpu), cfs_rq)print_cfs_rq (m, cpu, cfs_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif
kernel/sched_rt.c +6 -0
@@ -1538,9 +1538,15 @@ static void print_rt_stats(struct seq_file *m, int cpu) {struct rt_rq *rt_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_rt_rq (rt_rq,cpu_rq (cpu))print_rt_rq (m, cpu, rt_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif /* CONFIG_SCHED_DEBUG */
Signature · 1 line
1.5.4.rc3
Quoted 31 lines · Li Zefan
Quoted 2 lines
i merged it up in tip/master, could you please check whether it's ok?
Sorry, though this patch avoids accessing a half-created cgroup, but I found current code may access a cgroup which has been destroyed.
The simplest fix is to take
cgroup_lock()beforefor_each_leaf_cfs_rq.Could you revert this patch and apply the following new one? My box has survived for 16 hours with it applied.
==========
From: Li Zefan <[email protected]>
Date: Sun, 14 Dec 2008 09:53:28 +0800
Subject: [PATCH] sched: fix another race when reading/proc/sched_debugI fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.The above commit fixed a race that reading
/proc/sched_debugmay access NULLcgrp->dentryif a cgroup is being removed (viacgroup_rmdir), but hasn't been destroyed (viacgroup_diput).
Quoted 8 lines
But I found there's another different race, in that reading
sched_debugmay access a cgroup which is being created or has been destroyed, and thus dereference NULLcgrp->dentry!
task_groupis added to the global list while the cgroup is being created, and is removed from the global list while the cgroup is under destruction. So running through the list should be protected bycgroup_lock(), if cgroup data will be accessed (here by callingcgroup_path).
Can't we detect a dead task-group and skip those instead of adding this global lock?
Quoted 46 lines
Signed-off-by Li Zefan <[email protected]>--- kernel/sched_fair.c | 6 ++++++ kernel/sched_rt.c | 6 ++++++ 2 files changed, 12 insertions(+), 0 deletions(-)kernel/sched_fair.c +6
-0@@ -1737,9 +1737,15 @@ static void print_cfs_stats(struct seq_file *m, int cpu) {struct cfs_rq *cfs_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_cfs_rq (cpu_rq (cpu), cfs_rq)print_cfs_rq (m, cpu, cfs_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif kernel/sched_rt.c +6
-0@@ -1538,9 +1538,15 @@ static void print_rt_stats(struct seq_file *m, int cpu) {struct rt_rq *rt_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_rt_rq (rt_rq,cpu_rq (cpu))print_rt_rq (m, cpu, rt_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif /* CONFIG_SCHED_DEBUG */
Quoted 35 lines · Peter Zijlstra
Quoted 34 lines · Li Zefan
Quoted 2 lines
i merged it up in tip/master, could you please check whether it's ok?
Sorry, though this patch avoids accessing a half-created cgroup, but I found current code may access a cgroup which has been destroyed.
The simplest fix is to take
cgroup_lock()beforefor_each_leaf_cfs_rq.Could you revert this patch and apply the following new one? My box has survived for 16 hours with it applied.
==========
From: Li Zefan <[email protected]>
Date: Sun, 14 Dec 2008 09:53:28 +0800
Subject: [PATCH] sched: fix another race when reading/proc/sched_debugI fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.The above commit fixed a race that reading
/proc/sched_debugmay access NULLcgrp->dentryif a cgroup is being removed (viacgroup_rmdir), but hasn't been destroyed (viacgroup_diput).But I found there's another different race, in that reading
sched_debugmay access a cgroup which is being created or has been destroyed, and thus dereference NULLcgrp->dentry!
Quoted 8 lines
Quoted 4 lines
task_groupis added to the global list while the cgroup is being created, and is removed from the global list while the cgroup is under destruction. So running through the list should be protected bycgroup_lock(), if cgroup data will be accessed (here by callingcgroup_path).Can't we detect a dead task-group and skip those instead of adding this global lock?
I tried it, but I don't think it's feasable, without lock syncronization:
| print_cfs_rq()
| check task_group is dead
cgroup_diput() |
.. |
mark task_group as dead |
.. |
kfree(cgrp) |
| call cgroup_path()Quoted 49 lines
Quoted 46 lines
Signed-off-by Li Zefan <[email protected]>--- kernel/sched_fair.c | 6 ++++++ kernel/sched_rt.c | 6 ++++++ 2 files changed, 12 insertions(+), 0 deletions(-)kernel/sched_fair.c +6
-0@@ -1737,9 +1737,15 @@ static void print_cfs_stats(struct seq_file *m, int cpu) {struct cfs_rq *cfs_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_cfs_rq (cpu_rq (cpu), cfs_rq)print_cfs_rq (m, cpu, cfs_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif kernel/sched_rt.c +6
-0@@ -1538,9 +1538,15 @@ static void print_rt_stats(struct seq_file *m, int cpu) {struct rt_rq *rt_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_rt_rq (rt_rq,cpu_rq (cpu))print_rt_rq (m, cpu, rt_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif /* CONFIG_SCHED_DEBUG */
Quoted 5 lines · Paul Menage
I sent out some patches last week (search for
hierarchy_mutex) that would mean that you'd only need to take a subsystem-local lock to keep a cgroup alive. People seemed to like them so I'll tweak them based on feedback and send them on to Andrew.
Unfortunately, AFAICS the proposed hierarchy_mutex can't solve this bug. :(
Quoted 12 lines
Paul
On Dec 14, 2008 4:48 AM, "Peter Zijlstra" <[email protected]> wrote:
On Sun, 2008-12-14 at 10:54 +0800, Li Zefan wrote: > > i merged it up in tip/master, could you pleas...
Can't we detect a dead task-group and skip those instead of adding this global lock?Quoted 1 line
Signed-off-by Li Zefan <[email protected]> > --- > kernel/sched_fair.c| 6 ++++++ > kerne...
Quoted 9 lines · On Mon, 15 Dec 2008 09:39:18 +0800 Li Zefan <[email protected]> wrote:
Quoted 5 lines · Paul Menage
I sent out some patches last week (search for
hierarchy_mutex) that would mean that you'd only need to take a subsystem-local lock to keep a cgroup alive. People seemed to like them so I'll tweak them based on feedback and send them on to Andrew.Unfortunately, AFAICS the proposed
hierarchy_mutexcan't solve this bug. :(
Hmm ? how about this way if cgroup->dentry is problem ?
at creation:
- cpu_cgroup_populate() should record "tg" that "this cgroup has valid dentry"
at deletion
- css_tryget() will be useful to avoid the race.
thx,
-Kame
Quoted 19 lines
Quoted 4 lines
Paul
On Dec 14, 2008 4:48 AM, "Peter Zijlstra" <[email protected]> wrote:
Quoted 8 lines
On Sun, 2008-12-14 at 10:54 +0800, Li Zefan wrote: > > i merged it up in tip/master, could you pleas...
Can't we detect a dead task-group and skip those instead of adding this global lock?Quoted 1 line
Signed-off-by Li Zefan <[email protected]> > --- > kernel/sched_fair.c| 6 ++++++ > kerne...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Quoted 4 lines · KAMEZAWA Hiroyuki
Quoted 1 line · On Mon, 15 Dec 2008 09:39:18 +0800 Li Zefan <[email protected]> wrote:
Paul Menage wrote:
Quoted 9 lines
Quoted 7 lines
Quoted 5 lines
I sent out some patches last week (search for
hierarchy_mutex) that would mean that you'd only need to take a subsystem-local lock to keep a cgroup alive. People seemed to like them so I'll tweak them based on feedback and send them on to Andrew.Unfortunately, AFAICS the proposed
hierarchy_mutexcan't solve this bug. :(Hmm ? how about this way if
cgroup->dentryis problem ?
Sounds feasable, though the resulting code might a bit tricky.
Quoted 2 lines
at creation:
-cpu_cgroup_populate()should record "tg" that "this cgroup has valid dentry"
This avoids /proc/sched_debug accessing invalid cgrp dentry while a cgroup is being created.
Quoted 2 lines
at deletion
-css_tryget()will be useful to avoid the race.
And this avoids accessing a dead task_group.
Since css_tryget() is a 2.6.29 material, I think we can have this bug fixed for 2.6.28 by holding cgroup_lock, and try to remove the global lock for 2.6.29 ?
Quoted 47 lines · Li Zefan
Quoted 35 lines · Peter Zijlstra
Quoted 34 lines · Li Zefan
Quoted 2 lines
i merged it up in tip/master, could you please check whether it's ok?
Sorry, though this patch avoids accessing a half-created cgroup, but I found current code may access a cgroup which has been destroyed.
The simplest fix is to take
cgroup_lock()beforefor_each_leaf_cfs_rq.Could you revert this patch and apply the following new one? My box has survived for 16 hours with it applied.
==========
From: Li Zefan <[email protected]>
Date: Sun, 14 Dec 2008 09:53:28 +0800
Subject: [PATCH] sched: fix another race when reading/proc/sched_debugI fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.The above commit fixed a race that reading
/proc/sched_debugmay access NULLcgrp->dentryif a cgroup is being removed (viacgroup_rmdir), but hasn't been destroyed (viacgroup_diput).But I found there's another different race, in that reading
sched_debugmay access a cgroup which is being created or has been destroyed, and thus dereference NULLcgrp->dentry!Quoted 8 lines
Quoted 4 lines
task_groupis added to the global list while the cgroup is being created, and is removed from the global list while the cgroup is under destruction. So running through the list should be protected bycgroup_lock(), if cgroup data will be accessed (here by callingcgroup_path).Can't we detect a dead task-group and skip those instead of adding this global lock?
I tried it, but I don't think it's feasable, without lock syncronization:
Quoted 8 lines
| print_cfs_rq()
| check task_group is dead
cgroup_diput() |
.. |
mark task_group as dead |
.. |
kfree(cgrp) |
| call cgroup_path()rcu free cgrp
Quoted 14 lines
Quoted 5 lines
Quoted 3 lines
Can't we detect a dead task-group and skip those instead of adding this global lock?
I tried it, but I don't think it's feasable, without lock syncronization:
Quoted 8 lines
| print_cfs_rq() | check task_group is dead cgroup_diput() | .. | mark task_group as dead | .. | kfree(cgrp) | | call cgroup_path()
Quoted 2 lines
rcu free cgrp
I got your point, thanks.
Another way is use css_tryget(), and thus can avoid touching cgroup.c and adding synchronize_rcu(). css_tryget() is proposed by Kamezawa but I think won't be available until 2.6.29.
Anyway, here is the fix. I'll post a complete version with changelog when we agree on how to fix it.
--- kernel/cgroup.c | 6 ++++++ kernel/sched_debug.c | 17 +++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-)
kernel/cgroup.c +6 -0
@@ -624,6 +624,12 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)* created the cgroup */ deactivate_super (cgrp->root->sb); +/* +* Some subsystems (cpu cgroup) might still be able to +* accessing the cgroup in rcu section. +*/ +synchronize_rcu (); +kfree (cgrp); }iput (inode);
kernel/sched_debug.c +15 -2
@@ -127,8 +127,14 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +/* +* This task_group is dead or we race with cgroup creating. +*/ +if (cgroup_is_removed (cgroup) || !cgroup->dentry) +return ;cgroup_path (cgroup, path,sizeof (path)); + }SEQ_printf (m,"\ncfs_rq[%d]:%s\n" , cpu, path);#else
@@ -181,8 +187,15 @@ void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +/* +* This task_group is dead or we race with cgroup creating. +*/ +if (cgroup_is_removed (cgroup) || !cgroup->dentry) +return ; +cgroup_path (cgroup, path,sizeof (path)); + }SEQ_printf (m,"\nrt_rq[%d]:%s\n" , cpu, path);#else
Signature · 1 line
1.5.4.rc3
Quoted 17 lines · Li Zefan
Quoted 14 lines
Quoted 5 lines
Quoted 3 lines
Can't we detect a dead task-group and skip those instead of adding this global lock?
I tried it, but I don't think it's feasable, without lock syncronization:
Quoted 8 lines
| print_cfs_rq() | check task_group is dead cgroup_diput() | .. | mark task_group as dead | .. | kfree(cgrp) | | call cgroup_path()Quoted 2 lines
rcu free cgrp
Quoted 8 lines
I got your point, thanks.
Another way is use
css_tryget(), and thus can avoid touchingcgroup.cand addingsynchronize_rcu().css_tryget()is proposed by Kamezawa but I think won't be available until 2.6.29.Anyway, here is the fix. I'll post a complete version with changelog when we agree on how to fix it.
Yeah, the below looks like a suitable fix.
Quoted 13 lines
Signed-off-by Li Zefan <[email protected]>--- kernel/cgroup.c | 6 ++++++ kernel/sched_debug.c | 17 +++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-)kernel/cgroup.c +0
-0@@ -624,6 +624,12 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)* created the cgroup */ deactivate_super (cgrp->root->sb);
kernel/cgroup.c static void cgroup_diput(struct dentry *dentry, struct inode *inode) · line 635
Quoted 10 lines
kernel/cgroup.c +6
-0+/* +* Some subsystems (cpu cgroup) might still be able to +* accessing the cgroup in rcu section. +*/ +synchronize_rcu (); +kfree (cgrp); }iput (inode);
Is there any reason we cannot use call_rcu() ?
Quoted 7 lines
kernel/sched_debug.c +0
-0@@ -127,8 +127,14 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)if (tg) cgroup = tg->css.cgroup;
kernel/sched_debug.c void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq) · line 137
Quoted 10 lines
kernel/sched_debug.c +7
-1-if (cgroup) +if (cgroup) { +/* +* This task_group is dead or we race with cgroup creating. +*/ +if (cgroup_is_removed (cgroup) || !cgroup->dentry) +return ;cgroup_path (cgroup, path,sizeof (path)); + }
Perhaps wrap that check in a cgroup_*() helper? That would avoid the duplication, be clearer and not open code the ->dentry assumption.
cgroup_is_active() perhaps?
Quoted 19 lines
kernel/sched_debug.c +8
-1SEQ_printf (m,"\ncfs_rq[%d]:%s\n" , cpu, path);#else @@ -181,8 +187,15 @@ void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) +if (cgroup) { +/* +* This task_group is dead or we race with cgroup creating. +*/ +if (cgroup_is_removed (cgroup) || !cgroup->dentry) +return ; +cgroup_path (cgroup, path,sizeof (path)); + }SEQ_printf (m,"\nrt_rq[%d]:%s\n" , cpu, path);#else
Quoted 10 lines · On Mon, 15 Dec 2008 11:43:49 +0100 Peter Zijlstra <[email protected]>…
kernel/sched_debug.c void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq) · line 137
Quoted 4 lines
kernel/sched_debug.c +3
-0+if (cgroup_is_removed (cgroup) || !cgroup->dentry) +return ;cgroup_path (cgroup, path,sizeof (path)); + }Perhaps wrap that check in a cgroup_*() helper? That would avoid the duplication, be clearer and not open code the ->dentry assumption.
cgroup_is_active()perhaps?
I vote for cgroup_is_populated().
Thx,
-Kame
Quoted 10 lines · KAMEZAWA Hiroyuki
Quoted 7 lines · On Mon, 15 Dec 2008 11:43:49 +0100 Peter Zijlstra <[email protected]>…
kernel/sched_debug.c void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq) · line 137
Quoted 4 lines
kernel/sched_debug.c +3
-0+if (cgroup_is_removed (cgroup) || !cgroup->dentry) +return ;cgroup_path (cgroup, path,sizeof (path)); + }Perhaps wrap that check in a cgroup_*() helper? That would avoid the duplication, be clearer and not open code the ->dentry assumption.
Quoted 2 lines
Quoted 2 lines
cgroup_is_active()perhaps?
We are fixing this particular race, so open code and comment can document the problem clearer.
And this wrapper shouldn't be used by other cgroup users, if one needs to do these checks, he should have a reason and comment is needed to explain what's happening.
Also, I can't think out an appropriate name..
Quoted 3 lines
I vote for
cgroup_is_populated().
But non-populated means the directory isn't filled up with control files, this function name doesn't indicate if the dir dentry itself is valid or not..
How about this :
====================
I fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.
The above commit fixed a race that reading /proc/sched_debug may access NULL cgrp->dentry if a cgroup is being removed (via cgroup_rmdir), but hasn't been destroyed (via cgroup_diput).
But I found there's another different race, in that reading sched_debug may access a cgroup which is being created or has been destroyed, and thus dereference NULL cgrp->dentry!
We fix the former issue by checking if the cgroup is being created, and fix the latter issue by synchronize free cgroup with rcu.
--- include/linux/cgroup.h | 12 +++++++++++- kernel/cgroup.c | 14 ++++++++------ kernel/sched_debug.c | 18 ++++++++++++++---- 3 files changed, 33 insertions(+), 11 deletions(-)
include/linux/cgroup.h +11 -1
@@ -91,6 +91,8 @@ static inline void css_put(struct cgroup_subsys_state *css)/* bits in struct cgroup flags field */ enum { +/* Control Group is completely created */ +CGRP_CREATED ,/* Control Group is dead */ CGRP_REMOVED ,/* Control Group has previously had a child cgroup or a task,
@@ -303,7 +305,15 @@ int cgroup_add_files(struct cgroup *cgrp,const struct cftype cft[], int count); -int cgroup_is_removed (const struct cgroup *cgrp); +static inline int cgroup_is_removed (const struct cgroup *cgrp) +{ +return test_bit (CGRP_REMOVED , &cgrp->flags); +} + +static inline int cgroup_is_created (const struct cgroup *cgrp) +{ +return test_bit (CGRP_CREATED , &cgrp->flags); +}int cgroup_path (const struct cgroup *cgrp,char *buf,int buflen);
kernel/cgroup.c +8 -6
@@ -118,12 +118,6 @@ static int root_count;static int need_forkexit_callback__read_mostly ;static int need_mm_owner_callback__read_mostly ; -/* convenient tests for these bits */ -inline int cgroup_is_removed (const struct cgroup *cgrp) -{ -return test_bit (CGRP_REMOVED , &cgrp->flags); -} -/* bits in struct cgroupfs_root flags field */ enum {ROOT_NOPREFIX ,/* mounted subsystems have no named prefix */
@@ -624,6 +618,12 @@ static void cgroup_diput(struct dentry *dentry, struct inode *inode)* created the cgroup */ deactivate_super (cgrp->root->sb); +/* +* Some subsystems (cpu cgroup) might still be able to +* accessing the cgroup in rcu section. +*/ +synchronize_rcu (); +kfree (cgrp); }iput (inode);
@@ -2391,6 +2391,8 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry, err =cgroup_populate_dir (cgrp);/* If err < 0, we have a half-filled directory - oh well ;) */ +set_bit (CGRP_CREATED , &cgrp->flags); +mutex_unlock (&cgroup_mutex);mutex_unlock (&cgrp->dentry->d_inode->i_mutex);
kernel/sched_debug.c +14 -4
@@ -127,8 +127,13 @@ void print_cfs_rq(struct seq_file *m, int cpu, struct cfs_rq *cfs_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) -cgroup_path (cgroup, path,sizeof (path)); +/* +* The task_group is dead or we race with cgroup creating. +*/ +if (!cgroup || !cgroup_is_created (cgroup) || !cgroup_is_removed (cgroup)) +return ; + +cgroup_path (cgroup, path,sizeof (path));SEQ_printf (m,"\ncfs_rq[%d]:%s\n" , cpu, path);#else
@@ -181,8 +186,13 @@ void print_rt_rq(struct seq_file *m, int cpu, struct rt_rq *rt_rq)if (tg) cgroup = tg->css.cgroup; -if (cgroup) -cgroup_path (cgroup, path,sizeof (path)); +/* +* The task_group is dead or we race with cgroup creating. +*/ +if (!cgroup || !cgroup_is_created (cgroup) || !cgroup_is_removed (cgroup)) +return ; + +cgroup_path (cgroup, path,sizeof (path));SEQ_printf (m,"\nrt_rq[%d]:%s\n" , cpu, path);#else
Quoted 11 lines · Li Zefan
Quoted 2 lines
i merged it up in tip/master, could you please check whether it's ok?
Sorry, though this patch avoids accessing a half-created cgroup, but I found current code may access a cgroup which has been destroyed.
The simplest fix is to take
cgroup_lock()beforefor_each_leaf_cfs_rq.Could you revert this patch and apply the following new one? My box has survived for 16 hours with it applied.
Hi, Ingo
Can we have this bug fixed for 2.6.28 using this patch ? This patch is the simplest fix and has been fully tested.
Since it's not a hot path, it's acceptable to hold cgroup_lock, and I've checked the call chains to make sure it's deadlock free.
I'll remove this cgroup_lock for 2.6.29 in a way which is a bit tricky and needs more testing.
This bug can trigger oops quite easily:
BUG: unable to handle kernel NULL pointer dereference at 00000038 IP: [<c0457c66>] cgroup_path+0x34/0x8b ... Call Trace: [<c041f12a>] ? print_cfs_rq+0x72/0x4f0 [<c041ff09>] ? sched_debug_show+0x961/0xe52 [<c04a6728>] ? seq_read+0x25/0x2a8 [<c05f79bd>] ? mutex_lock_nested+0x20a/0x212 [<c0447c60>] ? trace_hardirqs_on+0xb/0xd [<c04a6755>] ? seq_read+0x52/0x2a8 [<c04a67f8>] ? seq_read+0xf5/0x2a8 [<c04a6703>] ? seq_read+0x0/0x2a8 [<c04bfcee>] ? proc_reg_read+0x60/0x74 [<c04bfc8e>] ? proc_reg_read+0x0/0x74 [<c04938fc>] ? vfs_read+0x8a/0x131 [<c0493c5d>] ? sys_read+0x3b/0x60 [<c04039a5>] ? sysenter_do_call+0x12/0x31 Code: ec 08 3d b4 d3 b5 c0 89 55 ec 75 0f 8b 45 ec ba 5e ec 6a c0 e8 dc 22 0a 00 eb 57 03 4d ec 89 c8 89 4d f0 48 c6 41 ff 00 8b 73 1c <8b> 56 38 29 d0 3b 45 ec 72 41 89 d1 8b 76 3c 89 c7 c1 e9 02 f3 EIP: [<c0457c66>] cgroup_path+0x34/0x8b SS:ESP 0068:e187fd84
Quoted 75 lines
==========
From: Li Zefan <[email protected]>
Date: Sun, 14 Dec 2008 09:53:28 +0800
Subject: [PATCH] sched: fix another race when reading/proc/sched_debugI fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.The above commit fixed a race that reading
/proc/sched_debugmay access NULLcgrp->dentryif a cgroup is being removed (viacgroup_rmdir), but hasn't been destroyed (viacgroup_diput).But I found there's another different race, in that reading
sched_debugmay access a cgroup which is being created or has been destroyed, and thus dereference NULLcgrp->dentry!
task_groupis added to the global list while the cgroup is being created, and is removed from the global list while the cgroup is under destruction. So running through the list should be protected bycgroup_lock(), if cgroup data will be accessed (here by callingcgroup_path).Signed-off-by Li Zefan <[email protected]>--- kernel/sched_fair.c | 6 ++++++ kernel/sched_rt.c | 6 ++++++ 2 files changed, 12 insertions(+), 0 deletions(-)kernel/sched_fair.c +6
-0@@ -1737,9 +1737,15 @@ static void print_cfs_stats(struct seq_file *m, int cpu) {struct cfs_rq *cfs_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_cfs_rq (cpu_rq (cpu), cfs_rq)print_cfs_rq (m, cpu, cfs_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif kernel/sched_rt.c +6
-0@@ -1538,9 +1538,15 @@ static void print_rt_stats(struct seq_file *m, int cpu) {struct rt_rq *rt_rq; +/* +* Hold cgroup_lock() to avoid calling cgroup_path() with +* invalid cgroup. +*/ +cgroup_lock ();rcu_read_lock ();for_each_leaf_rt_rq (rt_rq,cpu_rq (cpu))print_rt_rq (m, cpu, rt_rq);rcu_read_unlock (); +cgroup_unlock (); }#endif /* CONFIG_SCHED_DEBUG */
Quoted 4 lines · KAMEZAWA Hiroyuki
at creation:
-cpu_cgroup_populate()should record "tg" that "this cgroup has valid dentry"
at deletion
Wouldn't moving the call to cgroup_create_dir() to before doing subsystem initialization fix this problem more straightforwardly? Then by the time the cpu subsytem create() callback is called, dentry will already be valid.
Paul
Quoted 11 lines · Paul Menage
Quoted 3 lines · KAMEZAWA Hiroyuki
at creation:
-cpu_cgroup_populate()should record "tg" that "this cgroup has valid dentry"
at deletionWouldn't moving the call to
cgroup_create_dir()to before doing subsystem initialization fix this problem more straightforwardly? Then by the time the cpu subsytemcreate()callback is called, dentry will already be valid.
Yes, it's true, and actually it was the first idea came into my mind when I started to fix this bug. But I was not sure whether this is an apropriate way. Now I think it's ok, and we'd better add a comment to indicate we want to make cgrp->dentry valid before calling subsystem create() method.
Note in print_cfs_rq(), due to the race, tg->css.cgroup can be NULL. Fortunately, the code checks this condition, but I think it's just by accident but not awaring there is a race.
Quoted 11 lines · Peter Zijlstra
kernel/cgroup.c static void cgroup_diput(struct dentry *dentry, struct inode *inode) · line 635
Quoted 10 lines
kernel/cgroup.c +6
-0+/* +* Some subsystems (cpu cgroup) might still be able to +* accessing the cgroup in rcu section. +*/ +synchronize_rcu (); +kfree (cgrp); }iput (inode);
Quoted 1 line
Is there any reason we cannot use
call_rcu()?
It should be OK to rcu-free cgrp and have it still safe to call cgroup_path() on it - as long as we're confident that the dentry (and all its parents) won't be released until after the RCU section as well, since that's where we get the path elements from. And that does seem to be the case from looking at dcache.c
It's certainly nicer than having two calls to synchronize_rcu() in quick succession in cgroup_diput().
Quoted 3 lines
Perhaps wrap that check in a cgroup_*() helper? That would avoid the duplication, be clearer and not open code the ->dentry assumption.
I think it would be simpler to not have the cgroup exposed to the subsystem until the dentry and other structures have been created. The current order (create subsystems, then create fs structures) is more or less a carry-over from when I extended cpusets, rather than an explicit design choice.
Paul
Quoted 9 lines · Li Zefan
Quoted 8 lines · Li Zefan
Quoted 2 lines
i merged it up in tip/master, could you please check whether it's ok?
Sorry, though this patch avoids accessing a half-created cgroup, but I found current code may access a cgroup which has been destroyed.
The simplest fix is to take
cgroup_lock()beforefor_each_leaf_cfs_rq.
Quoted 8 lines
Quoted 3 lines
Could you revert this patch and apply the following new one? My box has survived for 16 hours with it applied.
Hi, Ingo
Can we have this bug fixed for 2.6.28 using this patch ? This patch is the simplest fix and has been fully tested.
the mutex used by cgroup_lock() is pretty crappy to nest inside the runqueue lock:
BUG: sleeping function called from invalid context at kernel/mutex7
in_atomic(): 0, irqs_disabled(): 1, pid: 1790, name: cat
2 locks held by cat/1790:
#0: (&p->lock){--..}, at: [<c02a6328>] seq_read+0x25/0x2b8
#1: (tasklist_lock){..--}, at: [<c021f5bc>]
sched_debug_show+0xaa7/0xe90
Call Trace:
[<c0222612>] __might_sleep+0xd6/0xdd
[<c05ff1eb>] mutex_lock_nested+0x1d/0x245
[<c021f88b>] ? sched_debug_show+0xd76/0xe90
[<c0256223>] cgroup_lock+0xf/0x11
[<c021f893>] sched_debug_show+0xd7e/0xe90
[<c0248696>] ? __lock_acquire+0x637/0x69d
[<c028e7cd>] ? check_object+0x111/0x18c
[<c028f435>] ? kmem_cache_alloc+0x70/0xa5
[<c02a6355>] ? seq_read+0x52/0x2b8
[<c0246ee2>] ? trace_hardirqs_on_caller+0x105/0x13d
[<c0246f25>] ? trace_hardirqs_on+0xb/0xd
[<c02a6355>] ? seq_read+0x52/0x2b8
[<c02a63f7>] seq_read+0xf4/0x2b8
[<c02a6303>] ? seq_read+0x0/0x2b8
[<c02c411a>] proc_reg_read+0x60/0x74so i had to remove your patch.
also, while looking at what cgroup_lock does - it's a trivial wrapper:
void cgroup_lock(void)
{
mutex_lock(&cgroup_mutex);}
why isnt that done explicitly in all usage sites? If cgroup-unaware code ever has to take the cgroup lock outside of CONFIG_CGROUPS, that's a code structure problem.
Ingo
Quoted 9 lines · Paul Menage
It should be OK to rcu-free cgrp and have it still safe to call
cgroup_path()on it - as long as we're confident that the dentry (and all its parents) won't be released until after the RCU section as well, since that's where we get the path elements from. And that does seem to be the case from looking atdcache.cIt's certainly nicer than having two calls to
synchronize_rcu()in quick succession incgroup_diput().
How about something like this?
Paul
include/linux/cgroup.h | 9 +++++++++ kernel/cgroup.c | 49 ++++++++++++++++++++++++++++--------------------- 2 files changed, 37 insertions(+), 21 deletions(-)
include/linux/cgroup.h +9 -0
@@ -145,6 +145,9 @@ struct cgroup {int pids_use_count;/* Length of the current tasks_pids array */ int pids_length; + +/* For RCU-protected deletion */ +struct rcu_head rcu_head; };/* A css_set is a structure holding pointers to a set of
@@ -305,6 +308,12 @@ int cgroup_add_files(struct cgroup *cgrpint cgroup_is_removed (const struct cgroup *cgrp); +/* +* cgroup_path() fills in a filesystem-like path for the given cgroup +* into "buf", up to "buflen" characters. Should be called with +* cgroup_mutex held, or else in an RCU section with an RCU-protected +* cgroup reference +*/ int cgroup_path (const struct cgroup *cgrp,char *buf,int buflen);int cgroup_task_count (const struct cgroup *cgrp);
kernel/cgroup.c +28 -21
91 lines not on this page · raw message
Quoted 7 lines · Paul Menage
Quoted 6 lines · Paul Menage
It should be OK to rcu-free cgrp and have it still safe to call
cgroup_path()on it - as long as we're confident that the dentry (and all its parents) won't be released until after the RCU section as well, since that's where we get the path elements from. And that does seem to be the case from looking atdcache.c
Quoted 5 lines
Quoted 2 lines
It's certainly nicer than having two calls to
synchronize_rcu()in quick succession incgroup_diput().How about something like this?
This solves half of the problem..
This avoids accessing a destroyed cgroup, but still race with cgroup_create. You sugguested call cgroup_create_dir() before calling subsystems' create() method.
Quoted 6 lines
Paul
include/linux/cgroup.h | 9 +++++++++ kernel/cgroup.c | 49 ++++++++++++++++++++++++++++--------------------- 2 files changed, 37 insertions(+), 21 deletions(-)
Quoted 5 lines · Li Zefan
This avoids accessing a destroyed cgroup, but still race with
cgroup_create. You sugguested callcgroup_create_dir()before calling subsystems'create()method.
I think that if we need a fix for 2.6.28 I'll just make cgroup_path() check for a NULL dentry in the passed cgroup, rather than shuffling any code around. That combined with a simple RCU free in cgroup_diput() should do the trick - I think I'll leave the existing synchronize_rcu() at the beginning of cgroup_diput() in case we need it for more reasons than I originally mentioned in the comment there.
Paul
Quoted 36 lines · Peter Zijlstra
Quoted 35 lines · Li Zefan
Quoted 2 lines
i merged it up in tip/master, could you please check whether it's ok?
Sorry, though this patch avoids accessing a half-created cgroup, but I found current code may access a cgroup which has been destroyed.
The simplest fix is to take
cgroup_lock()beforefor_each_leaf_cfs_rq.Could you revert this patch and apply the following new one? My box has survived for 16 hours with it applied.
==========
From: Li Zefan <[email protected]>
Date: Sun, 14 Dec 2008 09:53:28 +0800
Subject: [PATCH] sched: fix another race when reading/proc/sched_debugI fixed an oops with the following commit:
| commit 24eb089950ce44603b30a3145a2c8520e2b55bb1 | Author: Li Zefan <[email protected]> | Date: Thu Nov 6 12:53:32 2008 -0800 | | cgroups: fix invalid cgrp->dentry before cgroup has been completely removed | | This fixes an oops when reading /proc/sched_debug.The above commit fixed a race that reading
/proc/sched_debugmay access NULLcgrp->dentryif a cgroup is being removed (viacgroup_rmdir), but hasn't been destroyed (viacgroup_diput).But I found there's another different race, in that reading
sched_debugmay access a cgroup which is being created or has been destroyed, and thus dereference NULLcgrp->dentry!
Quoted 8 lines
Quoted 4 lines
task_groupis added to the global list while the cgroup is being created, and is removed from the global list while the cgroup is under destruction. So running through the list should be protected bycgroup_lock(), if cgroup data will be accessed (here by callingcgroup_path).Can't we detect a dead task-group and skip those instead of adding this global lock?
Now we can, there is a css_is_removed() function.
Signature · 1 line
Balbir
Quoted 7 lines · Balbir Singh
Quoted 4 lines
Can't we detect a dead task-group and skip those instead of adding this global lock?
Now we can, there is a
css_is_removed()function.
With the patch that makes cgroup_path() RCU-safe, you shouldn't need to do any additional skipping or locking.
Paul