Hello, Neeraj.
On Wed, Aug 30, 2017 at 06:24:09PM +0530, Neeraj Upadhyay wrote:
> There is a potential race between cgroup_exit() and the
> migration path. This race happens because cgroup_exit path
> reads the css_set and does cg_list empty check outside of
> css_set lock. This can potentially race with the migrate path
> trying to move the tasks to a different css_set. For instance,
> below is the interleaved sequence of events, where race is
> observed:
>
> cpuset_hotplug_workfn()
> cgroup_transfer_tasks()
> cgroup_migrate()
> cgroup_migrate_execute()
> css_set_move_task()
> list_del_init(&task->cg_list);
> <TASK EXIT>
> cgroup_exit()
> cset = task_css_set(tsk);
> if (!list_empty(&tsk->cg_list))
> <TASK NOT DISSOCIATED FROM ITS CSS_SET>
> list_add_tail(&task->cg_list, use_mg_tasks
>
> In above sequence, as cgroup_exit() read the cg_list for
> the task as empty, it didn't disassociate it from its
> current css_set, and was moved to new css_set instance
> css_set_move_task() called from cpuset_hotplug_workfn()
> path. This eventually can result in use after free scenarios,
> while accessing the same task_struct again, like in following
> sequence:
>
> kernfs_seq_start()
> cgroup_seqfile_start()
> cgroup_pidlist_start()
> css_task_iter_next()
> __put_task_struct()
> <NULL pointer dereference>
>
> Fix this problem, by moving the css_set and cg_list fetch in
> cgroup_exit() inside css_set lock.
Hmm... I haven't really thought through but could the problem be that
css_set_move_task() is temporarily making ->cg_list empty? The
use_task_css_set_links optimization can't handle that.
Would something like the following fix the issue? Thanks.
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index df2e0f1..cd85ca0 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -683,7 +683,7 @@ static void css_set_move_task(struct task_struct *task,
if (it->task_pos == &task->cg_list)
css_task_iter_advance(it);
- list_del_init(&task->cg_list);
+ list_del(&task->cg_list);
if (!css_set_populated(from_cset))
css_set_update_populated(from_cset, false);
} else {
