lkml.org 
[lkml]   [2011]   [Aug]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: + 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.



    \
     
     \ /
      Last update: 2011-08-26 17:25    [W:5.076 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site