[PATCH cgroup/for-3.14-fixes] cgroup: update cgroup_enable_task_cg_lists() to grab siglock

[PATCH cgroup/for-3.14-fixes] cgroup: update cgroup_enable_task_cg_lists() to grab siglock

[Tejun Heo]

Currently, there's nothing preventing cgroup_enable_task_cg_lists()
from missing set PF_EXITING and race against cgroup_exit().  Depending
on the timing, cgroup_exit() may finish with the task still linked on
css_set leading to list corruption.  Fix it by grabbing siglock in
cgroup_enable_task_cg_lists() so that PF_EXITING is guaranteed to be
visible.

This whole on-demand cg_list optimization is extremely fragile and has
ample possibility to lead to bugs which can cause things like
once-a-year oops during boot.  I'm wondering whether the better
approach would be just adding "cgroup_disable=all" handling which
disables the whole cgroup rather than tempting fate with this
on-demand craziness.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 kernel/cgroup.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 68d8710..105f273 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2905,9 +2905,14 @@ static void cgroup_enable_task_cg_lists(void)
 		 * We should check if the process is exiting, otherwise
 		 * it will race with cgroup_exit() in that the list
 		 * entry won't be deleted though the process has exited.
+		 * Do it while holding siglock so that we don't end up
+		 * racing against cgroup_exit().
 		 */
+		spin_lock_irq(&p->sighand->siglock);
 		if (!(p->flags & PF_EXITING) && list_empty(&p->cg_list))
 			list_add(&p->cg_list, &task_css_set(p)->tasks);
+		spin_unlock_irq(&p->sighand->siglock);
+
 		task_unlock(p);
 	} while_each_thread(g, p);
 	read_unlock(&tasklist_lock);

Re: [PATCH cgroup/for-3.14-fixes] cgroup: update cgroup_enable_task_cg_lists() to grab siglock

[Li Zefan]

On 2014/2/14 2:29, Tejun Heo wrote:
> Currently, there's nothing preventing cgroup_enable_task_cg_lists()
> from missing set PF_EXITING and race against cgroup_exit().  Depending
> on the timing, cgroup_exit() may finish with the task still linked on
> css_set leading to list corruption.  Fix it by grabbing siglock in
> cgroup_enable_task_cg_lists() so that PF_EXITING is guaranteed to be
> visible.
> 
> This whole on-demand cg_list optimization is extremely fragile and has
> ample possibility to lead to bugs which can cause things like
> once-a-year oops during boot.

I added the PF_EXITING check years ago:

commit 0e04388f0189fa1f6812a8e1cb6172136eada87e
Author: Li Zefan <lizf-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
Date:   Thu Apr 17 11:37:15 2008 +0800

    cgroup: fix a race condition in manipulating tsk->cg_list

Now the only race I see is caused by checking tsk->cg_list without locking
in cgroup_exit():

cgroup_enable_task_cg_lists()
  ...
  if (!p->flags & PF_EXITING) && list_empty(p->cg_list))
								exit_signal(tsk)  <-- set PF_EXTING;
								...
								if (!list_empty(&tsk->cg_list)) {
								        down_write(&css_set_rwsem);
								        if (!list_empty(&tsk->cg_list))
								                list_del_init(&tsk->cg_list);
								        up_write(&css_set_rwsem);
								}
  	list_add(p->cg_list, ...);

Your patch can fix this race, but after diving into the code I don't think
the race exists, because exit_mm() locks&unlocks task_lock, and exit_mm()
is called after exit_signal() and before cgroup_exit(), and task_lock is
also taken by cgroup_enable_task_cg_lists().

I totally agree the code is fragile and we should take your patch. I just
want to make it clear if the bug exists in real life or not, and then we
can write better changelog and decide to queue the patch for 3.14 or 3.15
and decide to mark it for stable or not.

> I'm wondering whether the better
> approach would be just adding "cgroup_disable=all" handling which
> disables the whole cgroup rather than tempting fate with this
> on-demand craziness.
> 

:)

> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  kernel/cgroup.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 68d8710..105f273 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -2905,9 +2905,14 @@ static void cgroup_enable_task_cg_lists(void)
>  		 * We should check if the process is exiting, otherwise
>  		 * it will race with cgroup_exit() in that the list
>  		 * entry won't be deleted though the process has exited.
> +		 * Do it while holding siglock so that we don't end up
> +		 * racing against cgroup_exit().
>  		 */
> +		spin_lock_irq(&p->sighand->siglock);
>  		if (!(p->flags & PF_EXITING) && list_empty(&p->cg_list))
>  			list_add(&p->cg_list, &task_css_set(p)->tasks);
> +		spin_unlock_irq(&p->sighand->siglock);
> +
>  		task_unlock(p);
>  	} while_each_thread(g, p);
>  	read_unlock(&tasklist_lock);
> 

Re: [PATCH cgroup/for-3.14-fixes] cgroup: update cgroup_enable_task_cg_lists() to grab siglock

[Tejun Heo]

Hello,

On Fri, Feb 14, 2014 at 11:49:42AM +0800, Li Zefan wrote:
> Now the only race I see is caused by checking tsk->cg_list without locking
> in cgroup_exit():

Yeah, that's the one I was trying to fix.

> Your patch can fix this race, but after diving into the code I don't think
> the race exists, because exit_mm() locks&unlocks task_lock, and exit_mm()
> is called after exit_signal() and before cgroup_exit(), and task_lock is
> also taken by cgroup_enable_task_cg_lists().

Ah, okay, so there's a task_lock somewhere in the exit path.
Extremely fragile, but not broken.

> I totally agree the code is fragile and we should take your patch. I just
> want to make it clear if the bug exists in real life or not, and then we
> can write better changelog and decide to queue the patch for 3.14 or 3.15
> and decide to mark it for stable or not.

Yeap, no reason to mark it for -stable if it doesn't actually happen.
I'll update the description and respin it for for-3.15.

Thanks!

-- 
tejun

[PATCH v2 cgroup/for-3.15] cgroup: make cgroup_enable_task_cg_lists() to grab siglock

[Tejun Heo]

Currently, there's nothing explicitly preventing
cgroup_enable_task_cg_lists() from missing set PF_EXITING and race
against cgroup_exit(), and, depending on the timing, cgroup_exit()
seemingly may finish with the task still linked on css_set leading to
list corruption because cgroup_enable_task_cg_lists() can end up
linking it after list_empty(&tsk->cg_list) test in cgroup_exit().

This can't really happen because exit_mm() grabs and release
task_lock() between setting of PF_EXITING and cgroup_exit(), and
cgroup_enable_task_cg_lists() synchronizes against task_lock too;
however, this is fragile and more of a happy accident.  Let's make the
synchronization explicit by making cgroup_enable_task_cg_lists() grab
siglock around PF_EXITING testing.

This whole on-demand cg_list optimization is extremely fragile and has
ample possibility to lead to bugs which can cause things like
once-a-year oops during boot.  I'm wondering whether the better
approach would be just adding "cgroup_disable=all" handling which
disables the whole cgroup rather than tempting fate with this dynamic
optimization craziness.

v2: Li pointed out that the race condition can't actually happen due
    to task_lock locking in exit_mm().  Updated the patch description
    accordingly and dropped -stable cc.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c |    4 ++++
 1 file changed, 4 insertions(+)

--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1331,9 +1331,13 @@ static void cgroup_enable_task_cg_lists(
 		 * We should check if the process is exiting, otherwise
 		 * it will race with cgroup_exit() in that the list
 		 * entry won't be deleted though the process has exited.
+		 * Do it while holding siglock so that we don't end up
+		 * racing against cgroup_exit().
 		 */
+		spin_lock_irq(&p->sighand->siglock);
 		if (!(p->flags & PF_EXITING))
 			list_add(&p->cg_list, &task_css_set(p)->tasks);
+		spin_unlock_irq(&p->sighand->siglock);
 
 		task_unlock(p);
 	} while_each_thread(g, p);

Re: [PATCH v2 cgroup/for-3.15] cgroup: make cgroup_enable_task_cg_lists() to grab siglock

[Li Zefan]

On 2014/2/15 4:47, Tejun Heo wrote:
> Currently, there's nothing explicitly preventing
> cgroup_enable_task_cg_lists() from missing set PF_EXITING and race
> against cgroup_exit(), and, depending on the timing, cgroup_exit()
> seemingly may finish with the task still linked on css_set leading to
> list corruption because cgroup_enable_task_cg_lists() can end up
> linking it after list_empty(&tsk->cg_list) test in cgroup_exit().
> 
> This can't really happen because exit_mm() grabs and release
> task_lock() between setting of PF_EXITING and cgroup_exit(), and
> cgroup_enable_task_cg_lists() synchronizes against task_lock too;
> however, this is fragile and more of a happy accident.  Let's make the
> synchronization explicit by making cgroup_enable_task_cg_lists() grab
> siglock around PF_EXITING testing.
> 
> This whole on-demand cg_list optimization is extremely fragile and has
> ample possibility to lead to bugs which can cause things like
> once-a-year oops during boot.  I'm wondering whether the better
> approach would be just adding "cgroup_disable=all" handling which
> disables the whole cgroup rather than tempting fate with this dynamic
> optimization craziness.
> 
> v2: Li pointed out that the race condition can't actually happen due
>     to task_lock locking in exit_mm().  Updated the patch description
>     accordingly and dropped -stable cc.
> 

I realise exit_mm() is a no-op for threads... There're quite a few places
task_lock is used between exit_signal() and cgroup_exit(), but they're
all conditional, so I think your original changelog stands!

Re: [PATCH cgroup/for-3.14-fixes] cgroup: update cgroup_enable_task_cg_lists() to grab siglock

[Li Zefan]

On 2014/2/14 2:29, Tejun Heo wrote:
> Currently, there's nothing preventing cgroup_enable_task_cg_lists()
> from missing set PF_EXITING and race against cgroup_exit().  Depending
> on the timing, cgroup_exit() may finish with the task still linked on
> css_set leading to list corruption.  Fix it by grabbing siglock in
> cgroup_enable_task_cg_lists() so that PF_EXITING is guaranteed to be
> visible.
> 
> This whole on-demand cg_list optimization is extremely fragile and has
> ample possibility to lead to bugs which can cause things like
> once-a-year oops during boot.  I'm wondering whether the better
> approach would be just adding "cgroup_disable=all" handling which
> disables the whole cgroup rather than tempting fate with this
> on-demand craziness.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Re: [PATCH cgroup/for-3.14-fixes] cgroup: update cgroup_enable_task_cg_lists() to grab siglock

[Tejun Heo]

On Thu, Feb 13, 2014 at 01:29:31PM -0500, Tejun Heo wrote:
> Currently, there's nothing preventing cgroup_enable_task_cg_lists()
> from missing set PF_EXITING and race against cgroup_exit().  Depending
> on the timing, cgroup_exit() may finish with the task still linked on
> css_set leading to list corruption.  Fix it by grabbing siglock in
> cgroup_enable_task_cg_lists() so that PF_EXITING is guaranteed to be
> visible.
> 
> This whole on-demand cg_list optimization is extremely fragile and has
> ample possibility to lead to bugs which can cause things like
> once-a-year oops during boot.  I'm wondering whether the better
> approach would be just adding "cgroup_disable=all" handling which
> disables the whole cgroup rather than tempting fate with this
> on-demand craziness.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Applied to cgroup/for-3.14-fixes.

Thanks.

-- 
tejun