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

From: Ben Blum <[email protected]>

kernel/cgroup.c renamed from kernel/cgroup.c~cgroups-fix-ordering-of-calls-in-cgroup_attach_proc +0 -4

@@ -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);

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?

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.

Please take a look at the following series.

http://thread.gmane.org/gmane.linux.kernel/1184375

attach racing against exit/exec is problematic and maybe we should extend task->threadgroup_fork_lock protection to cover both exit and exec. I can't like it tho.

The patch titled
cgroups: fix ordering of calls in cgroup_attach_proc
has been added to the -mm tree. Its filename is
cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch

Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: cgroups: fix ordering of calls in cgroup_attach_proc
From: Ben Blum <[email protected]>

awaiting useful changelog...

From a9b84e395a0355cd1aa5ee0f525cd682b16dad63 Mon Sep 17 00:00:00 2001

Quoted 23 lines · On Thu, Aug 25, 2011 at 01:44:36PM -0700, [email protected] wrote:

The patch titled
cgroups: fix ordering of calls in cgroup_attach_proc
has been added to the -mm tree. Its filename is
cgroups-fix-ordering-of-calls-in-cgroup_attach_proc.patch

Before you just go and hit "reply", please:
a) Consider who else should be cc'ed
b) Prefer to cc a suitable mailing list as well
c) Ideally: find the original patch on the mailing list and do a reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: cgroups: fix ordering of calls in cgroup_attach_proc
From: Ben Blum <[email protected]>

Quoted 2 lines

awaiting useful changelog...

Here is the patch with a (trial of a) useful changelog. Subject has been changed as well:

---
From a9b84e395a0355cd1aa5ee0f525cd682b16dad63 Mon Sep 17 00:00:00 2001
From: Ben Blum <[email protected]>
Date: Thu, 25 Aug 2011 13:44:36 -0700
Subject: [PATCH] cgroups: Don't attach task to subsystem if migration failed

If a task has exited to the point it has called cgroup_exit() already, then we can't migrate it to another cgroup anymore.

This can happen when we are attaching a task to a new cgroup between the call to ->can_attach_task() on subsystems and the migration that is eventually tried in cgroup_task_migrate().

In this case cgroup_task_migrate() returns -ESRCH and we don't want to attach the task to the subsystems because the attachment to the new cgroup itself failed.

Fix this by only calling ->attach_task() on the subsystems if the cgroup migration succeeded.

Reported-by Oleg Nesterov <[email protected]>

Signed-off-by Ben Blum <[email protected]>

Acked-by Paul Menage <[email protected]>

Cc Li Zefan <[email protected]>

Cc Tejun Heo <[email protected]>

Signed-off-by Frederic Weisbecker <[email protected]>

---
 kernel/cgroup.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

kernel/cgroup.c +9 -6

index 1d2b6ce..84bdace 100644
@@ -2135,14 +2135,17 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 		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);
+			}
+		} else {
+			BUG_ON(retval != -ESRCH);
+		}
 	}
 	/* nothing is sensitive to fork() after this point. */
 

Signature · 1 line

1.7.5.4