Messages in this thread | | | Date | Fri, 26 Aug 2011 17:12:45 +0200 | From | Oleg Nesterov <> | Subject | Re: + cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch added to -mm tree |
| |
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.
| |