On Mon, Feb 7, 2011 at 5:39 PM, Ben Blum <bblum@xxxxxxxxxxxxxx> wrote:
> Makes procs file writable to move all threads by tgid at once
>
> From: Ben Blum <bblum@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxx>
> ---
> + /* 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?
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
