cgroup: fix race between fork and cgroup freezing

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.

kernel/cgroup_freezer.c +8 -0

+static void freezer_post_fork(struct cgroup_subsys *ss,
+			      struct task_struct *task)
+{
+	struct freezer *freezer;
+
+	cgroup_lock();
+
+	freezer = task_freezer(task);

Hello, Li, Frederic.

Quoted 8 lines · Li Zefan

kernel/cgroup_freezer.c +8 -0

+static void freezer_post_fork(struct cgroup_subsys *ss,
+			      struct task_struct *task)
+{
+	struct freezer *freezer;
+
+	cgroup_lock();
+
+	freezer = task_freezer(task);

kernel/cgroup_freezer.c static void freezer_fork(struct cgroup_subsys *ss, struct task_struct *task) · line 236

Quoted 10 lines

kernel/cgroup_freezer.c +10 -0

+	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.

Hello, Li.

Quoted 10 lines · Li Zefan

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.

- 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.

Hello,

Quoted 3 lines · Li Zefan

- 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?

Quoted 3 lines

- 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.

Quoted 2 lines

- 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?

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

As reported by Frederic:

Quoted 9 lines

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:

Quoted 5 lines

- 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 <[email protected]>

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