lkml.org 
[lkml]   [2011]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v8 3/3] cgroups: make procs file writable
    On Mon, Feb 7, 2011 at 5:39 PM, Ben Blum <bblum@andrew.cmu.edu> wrote:
    > Makes procs file writable to move all threads by tgid at once
    >
    > From: Ben Blum <bblum@andrew.cmu.edu>
    >
    > This patch adds functionality that enables users to move all threads in a
    > threadgroup at once to a cgroup by writing the tgid to the 'cgroup.procs'
    > file. This current implementation makes use of a per-threadgroup rwsem that's
    > taken for reading in the fork() path to prevent newly forking threads within
    > the threadgroup from "escaping" while the move is in progress.
    >
    > Signed-off-by: Ben Blum <bblum@andrew.cmu.edu>
    > ---
    > +       /* remember the number of threads in the array for later. */
    > +       BUG_ON(i == 0);

    This BUG_ON() seems unnecessary, given the i++ directly above it.

    > +       group_size = i;
    > +       rcu_read_unlock();
    > +
    > +       /*
    > +        * step 1: check that we can legitimately attach to the cgroup.
    > +        */
    > +       for_each_subsys(root, ss) {
    > +               if (ss->can_attach) {
    > +                       retval = ss->can_attach(ss, cgrp, leader);
    > +                       if (retval) {
    > +                               failed_ss = ss;
    > +                               goto out_cancel_attach;
    > +                       }
    > +               }
    > +               /* a callback to be run on every thread in the threadgroup. */
    > +               if (ss->can_attach_task) {
    > +                       /* run on each task in the threadgroup. */
    > +                       for (i = 0; i < group_size; i++) {
    > +                               retval = ss->can_attach_task(cgrp, group[i]);
    > +                               if (retval) {
    > +                                       failed_ss = ss;

    Should we be setting failed_ss here? Doesn't that mean that if all
    subsystems pass the can_attach() check but the first one fails a
    can_attach_task() check, we don't call any cancel_attach() methods?

    What are the rollback semantics for failing a can_attach_task() check?

    > +               if (threadgroup) {
    > +                       /*
    > +                        * it is safe to find group_leader because tsk was found
    > +                        * in the tid map, meaning it can't have been unhashed
    > +                        * by someone in de_thread changing the leadership.
    > +                        */
    > +                       tsk = tsk->group_leader;
    > +                       BUG_ON(!thread_group_leader(tsk));

    Can this race with an exiting/execing group leader?

    > +               } else if (tsk->flags & PF_EXITING) {

    The check for PF_EXITING doesn't apply to group leaders?

    > +                       /* optimization for the single-task-only case */
    > +                       rcu_read_unlock();
    > +                       cgroup_unlock();
    >                        return -ESRCH;
    >                }
    >
    > +               /*
    > +                * even if we're attaching all tasks in the thread group, we
    > +                * only need to check permissions on one of them.
    > +                */
    >                tcred = __task_cred(tsk);
    >                if (cred->euid &&
    >                    cred->euid != tcred->uid &&
    >                    cred->euid != tcred->suid) {
    >                        rcu_read_unlock();
    > +                       cgroup_unlock();
    >                        return -EACCES;

    Maybe turn these returns into "goto out;" statements and put the
    unlock after the out: label?
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-03-03 19:41    [W:9.164 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site