cgroup_freezer: allow migration regardless of freezer state and update locking

cgroup core has a bug which violates a basic rule about event notifications - when a new entity needs to be added, you add that to the notification list first and then make the new entity conform to the current state. If done in the reverse order, an event happening inbetween will be lost.

cgroup_subsys->fork() is invoked way before the new task is added to the css_set. Currently, cgroup_freezer is the only user of ->fork() and uses it to make new tasks conform to the current state of the freezer. If FROZEN state is requested while fork is in progress between cgroup_fork_callbacks() and cgroup_post_fork(), the child could escape freezing - the cgroup isn't frozen when ->fork() is called and the freezer couldn't see the new task on the css_set.

This patch moves cgroup_subsys->fork() invocation to cgroup_post_fork() after the new task is added to the css_set. cgroup_fork_callbacks() is removed.

Because now a task may be migrated during cgroup_subsys->fork(), freezer_fork() is updated so that it adheres to the usual RCU locking and the rather pointless comment on why locking can be different there is removed (if it doesn't make anything simpler, why even bother?).

Signed-off-by Tejun Heo <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc [email protected]

---
 include/linux/cgroup.h  |    1 -
 kernel/cgroup.c         |   62 ++++++++++++++++++++++------------------------
 kernel/cgroup_freezer.c |   13 +++-------
 kernel/fork.c           |    9 +------
 4 files changed, 35 insertions(+), 50 deletions(-)

include/linux/cgroup.h +0 -1

index f8a030c..4cd1d0f 100644
@@ -34,7 +34,6 @@ extern int cgroup_lock_is_held(void);
 extern bool cgroup_lock_live_group(struct cgroup *cgrp);
 extern void cgroup_unlock(void);
 extern void cgroup_fork(struct task_struct *p);
-extern void cgroup_fork_callbacks(struct task_struct *p);
 extern void cgroup_post_fork(struct task_struct *p);
 extern void cgroup_exit(struct task_struct *p, int run_callbacks);
 extern int cgroupstats_build(struct cgroupstats *stats,

kernel/cgroup.c +30 -32

index 13774b3..b7a0171 100644
@@ -4844,44 +4844,19 @@ void cgroup_fork(struct task_struct *child)
 }
 
 /**
- * cgroup_fork_callbacks - run fork callbacks
- * @child: the new task
- *
- * Called on a new task very soon before adding it to the
- * tasklist. No need to take any locks since no-one can
- * be operating on this task.
- */
-void cgroup_fork_callbacks(struct task_struct *child)
-{
-	if (need_forkexit_callback) {
-		int i;
-		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
-			struct cgroup_subsys *ss = subsys[i];
-
-			/*
-			 * forkexit callbacks are only supported for
-			 * builtin subsystems.
-			 */
-			if (!ss || ss->module)
-				continue;
-
-			if (ss->fork)
-				ss->fork(child);
-		}
-	}
-}
-
-/**
  * cgroup_post_fork - called on a new task after adding it to the task list
  * @child: the task in question
  *
- * Adds the task to the list running through its css_set if necessary.
- * Has to be after the task is visible on the task list in case we race
- * with the first call to cgroup_iter_start() - to guarantee that the
- * new task ends up on its list.
+ * Adds the task to the list running through its css_set if necessary and
+ * call the subsystem fork() callbacks.  Has to be after the task is
+ * visible on the task list in case we race with the first call to
+ * cgroup_iter_start() - to guarantee that the new task ends up on its
+ * list.
  */
 void cgroup_post_fork(struct task_struct *child)
 {
+	int i;
+
 	/*
 	 * use_task_css_set_links is set to 1 before we walk the tasklist
 	 * under the tasklist_lock and we read it here after we added the child

@@ -4910,7 +4885,30 @@ void cgroup_post_fork(struct task_struct *child)
 		}
 		write_unlock(&css_set_lock);
 	}
+
+	/*
+	 * Call ss->fork().  This must happen after @child is linked on
+	 * css_set; otherwise, @child might change state between ->fork()
+	 * and addition to css_set.
+	 */
+	if (need_forkexit_callback) {
+		for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
+			struct cgroup_subsys *ss = subsys[i];
+
+			/*
+			 * fork/exit callbacks are supported only for
+			 * builtin subsystems and we don't need further
+			 * synchronization as they never go away.
+			 */
+			if (!ss || ss->module)
+				continue;
+
+			if (ss->fork)
+				ss->fork(child);
+		}
+	}
 }
+
 /**
  * cgroup_exit - detach cgroup from exiting task
  * @tsk: pointer to task_struct of exiting process

kernel/cgroup_freezer.c +4 -9

index b1724ce..12bfedb 100644
@@ -186,23 +186,15 @@ static void freezer_fork(struct task_struct *task)
 {
 	struct freezer *freezer;
 
-	/*
-	 * No lock is needed, since the task isn't on tasklist yet,
-	 * so it can't be moved to another cgroup, which means the
-	 * freezer won't be removed and will be valid during this
-	 * function call.  Nevertheless, apply RCU read-side critical
-	 * section to suppress RCU lockdep false positives.
-	 */
 	rcu_read_lock();
 	freezer = task_freezer(task);
-	rcu_read_unlock();
 
 	/*
 	 * The root cgroup is non-freezable, so we can skip the
 	 * following check.
 	 */
 	if (!freezer->css.cgroup->parent)
-		return;
+		goto out;
 
 	spin_lock_irq(&freezer->lock);
 	BUG_ON(freezer->state == CGROUP_FROZEN);

@@ -210,7 +202,10 @@ static void freezer_fork(struct task_struct *task)
 	/* Locking avoids race with FREEZING -> THAWED transitions. */
 	if (freezer->state == CGROUP_FREEZING)
 		freeze_task(task);
+
 	spin_unlock_irq(&freezer->lock);
+out:
+	rcu_read_unlock();
 }
 
 /*

kernel/fork.c +1 -8

index 8b20ab7..acc4cb6 100644
@@ -1135,7 +1135,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 {
 	int retval;
 	struct task_struct *p;
-	int cgroup_callbacks_done = 0;
 
 	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
 		return ERR_PTR(-EINVAL);

@@ -1393,12 +1392,6 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	INIT_LIST_HEAD(&p->thread_group);
 	p->task_works = NULL;
 
-	/* Now that the task is set up, run cgroup callbacks if
-	 * necessary. We need to run them before the task is visible
-	 * on the tasklist. */
-	cgroup_fork_callbacks(p);
-	cgroup_callbacks_done = 1;
-
 	/* Need tasklist lock for parent etc handling! */
 	write_lock_irq(&tasklist_lock);
 

@@ -1503,7 +1496,7 @@ bad_fork_cleanup_cgroup:
 #endif
 	if (clone_flags & CLONE_THREAD)
 		threadgroup_change_end(current);
-	cgroup_exit(p, cgroup_callbacks_done);
+	cgroup_exit(p, 0);
 	delayacct_tsk_free(p);
 	module_put(task_thread_info(p)->exec_domain->module);
 bad_fork_cleanup_count:

Quoted 14 lines

cgroup core has a bug which violates a basic rule about event notifications - when a new entity needs to be added, you add that to the notification list first and then make the new entity conform to the current state. If done in the reverse order, an event happening inbetween will be lost.

cgroup_subsys->fork() is invoked way before the new task is added to the css_set. Currently, cgroup_freezer is the only user of ->fork() and uses it to make new tasks conform to the current state of the freezer. If FROZEN state is requested while fork is in progress between cgroup_fork_callbacks() and cgroup_post_fork(), the child could escape freezing - the cgroup isn't frozen when ->fork() is called and the freezer couldn't see the new task on the css_set.

Quoted 9 lines

This patch moves cgroup_subsys->fork() invocation to cgroup_post_fork() after the new task is added to the css_set. cgroup_fork_callbacks() is removed.

Because now a task may be migrated during cgroup_subsys->fork(), freezer_fork() is updated so that it adheres to the usual RCU locking and the rather pointless comment on why locking can be different there is removed (if it doesn't make anything simpler, why even bother?).

I don't think rcu read section is sufficient. It guarantees the data you're accessing is valid, but the data can be new or can be old.

So a case below is possible:

in freezer_fork():
rcu_read_lock();
freezer = task_freezer(task);
move task from freezer to freezer2 which is in FREEZING/FROZEN state
freezer is in THAWED state,
nothing to do.
rcu_read_unlock();

Quoted 14 lines · Tejun Heo

cgroup core has a bug which violates a basic rule about event notifications - when a new entity needs to be added, you add that to the notification list first and then make the new entity conform to the current state. If done in the reverse order, an event happening inbetween will be lost.

cgroup_subsys->fork() is invoked way before the new task is added to the css_set. Currently, cgroup_freezer is the only user of ->fork() and uses it to make new tasks conform to the current state of the freezer. If FROZEN state is requested while fork is in progress between cgroup_fork_callbacks() and cgroup_post_fork(), the child could escape freezing - the cgroup isn't frozen when ->fork() is called and the freezer couldn't see the new task on the css_set.

Quoted 9 lines

This patch moves cgroup_subsys->fork() invocation to cgroup_post_fork() after the new task is added to the css_set. cgroup_fork_callbacks() is removed.

Because now a task may be migrated during cgroup_subsys->fork(), freezer_fork() is updated so that it adheres to the usual RCU locking and the rather pointless comment on why locking can be different there is removed (if it doesn't make anything simpler, why even bother?).

I don't think rcu read section is sufficient. It guarantees the data you're accessing is valid, but the data can be new or can be old.

So a case below is possible:

in freezer_fork():
rcu_read_lock();
freezer = task_freezer(task);
move task from freezer to freezer2 which is in FREEZING/FROZEN state
freezer is in THAWED state,
nothing to do.
rcu_read_unlock();

cgroup_subsys->fork() is invoked way before the new task is added to the css_set.

A couple of off-topic questions. With or without this patch I do not understand cgroup_fork,

	/*
	 * We don't need to task_lock() current because current->cgroups
	 * can't be changed concurrently here. The parent obviously hasn't
	 * exited and called cgroup_exit(), and we are synchronized against
	 * cgroup migration through threadgroup_change_begin().
	 */
	child->cgroups = current->cgroups;
	get_css_set(child->cgroups);

How so? threadgroup_change_begin() is only called if CLONE_THREAD. So in theory this copy + atomic_add looks racy...

And it seems that fork() can race with cgroup iterator. post_fork will notice use_task_css_set_links, but until then the child belongs to the parent's css and it is not "visible" to iterator (and right after cgroup_fork() it is not visible to do_each_thread() if use_task_css_set_links is not set).

For example. Suppose that the child migrates to another cgroup after copy_process() makes it visible to the user-space. Then update_if_frozen sets CGROUP_FROZEN (again, cgroup_iter_next do not see this child).

Now, post_fork calls freezer_fork() and hits BUG_ON(CGROUP_FROZEN).

But again, I do not blame this patch.

I am starting to think again about a big-rw-lock around copy_process. Recently I tried to add one around dup_mmap for uprobes, but perhaps cgroups can use it too...

Quoted 10 lines · Oleg Nesterov

And it seems that fork() can race with cgroup iterator. post_fork will notice use_task_css_set_links, but until then the child belongs to the parent's css and it is not "visible" to iterator (and right after cgroup_fork() it is not visible to do_each_thread() if use_task_css_set_links is not set).

For example. Suppose that the child migrates to another cgroup after copy_process() makes it visible to the user-space. Then update_if_frozen sets CGROUP_FROZEN (again, cgroup_iter_next do not see this child).

Quoted 3 lines

Now, post_fork calls freezer_fork() and hits BUG_ON(CGROUP_FROZEN).

But again, I do not blame this patch.

I'm planning to update it to,

* Clear ->cgroup to %NULL during copy_process().

Quoted 3 lines

I am starting to think again about a big-rw-lock around copy_process. Recently I tried to add one around dup_mmap for uprobes, but perhaps cgroups can use it too...

If some other subsystems need it, maybe just make threadgroup locking coarser?

I *think* I can make cgroup work correctly without a agiant rwlock

Quoted 1 line

* Clear ->cgroup to %NULL during copy_process().

I completely agree. new_child->cgroups copied from parent looks simply strange until post_fork. If nothing else, the new task is still under construction by the time cgroup_fork() is called.

Quoted 6 lines

Quoted 3 lines

I am starting to think again about a big-rw-lock around copy_process. Recently I tried to add one around dup_mmap for uprobes, but perhaps cgroups can use it too...

If some other subsystems need it, maybe just make threadgroup locking coarser?

What do you mean?

Quoted 4 lines · Oleg Nesterov

Quoted 4 lines

Quoted 3 lines

I am starting to think again about a big-rw-lock around copy_process. Recently I tried to add one around dup_mmap for uprobes, but perhaps cgroups can use it too...

Quoted 4 lines

Quoted 2 lines

If some other subsystems need it, maybe just make threadgroup locking coarser?

What do you mean?

I probabl have misunderstood you

but If you're gonna add big-rw-lock
around copy-process which is always gonna be grabbed, I was suggesting maybe we could simply repurpose the existing threadgroup locking.

Yes, yes. But in this case (I mean, for uprobes) "threadgroup" in the name is misleading. It should be called unconditially without any argument.

Please see

	[PATCH 1/2] brw_mutex: big read-write mutex
	http://marc.info/?l=linux-kernel&m=135032816223715

	[PATCH 2/2] uprobes: Use brw_mutex to fix register/unregister vs dup_mmap() race
	http://marc.info/?l=linux-kernel&m=135032817823720

for details, but in short 2/2 needs this giant lock to block dup_mmap() system-wide, while cgroup (currently) only needs threadgroup lock if CLONE_THREAD (ignoring do_exit) and per-task.

So please forget, I no longer think it makes sense to use the same thing for uprobes and cgroups.

Hello,

Quoted 7 lines · Oleg Nesterov

Yes, yes. But in this case (I mean, for uprobes) "threadgroup" in the name is misleading. It should be called unconditially without any argument.

Please see

	[PATCH 1/2] brw_mutex: big read-write mutex
	http://marc.info/?l=linux-kernel&m=135032816223715

Ooh... that's something completely different.

Quoted 7 lines

	[PATCH 2/2] uprobes: Use brw_mutex to fix register/unregister vs dup_mmap() race
	http://marc.info/?l=linux-kernel&m=135032817823720

for details, but in short 2/2 needs this giant lock to block dup_mmap() system-wide, while cgroup (currently) only needs threadgroup lock if CLONE_THREAD (ignoring do_exit) and per-task.

Quoted 2 lines

So please forget, I no longer think it makes sense to use the same thing for uprobes and cgroups.

It is quite tempting to reduce hot path overhead and penalize cgroup migration ops more tho. Write-locking brw_mutex on migration might not be too bad. Why did you change your mind?

A couple of off-topic questions. With or without this patch I do not understand cgroup_fork,

	/*
	 * We don't need to task_lock() current because current->cgroups
	 * can't be changed concurrently here. The parent obviously hasn't
	 * exited and called cgroup_exit(), and we are synchronized against
	 * cgroup migration through threadgroup_change_begin().
	 */
	child->cgroups = current->cgroups;
	get_css_set(child->cgroups);

How so? threadgroup_change_begin() is only called if CLONE_THREAD. So in theory this copy + atomic_add looks racy...

And it seems that fork() can race with cgroup iterator. post_fork will notice use_task_css_set_links, but until then the child belongs to the parent's css and it is not "visible" to iterator (and right after cgroup_fork() it is not visible to do_each_thread() if use_task_css_set_links is not set).

For example. Suppose that the child migrates to another cgroup after copy_process() makes it visible to the user-space. Then update_if_frozen sets CGROUP_FROZEN (again, cgroup_iter_next do not see this child).

Now, post_fork calls freezer_fork() and hits BUG_ON(CGROUP_FROZEN).

But again, I do not blame this patch.

I am starting to think again about a big-rw-lock around copy_process. Recently I tried to add one around dup_mmap for uprobes, but perhaps cgroups can use it too...

include/linux/freezer.h +6 -0

+/**
+ * freezer_count - tell freezer to stop ignoring %current
+ *
+ * Undo freezer_do_not_count().  It tells freezers that %current should be
+ * considered again and tries to freeze if freezing condition is already in
+ * effect.
  */

include/linux/freezer.h +1 -2

-static inline int freezer_should_skip(struct task_struct *p)
+static inline bool freezer_should_skip(struct task_struct *p)
 {
-	return !!(p->flags & PF_FREEZER_SKIP);

Quoted 4 lines

static inline void freezer_count(void)
{

(quoted diff) +1 -0

 	current->flags &= ~PF_FREEZER_SKIP;
+	/*

include/linux/freezer.h static inline bool cgroup_freezing(struct task_struct *task) · line 110

Quoted 6 lines

include/linux/freezer.h +5 -0

+	 * If freezing is in progress, the following paired with smp_mb()
+	 * in freezer_should_skip() ensures that either we see %true
+	 * freezing() or freezer_should_skip() sees !PF_FREEZER_SKIP.
+	 */
+	smp_mb();
 	try_to_freeze();

I agree, this looks like a bug fix.

Quoted 10 lines

include/linux/freezer.h +7 -2

-static inline int freezer_should_skip(struct task_struct *p)
+static inline bool freezer_should_skip(struct task_struct *p)
 {
-	return !!(p->flags & PF_FREEZER_SKIP);
+	/*
+	 * The following smp_mb() paired with the one in freezer_count()
+	 * ensures that either freezer_count() sees %true freezing() or we
+	 * see cleared %PF_FREEZER_SKIP and return %false.  This makes it
+	 * impossible for a task to slip frozen state testing after
+	 * clearing %PF_FREEZER_SKIP.

include/linux/freezer.h static inline bool cgroup_freezing(struct task_struct *task) · line 134

Quoted 4 lines

include/linux/freezer.h +3 -0

+	 */
+	smp_mb();
+	return p->flags & PF_FREEZER_SKIP;
 }

I am not sure we really need smp_mb() here. Speaking of cgroup_freezer, it seems that a single mb() after "->state = CGROUP_FREEZING" should be enough.

But even if I am right, I agree that it looks better in freezer_should_skip() and this is more robust.

So I think the patch is fine and fixes the bug.

We probably have another similar race. If ptrace_stop()->may_ptrace_stop() returns false, the task does

	__set_current_state(TASK_RUNNING);
	// no mb in between
	try_to_freeze();

And this can race with task_is_stopped_or_traced() check in the same way. (of course this is only theoretical).

do_signal_stop() is probably fine, we can rely on ->siglock.

Quoted 7 lines · Oleg Nesterov

We probably have another similar race. If ptrace_stop()->may_ptrace_stop() returns false, the task does

	__set_current_state(TASK_RUNNING);
	// no mb in between
	try_to_freeze();

Quoted 4 lines

And this can race with task_is_stopped_or_traced() check in the same way. (of course this is only theoretical).

do_signal_stop() is probably fine, we can rely on ->siglock.

Hmm.... Guess we should drop __ from set_current_state.

I wonder
whether we should just add mb to freezing()? What do you think?

Quoted 1 line

Hmm.... Guess we should drop __ from set_current_state.

Yes.

Or we can change ptrace_stop() and do_signal_stop() to use freezer_do_not_count/ freezer_count and remove task_is_stopped_or_traced() from update_if_frozen() and try_to_freeze_tasks(). But this means that do_signal_stop() will call try_to_freeze() twice, unless we add __freezer_count() which only clears PF_FREEZER_SKIP.

Hello, Oleg.

Quoted 4 lines · Oleg Nesterov

Quoted 1 line

Hmm.... Guess we should drop __ from set_current_state.

Yes.

Quoted 5 lines

Or we can change ptrace_stop() and do_signal_stop() to use freezer_do_not_count/ freezer_count and remove task_is_stopped_or_traced() from update_if_frozen() and try_to_freeze_tasks(). But this means that do_signal_stop() will call try_to_freeze() twice, unless we add __freezer_count() which only clears PF_FREEZER_SKIP.

Ooh, I like this idea. If we have a mechanism to mark a task "frozen enough", it makes sense to use it universally.

Change ptrace_stop() and do_signal_stop() to use freezable_schedule() rather than rely on subsequent try_to_freeze().

This allows to remove the task_is_stopped_or_traced() checks from try_to_freeze_tasks() and update_if_frozen(), and this fixes the unlikely race with ptrace_stop(). If the tracee does not schedule() it can miss a freezing condition.

kernel/power/process.c +0 -6

@@ -48,18 +48,7 @@ static int try_to_freeze_tasks(bool user_only)
 			if (p == current || !freeze_task(p))
 				continue;
 
-			/*
-			 * Now that we've done set_freeze_flag, don't
-			 * perturb a task in TASK_STOPPED or TASK_TRACED.
-			 * It is "frozen enough".  If the task does wake
-			 * up, it will immediately call try_to_freeze.
-			 *

kernel/signal.c +1 -8

index 0af8868..1660d7d 100644
@@ -1908,7 +1908,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 		preempt_disable();
 		read_unlock(&tasklist_lock);
 		preempt_enable_no_resched();
-		schedule();
+		freezable_schedule();
 	} else {
 		/*
 		 * By the time we got the lock, our tracer went away.

@@ -1930,13 +1930,6 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	}
 
 	/*
-	 * While in TASK_TRACED, we were considered "frozen enough".
-	 * Now that we woke up, it's crucial if we're supposed to be
-	 * frozen that we freeze now before running anything substantial.
-	 */
-	try_to_freeze();
-
-	/*
 	 * We are back.  Now reacquire the siglock before touching
 	 * last_siginfo, so that we are sure to have synchronized with
 	 * any signal-sending on another CPU that wants to examine it.

Hello, Oleg.

Quoted 3 lines · Oleg Nesterov

Change ptrace_stop() and do_signal_stop() to use freezable_schedule() rather than rely on subsequent try_to_freeze().

Quoted 4 lines

This allows to remove the task_is_stopped_or_traced() checks from try_to_freeze_tasks() and update_if_frozen(), and this fixes the unlikely race with ptrace_stop(). If the tracee does not schedule() it can miss a freezing condition.

I think it would be great if the description is more detailed. This code path always makes my head spin and I think we can definitely use some more guiding in understanding this dang thing. :)

kernel/signal.c static bool do_signal_stop(int signr) · line 2088

Quoted 6 lines

kernel/signal.c +1 -1

@@ -2092,7 +2085,7 @@ static bool do_signal_stop(int signr)
 		}
 
 		/* Now we don't run again until woken by SIGCONT or SIGKILL */
-		schedule();
+		freezable_schedule();

This makes me wonder whether we still need try_to_freeze() in get_signal_to_deliver() right after the relock: label. Freezer no longer treats STOPPED/TRACED special and both sleeping sites in signal deliver path are marked freezable_schedule(). We shouldn't need the explicit try_to_freeze(), right?

Quoted 3 lines

I think it would be great if the description is more detailed. This code path always makes my head spin and I think we can definitely use some more guiding in understanding this dang thing. :)

Do you mean describe the race in more details? OK, will do and resend tomorrow.

Quoted 12 lines

kernel/signal.c static bool do_signal_stop(int signr) · line 2088

Quoted 6 lines

kernel/signal.c +1 -1

@@ -2092,7 +2085,7 @@ static bool do_signal_stop(int signr)
 		}
 
 		/* Now we don't run again until woken by SIGCONT or SIGKILL */
-		schedule();
+		freezable_schedule();

This makes me wonder whether we still need try_to_freeze() in get_signal_to_deliver() right after the relock: label. Freezer no longer treats STOPPED/TRACED special and both sleeping sites in signal deliver path are marked freezable_schedule(). We shouldn't need the explicit try_to_freeze(), right?

OOPS.

I'd say this doesn't really matter but yes we can move it up, get_signal_to_deliver() will be called again.

But! the comment above try_to_freeze() becomes misleading with this patch, so this really needs v2.

Quoted 3 lines

But! the comment above try_to_freeze() becomes misleading with this patch, so this really needs v2.

But, yeah, I think we should move it above relock: and update the comment to explain that that's the usual freezing site.

try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks to ensure that a task doing STOPPED/TRACED -> RUNNING transition can't escape freezing. This mostly works, but ptrace_stop() does not necessarily call schedule(), it can change task->state back to RUNNING and check freezing() without any lock/barrier in between.

We could add the necessary barrier, but this patch changes ptrace_stop() and do_signal_stop() to use freezable_schedule(). This fixes the race, freezer_count() and freezer_should_skip() carefully avoid the race.

And this simplifies the code, try_to_freeze_tasks/update_if_frozen no longer need to use task_is_stopped_or_traced() checks with the non trivial assumptions. We can rely on the mechanism which was specially designed to mark the sleeping task as "frozen enough".

v2: As Tejun pointed out, we can also change get_signal_to_deliver() and move try_to_freeze() up before 'relock' label.

Signed-off-by Oleg Nesterov <[email protected]>

Acked-by Tejun Heo <[email protected]>

Rafael, sorry that this one doesn't have pm cc'd

but can you please
pick up this one too?

Quoted 2 lines · Tejun Heo

Acked-by Tejun Heo <[email protected]>

Thanks!

Quoted 1 line

Rafael, sorry that this one doesn't have pm cc'd

Ah, sorry Rafael. Yes, I have read you email, and I was going to add linux-pm but forgot.

Quoted 2 lines

but can you please
pick up this one too?

Please, and thanks.

Quoted 11 lines · Oleg Nesterov

Quoted 2 lines · Tejun Heo

Acked-by Tejun Heo <[email protected]>

Thanks!

Quoted 1 line

Rafael, sorry that this one doesn't have pm cc'd

Ah, sorry Rafael. Yes, I have read you email, and I was going to add linux-pm but forgot.

Quoted 4 lines

Quoted 2 lines

but can you please
pick up this one too?

Please, and thanks.

OK, but that will go to Linus in the next batch.

Actually, what tree is it supposed to apply to?

The change in kernel/cgroup_freezer.c doesn't look like anything in the current Linus' tree to me.

Hello,

Quoted 2 lines · Rafael J. Wysocki

Actually, what tree is it supposed to apply to?

Quoted 2 lines

The change in kernel/cgroup_freezer.c doesn't look like anything in the current Linus' tree to me.

Ooh, right. This depends on the earlier cgroup_freezer changes. Sorry about the confusion. I'll apply it to the following branch (the same one used for the previous cgroup_freezer updates).

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git cgroup-freezer

Hello,

Quoted 2 lines · Rafael J. Wysocki

Actually, what tree is it supposed to apply to?

Quoted 2 lines

The change in kernel/cgroup_freezer.c doesn't look like anything in the current Linus' tree to me.

Ooh, right. This depends on the earlier cgroup_freezer changes. Sorry about the confusion. I'll apply it to the following branch (the same one used for the previous cgroup_freezer updates).

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git cgroup-freezer

Actually, what tree is it supposed to apply to?

The change in kernel/cgroup_freezer.c doesn't look like anything in the current Linus' tree to me.

Quoted 2 lines · Tejun Heo

Acked-by Tejun Heo <[email protected]>

Thanks!

Quoted 1 line

Rafael, sorry that this one doesn't have pm cc'd

Ah, sorry Rafael. Yes, I have read you email, and I was going to add linux-pm but forgot.

Quoted 2 lines

but can you please
pick up this one too?

Please, and thanks.

Acked-by Tejun Heo <[email protected]>

Rafael, sorry that this one doesn't have pm cc'd

but can you please
pick up this one too?

try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks to ensure that a task doing STOPPED/TRACED -> RUNNING transition can't escape freezing. This mostly works, but ptrace_stop() does not necessarily call schedule(), it can change task->state back to RUNNING and check freezing() without any lock/barrier in between.

We could add the necessary barrier, but this patch changes ptrace_stop() and do_signal_stop() to use freezable_schedule(). This fixes the race, freezer_count() and freezer_should_skip() carefully avoid the race.

And this simplifies the code, try_to_freeze_tasks/update_if_frozen no longer need to use task_is_stopped_or_traced() checks with the non trivial assumptions. We can rely on the mechanism which was specially designed to mark the sleeping task as "frozen enough".

v2: As Tejun pointed out, we can also change get_signal_to_deliver() and move try_to_freeze() up before 'relock' label.

Signed-off-by Oleg Nesterov <[email protected]>

Quoted 16 lines · Oleg Nesterov

try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks to ensure that a task doing STOPPED/TRACED -> RUNNING transition can't escape freezing. This mostly works, but ptrace_stop() does not necessarily call schedule(), it can change task->state back to RUNNING and check freezing() without any lock/barrier in between.

We could add the necessary barrier, but this patch changes ptrace_stop() and do_signal_stop() to use freezable_schedule(). This fixes the race, freezer_count() and freezer_should_skip() carefully avoid the race.

And this simplifies the code, try_to_freeze_tasks/update_if_frozen no longer need to use task_is_stopped_or_traced() checks with the non trivial assumptions. We can rely on the mechanism which was specially designed to mark the sleeping task as "frozen enough".

Quoted 4 lines

v2: As Tejun pointed out, we can also change get_signal_to_deliver() and move try_to_freeze() up before 'relock' label.

Signed-off-by Oleg Nesterov <[email protected]>

[...]

This is not the correct way to submit a change to stable. Please see Documentation/stable_kernel_rules.txt

try_to_freeze_tasks() and cgroup_freezer rely on scheduler locks to ensure that a task doing STOPPED/TRACED -> RUNNING transition can't escape freezing. This mostly works, but ptrace_stop() does not necessarily call schedule(), it can change task->state back to RUNNING and check freezing() without any lock/barrier in between.

We could add the necessary barrier, but this patch changes ptrace_stop() and do_signal_stop() to use freezable_schedule(). This fixes the race, freezer_count() and freezer_should_skip() carefully avoid the race.

And this simplifies the code, try_to_freeze_tasks/update_if_frozen no longer need to use task_is_stopped_or_traced() checks with the non trivial assumptions. We can rely on the mechanism which was specially designed to mark the sleeping task as "frozen enough".

v2: As Tejun pointed out, we can also change get_signal_to_deliver() and move try_to_freeze() up before 'relock' label.

Signed-off-by Oleg Nesterov <[email protected]>

Hello, Oleg.

Quoted 3 lines · Oleg Nesterov

Change ptrace_stop() and do_signal_stop() to use freezable_schedule() rather than rely on subsequent try_to_freeze().

Quoted 4 lines

This allows to remove the task_is_stopped_or_traced() checks from try_to_freeze_tasks() and update_if_frozen(), and this fixes the unlikely race with ptrace_stop(). If the tracee does not schedule() it can miss a freezing condition.

I think it would be great if the description is more detailed. This code path always makes my head spin and I think we can definitely use some more guiding in understanding this dang thing. :)

kernel/signal.c static bool do_signal_stop(int signr) · line 2088

Quoted 6 lines

kernel/signal.c +1 -1

@@ -2092,7 +2085,7 @@ static bool do_signal_stop(int signr)
 		}
 
 		/* Now we don't run again until woken by SIGCONT or SIGKILL */
-		schedule();
+		freezable_schedule();

This makes me wonder whether we still need try_to_freeze() in get_signal_to_deliver() right after the relock: label. Freezer no longer treats STOPPED/TRACED special and both sleeping sites in signal deliver path are marked freezable_schedule(). We shouldn't need the explicit try_to_freeze(), right?

Change ptrace_stop() and do_signal_stop() to use freezable_schedule() rather than rely on subsequent try_to_freeze().

This allows to remove the task_is_stopped_or_traced() checks from try_to_freeze_tasks() and update_if_frozen(), and this fixes the unlikely race with ptrace_stop(). If the tracee does not schedule() it can miss a freezing condition.

kernel/power/process.c +0 -6

@@ -48,18 +48,7 @@ static int try_to_freeze_tasks(bool user_only)
 			if (p == current || !freeze_task(p))
 				continue;
 
-			/*
-			 * Now that we've done set_freeze_flag, don't
-			 * perturb a task in TASK_STOPPED or TASK_TRACED.
-			 * It is "frozen enough".  If the task does wake
-			 * up, it will immediately call try_to_freeze.
-			 *

kernel/signal.c +1 -8

index 0af8868..1660d7d 100644
@@ -1908,7 +1908,7 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 		preempt_disable();
 		read_unlock(&tasklist_lock);
 		preempt_enable_no_resched();
-		schedule();
+		freezable_schedule();
 	} else {
 		/*
 		 * By the time we got the lock, our tracer went away.

@@ -1930,13 +1930,6 @@ static void ptrace_stop(int exit_code, int why, int clear_code, siginfo_t *info)
 	}
 
 	/*
-	 * While in TASK_TRACED, we were considered "frozen enough".
-	 * Now that we woke up, it's crucial if we're supposed to be
-	 * frozen that we freeze now before running anything substantial.
-	 */
-	try_to_freeze();
-
-	/*
 	 * We are back.  Now reacquire the siglock before touching
 	 * last_siginfo, so that we are sure to have synchronized with
 	 * any signal-sending on another CPU that wants to examine it.

Quoted 1 line

Hmm.... Guess we should drop __ from set_current_state.

Yes.

Or we can change ptrace_stop() and do_signal_stop() to use freezer_do_not_count/ freezer_count and remove task_is_stopped_or_traced() from update_if_frozen() and try_to_freeze_tasks(). But this means that do_signal_stop() will call try_to_freeze() twice, unless we add __freezer_count() which only clears PF_FREEZER_SKIP.

cgroup_freezer doesn't transition from FREEZING to FROZEN if the cgroup contains PF_NOFREEZE tasks or tasks sleeping with PF_FREEZER_SKIP set.

Only kernel tasks can be non-freezable (PF_NOFREEZE)

Quoted 4 lines · Tejun Heo

cgroup_freezer doesn't transition from FREEZING to FROZEN if the cgroup contains PF_NOFREEZE tasks or tasks sleeping with PF_FREEZER_SKIP set.

And thus the patch looks like another bugfix to me.

Just one question, and this question is offtopic again,

Quoted 1 line

Only kernel tasks can be non-freezable (PF_NOFREEZE)

Hmm. We seem to "leak" PF_NOFREEZE if a kernel thread execs? Perhaps do_execve_common() should do set_freezable() before return.

Or, at least, simply clear this flag along with PF_KTHREAD in flush_old_exec().

Quoted 6 lines · Oleg Nesterov

Hmm. We seem to "leak" PF_NOFREEZE if a kernel thread execs? Perhaps do_execve_common() should do set_freezable() before return.

Or, at least, simply clear this flag along with PF_KTHREAD in flush_old_exec().

Ooh, ouch, definitely. We should clear that. Can you please make a patch?

Quoted 2 lines

Ooh, ouch, definitely. We should clear that. Can you please make a patch?

Sure... but what do you think is better?

I'd prefer to simply clear PF_NOFREEZE (without set_freezable), but obviously this doesn't look exactly right from cgroup_freezer pov.

Quoted 3 lines

I'd prefer to simply clear PF_NOFREEZE (without set_freezable), but obviously this doesn't look exactly right from cgroup_freezer pov.

I don't think it matters all that much. It's a pretty special case.

Maybe define PF_CLEAR_ON_EXEC_MASK or something and just include PF_NOFREEZE in it?

flush_old_exec() clears PF_KTHREAD but forgets about PF_NOFREEZE.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/exec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

fs/exec.c +2 -1

index 8b9011b..0039055 100644
@@ -1083,7 +1083,8 @@ int flush_old_exec(struct linux_binprm * bprm)
 	bprm->mm = NULL;		/* We're using it now */
 
 	set_fs(USER_DS);
-	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD);
+	current->flags &=
+		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE);
 	flush_thread();
 	current->personality &= ~bprm->per_clear;
 

Signature · 1 line

1.5.5.1

Quoted 3 lines · Oleg Nesterov

flush_old_exec() clears PF_KTHREAD but forgets about PF_NOFREEZE.

Signed-off-by Oleg Nesterov <[email protected]>

Probably needs Cc: stable.

Acked-by Tejun Heo <[email protected]>

How should this be routed? -mm? Andrew, can you please pick this up?

Quoted 1 line

How should this be routed? -mm? Andrew, can you please pick this up?

or perhaps Rafael can take it?

Hello,

Quoted 3 lines · Oleg Nesterov

Quoted 1 line

How should this be routed? -mm? Andrew, can you please pick this up?

or perhaps Rafael can take it?

Yeah, that works too. Rafael?

Hello,

Quoted 3 lines · Oleg Nesterov

Quoted 1 line

How should this be routed? -mm? Andrew, can you please pick this up?

or perhaps Rafael can take it?

Yeah, that works too. Rafael?

flush_old_exec() clears PF_KTHREAD but forgets about PF_NOFREEZE.

Signed-off-by Oleg Nesterov <[email protected]>

---
 fs/exec.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

fs/exec.c +2 -1

index 8b9011b..0039055 100644
@@ -1083,7 +1083,8 @@ int flush_old_exec(struct linux_binprm * bprm)
 	bprm->mm = NULL;		/* We're using it now */
 
 	set_fs(USER_DS);
-	current->flags &= ~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD);
+	current->flags &=
+		~(PF_RANDOMIZE | PF_FORKNOEXEC | PF_KTHREAD | PF_NOFREEZE);
 	flush_thread();
 	current->personality &= ~bprm->per_clear;
 

Signature · 1 line

1.5.5.1

Quoted 6 lines · Oleg Nesterov

Hmm. We seem to "leak" PF_NOFREEZE if a kernel thread execs? Perhaps do_execve_common() should do set_freezable() before return.

Or, at least, simply clear this flag along with PF_KTHREAD in flush_old_exec().

Ooh, ouch, definitely. We should clear that. Can you please make a patch?

cgroup_freezer doesn't transition from FREEZING to FROZEN if the cgroup contains PF_NOFREEZE tasks or tasks sleeping with PF_FREEZER_SKIP set.

Only kernel tasks can be non-freezable (PF_NOFREEZE)

kernel/cgroup_freezer.c static void freezer_fork(struct task_struct *task) · line 205

Quoted 10 lines

kernel/cgroup_freezer.c +2 -4

@@ -190,12 +201,12 @@ static void freezer_fork(struct task_struct *task)
 		goto out;
 
 	spin_lock_irq(&freezer->lock);
-	BUG_ON(freezer->state == CGROUP_FROZEN);
-
-	/* Locking avoids race with FREEZING -> THAWED transitions. */
-	if (freezer->state == CGROUP_FREEZING)
+	/*
+	 * @task might have been just migrated into a FROZEN cgroup.

Confused. If it was migrated, then freezer_attach() should take care do freeze_task?

Hello,

Quoted 13 lines · Oleg Nesterov

kernel/cgroup_freezer.c static void freezer_fork(struct task_struct *task) · line 205

Quoted 10 lines

kernel/cgroup_freezer.c +2 -4

@@ -190,12 +201,12 @@ static void freezer_fork(struct task_struct *task)
 		goto out;
 
 	spin_lock_irq(&freezer->lock);
-	BUG_ON(freezer->state == CGROUP_FROZEN);
-
-	/* Locking avoids race with FREEZING -> THAWED transitions. */
-	if (freezer->state == CGROUP_FREEZING)
+	/*
+	 * @task might have been just migrated into a FROZEN cgroup.

Confused. If it was migrated, then freezer_attach() should take care do freeze_task?

Hmmm... there's a window where a task is migrated but ->attach() hasn't completed yet, so freezer_attach() might not have kicked in yet.

Hello,

Quoted 13 lines · Oleg Nesterov

kernel/cgroup_freezer.c static void freezer_fork(struct task_struct *task) · line 205

Quoted 10 lines

kernel/cgroup_freezer.c +2 -4

@@ -190,12 +201,12 @@ static void freezer_fork(struct task_struct *task)
 		goto out;
 
 	spin_lock_irq(&freezer->lock);
-	BUG_ON(freezer->state == CGROUP_FROZEN);
-
-	/* Locking avoids race with FREEZING -> THAWED transitions. */
-	if (freezer->state == CGROUP_FREEZING)
+	/*
+	 * @task might have been just migrated into a FROZEN cgroup.

Confused. If it was migrated, then freezer_attach() should take care do freeze_task?

Hmmm... there's a window where a task is migrated but ->attach() hasn't completed yet, so freezer_attach() might not have kicked in yet.

kernel/cgroup_freezer.c static void freezer_fork(struct task_struct *task) · line 205

Quoted 10 lines

kernel/cgroup_freezer.c +2 -4

@@ -190,12 +201,12 @@ static void freezer_fork(struct task_struct *task)
 		goto out;
 
 	spin_lock_irq(&freezer->lock);
-	BUG_ON(freezer->state == CGROUP_FROZEN);
-
-	/* Locking avoids race with FREEZING -> THAWED transitions. */
-	if (freezer->state == CGROUP_FREEZING)
+	/*
+	 * @task might have been just migrated into a FROZEN cgroup.

Confused. If it was migrated, then freezer_attach() should take care do freeze_task?

Hello,

This patchset updates cgroup_freezer so that

* Unfreezable kernel tasks don't prevent a cgroup from transitioning into FROZEN from FREEZING. There's nothing userland can do with or about such tasks.

* Tasks can be moved in and out of a frozen cgroup. Tasks are made to conform to the state of the new cgroup during migration. This behavior makes a lot more sense and removes the use of ->can_attach() which makes co-mounting difficult.

* Remove use of cgroup_lock_live_group(). Grabbing cgroup_lock from outside cgroup proper creates a painful locking dependency and is being phased out. With the above behavior change, removing dependency on cgroup_lock is pretty easy. IMHO, it was simply the wrong behavior to implement which forced the wrong implementation.

This patchset contains the following seven patches.

0001-cgroup-cgroup_subsys-fork-should-be-called-after-the.patch 0002-freezer-add-missing-mb-s-to-freezer_count-and-freeze.patch 0003-cgroup_freezer-make-it-official-that-writes-to-freez.patch 0004-cgroup_freezer-don-t-stall-transition-to-FROZEN-for-.patch 0005-cgroup_freezer-allow-moving-tasks-in-and-out-of-a-fr.patch 0006-cgroup_freezer-prepare-update_if_frozen-for-locking-.patch 0007-cgroup_freezer-don-t-use-cgroup_lock_live_group.patch

0001 is a fix for a rather embarrassing bug in cgroup core. It does things in the wrong order leaving a window for racing during fork.

0002 adds a missing mb() around freezing condition updates / checks.

0003-0004 make cgroup_freezer ignore unfreezable kernel tasks and handle PF_FREEZER_SKIP correctly.

0005 allows migrating tasks in and out of a frozen cgroup.

0006-0007 remove the use of cgroup_lock_live_group().

This patchset is on top of v3.7-rc1 and available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup_freezer-locking

Hello,

This patchset updates cgroup_freezer so that

* Unfreezable kernel tasks don't prevent a cgroup from transitioning into FROZEN from FREEZING. There's nothing userland can do with or about such tasks.

* Tasks can be moved in and out of a frozen cgroup. Tasks are made to conform to the state of the new cgroup during migration. This behavior makes a lot more sense and removes the use of ->can_attach() which makes co-mounting difficult.

Quoted 3 lines

* Unfreezable kernel tasks don't prevent a cgroup from transitioning into FROZEN from FREEZING. There's nothing userland can do with or about such tasks.

Seems like a non-problem. Do you have a testcase showing how kernel threads prevent cgroups that should be freezable from being frozen? It used to be that you couldn't move kernel threads out of the root cgroup and the root cgroup was not freezable. So this was never a problem before. Is there some change here that I'm unaware of?

Quoted 4 lines

* Tasks can be moved in and out of a frozen cgroup. Tasks are made to conform to the state of the new cgroup during migration. This behavior makes a lot more sense and removes the use of ->can_attach() which makes co-mounting difficult.

One nice aspect of freezing the set of tasks in the cgroup as well as the tasks themselves was you had a fixed set of tasks to work with (from userspace or otherwise). With this change that will no longer be true. This is a userspace-visible behavior change and userspace code may have relied on this feature.

Will this work for the CRIU folks? With this patch one of the tasks being checkpointed could become thawed simply by some other process writing the pid into a different cgroup's tasks file. In contrast, with the current code they'd have to explicitly thaw its cgroup first.

Hello, Matt.

Quoted 9 lines · Matt Helsley

Quoted 3 lines

* Unfreezable kernel tasks don't prevent a cgroup from transitioning into FROZEN from FREEZING. There's nothing userland can do with or about such tasks.

Seems like a non-problem. Do you have a testcase showing how kernel threads prevent cgroups that should be freezable from being frozen? It used to be that you couldn't move kernel threads out of the root cgroup and the root cgroup was not freezable. So this was never a problem before. Is there some change here that I'm unaware of?

Hmmm? Nothing prevents kthreads from being moved around. We only recently added the restriction to prevent migration of the kthreadd (the one which creates other kthreads). You can reproduce it with khungtask or any other !freezable kthreads.

Quoted 12 lines

Quoted 5 lines

* Tasks can be moved in and out of a frozen cgroup. Tasks are made to conform to the state of the new cgroup during migration. This behavior makes a lot more sense and removes the use of ->can_attach() which makes co-mounting difficult.

One nice aspect of freezing the set of tasks in the cgroup as well as the tasks themselves was you had a fixed set of tasks to work with (from userspace or otherwise). With this change that will no longer be true. This is a userspace-visible behavior change and userspace code may have relied on this feature.

Quoted 4 lines

Will this work for the CRIU folks? With this patch one of the tasks being checkpointed could become thawed simply by some other process writing the pid into a different cgroup's tasks file. In contrast, with the current code they'd have to explicitly thaw its cgroup first.

Yeap, this is a userland visible behavior change. Don't see any other way around it tho and if you can't prevent someone else moving things in/out of your cgroup, what prevents someone else echoing THAWED to freezer.state?

As for CRIU, it isn't using cgroup freezer at the moment because frozen tasks can't be ptraced currently. Something I'm hoping to change but not sure when it can be done.

Quoted 4 lines

Hmmm? Nothing prevents kthreads from being moved around. We only recently added the restriction to prevent migration of the kthreadd (the one which creates other kthreads). You can reproduce it with khungtask or any other !freezable kthreads.

Ok. I don't immmediately see why that'd be a good idea but it's possible..

<off_topic>
"Whoever" did the freeze needs a way to "lock" access to the freezer state, change the freezer state itself, do something (e.g. CRIU) which relies on the state not changing, and then release the lock. Plus the lock has to be released by the kernel if "Whoever" dies without the chance to release it. So I was thinking who holds the lock and its lifetime could be represented in userspace by file descriptor(s).
</off_topic>

Quoted 3 lines

As for CRIU, it isn't using cgroup freezer at the moment because frozen tasks can't be ptraced currently. Something I'm hoping to change but not sure when it can be done.

OK, but that doesn't mean the frozen nature of the task list itself won't be useful in the future.

Hello, Matt.

Quoted 7 lines · Matt Helsley

Quoted 4 lines

Hmmm? Nothing prevents kthreads from being moved around. We only recently added the restriction to prevent migration of the kthreadd (the one which creates other kthreads). You can reproduce it with khungtask or any other !freezable kthreads.

Ok. I don't immmediately see why that'd be a good idea but it's possible..

Beats me too. There were talks about restricting all kthreads from being moved out of the root cgroup. Dunno. Maybe.

Quoted 8 lines

<off_topic>
"Whoever" did the freeze needs a way to "lock" access to the freezer state, change the freezer state itself, do something (e.g. CRIU) which relies on the state not changing, and then release the lock. Plus the lock has to be released by the kernel if "Whoever" dies without the chance to release it. So I was thinking who holds the lock and its lifetime could be represented in userspace by file descriptor(s).
</off_topic>

Long term, I don't think it's feasible to continue to use cgroup kernel interface as the multiplexing layer among different users. cgroup core simply doesn't have enough context or infrastructure to support such usages. It's somewhere between being a pure interface to the provided kernel feature and fully multiplexed interface which userland applications can depend on for arbitration. It tries to have the appearance of the latter but fails.

I think the only sane way would be having a userland arbitrator which owns the kernel interface to itself and makes policy decisions from userland clients and configures cgroup accordingly.

Quoted 7 lines

Quoted 4 lines

As for CRIU, it isn't using cgroup freezer at the moment because frozen tasks can't be ptraced currently. Something I'm hoping to change but not sure when it can be done.

OK, but that doesn't mean the frozen nature of the task list itself won't be useful in the future.

I think that should be solved via userland policies rather than depending on this accidental cgroup_freezer feature.

Quoted 3 lines

I think the only sane way would be having a userland arbitrator which owns the kernel interface to itself and makes policy decisions from userland clients and configures cgroup accordingly.

OK -- yeah, solving the arbitration issue in userspace might be best.

Quoted 2 lines

I think that should be solved via userland policies rather than depending on this accidental cgroup_freezer feature.

It's not accidental -- it *was an intended feature*:

22 # This bash script tests freezer code by starting a long sleep process. 23 # The sleep process is frozen. We then move the sleep process to a THAWED 24 # cgroup. We expect moving the sleep process to fail.

( This atrocious link is the easiest way to see the testcase:
http://ltp.git.sourceforge.net/git/gitweb.cgi?p=ltp/ltp.git;a=blob;f=testcases/kernel/controllers/freezer/freeze_move_thaw.sh;h=b2d5a83506a8425b117be9ff775d9f73d2d58393;hb=0436176dbfe6fdaaf97590d2356eb23d2739b2c2
)

It was intended for something very much like the CRIU case I mentioned :).

I probably have chosen the wrong word. I mean that it's a hierarchy management feature implemented at the wrong layer. If we want to provide cgroup migration locking, it should be implemented at the cgroup core layer as a controller independent feature. It's kinda interesting the incorrect layering here almost directly caused messy locking problem too. I hope we don't need it with (the imaginary) proper userland arbitration but even if we do implementing it in cgroup proper as a separate feature would be a lot less messy.

Yeah, that would be a nice cleanup too. I guess the ultra-careful way to remove this feature would be something like:

Add an internal migration restriction (which may or may not be
exported as a userspace interface in a subsequent patch).

Make the cgroup freezer use it.

Make the cgroup freezer WARN_ONCE() when the subsystem is first
mounted. Indicates that the behavior is going to change.

... time passes ...

Remove the use of the migration "lock" from the cgroup freezer
and the WARN_ONCE().

Which would also make the feature more obvious.

Yeah, that would be a nice cleanup too. I guess the ultra-careful way to remove this feature would be something like:

Add an internal migration restriction (which may or may not be
exported as a userspace interface in a subsequent patch).

Make the cgroup freezer use it.

Make the cgroup freezer WARN_ONCE() when the subsystem is first
mounted. Indicates that the behavior is going to change.

... time passes ...

Remove the use of the migration "lock" from the cgroup freezer
and the WARN_ONCE().

Which would also make the feature more obvious.

I probably have chosen the wrong word. I mean that it's a hierarchy management feature implemented at the wrong layer. If we want to provide cgroup migration locking, it should be implemented at the cgroup core layer as a controller independent feature. It's kinda interesting the incorrect layering here almost directly caused messy locking problem too. I hope we don't need it with (the imaginary) proper userland arbitration but even if we do implementing it in cgroup proper as a separate feature would be a lot less messy.

Quoted 3 lines

I think the only sane way would be having a userland arbitrator which owns the kernel interface to itself and makes policy decisions from userland clients and configures cgroup accordingly.

OK -- yeah, solving the arbitration issue in userspace might be best.

Quoted 2 lines

I think that should be solved via userland policies rather than depending on this accidental cgroup_freezer feature.

It's not accidental -- it *was an intended feature*:

22 # This bash script tests freezer code by starting a long sleep process. 23 # The sleep process is frozen. We then move the sleep process to a THAWED 24 # cgroup. We expect moving the sleep process to fail.

( This atrocious link is the easiest way to see the testcase:
http://ltp.git.sourceforge.net/git/gitweb.cgi?p=ltp/ltp.git;a=blob;f=testcases/kernel/controllers/freezer/freeze_move_thaw.sh;h=b2d5a83506a8425b117be9ff775d9f73d2d58393;hb=0436176dbfe6fdaaf97590d2356eb23d2739b2c2
)

It was intended for something very much like the CRIU case I mentioned :).

Quoted 3 lines

* Unfreezable kernel tasks don't prevent a cgroup from transitioning into FROZEN from FREEZING. There's nothing userland can do with or about such tasks.

Seems like a non-problem. Do you have a testcase showing how kernel threads prevent cgroups that should be freezable from being frozen? It used to be that you couldn't move kernel threads out of the root cgroup and the root cgroup was not freezable. So this was never a problem before. Is there some change here that I'm unaware of?

Quoted 4 lines

* Tasks can be moved in and out of a frozen cgroup. Tasks are made to conform to the state of the new cgroup during migration. This behavior makes a lot more sense and removes the use of ->can_attach() which makes co-mounting difficult.

One nice aspect of freezing the set of tasks in the cgroup as well as the tasks themselves was you had a fixed set of tasks to work with (from userspace or otherwise). With this change that will no longer be true. This is a userspace-visible behavior change and userspace code may have relied on this feature.

Will this work for the CRIU folks? With this patch one of the tasks being checkpointed could become thawed simply by some other process writing the pid into a different cgroup's tasks file. In contrast, with the current code they'd have to explicitly thaw its cgroup first.

Hello,

This patchset updates cgroup_freezer so that

* Unfreezable kernel tasks don't prevent a cgroup from transitioning into FROZEN from FREEZING. There's nothing userland can do with or about such tasks.

* Tasks can be moved in and out of a frozen cgroup. Tasks are made to conform to the state of the new cgroup during migration. This behavior makes a lot more sense and removes the use of ->can_attach() which makes co-mounting difficult.

* Remove use of cgroup_lock_live_group(). Grabbing cgroup_lock from outside cgroup proper creates a painful locking dependency and is being phased out. With the above behavior change, removing dependency on cgroup_lock is pretty easy. IMHO, it was simply the wrong behavior to implement which forced the wrong implementation.

This patchset contains the following seven patches.

0001-cgroup-cgroup_subsys-fork-should-be-called-after-the.patch 0002-freezer-add-missing-mb-s-to-freezer_count-and-freeze.patch 0003-cgroup_freezer-make-it-official-that-writes-to-freez.patch 0004-cgroup_freezer-don-t-stall-transition-to-FROZEN-for-.patch 0005-cgroup_freezer-allow-moving-tasks-in-and-out-of-a-fr.patch 0006-cgroup_freezer-prepare-update_if_frozen-for-locking-.patch 0007-cgroup_freezer-don-t-use-cgroup_lock_live_group.patch

0001 is a fix for a rather embarrassing bug in cgroup core. It does things in the wrong order leaving a window for racing during fork.

0002 adds a missing mb() around freezing condition updates / checks.

0003-0004 make cgroup_freezer ignore unfreezable kernel tasks and handle PF_FREEZER_SKIP correctly.

0005 allows migrating tasks in and out of a frozen cgroup.

0006-0007 remove the use of cgroup_lock_live_group().

This patchset is on top of v3.7-rc1 and available in the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup_freezer-locking

Quoted 1 line

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup_freezer-locking

It seems that no one has any comments. :-)

Are you going to prepare a branch for me to pull from?

Quoted 5 lines · Rafael J. Wysocki

Quoted 1 line

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup_freezer-locking

It seems that no one has any comments. :-)

Are you going to prepare a branch for me to pull from?

I'm waiting for Oleg to poke some holes in the synchronization department but if that doesn't happen you can pull from the above branch. I'll pull it into cgroup/for-3.8 too.

Quoted 3 lines

I'm waiting for Oleg to poke some holes in the synchronization department but if that doesn't happen you can pull from the above branch. I'll pull it into cgroup/for-3.8 too.

Just in case, I see nothing bad ;)

But let me repeat I never understood this code in details, and I already forget everything.

Quoted 1 line

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-cgroup_freezer-locking

It seems that no one has any comments. :-)

Are you going to prepare a branch for me to pull from?