Re: [PATCH] cgroup: Fix potential race between cgroup_exit and migrate path

[Tejun Heo]

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 {