Re: [PATCH] sched: fix another race when reading /proc/sched_debug

Peter Zijlstra
2008-12-15T10:44:17Z

On Mon, 2008-12-15 at 17:51 +0800, Li Zefan wrote:
> >>> 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()
> >
> > 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.

Yeah, the below looks like a suitable fix.

> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> kernel/cgroup.c | 6 ++++++
> kernel/sched_debug.c | 17 +++++++++++++++--
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index fe00b3b..3c54d1b 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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);

Is there any reason we cannot use call_rcu() ?

> diff --git a/kernel/sched_debug.c b/kernel/sched_debug.c
> index 26ed8e3..174c072 100644
> --- a/kernel/sched_debug.c
> +++ b/kernel/sched_debug.c
> @@ -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));
> + }

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?

> 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