* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree [not found] <201109012108.p81L8X0b029484@imap1.linux-foundation.org> @ 2011-09-02 12:37 ` Oleg Nesterov 2011-09-02 14:00 ` Oleg Nesterov 0 siblings, 1 reply; 10+ messages in thread From: Oleg Nesterov @ 2011-09-02 12:37 UTC (permalink / raw) To: akpm; +Cc: linux-kernel, bblum, fweisbec, neilb, paul, paulmck > From: Ben Blum <bblum@andrew.cmu.edu> > > Fix unstable tasklist locking in cgroup_attach_proc. > > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is > not sufficient to guarantee the tasklist is stable w.r.t. de_thread and > exit. Taking tasklist_lock for reading, instead of rcu_read_lock, ensures > proper exclusion. I still think we should avoid the global lock. In any case, with tasklist or siglock, > - rcu_read_lock(); > + read_lock(&tasklist_lock); > if (!thread_group_leader(leader)) { > /* > * a race with de_thread from another thread's exec() may strip > @@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg > * throw this task away and try again (from cgroup_procs_write); > * this is "double-double-toil-and-trouble-check locking". > */ > - rcu_read_unlock(); > + read_unlock(&tasklist_lock); > retval = -EAGAIN; this check+comment becomes completely pointless and imho very confusing. Oleg. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-02 12:37 ` + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree Oleg Nesterov @ 2011-09-02 14:00 ` Oleg Nesterov 2011-09-02 14:15 ` Ben Blum 0 siblings, 1 reply; 10+ messages in thread From: Oleg Nesterov @ 2011-09-02 14:00 UTC (permalink / raw) To: akpm; +Cc: linux-kernel, bblum, fweisbec, neilb, paul, paulmck Forgot to mention, sorry... That said, I believe the patch is correct and should fix the problem. On 09/02, Oleg Nesterov wrote: > > > From: Ben Blum <bblum@andrew.cmu.edu> > > > > Fix unstable tasklist locking in cgroup_attach_proc. > > > > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is > > not sufficient to guarantee the tasklist is stable w.r.t. de_thread and > > exit. Taking tasklist_lock for reading, instead of rcu_read_lock, ensures > > proper exclusion. > > I still think we should avoid the global lock. > > In any case, with tasklist or siglock, > > > - rcu_read_lock(); > > + read_lock(&tasklist_lock); > > if (!thread_group_leader(leader)) { > > /* > > * a race with de_thread from another thread's exec() may strip > > @@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg > > * throw this task away and try again (from cgroup_procs_write); > > * this is "double-double-toil-and-trouble-check locking". > > */ > > - rcu_read_unlock(); > > + read_unlock(&tasklist_lock); > > retval = -EAGAIN; > > this check+comment becomes completely pointless and imho very confusing. > > Oleg. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-02 14:00 ` Oleg Nesterov @ 2011-09-02 14:15 ` Ben Blum 2011-09-02 15:55 ` Oleg Nesterov 0 siblings, 1 reply; 10+ messages in thread From: Ben Blum @ 2011-09-02 14:15 UTC (permalink / raw) To: Oleg Nesterov; +Cc: akpm, linux-kernel, bblum, fweisbec, neilb, paul, paulmck On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote: > Forgot to mention, sorry... > > That said, I believe the patch is correct and should fix the problem. Thanks! But I don't think the check becomes pointless? If a sub-thread execs right before read_lock(&tasklist_lock) (but after the find_task_by_vpid in attach_task_by_pid), that causes the case that the comment refers to. -- Ben > > On 09/02, Oleg Nesterov wrote: > > > > > From: Ben Blum <bblum@andrew.cmu.edu> > > > > > > Fix unstable tasklist locking in cgroup_attach_proc. > > > > > > According to this thread - https://lkml.org/lkml/2011/7/27/243 - RCU is > > > not sufficient to guarantee the tasklist is stable w.r.t. de_thread and > > > exit. Taking tasklist_lock for reading, instead of rcu_read_lock, ensures > > > proper exclusion. > > > > I still think we should avoid the global lock. > > > > In any case, with tasklist or siglock, > > > > > - rcu_read_lock(); > > > + read_lock(&tasklist_lock); > > > if (!thread_group_leader(leader)) { > > > /* > > > * a race with de_thread from another thread's exec() may strip > > > @@ -2036,7 +2036,7 @@ int cgroup_attach_proc(struct cgroup *cg > > > * throw this task away and try again (from cgroup_procs_write); > > > * this is "double-double-toil-and-trouble-check locking". > > > */ > > > - rcu_read_unlock(); > > > + read_unlock(&tasklist_lock); > > > retval = -EAGAIN; > > > > this check+comment becomes completely pointless and imho very confusing. > > > > Oleg. > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-02 14:15 ` Ben Blum @ 2011-09-02 15:55 ` Oleg Nesterov 2011-09-07 23:59 ` Ben Blum 0 siblings, 1 reply; 10+ messages in thread From: Oleg Nesterov @ 2011-09-02 15:55 UTC (permalink / raw) To: Ben Blum; +Cc: akpm, linux-kernel, fweisbec, neilb, paul, paulmck On 09/02, Ben Blum wrote: > > On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote: > > Forgot to mention, sorry... > > > > That said, I believe the patch is correct and should fix the problem. > > Thanks! > > But I don't think the check becomes pointless? If a sub-thread execs > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid > in attach_task_by_pid), that causes the case that the comment refers to. How so? The comment says: * a race with de_thread from another thread's exec() may strip * us of our leadership, making while_each_thread unsafe This is not true. And. Given that ->group_leader can be changed right after we drop tasklist this check is pointless. Yes, it can detect the case when this task_struct has nothing to do with this process sometimes, but not in general. (This connects to other problems I mentioned). IOW, personally I think it would be better to update the patch. But I won't insist. Oleg. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-02 15:55 ` Oleg Nesterov @ 2011-09-07 23:59 ` Ben Blum 2011-09-08 17:35 ` Oleg Nesterov 0 siblings, 1 reply; 10+ messages in thread From: Ben Blum @ 2011-09-07 23:59 UTC (permalink / raw) To: Oleg Nesterov Cc: Ben Blum, akpm, linux-kernel, fweisbec, neilb, paul, paulmck On Fri, Sep 02, 2011 at 05:55:34PM +0200, Oleg Nesterov wrote: > On 09/02, Ben Blum wrote: > > > > On Fri, Sep 02, 2011 at 04:00:15PM +0200, Oleg Nesterov wrote: > > > Forgot to mention, sorry... > > > > > > That said, I believe the patch is correct and should fix the problem. > > > > Thanks! > > > > But I don't think the check becomes pointless? If a sub-thread execs > > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid > > in attach_task_by_pid), that causes the case that the comment refers to. > > How so? The comment says: > > * a race with de_thread from another thread's exec() may strip > * us of our leadership, making while_each_thread unsafe > > This is not true. Sorry, the comment is unclear. The reason I think this is necessary is if de_thread happens, the leader would fall off the thread-group list: de_thread() => release_task(leader) => __exit_signal(leader) => __unhash_process(leader, false) => list_del_rcu(&leader->thread_group) which is the same list that while_each_thread() iterates over. and this looks like an unconditionally taken path? > > And. Given that ->group_leader can be changed right after we drop tasklist > this check is pointless. Yes, it can detect the case when this task_struct > has nothing to do with this process sometimes, but not in general. (This > connects to other problems I mentioned). I agree there is a problem later with the ss->attach(leader) calls. If the above reasoning is right, though, it's necessary here, and also guarantees that that the later iteration (in cgroup_attach_proc's "step 3") accurately reflects all threads in the group. Thanks, Ben > > IOW, personally I think it would be better to update the patch. But I > won't insist. > > Oleg. > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-07 23:59 ` Ben Blum @ 2011-09-08 17:35 ` Oleg Nesterov 2011-09-08 18:58 ` Ben Blum 0 siblings, 1 reply; 10+ messages in thread From: Oleg Nesterov @ 2011-09-08 17:35 UTC (permalink / raw) To: Ben Blum; +Cc: akpm, linux-kernel, fweisbec, neilb, paul, paulmck On 09/07, Ben Blum wrote: > > On Fri, Sep 02, 2011 at 05:55:34PM +0200, Oleg Nesterov wrote: > > On 09/02, Ben Blum wrote: > > > > > > But I don't think the check becomes pointless? If a sub-thread execs > > > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid > > > in attach_task_by_pid), that causes the case that the comment refers to. > > > > How so? The comment says: > > > > * a race with de_thread from another thread's exec() may strip > > * us of our leadership, making while_each_thread unsafe > > > > This is not true. > > Sorry, the comment is unclear. No, the comment is clear. In fact it was me who pointed out we can't do while_each_thread() blindly. And now I am tried to confuse you ;) So, sorry for noise, and thanks for correcting me. Somehow I forgot this is not safe even under tasklist. Partly I was confused because I was thinking about the patch I suggested, if we use ->siglock we are safe. If lock_task_sighand(task) succeeds, this task should be on list. Anyway, I was wrong, sorry. Oleg. --- x/kernel/cgroup.c +++ x/kernel/cgroup.c @@ -2000,6 +2000,7 @@ int cgroup_attach_proc(struct cgroup *cg /* threadgroup list cursor and array */ struct task_struct *tsk; struct flex_array *group; + unsigned long flags; /* * we need to make sure we have css_sets for all the tasks we're * going to move -before- we actually start moving them, so that in @@ -2027,19 +2028,10 @@ int cgroup_attach_proc(struct cgroup *cg goto out_free_group_list; /* prevent changes to the threadgroup list while we take a snapshot. */ - rcu_read_lock(); - if (!thread_group_leader(leader)) { - /* - * a race with de_thread from another thread's exec() may strip - * us of our leadership, making while_each_thread unsafe to use - * on this task. if this happens, there is no choice but to - * throw this task away and try again (from cgroup_procs_write); - * this is "double-double-toil-and-trouble-check locking". - */ - rcu_read_unlock(); - retval = -EAGAIN; + retval = -EAGAIN; + if (!lock_task_sighand(leader, &flags)) goto out_free_group_list; - } + /* take a reference on each task in the group to go in the array. */ tsk = leader; i = 0; @@ -2055,9 +2047,9 @@ int cgroup_attach_proc(struct cgroup *cg BUG_ON(retval != 0); i++; } while_each_thread(leader, tsk); + unlock_task_sighand(leader, &flags); /* remember the number of threads in the array for later. */ group_size = i; - rcu_read_unlock(); /* * step 1: check that we can legitimately attach to the cgroup. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-08 17:35 ` Oleg Nesterov @ 2011-09-08 18:58 ` Ben Blum 2011-09-08 21:31 ` Oleg Nesterov 0 siblings, 1 reply; 10+ messages in thread From: Ben Blum @ 2011-09-08 18:58 UTC (permalink / raw) To: Oleg Nesterov Cc: Ben Blum, akpm, linux-kernel, fweisbec, neilb, paul, paulmck On Thu, Sep 08, 2011 at 07:35:59PM +0200, Oleg Nesterov wrote: > On 09/07, Ben Blum wrote: > > > > On Fri, Sep 02, 2011 at 05:55:34PM +0200, Oleg Nesterov wrote: > > > On 09/02, Ben Blum wrote: > > > > > > > > But I don't think the check becomes pointless? If a sub-thread execs > > > > right before read_lock(&tasklist_lock) (but after the find_task_by_vpid > > > > in attach_task_by_pid), that causes the case that the comment refers to. > > > > > > How so? The comment says: > > > > > > * a race with de_thread from another thread's exec() may strip > > > * us of our leadership, making while_each_thread unsafe > > > > > > This is not true. > > > > Sorry, the comment is unclear. > > No, the comment is clear. In fact it was me who pointed out we can't > do while_each_thread() blindly. And now I am tried to confuse you ;) > > So, sorry for noise, and thanks for correcting me. Somehow I forgot > this is not safe even under tasklist. > > Partly I was confused because I was thinking about the patch I suggested, > if we use ->siglock we are safe. If lock_task_sighand(task) succeeds, > this task should be on list. > > Anyway, I was wrong, sorry. > > Oleg. All right, no problem. As for the patch below (which is the same as it was last time?): did you mean for Andrew to replace the old tasklist_lock patch with this one, or should one of us rewrite this against it? either way, it should have something like the comment I proposed in the first thread. Thanks, Ben > > --- x/kernel/cgroup.c > +++ x/kernel/cgroup.c > @@ -2000,6 +2000,7 @@ int cgroup_attach_proc(struct cgroup *cg > /* threadgroup list cursor and array */ > struct task_struct *tsk; > struct flex_array *group; > + unsigned long flags; > /* > * we need to make sure we have css_sets for all the tasks we're > * going to move -before- we actually start moving them, so that in > @@ -2027,19 +2028,10 @@ int cgroup_attach_proc(struct cgroup *cg > goto out_free_group_list; > > /* prevent changes to the threadgroup list while we take a snapshot. */ > - rcu_read_lock(); > - if (!thread_group_leader(leader)) { > - /* > - * a race with de_thread from another thread's exec() may strip > - * us of our leadership, making while_each_thread unsafe to use > - * on this task. if this happens, there is no choice but to > - * throw this task away and try again (from cgroup_procs_write); > - * this is "double-double-toil-and-trouble-check locking". > - */ > - rcu_read_unlock(); > - retval = -EAGAIN; > + retval = -EAGAIN; > + if (!lock_task_sighand(leader, &flags)) > goto out_free_group_list; > - } > + > /* take a reference on each task in the group to go in the array. */ > tsk = leader; > i = 0; > @@ -2055,9 +2047,9 @@ int cgroup_attach_proc(struct cgroup *cg > BUG_ON(retval != 0); > i++; > } while_each_thread(leader, tsk); > + unlock_task_sighand(leader, &flags); > /* remember the number of threads in the array for later. */ > group_size = i; > - rcu_read_unlock(); > > /* > * step 1: check that we can legitimately attach to the cgroup. > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-08 18:58 ` Ben Blum @ 2011-09-08 21:31 ` Oleg Nesterov 2011-09-09 2:11 ` Ben Blum 0 siblings, 1 reply; 10+ messages in thread From: Oleg Nesterov @ 2011-09-08 21:31 UTC (permalink / raw) To: Ben Blum; +Cc: akpm, linux-kernel, fweisbec, neilb, paul, paulmck On 09/08, Ben Blum wrote: > > As for the patch below (which is the same as it was last time?): It is the same, yes, I simply copied it from my old email. BTW, it wasn't tested at all, even compiled. > did you > mean for Andrew to replace the old tasklist_lock patch with this one, or > should one of us rewrite this against it? This is up to you. And just in case, please feel free to do nothing and keep the current fix. > either way, it should have > something like the comment I proposed in the first thread. Confused... Aha, I missed another email from you. You mean /* If the leader exits, its links on the thread_group list become * invalid. One way this can happen is if a sub-thread does exec() when * de_thread() calls release_task(leader) (and leader->sighand gets set * to NULL, in which case lock_task_sighand will fail). Since in that * case the threadgroup is still around, cgroup_procs_write should try * again (finding the new leader), which EAGAIN indicates here. This is * "double-double-toil-and-trouble-check locking". */ Agreed. Off-topic question... Looking at this code, it seems that attach_task_by_pid(zombie_pid, threadgroup => true) returns 0. single-task-only case fails with -ESRCH in this case. I am not saying this is wrong, just this looks a bit strange (unless I misread the code). Oleg. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-08 21:31 ` Oleg Nesterov @ 2011-09-09 2:11 ` Ben Blum 2011-09-09 16:41 ` Oleg Nesterov 0 siblings, 1 reply; 10+ messages in thread From: Ben Blum @ 2011-09-09 2:11 UTC (permalink / raw) To: Oleg Nesterov Cc: Ben Blum, akpm, linux-kernel, fweisbec, neilb, paul, paulmck On Thu, Sep 08, 2011 at 11:31:30PM +0200, Oleg Nesterov wrote: > On 09/08, Ben Blum wrote: > > > > As for the patch below (which is the same as it was last time?): > > It is the same, yes, I simply copied it from my old email. BTW, it > wasn't tested at all, even compiled. Testing is recommended ;) > > > did you > > mean for Andrew to replace the old tasklist_lock patch with this one, or > > should one of us rewrite this against it? > > This is up to you. And just in case, please feel free to do nothing and > keep the current fix. No, I do like the sighand method better now that I've thought about it. The way it subsumes the check for consistency of the leader's links is rather nice (much as it still requires deep understanding of de_thread). If you polished this patch off, I'd be happier, since I have a lot else on my plate. > > > either way, it should have > > something like the comment I proposed in the first thread. > > Confused... Aha, I missed another email from you. You mean > > /* If the leader exits, its links on the thread_group list become > * invalid. One way this can happen is if a sub-thread does exec() when > * de_thread() calls release_task(leader) (and leader->sighand gets set > * to NULL, in which case lock_task_sighand will fail). Since in that > * case the threadgroup is still around, cgroup_procs_write should try > * again (finding the new leader), which EAGAIN indicates here. This is > * "double-double-toil-and-trouble-check locking". */ > > Agreed. > > > > > > Off-topic question... Looking at this code, it seems that > attach_task_by_pid(zombie_pid, threadgroup => true) returns 0. > single-task-only case fails with -ESRCH in this case. I am not > saying this is wrong, just this looks a bit strange (unless I > misread the code). yeah, this is a side-effect of cgroup_attach_proc continuing to iterate in case any one of the sub-threads be still alive. you could keep track of whether all threads were zombies and return -ESRCH then, but it shouldn't matter, since the user-facing behaviour is indistinguishable from if the user had sent the command just before the task turned zombie but while it was still about to exit. Thanks, Ben > > Oleg. > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree 2011-09-09 2:11 ` Ben Blum @ 2011-09-09 16:41 ` Oleg Nesterov 0 siblings, 0 replies; 10+ messages in thread From: Oleg Nesterov @ 2011-09-09 16:41 UTC (permalink / raw) To: Ben Blum; +Cc: akpm, linux-kernel, fweisbec, neilb, paul, paulmck On 09/08, Ben Blum wrote: > > On Thu, Sep 08, 2011 at 11:31:30PM +0200, Oleg Nesterov wrote: > > On 09/08, Ben Blum wrote: > > > > > > As for the patch below (which is the same as it was last time?): > > > > It is the same, yes, I simply copied it from my old email. BTW, it > > wasn't tested at all, even compiled. > > Testing is recommended ;) Heh, that is why I didn't send the patch "officially". Not to mention I never used cgroups. > If you polished this patch off, I'd be happier, since I have a lot else > on my plate. Same here ;) > > Off-topic question... Looking at this code, it seems that > > attach_task_by_pid(zombie_pid, threadgroup => true) returns 0. > > single-task-only case fails with -ESRCH in this case. I am not > > saying this is wrong, just this looks a bit strange (unless I > > misread the code). > > yeah, this is a side-effect of cgroup_attach_proc continuing to iterate > in case any one of the sub-threads be still alive. you could keep track > of whether all threads were zombies and return -ESRCH then, Yes I see. Although PF_EXITING && thread_group_empty() could help. > but it > shouldn't matter, since the user-facing behaviour is indistinguishable > from if the user had sent the command just before the task turned zombie > but while it was still about to exit. Not sure. A task can spend a lot of time being zombie. In this case we return success or -ESRCH depending on threadgroup. But I agree, this is minor. Oleg. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-09-09 16:44 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <201109012108.p81L8X0b029484@imap1.linux-foundation.org>
2011-09-02 12:37 ` + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree Oleg Nesterov
2011-09-02 14:00 ` Oleg Nesterov
2011-09-02 14:15 ` Ben Blum
2011-09-02 15:55 ` Oleg Nesterov
2011-09-07 23:59 ` Ben Blum
2011-09-08 17:35 ` Oleg Nesterov
2011-09-08 18:58 ` Ben Blum
2011-09-08 21:31 ` Oleg Nesterov
2011-09-09 2:11 ` Ben Blum
2011-09-09 16:41 ` Oleg Nesterov
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).