[RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Li Zefan]

A similar bug exists in cpuset, and those are long-standing bugs.

As reported by Frederic:

> When a user freezes a cgroup, the freezer sets the subsystem state
> to CGROUP_FREEZING and then iterates over the tasks in the cgroup links.
> 
> But there is a possible race here, although unlikely, if a task
> forks and the parent is preempted between write_unlock(tasklist_lock)
> and cgroup_post_fork(). If we freeze the cgroup while the parent
> is sleeping and the parent wakes up thereafter, its child will
> be missing from the set of tasks to freeze because:
> 
> - The child was not yet linked to its css_set->tasks, as is done
> from cgroup_post_fork(). cgroup_iter_start() has thus missed it.
> 
> - The cgroup freezer's fork callback can handle that child but
> cgroup_fork_callbacks() has been called already.

I try to fix it by using seqcount. We read the counter before calling
cgroup_fork_callbacks(), and we check the counter after cgroup_post_fork().
If the seq numbers don't match, we know the forking task's cgroup
has been/is being frozen, so we freeze the child task.

cpuset can be fixed accordingly.

Reported-by: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 include/linux/cgroup.h    |    5 +++--
 include/linux/init_task.h |    8 ++++++++
 include/linux/sched.h     |    1 +
 kernel/cgroup.c           |   18 ++++++++++++++++--
 kernel/cgroup_freezer.c   |   22 ++++++++++++++++++++++
 kernel/fork.c             |    5 +++--
 6 files changed, 53 insertions(+), 6 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index e9b6021..38c82b2 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -32,8 +32,8 @@ 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_fork_callbacks(struct task_struct *p, int *seq);
+extern void cgroup_post_fork(struct task_struct *p, int seq);
 extern void cgroup_exit(struct task_struct *p, int run_callbacks);
 extern int cgroupstats_build(struct cgroupstats *stats,
 				struct dentry *dentry);
@@ -495,6 +495,7 @@ struct cgroup_subsys {
 	void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 		       struct cgroup_taskset *tset);
 	void (*fork)(struct cgroup_subsys *ss, struct task_struct *task);
+	void (*post_fork)(struct cgroup_subsys *ss, struct task_struct *task);
 	void (*exit)(struct cgroup_subsys *ss, struct cgroup *cgrp,
 			struct cgroup *old_cgrp, struct task_struct *task);
 	int (*populate)(struct cgroup_subsys *ss,
diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 9c66b1a..91665ce 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -125,6 +125,13 @@ extern struct cred init_cred;
 # define INIT_PERF_EVENTS(tsk)
 #endif
 
+#ifdef CONFIG_CGROUPS
+# define INIT_CGROUPS(tsk)						\
+	.fork_seq = SEQCNT_ZERO
+#else
+# define INIT_CGROUPS(tsk)
+#endif
+
 #define INIT_TASK_COMM "swapper"
 
 /*
@@ -192,6 +199,7 @@ extern struct cred init_cred;
 	INIT_FTRACE_GRAPH						\
 	INIT_TRACE_RECURSION						\
 	INIT_TASK_RCU_PREEMPT(tsk)					\
+	INIT_CGROUPS(tsk)						\
 }
 
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 7d379a6..d487777 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1507,6 +1507,7 @@ struct task_struct {
 	struct css_set __rcu *cgroups;
 	/* cg_list protected by css_set_lock and tsk->alloc_lock */
 	struct list_head cg_list;
+	seqcount_t fork_seq;
 #endif
 #ifdef CONFIG_FUTEX
 	struct robust_list_head __user *robust_list;
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a5d3b53..77435ec 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -60,6 +60,7 @@
 #include <linux/eventfd.h>
 #include <linux/poll.h>
 #include <linux/flex_array.h> /* used in cgroup_attach_proc */
+#include <linux/seqlock.h>
 
 #include <linux/atomic.h>
 
@@ -4558,6 +4559,7 @@ void cgroup_fork(struct task_struct *child)
 	child->cgroups = current->cgroups;
 	get_css_set(child->cgroups);
 	INIT_LIST_HEAD(&child->cg_list);
+	seqcount_init(&child->fork_seq);
 }
 
 /**
@@ -4568,8 +4570,10 @@ void cgroup_fork(struct task_struct *child)
  * tasklist. No need to take any locks since no-one can
  * be operating on this task.
  */
-void cgroup_fork_callbacks(struct task_struct *child)
+void cgroup_fork_callbacks(struct task_struct *child, int *seq)
 {
+	*seq = read_seqcount_begin(&current->fork_seq);
+
 	if (need_forkexit_callback) {
 		int i;
 		/*
@@ -4594,7 +4598,7 @@ void cgroup_fork_callbacks(struct task_struct *child)
  * 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)
+void cgroup_post_fork(struct task_struct *child, int seq)
 {
 	if (use_task_css_set_links) {
 		write_lock(&css_set_lock);
@@ -4612,6 +4616,16 @@ void cgroup_post_fork(struct task_struct *child)
 			list_add(&child->cg_list, &child->cgroups->tasks);
 		}
 		write_unlock(&css_set_lock);
+
+		if (read_seqcount_retry(&current->fork_seq, seq)) {
+			int i;
+
+			for (i = 0; i < CGROUP_BUILTIN_SUBSYS_COUNT; i++) {
+				struct cgroup_subsys *ss = subsys[i];
+				if (ss->post_fork)
+					ss->post_fork(ss, child);
+			}
+		}
 	}
 }
 /**
diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
index fc0646b..88f210e 100644
--- a/kernel/cgroup_freezer.c
+++ b/kernel/cgroup_freezer.c
@@ -216,6 +216,25 @@ static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task)
 	spin_unlock_irq(&freezer->lock);
 }
 
+static void freezer_post_fork(struct cgroup_subsys *ss,
+			      struct task_struct *task)
+{
+	struct freezer *freezer;
+
+	cgroup_lock();
+
+	freezer = task_freezer(task);
+	if (!freezer->css.cgroup->parent)
+		goto out;
+
+	spin_lock_irq(&freezer->lock);
+	if (freezer->state != CGROUP_THAWED)
+		freeze_task(task);
+	spin_unlock_irq(&freezer->lock);
+out:
+	cgroup_unlock();
+}
+
 /*
  * caller must hold freezer->lock
  */
@@ -286,6 +305,8 @@ static int try_to_freeze_cgroup(struct cgroup *cgroup, struct freezer *freezer)
 			continue;
 		if (!freezing(task) && !freezer_should_skip(task))
 			num_cant_freeze_now++;
+
+		write_seqcount_barrier(&task->fork_seq);
 	}
 	cgroup_iter_end(cgroup, &it);
 
@@ -381,4 +402,5 @@ struct cgroup_subsys freezer_subsys = {
 	.subsys_id	= freezer_subsys_id,
 	.can_attach	= freezer_can_attach,
 	.fork		= freezer_fork,
+	.post_fork	= freezer_post_fork,
 };
diff --git a/kernel/fork.c b/kernel/fork.c
index e2cd3e2..73abc25 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1077,6 +1077,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	int retval;
 	struct task_struct *p;
 	int cgroup_callbacks_done = 0;
+	int seq;
 
 	if ((clone_flags & (CLONE_NEWNS|CLONE_FS)) == (CLONE_NEWNS|CLONE_FS))
 		return ERR_PTR(-EINVAL);
@@ -1331,7 +1332,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	/* 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_fork_callbacks(p, &seq);
 	cgroup_callbacks_done = 1;
 
 	/* Need tasklist lock for parent etc handling! */
@@ -1395,7 +1396,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	spin_unlock(&current->sighand->siglock);
 	write_unlock_irq(&tasklist_lock);
 	proc_fork_connector(p);
-	cgroup_post_fork(p);
+	cgroup_post_fork(p, seq);
 	if (clone_flags & CLONE_THREAD)
 		threadgroup_change_end(current);
 	perf_event_fork(p);

Re: [RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Tejun Heo]

Hello, Li, Frederic.

On Thu, Mar 08, 2012 at 04:45:13PM +0800, Li Zefan wrote:
> +static void freezer_post_fork(struct cgroup_subsys *ss,
> +			      struct task_struct *task)
> +{
> +	struct freezer *freezer;
> +
> +	cgroup_lock();
> +
> +	freezer = task_freezer(task);
> +	if (!freezer->css.cgroup->parent)
> +		goto out;
> +
> +	spin_lock_irq(&freezer->lock);
> +	if (freezer->state != CGROUP_THAWED)
> +		freeze_task(task);
> +	spin_unlock_irq(&freezer->lock);
> +out:
> +	cgroup_unlock();
> +}

Urgh... this is requiring policy implementations to synchronize with
problem caused by cgroup core optimization and it's so very subtle.
IMHO, this definitely should be contained in cgroup core and in a very
confined form even inside cgroup core.

Any other ideas?

Thanks.

-- 
tejun

Re: [RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Li Zefan]

02:26, Tejun Heo wrote:
> Hello, Li, Frederic.
> 
> On Thu, Mar 08, 2012 at 04:45:13PM +0800, Li Zefan wrote:
>> +static void freezer_post_fork(struct cgroup_subsys *ss,
>> +			      struct task_struct *task)
>> +{
>> +	struct freezer *freezer;
>> +
>> +	cgroup_lock();
>> +
>> +	freezer = task_freezer(task);
>> +	if (!freezer->css.cgroup->parent)
>> +		goto out;
>> +
>> +	spin_lock_irq(&freezer->lock);
>> +	if (freezer->state != CGROUP_THAWED)
>> +		freeze_task(task);
>> +	spin_unlock_irq(&freezer->lock);
>> +out:
>> +	cgroup_unlock();
>> +}
> 
> Urgh... this is requiring policy implementations to synchronize with
> problem caused by cgroup core optimization and it's so very subtle.
> IMHO, this definitely should be contained in cgroup core and in a very
> confined form even inside cgroup core.
> 
> Any other ideas?
> 

The problem is, forks can happen at any time, so there's no way to prevent
forks from happening while iterating tasks in a cgroup, so controllers
have to deal with it. In fact freezer is somewhat aware of this issue,
that's why it provides the ->fork callback, but there's race.

This patch is not too bad (needs a bit modification). cgroup core will detect
(via seqcount) if something's happened to a cgroup and the tasks in it, and
then cgroup will notify controllers to check if newly-forked tasks should
be adjusted accordingly, so they will have consistent status with other tasks
in the same cgroup.

Re: [RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Tejun Heo]

Hello, Li.

On Fri, Mar 09, 2012 at 02:26:05PM +0800, Li Zefan wrote:
> The problem is, forks can happen at any time, so there's no way to prevent
> forks from happening while iterating tasks in a cgroup, so controllers
> have to deal with it. In fact freezer is somewhat aware of this issue,
> that's why it provides the ->fork callback, but there's race.
> 
> This patch is not too bad (needs a bit modification). cgroup core will detect
> (via seqcount) if something's happened to a cgroup and the tasks in it, and
> then cgroup will notify controllers to check if newly-forked tasks should
> be adjusted accordingly, so they will have consistent status with other tasks
> in the same cgroup.

But why can't we just do what every sane subsystem would do - link
first and then invoke notification callback?  I mean, we're now
essentially trying to do the following.

1. Take some action.
2. Trigger notification.
3. Link the result of the action to list.

So, of course, if someone tries to traverse the "results", there's a
race window between #2 and #3.  Your fix seems to change the traverser
to,

1. Traverse the list.
2. If something happened inbetween, take another look.

But, the right thing to do would be changing the fork path to

1. Take some action.
2. Link the result of the action to list.
3. Trigger notification.

If I'm missing something, please don't feel shy to hammer it into me
as I'm feeling very lost at the moment.

Thanks.

-- 
tejun

Re: [RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Li Zefan]

Tejun Heo wrote:
> Hello, Li.
> 
> On Fri, Mar 09, 2012 at 02:26:05PM +0800, Li Zefan wrote:
>> The problem is, forks can happen at any time, so there's no way to prevent
>> forks from happening while iterating tasks in a cgroup, so controllers
>> have to deal with it. In fact freezer is somewhat aware of this issue,
>> that's why it provides the ->fork callback, but there's race.
>>
>> This patch is not too bad (needs a bit modification). cgroup core will detect
>> (via seqcount) if something's happened to a cgroup and the tasks in it, and
>> then cgroup will notify controllers to check if newly-forked tasks should
>> be adjusted accordingly, so they will have consistent status with other tasks
>> in the same cgroup.
> 
> But why can't we just do what every sane subsystem would do - link
> first and then invoke notification callback?  I mean, we're now
> essentially trying to do the following.
> 
> 1. Take some action.
> 2. Trigger notification.
> 3. Link the result of the action to list.
> 
> So, of course, if someone tries to traverse the "results", there's a
> race window between #2 and #3.  Your fix seems to change the traverser
> to,
> 
> 1. Traverse the list.
> 2. If something happened inbetween, take another look.
> 
> But, the right thing to do would be changing the fork path to
> 
> 1. Take some action.
> 2. Link the result of the action to list.
> 3. Trigger notification.
> 

The reasons are

- We still need some kind of locking to syncronize fork and the traverser.
fork side is protected by tasklist_lock, while the traverser takes
css_set_lock.

- After linking the new task to css set list, the task is visible and thus
can be moved to another cgroup, which makes things more complicated and
the subsystem callbacks may have to acquire cgroup_mutex.

- The task_counter subsystem wants to get notified before the new task
is linked, so it's able to abort the fork.

Re: [RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Tejun Heo]

Hello,

On Mon, Mar 12, 2012 at 05:02:04PM +0800, Li Zefan wrote:
> - We still need some kind of locking to syncronize fork and the traverser.
> fork side is protected by tasklist_lock, while the traverser takes
> css_set_lock.

Can't we do both after tasklist_lock is released under css_set_lock?

> - After linking the new task to css set list, the task is visible and thus
> can be moved to another cgroup, which makes things more complicated and
> the subsystem callbacks may have to acquire cgroup_mutex.

Hmmm... freezer currently doesn't allow migrating in and out of frozen
cgroup and even when it does callbacks in the migration path should
synchronize against freezer->lock.  I *think* that should be enough
and can't see why this will be simpler or more complex depending on
when fork callback is called.

> - The task_counter subsystem wants to get notified before the new task
> is linked, so it's able to abort the fork.

This one maybe but for this cgroup_fork_callbacks() is already too
late, isn't it?  We better have pre-fork callbacks instead, no?

Thanks.

-- 
tejun

Re: [RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Frederic Weisbecker]

On Mon, Mar 12, 2012 at 09:10:40AM -0700, Tejun Heo wrote:
> Hello,
> 
> On Mon, Mar 12, 2012 at 05:02:04PM +0800, Li Zefan wrote:
> > - We still need some kind of locking to syncronize fork and the traverser.
> > fork side is protected by tasklist_lock, while the traverser takes
> > css_set_lock.
> 
> Can't we do both after tasklist_lock is released under css_set_lock?
> 
> > - After linking the new task to css set list, the task is visible and thus
> > can be moved to another cgroup, which makes things more complicated and
> > the subsystem callbacks may have to acquire cgroup_mutex.
> 
> Hmmm... freezer currently doesn't allow migrating in and out of frozen
> cgroup and even when it does callbacks in the migration path should
> synchronize against freezer->lock.  I *think* that should be enough
> and can't see why this will be simpler or more complex depending on
> when fork callback is called.
> 
> > - The task_counter subsystem wants to get notified before the new task
> > is linked, so it's able to abort the fork.
> 
> This one maybe but for this cgroup_fork_callbacks() is already too
> late, isn't it?  We better have pre-fork callbacks instead, no?

Nope, cgroup_fork_callbacks() is called soon enough to be able
to cancel a fork. The task counter subsystem cancels from that point.

Re: [RFC][PATCH] cgroup: fix race between fork and cgroup freezing

[Frederic Weisbecker]

On Thu, Mar 08, 2012 at 04:45:13PM +0800, Li Zefan wrote:
> A similar bug exists in cpuset, and those are long-standing bugs.
> 
> As reported by Frederic:
> 
> > When a user freezes a cgroup, the freezer sets the subsystem state
> > to CGROUP_FREEZING and then iterates over the tasks in the cgroup links.
> > 
> > But there is a possible race here, although unlikely, if a task
> > forks and the parent is preempted between write_unlock(tasklist_lock)
> > and cgroup_post_fork(). If we freeze the cgroup while the parent
> > is sleeping and the parent wakes up thereafter, its child will
> > be missing from the set of tasks to freeze because:
> > 
> > - The child was not yet linked to its css_set->tasks, as is done
> > from cgroup_post_fork(). cgroup_iter_start() has thus missed it.
> > 
> > - The cgroup freezer's fork callback can handle that child but
> > cgroup_fork_callbacks() has been called already.
> 
> I try to fix it by using seqcount. We read the counter before calling
> cgroup_fork_callbacks(), and we check the counter after cgroup_post_fork().
> If the seq numbers don't match, we know the forking task's cgroup
> has been/is being frozen, so we freeze the child task.
> 
> cpuset can be fixed accordingly.
> 
> Reported-by: Frederic Weisbecker <fweisbec-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

I feel we are a bit stuck here. All these complications come from the
fact we are conditionally setting this css_set link.

I wish we could set it unconditionally on cgroup_fork() time.
This unfortunately implies at least locking the css_set and to do
a list_add() unconditionally. And at times where cgroup is often
critisized for the overhead it involves, I guess this is not welcome.

This ->post_fork() based solution is not pretty, unfortunately I can't
come with a better idea.