Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1723293 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] cgroup: Fix potential race between cgroup_exit and migrate path Neeraj Upadhyay <neeraju@codeaurora.org> - 2017-08-30 15:00 +0200
    Re: [PATCH] cgroup: Fix potential race between cgroup_exit and  migrate path Tejun Heo <tj@kernel.org> - 2017-08-31 03:00 +0200
      Re: [PATCH] cgroup: Fix potential race between cgroup_exit and  migrate path Tejun Heo <tj@kernel.org> - 2017-08-31 03:10 +0200
        Re: [PATCH] cgroup: Fix potential race between cgroup_exit and  migrate path Tejun Heo <tj@kernel.org> - 2017-08-31 03:20 +0200
          Re: [PATCH] cgroup: Fix potential race between cgroup_exit and  migrate path Neeraj Upadhyay <neeraju@codeaurora.org> - 2017-09-04 15:30 +0200

#1723293 — [PATCH] cgroup: Fix potential race between cgroup_exit and migrate path

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.

Signed-off-by: Neeraj Upadhyay <neeraju@codeaurora.org>
---
Hi,

We observed this issue for cgroup code corresponding to stable
v4.4.85 snapshot 3144d81 ("cgroup, kthread: close race window where
new kthreads can be migrated to non-root cgroups"). Can you please
tell us, if there are any patches in latest code, which
fixes these issue?

 kernel/cgroup/cgroup.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index df2e0f1..f746b70 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -692,10 +692,10 @@ static void css_set_move_task(struct task_struct *task,
 
 	if (to_cset) {
 		/*
-		 * We are synchronized through cgroup_threadgroup_rwsem
-		 * against PF_EXITING setting such that we can't race
-		 * against cgroup_exit() changing the css_set to
-		 * init_css_set and dropping the old one.
+		 * We are synchronized through css_set_lock against
+		 * PF_EXITING setting such that we can't race against
+		 * cgroup_exit() disassociating the task from the
+		 * css_set.
 		 */
 		WARN_ON_ONCE(task->flags & PF_EXITING);
 
@@ -4934,20 +4934,24 @@ void cgroup_exit(struct task_struct *tsk)
 	int i;
 
 	/*
-	 * Unlink from @tsk from its css_set.  As migration path can't race
-	 * with us, we can check css_set and cg_list without synchronization.
+	 * Avoid potential race with the migrate path.
+	 */
+	spin_lock_irq(&css_set_lock);
+
+	/*
+	 * Unlink from @tsk from its css_set.
 	 */
 	cset = task_css_set(tsk);
 
 	if (!list_empty(&tsk->cg_list)) {
-		spin_lock_irq(&css_set_lock);
 		css_set_move_task(tsk, cset, NULL, false);
 		cset->nr_tasks--;
-		spin_unlock_irq(&css_set_lock);
 	} else {
 		get_css_set(cset);
 	}
 
+	spin_unlock_irq(&css_set_lock);
+
 	/* see cgroup_post_fork() for details */
 	do_each_subsys_mask(ss, i, have_exit_callback) {
 		ss->exit(tsk);
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

[toc] | [next] | [standalone]

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

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 {

[toc] | [prev] | [next] | [standalone]

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

On Wed, Aug 30, 2017 at 05:55:45PM -0700, Tejun Heo wrote:
> 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 {

Oops, more like the following.

diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index df2e0f1..6f34025 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 {
@@ -702,6 +702,8 @@ static void css_set_move_task(struct task_struct *task,
 		rcu_assign_pointer(task->cgroups, to_cset);
 		list_add_tail(&task->cg_list, use_mg_tasks ? &to_cset->mg_tasks :
 							     &to_cset->tasks);
+	} else {
+		INIT_LIST_HEAD(&task->cg_list);
 	}
 }
 

[toc] | [prev] | [next] | [standalone]

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

On Wed, Aug 30, 2017 at 06:03:19PM -0700, Tejun Heo wrote:
> Oops, more like the following.
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index df2e0f1..6f34025 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 {
> @@ -702,6 +702,8 @@ static void css_set_move_task(struct task_struct *task,
>  		rcu_assign_pointer(task->cgroups, to_cset);
>  		list_add_tail(&task->cg_list, use_mg_tasks ? &to_cset->mg_tasks :
>  							     &to_cset->tasks);
> +	} else {
> +		INIT_LIST_HEAD(&task->cg_list);
>  	}
>  }

On the third thought, I don't think this can happen either because now
migration is strongly synchronized against exits.  Please take a look
at the changes around cgroup_threadgroup_rwsem.

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

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

On 08/31/2017 06:42 AM, Tejun Heo wrote:
> On Wed, Aug 30, 2017 at 06:03:19PM -0700, Tejun Heo wrote:
>> Oops, more like the following.
>>
>> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
>> index df2e0f1..6f34025 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 {
>> @@ -702,6 +702,8 @@ static void css_set_move_task(struct task_struct *task,
>>   		rcu_assign_pointer(task->cgroups, to_cset);
>>   		list_add_tail(&task->cg_list, use_mg_tasks ? &to_cset->mg_tasks :
>>   							     &to_cset->tasks);
>> +	} else {
>> +		INIT_LIST_HEAD(&task->cg_list);
>>   	}
>>   }
> On the third thought, I don't think this can happen either because now
> migration is strongly synchronized against exits.  Please take a look
> at the changes around cgroup_threadgroup_rwsem.
>
> Thanks.
>
Thank you for the suggestion; found below fix, which is not present in 
v4.4.86
stable code base. Please let me know in case I am missing something:

eedd0f4 cgroupns: Close race between cgroup_post_fork and copy_cgroup_ns

Thanks.

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web