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

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

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