Re: + cgroups-more-safe-tasklist-locking-in-cgroup_attach_proc.patch added to -mm tree

[Ben Blum <bblum@andrew.cmu.edu>]

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.
> 
>