Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree

[Tejun Heo <tj@kernel.org>]

Hello, Oleg.

On Fri, Aug 26, 2011 at 05:12:45PM +0200, Oleg Nesterov wrote:
> Yes, I think this is what we need, the patch itself looks fine.
> 
> But this doesn't answer my another question. After that the code does
> 
> 	 * step 4: do expensive, non-thread-specific subsystem callbacks.
> 
> 		ss->attach(ss, cgrp, oldcgrp, leader);
> 
> OK, non-thread-specific is nice, but how can this "leader" represent
> the process?
> 
> It can be zombie (but still group_leader) even without any races.
> Say, cpuset_attach() and mem_cgroup_move_task() need get_task_mm(p).
> How this can work if the leader is dead?
> 
> Also. Even if we add the locking around while_each_thread() (btw,
> we need this in any case), we can race with exec which can change
> the leader. In this case this task_struct has nothing to do with
> the process we are going to attach, at all.
> 
> And, ss->can_attach(leader) has the same problems, it seems.

Please take a look at the following series.

  http://thread.gmane.org/gmane.linux.kernel/1184375

attach racing against exit/exec is problematic and maybe we should
extend task->threadgroup_fork_lock protection to cover both exit and
exec.  I can't like it tho.  I hope this can be somehow done in
lighter way.  cgroup attaches are quite cold paths and we're putting
an extra rwsem in each task for that.

Thanks.

-- 
tejun