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

[Oleg Nesterov <oleg@redhat.com>]

On 08/25, Andrew Morton wrote:
>
> From: Ben Blum <bblum@andrew.cmu.edu>
>
> @@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cg
>  		oldcgrp = task_cgroup_from_root(tsk, root);
>  		if (cgrp == oldcgrp)
>  			continue;
> -		/* attach each task to each subsystem */
> -		for_each_subsys(root, ss) {
> -			if (ss->attach_task)
> -				ss->attach_task(cgrp, tsk);
> -		}
>  		/* if the thread is PF_EXITING, it can just get skipped. */
>  		retval = cgroup_task_migrate(cgrp, oldcgrp, tsk, true);
> -		BUG_ON(retval != 0 && retval != -ESRCH);
> +		if (retval == 0) {
> +			/* attach each task to each subsystem */
> +			for_each_subsys(root, ss) {
> +				if (ss->attach_task)
> +					ss->attach_task(cgrp, tsk);
> +			}

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.




And. Say, devcgroup_can_attach() checks CAP_SYS_ADMIN. This is
security check. Why it is enough to check the leader only? We are
going to attach all threads. OK, this is probably fine, and I never
understood why capable/creds are not per-process, but this looks
so strange.

Oleg.