Messages in this thread | | | Date | Wed, 16 Apr 2008 21:11:44 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH] cgroup: fix a race condition in manipulating tsk->cg_list |
| |
On Thu, 17 Apr 2008 11:37:15 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:
> When I ran a test program to fork mass processes and at the same time > 'cat /cgroup/tasks', I got the following oops: > > ------------[ cut here ]------------ > kernel BUG at lib/list_debug.c:72! > invalid opcode: 0000 [#1] SMP > Pid: 4178, comm: a.out Not tainted (2.6.25-rc9 #72) > ... > Call Trace: > [<c044a5f9>] ? cgroup_exit+0x55/0x94 > [<c0427acf>] ? do_exit+0x217/0x5ba > [<c0427ed7>] ? do_group_exit+0.65/0x7c > [<c0427efd>] ? sys_exit_group+0xf/0x11 > [<c0404842>] ? syscall_call+0x7/0xb > [<c05e0000>] ? init_cyrix+0x2fa/0x479 > ... > EIP: [<c04df671>] list_del+0x35/0x53 SS:ESP 0068:ebc7df4 > ---[ end trace caffb7332252612b ]--- > Fixing recursive fault but reboot is needed! > > After digging into the code and debugging, I finlly found out a race > situation: > do_exit() > ->cgroup_exit() > ->if (!list_empty(&tsk->cg_list)) > list_del(&tsk->cg_list); > > cgroup_iter_start() > ->cgroup_enable_task_cg_list() > ->list_add(&tsk->cg_list, ..); > > In this case the list won't be deleted though the process has exited.
I don't fully understand the race. Both paths hold css_set_lock.
Can you describe it in more detail please?
> We got two bug reports in the past, which seem to be the same bug as > this one: > http://lkml.org/lkml/2008/3/5/332 > http://lkml.org/lkml/2007/10/17/224 > > Actually sometimes I got oops on list_del, sometimes oops on list_add. > And I can change my test program a bit to trigger other oops. > > The patch has been tested both on x86_32 and x86_64. > > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> > --- > kernel/cgroup.c | 7 ++++++- > 1 files changed, 6 insertions(+), 1 deletions(-) > > diff --git a/kernel/cgroup.c b/kernel/cgroup.c > index 2727f92..6d8de05 100644 > --- a/kernel/cgroup.c > +++ b/kernel/cgroup.c > @@ -1722,7 +1722,12 @@ void cgroup_enable_task_cg_lists(void) > use_task_css_set_links = 1; > do_each_thread(g, p) { > task_lock(p); > - if (list_empty(&p->cg_list)) > + /* > + * We should check if the process is exiting, otherwise > + * it will race with cgroup_exit() in that the list > + * entry won't be deleted though the process has exited. > + */ > + if (!(p->flags & PF_EXITING) && list_empty(&p->cg_list)) > list_add(&p->cg_list, &p->cgroups->tasks); > task_unlock(p); > } while_each_thread(g, p);
Don't think I understand the fix either :(
afacit the task at *p could set PF_EXITING immediately after this code has tested PF_EXITING and then the task at *p could proceed until we hit the same race (whatever that is).
Perhaps taking p->sighand->siglock would fix that up, but that's just a guess at this stage.
| |