cgroups: don't cache common ancestor in task counter subsys

To reproduce the bug:

  # mount -t cgroup -o tasks xxx /cgroup
  # mkdir /cgroup/tmp
  # echo PID > /cgroup/tmp/cgroups.proc (PID has child threads)
  # echo PID > /cgroup/tasks
  # ecoh PID > /cgroup/tmp/cgroups.proc
  (oops!)

the call graph is:

	for_each_thread()
	    can_attach_task();

	for_each_thead()
	    attach_task();

but not:

	for_each_thread() {
	    can_attach_task();
	    attach_task();
	}

So common_ancestor used in attach_task() is always the value calculated in the last can_attach_task() call.

To fix it, instead of caching, we always calculate the value every time we want to use it.

Reported-by Ben Blum <[email protected]>

Signed-off-by Li Zefan <[email protected]>

---
 Documentation/cgroups/cgroups.txt |    3 ++-
 include/linux/cgroup.h            |    2 +-
 kernel/cgroup.c                   |    7 ++++---
 kernel/cgroup_task_counter.c      |   24 +++++++++++++++++-------
 4 files changed, 24 insertions(+), 12 deletions(-)

Documentation/cgroups/cgroups.txt +2 -1

index 3bc1dc9..f9e51a8 100644
@@ -623,7 +623,8 @@ function, so that the subsystem can implement a rollback. If not, not necessary.
 This will be called only about subsystems whose can_attach() operation have
 succeeded.
 
-void cancel_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+void cancel_attach_task(struct cgroup *cgrp, struct cgroup *oldcgrp,
+			struct task_struct *tsk)
 (cgroup_mutex held by caller)
 
 As cancel_attach, but for operations that must be cancelled once per

include/linux/cgroup.h +1 -1

index 9c8151e..e4659c4 100644
@@ -475,7 +475,7 @@ struct cgroup_subsys {
 			       struct task_struct *tsk);
 	void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			      struct task_struct *tsk);
-	void (*cancel_attach_task)(struct cgroup *cgrp,
+	void (*cancel_attach_task)(struct cgroup *cgrp, struct cgroup *oldcgrp,
 				   struct task_struct *tsk);
 	void (*pre_attach)(struct cgroup *cgrp);
 	void (*attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp,

kernel/cgroup.c +2 -2

index 018df9d..91abc9b 100644
@@ -1885,7 +1885,7 @@ out:
 				break;
 
 			if (ss->cancel_attach_task)
-				ss->cancel_attach_task(cgrp, tsk);
+				ss->cancel_attach_task(cgrp, oldcgrp, tsk);
 			if (ss->cancel_attach)
 				ss->cancel_attach(ss, cgrp, tsk);
 		}

@@ -2151,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 			}
 		} else if (retval == -ESRCH) {
 			if (ss->cancel_attach_task)
-				ss->cancel_attach_task(cgrp, tsk);
+				ss->cancel_attach_task(cgrp, oldcgrp, tsk);
 		} else {
 			BUG_ON(1);
 		}

Quoted 19 lines · Li Zefan

To reproduce the bug:

  # mount -t cgroup -o tasks xxx /cgroup
  # mkdir /cgroup/tmp
  # echo PID > /cgroup/tmp/cgroups.proc (PID has child threads)
  # echo PID > /cgroup/tasks
  # ecoh PID > /cgroup/tmp/cgroups.proc
  (oops!)

the call graph is:

	for_each_thread()
	    can_attach_task();

	for_each_thead()
	    attach_task();

but not:

Quoted 7 lines

	for_each_thread() {
	    can_attach_task();
	    attach_task();
	}

So common_ancestor used in attach_task() is always the value calculated in the last can_attach_task() call.

Ah! good catch!

Just a comment below:

Quoted 63 lines

To fix it, instead of caching, we always calculate the value every time we want to use it.

Reported-by Ben Blum <[email protected]>

Signed-off-by Li Zefan <[email protected]>

---
 Documentation/cgroups/cgroups.txt |    3 ++-
 include/linux/cgroup.h            |    2 +-
 kernel/cgroup.c                   |    7 ++++---
 kernel/cgroup_task_counter.c      |   24 +++++++++++++++++-------
 4 files changed, 24 insertions(+), 12 deletions(-)

Documentation/cgroups/cgroups.txt +2 -1

index 3bc1dc9..f9e51a8 100644
@@ -623,7 +623,8 @@ function, so that the subsystem can implement a rollback. If not, not necessary.
 This will be called only about subsystems whose can_attach() operation have
 succeeded.
 
-void cancel_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
+void cancel_attach_task(struct cgroup *cgrp, struct cgroup *oldcgrp,
+			struct task_struct *tsk)
 (cgroup_mutex held by caller)
 
 As cancel_attach, but for operations that must be cancelled once per

include/linux/cgroup.h +1 -1

index 9c8151e..e4659c4 100644
@@ -475,7 +475,7 @@ struct cgroup_subsys {
 			       struct task_struct *tsk);
 	void (*cancel_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			      struct task_struct *tsk);
-	void (*cancel_attach_task)(struct cgroup *cgrp,
+	void (*cancel_attach_task)(struct cgroup *cgrp, struct cgroup *oldcgrp,
 				   struct task_struct *tsk);
 	void (*pre_attach)(struct cgroup *cgrp);
 	void (*attach_task)(struct cgroup *cgrp, struct cgroup *old_cgrp,

kernel/cgroup.c +2 -2

index 018df9d..91abc9b 100644
@@ -1885,7 +1885,7 @@ out:
 				break;
 
 			if (ss->cancel_attach_task)
-				ss->cancel_attach_task(cgrp, tsk);
+				ss->cancel_attach_task(cgrp, oldcgrp, tsk);
 			if (ss->cancel_attach)
 				ss->cancel_attach(ss, cgrp, tsk);
 		}

@@ -2151,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 			}
 		} else if (retval == -ESRCH) {
 			if (ss->cancel_attach_task)
-				ss->cancel_attach_task(cgrp, tsk);
+				ss->cancel_attach_task(cgrp, oldcgrp, tsk);
 		} else {
 			BUG_ON(1);
 		}

kernel/cgroup.c out_cancel_attach: · line 2195

Quoted 6 lines

kernel/cgroup.c +2 -1

@@ -2191,7 +2191,8 @@ out_cancel_attach:
 					tsk = flex_array_get_ptr(group, i);
 					if (tsk == failed_task)
 						break;
-					ss->cancel_attach_task(cgrp, tsk);
+					ss->cancel_attach_task(cgrp, oldcgrp,
+							       tsk);

When we rollback there, we are dealing with oldcgrp of the last thread we have treated. All threads in the rollback list don't necessary belonged to that old_cgroup.

And we can't try to retrieve these old_cgroup through task_cgroup_from_root() because the threads might have exited and thus could be assigned to the init cgroup.

I believe we need to cache these old cgroups in the flex array.

Quoted 7 lines · Frederic Weisbecker

When we rollback there, we are dealing with oldcgrp of the last thread we have treated. All threads in the rollback list don't necessary belonged to that old_cgroup.

And we can't try to retrieve these old_cgroup through task_cgroup_from_root() because the threads might have exited and thus could be assigned to the init cgroup.

Quoted 2 lines

I believe we need to cache these old cgroups in the flex array.

Doing this would fulfill the "TODO" in cgroup_attach_proc for being able to pass the oldcgrp for each task in the loop around the ss->attach call, so if you do that, remove the corresponding comment. :)

-- Ben

Quoted 8 lines · Ben Blum

Quoted 7 lines · Frederic Weisbecker

When we rollback there, we are dealing with oldcgrp of the last thread we have treated. All threads in the rollback list don't necessary belonged to that old_cgroup.

And we can't try to retrieve these old_cgroup through task_cgroup_from_root() because the threads might have exited and thus could be assigned to the init cgroup.

Quoted 8 lines

Quoted 2 lines

I believe we need to cache these old cgroups in the flex array.

Doing this would fulfill the "TODO" in cgroup_attach_proc for being able to pass the oldcgrp for each task in the loop around the ss->attach call, so if you do that, remove the corresponding comment. :)

-- Ben

I don't know, looking at the code, there is a separate issue to solve there. The old cgroup passed in ->attach() is the one of a random task from the thread group. The migrations that happen right before leave oldcgrp with the old cgroup of the last thread in the group.

This doesn't seem to make any sense. As far as I checked this is only used by the cpuset subsystem that does a migration in cpuset_attach() from the old cgroup to the new.

I'm not sure about the details but that looks buggy.

Quoted 12 lines

kernel/cgroup.c +1 -1

@@ -2151,7 +2151,7 @@ int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 			}
 		} else if (retval == -ESRCH) {
 			if (ss->cancel_attach_task)
-				ss->cancel_attach_task(cgrp, tsk);
+				ss->cancel_attach_task(cgrp, oldcgrp, tsk);
 		} else {
 			BUG_ON(1);
 		}

@@ -2191,7 +2191,8 @@ out_cancel_attach:
 					tsk = flex_array_get_ptr(group, i);
 					if (tsk == failed_task)

kernel/cgroup.c out_cancel_attach: · line 2195

Quoted 4 lines

kernel/cgroup.c +2 -1

 						break;
-					ss->cancel_attach_task(cgrp, tsk);
+					ss->cancel_attach_task(cgrp, oldcgrp,
+							       tsk);

When we rollback there, we are dealing with oldcgrp of the last thread we have treated. All threads in the rollback list don't necessary belonged to that old_cgroup.

And we can't try to retrieve these old_cgroup through task_cgroup_from_root() because the threads might have exited and thus could be assigned to the init cgroup.

I believe we need to cache these old cgroups in the flex array.