[PATCH] cgroup: fix bug on cgroup_create() fail path

[PATCH] cgroup: fix bug on cgroup_create() fail path

[Vladimir Davydov]

If cgroup_create() fails to online_css() we will get a bug:

BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
IP: [<ffffffff810eeaa8>] cgroup_destroy_locked+0x118/0x2f0
PGD a780a067 PUD aadbe067 PMD 0
Oops: 0000 [#1] SMP
Modules linked in:
CPU: 6 PID: 7360 Comm: mkdir Not tainted 3.13.0-rc2+ #69
Hardware name:
task: ffff8800b9dbec00 ti: ffff8800a781a000 task.ti: ffff8800a781a000
RIP: 0010:[<ffffffff810eeaa8>]  [<ffffffff810eeaa8>] cgroup_destroy_locked+0x118/0x2f0
RSP: 0018:ffff8800a781bd98  EFLAGS: 00010282
RAX: ffff880586903878 RBX: ffff880586903800 RCX: ffff880586903820
RDX: ffff880586903860 RSI: ffff8800a781bdb0 RDI: ffff880586903820
RBP: ffff8800a781bde8 R08: ffff88060e0b8048 R09: ffffffff811d7bc1
R10: 000000000000008c R11: 0000000000000001 R12: ffff8800a72286c0
R13: 0000000000000000 R14: ffffffff81cf7a40 R15: 0000000000000001
FS:  00007f60ecda57a0(0000) GS:ffff8806272c0000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000008 CR3: 00000000a7a03000 CR4: 00000000000007e0
Stack:
 ffff880586903860 ffff880586903910 ffff8800a72286c0 ffff880586903820
 ffffffff81cf7a40 ffff880586903800 ffff88060e0b8018 ffffffff81cf7a40
 ffff8800b9dbec00 ffff8800b9dbf098 ffff8800a781bec8 ffffffff810ef5bf
Call Trace:
 [<ffffffff810ef5bf>] cgroup_mkdir+0x55f/0x5f0
 [<ffffffff811c90ae>] vfs_mkdir+0xee/0x140
 [<ffffffff811cb07e>] SyS_mkdirat+0x6e/0xf0
 [<ffffffff811c6a19>] SyS_mkdir+0x19/0x20
 [<ffffffff8169e569>] system_call_fastpath+0x16/0x1b

The point is that cgroup_destroy_locked() that is called on the fail
path assumes all css's have already been assigned to the cgroup, which
is not true, and calls kill_css() to destroy them.

The patch makes css_online() proceed to assigning css to a cgroup even
if subsys-specific css_online method fails - it only skips setting
CSS_ONLINE flag then. Respectively, offline_css() should skip only
subsys-specific css_offline method if CSS_ONLINE is not set. Besides, it
makes cgroup_create() call online_css() for all css's before going to
cgroup_destroy_locked(). It is not that optimal, but it's only a fail
path.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c |   28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 8b729c2..1846923 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4296,11 +4296,10 @@ static int online_css(struct cgroup_subsys_state *css)
 
 	if (ss->css_online)
 		ret = ss->css_online(css);
-	if (!ret) {
+	if (!ret)
 		css->flags |= CSS_ONLINE;
-		css->cgroup->nr_css++;
-		rcu_assign_pointer(css->cgroup->subsys[ss->subsys_id], css);
-	}
+	css->cgroup->nr_css++;
+	rcu_assign_pointer(css->cgroup->subsys[ss->subsys_id], css);
 	return ret;
 }
 
@@ -4311,10 +4310,7 @@ static void offline_css(struct cgroup_subsys_state *css)
 
 	lockdep_assert_held(&cgroup_mutex);
 
-	if (!(css->flags & CSS_ONLINE))
-		return;
-
-	if (ss->css_offline)
+	if ((css->flags & CSS_ONLINE) && ss->css_offline)
 		ss->css_offline(css);
 
 	css->flags &= ~CSS_ONLINE;
@@ -4437,13 +4433,20 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 	/* hold a ref to the parent's dentry */
 	dget(parent->dentry);
 
+	err = 0;
+
 	/* creation succeeded, notify subsystems */
 	for_each_root_subsys(root, ss) {
 		struct cgroup_subsys_state *css = css_ar[ss->subsys_id];
+		int ret;
 
-		err = online_css(css);
-		if (err)
-			goto err_destroy;
+		/* Continue assigning css's to this cgroup on failure so that
+		 * all css's will be killed by cgroup_destroy_locked(). */
+		ret = online_css(css);
+		if (ret) {
+			err = ret;
+			continue;
+		}
 
 		if (ss->broken_hierarchy && !ss->warned_broken_hierarchy &&
 		    parent->parent) {
@@ -4455,6 +4458,9 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 		}
 	}
 
+	if (err)
+		goto err_destroy;
+
 	idr_replace(&root->cgroup_idr, cgrp, cgrp->id);
 
 	err = cgroup_addrm_files(cgrp, cgroup_base_files, true);
-- 
1.7.10.4

[PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init failure path

[Tejun Heo]

Hello, Vladimir.

Thanks a lot for the report and fix; however, I really wanna make sure
that only online css's become visible, so I wrote up a different fix.
Can you please test this one?

Thanks a lot!

------ 8< ------
ae7f164a0940 ("cgroup: move cgroup->subsys[] assignment to
online_css()") moved cgrp->subsys[] assignment from allocation to
online_css().  This was done to decouple the lifetime of css
(cgroup_subsys_states) from that of the associated cgroup - ie. css
should only become visible only after it's fully online.
Unfortunately, the commit failed to update init failure path
accordingly.

If one of the css fails onlining, cgroup destruction path is invoked
on the partially constructed cgroup; however, the function is not
ready to handle empty slots in cgrp->subsys[] array and triggers the
following oops.

  BUG: unable to handle kernel NULL pointer dereference at 0000000000000008
  IP: [<ffffffff810eeaa8>] cgroup_destroy_locked+0x118/0x2f0
  PGD a780a067 PUD aadbe067 PMD 0
  Oops: 0000 [#1] SMP
  Modules linked in:
  CPU: 6 PID: 7360 Comm: mkdir Not tainted 3.13.0-rc2+ #69
  Hardware name:
  task: ffff8800b9dbec00 ti: ffff8800a781a000 task.ti: ffff8800a781a000
  RIP: 0010:[<ffffffff810eeaa8>]  [<ffffffff810eeaa8>] cgroup_destroy_locked+0x118/0x2f0
  RSP: 0018:ffff8800a781bd98  EFLAGS: 00010282
  RAX: ffff880586903878 RBX: ffff880586903800 RCX: ffff880586903820
  RDX: ffff880586903860 RSI: ffff8800a781bdb0 RDI: ffff880586903820
  RBP: ffff8800a781bde8 R08: ffff88060e0b8048 R09: ffffffff811d7bc1
  R10: 000000000000008c R11: 0000000000000001 R12: ffff8800a72286c0
  R13: 0000000000000000 R14: ffffffff81cf7a40 R15: 0000000000000001
  FS:  00007f60ecda57a0(0000) GS:ffff8806272c0000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000000000008 CR3: 00000000a7a03000 CR4: 00000000000007e0
  Stack:
   ffff880586903860 ffff880586903910 ffff8800a72286c0 ffff880586903820
   ffffffff81cf7a40 ffff880586903800 ffff88060e0b8018 ffffffff81cf7a40
   ffff8800b9dbec00 ffff8800b9dbf098 ffff8800a781bec8 ffffffff810ef5bf
  Call Trace:
   [<ffffffff810ef5bf>] cgroup_mkdir+0x55f/0x5f0
   [<ffffffff811c90ae>] vfs_mkdir+0xee/0x140
   [<ffffffff811cb07e>] SyS_mkdirat+0x6e/0xf0
   [<ffffffff811c6a19>] SyS_mkdir+0x19/0x20
   [<ffffffff8169e569>] system_call_fastpath+0x16/0x1b

Fix it by updating cgroup_destroy_locked() skip over empty
cgrp->subsys[] entries and making cgroup_create() free css's which
didn't get onlined in the failure path.  This moves css_free
invocation in the failure path outside cgroup_mutex but the function
is supposed to not care about it and already being called outside
cgroup_mutex in the normal destruction path.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Vladimir Davydov <vdavydov@parallels.com>
Cc: stable@vger.kernel.org # v3.12+
---
 kernel/cgroup.c |   40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4399,13 +4399,13 @@ static long cgroup_create(struct cgroup
 		css = ss->css_alloc(cgroup_css(parent, ss));
 		if (IS_ERR(css)) {
 			err = PTR_ERR(css);
-			goto err_free_all;
+			goto err_deactivate;
 		}
 		css_ar[ss->subsys_id] = css;
 
 		err = percpu_ref_init(&css->refcnt, css_release);
 		if (err)
-			goto err_free_all;
+			goto err_deactivate;
 
 		init_css(css, ss, cgrp);
 	}
@@ -4417,7 +4417,7 @@ static long cgroup_create(struct cgroup
 	 */
 	err = cgroup_create_file(dentry, S_IFDIR | mode, sb);
 	if (err < 0)
-		goto err_free_all;
+		goto err_deactivate;
 	lockdep_assert_held(&dentry->d_inode->i_mutex);
 
 	cgrp->serial_nr = cgroup_serial_nr_next++;
@@ -4445,6 +4445,9 @@ static long cgroup_create(struct cgroup
 		if (err)
 			goto err_destroy;
 
+		/* @css successfully attached, now owned by @cgrp */
+		css_ar[ss->subsys_id] = NULL;
+
 		if (ss->broken_hierarchy && !ss->warned_broken_hierarchy &&
 		    parent->parent) {
 			pr_warning("cgroup: %s (%d) created nested cgroup for controller \"%s\" which has incomplete hierarchy support. Nested cgroups may change behavior in the future.\n",
@@ -4470,15 +4473,7 @@ static long cgroup_create(struct cgroup
 
 	return 0;
 
-err_free_all:
-	for_each_root_subsys(root, ss) {
-		struct cgroup_subsys_state *css = css_ar[ss->subsys_id];
-
-		if (css) {
-			percpu_ref_cancel_init(&css->refcnt);
-			ss->css_free(css);
-		}
-	}
+err_deactivate:
 	mutex_unlock(&cgroup_mutex);
 	/* Release the reference count that we took on the superblock */
 	deactivate_super(sb);
@@ -4488,12 +4483,21 @@ err_free_name:
 	kfree(rcu_dereference_raw(cgrp->name));
 err_free_cgrp:
 	kfree(cgrp);
-	return err;
+	goto out_free_css_ar;
 
 err_destroy:
 	cgroup_destroy_locked(cgrp);
 	mutex_unlock(&cgroup_mutex);
 	mutex_unlock(&dentry->d_inode->i_mutex);
+out_free_css_ar:
+	for_each_root_subsys(root, ss) {
+		struct cgroup_subsys_state *css = css_ar[ss->subsys_id];
+
+		if (css) {
+			percpu_ref_cancel_init(&css->refcnt);
+			ss->css_free(css);
+		}
+	}
 	return err;
 }
 
@@ -4650,10 +4654,14 @@ static int cgroup_destroy_locked(struct
 	/*
 	 * Initiate massacre of all css's.  cgroup_destroy_css_killed()
 	 * will be invoked to perform the rest of destruction once the
-	 * percpu refs of all css's are confirmed to be killed.
+	 * percpu refs of all css's are confirmed to be killed.  Not all
+	 * css's may be present if @cgrp failed init half-way.
 	 */
-	for_each_root_subsys(cgrp->root, ss)
-		kill_css(cgroup_css(cgrp, ss));
+	for_each_root_subsys(cgrp->root, ss) {
+		struct cgroup_subsys_state *css = cgroup_css(cgrp, ss);
+		if (css)
+			kill_css(cgroup_css(cgrp, ss));
+	}
 
 	/*
 	 * Mark @cgrp dead.  This prevents further task migration and child

Re: [PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init failure path

[Vladimir Davydov]

On 12/06/2013 01:18 AM, Tejun Heo wrote:
> Hello, Vladimir.
>
> Thanks a lot for the report and fix; however, I really wanna make sure
> that only online css's become visible, so I wrote up a different fix.
> Can you please test this one?

Hi, Tejun

This patch fixes this bug, but I have a couple of questions regarding it.

First, cgroup_load_subsys() also calls css_online(), and if it fails, it
calls cgroup_unload_subsys() to rollback. The latter function executes
the following command:

    offline_css(cgroup_css(cgroup_dummy_top, ss));

But since we failed to online_css(), cgroup_css() will return NULL
resulting in another oops.

Second, it's not clear to me why we need the CSS_ONLINE flag at all if
we never assign css's that we fail to online to a cgroup. AFAIU we will
never see such css's, because in all places we call offline_css(),
namely cgroup_destroy_locked() (via kill_css()) and
cgroup_unload_subsys(), we use cgroup_css() which will return NULL for them.

Third, please see commends inline.

Thanks.

> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -4399,13 +4399,13 @@ static long cgroup_create(struct cgroup
>  		css = ss->css_alloc(cgroup_css(parent, ss));
>  		if (IS_ERR(css)) {
>  			err = PTR_ERR(css);
> -			goto err_free_all;
> +			goto err_deactivate;
>  		}
>  		css_ar[ss->subsys_id] = css;
>  
>  		err = percpu_ref_init(&css->refcnt, css_release);
>  		if (err)
> -			goto err_free_all;
> +			goto err_deactivate;
>  
>  		init_css(css, ss, cgrp);
>  	}
> @@ -4417,7 +4417,7 @@ static long cgroup_create(struct cgroup
>  	 */
>  	err = cgroup_create_file(dentry, S_IFDIR | mode, sb);
>  	if (err < 0)
> -		goto err_free_all;
> +		goto err_deactivate;
>  	lockdep_assert_held(&dentry->d_inode->i_mutex);
>  
>  	cgrp->serial_nr = cgroup_serial_nr_next++;
> @@ -4445,6 +4445,9 @@ static long cgroup_create(struct cgroup
>  		if (err)
>  			goto err_destroy;

Before we get here, we call

    /* each css holds a ref to the cgroup's dentry and the parent css */
    for_each_root_subsys(root, ss) {
        struct cgroup_subsys_state *css = css_ar[ss->subsys_id];

        dget(dentry);
        css_get(css->parent);
    }

If we fail to online a css, we will only call

    ss->css_free(css);

on it skipping css_put() on parent.

css_put() is called on parent in css_release() on normal destroy path.

>  
> +		/* @css successfully attached, now owned by @cgrp */
> +		css_ar[ss->subsys_id] = NULL;
> +
>  		if (ss->broken_hierarchy && !ss->warned_broken_hierarchy &&
>  		    parent->parent) {
>  			pr_warning("cgroup: %s (%d) created nested cgroup for controller \"%s\" which has incomplete hierarchy support. Nested cgroups may change behavior in the future.\n",
> @@ -4470,15 +4473,7 @@ static long cgroup_create(struct cgroup
>  
>  	return 0;
>  
> -err_free_all:
> -	for_each_root_subsys(root, ss) {
> -		struct cgroup_subsys_state *css = css_ar[ss->subsys_id];
> -
> -		if (css) {
> -			percpu_ref_cancel_init(&css->refcnt);
> -			ss->css_free(css);
> -		}
> -	}
> +err_deactivate:
>  	mutex_unlock(&cgroup_mutex);
>  	/* Release the reference count that we took on the superblock */
>  	deactivate_super(sb);
> @@ -4488,12 +4483,21 @@ err_free_name:
>  	kfree(rcu_dereference_raw(cgrp->name));
>  err_free_cgrp:
>  	kfree(cgrp);
> -	return err;
> +	goto out_free_css_ar;
>  
>  err_destroy:
>  	cgroup_destroy_locked(cgrp);
>  	mutex_unlock(&cgroup_mutex);
>  	mutex_unlock(&dentry->d_inode->i_mutex);
> +out_free_css_ar:
> +	for_each_root_subsys(root, ss) {
> +		struct cgroup_subsys_state *css = css_ar[ss->subsys_id];
> +
> +		if (css) {
> +			percpu_ref_cancel_init(&css->refcnt);
> +			ss->css_free(css);
> +		}
> +	}
>  	return err;
>  }
>  
> @@ -4650,10 +4654,14 @@ static int cgroup_destroy_locked(struct
>  	/*
>  	 * Initiate massacre of all css's.  cgroup_destroy_css_killed()
>  	 * will be invoked to perform the rest of destruction once the
> -	 * percpu refs of all css's are confirmed to be killed.
> +	 * percpu refs of all css's are confirmed to be killed.  Not all
> +	 * css's may be present if @cgrp failed init half-way.
>  	 */
> -	for_each_root_subsys(cgrp->root, ss)
> -		kill_css(cgroup_css(cgrp, ss));
> +	for_each_root_subsys(cgrp->root, ss) {
> +		struct cgroup_subsys_state *css = cgroup_css(cgrp, ss);
> +		if (css)
> +			kill_css(cgroup_css(cgrp, ss));
> +	}
>  
>  	/*
>  	 * Mark @cgrp dead.  This prevents further task migration and child

Re: [PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init failure path

[Tejun Heo]

Hello, Vladimir.

On Fri, Dec 06, 2013 at 11:02:07AM +0400, Vladimir Davydov wrote:
> This patch fixes this bug, but I have a couple of questions regarding it.
> 
> First, cgroup_load_subsys() also calls css_online(), and if it fails, it
> calls cgroup_unload_subsys() to rollback. The latter function executes
> the following command:
> 
>     offline_css(cgroup_css(cgroup_dummy_top, ss));
> 
> But since we failed to online_css(), cgroup_css() will return NULL
> resulting in another oops.

I don't think the root css onlining fails for any existing controllers
but yeah that looks wrong.  Can you please send a patch?

> Second, it's not clear to me why we need the CSS_ONLINE flag at all if
> we never assign css's that we fail to online to a cgroup. AFAIU we will
> never see such css's, because in all places we call offline_css(),
> namely cgroup_destroy_locked() (via kill_css()) and
> cgroup_unload_subsys(), we use cgroup_css() which will return NULL for them.

The whole thing is in flux and will look very different in near
future.  I actually had patches queued which deal with the issue you
spotted but they are being blocked on other changes ATM.  So, yeah,
there are some spurious stuff now.

> Before we get here, we call
> 
>     /* each css holds a ref to the cgroup's dentry and the parent css */
>     for_each_root_subsys(root, ss) {
>         struct cgroup_subsys_state *css = css_ar[ss->subsys_id];
> 
>         dget(dentry);
>         css_get(css->parent);
>     }
> 
> If we fail to online a css, we will only call
> 
>     ss->css_free(css);
> 
> on it skipping css_put() on parent.
> 
> css_put() is called on parent in css_release() on normal destroy path.

You're right.  I'll revise the patch.

Thanks.

-- 
tejun

Re: [PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init failure path

[Tejun Heo]

On Fri, Dec 06, 2013 at 11:13:12AM -0500, Tejun Heo wrote:
> > Second, it's not clear to me why we need the CSS_ONLINE flag at all if
> > we never assign css's that we fail to online to a cgroup. AFAIU we will
> > never see such css's, because in all places we call offline_css(),
> > namely cgroup_destroy_locked() (via kill_css()) and
> > cgroup_unload_subsys(), we use cgroup_css() which will return NULL for them.
> 
> The whole thing is in flux and will look very different in near
> future.  I actually had patches queued which deal with the issue you
> spotted but they are being blocked on other changes ATM.  So, yeah,
> there are some spurious stuff now.

LOL, I found the patch.  It was posted and acked I just forgot to
apply the whole series.  I'm a moron.

  http://permalink.gmane.org/gmane.linux.kernel.containers/26804

This should do it, right?  I'll update the patch description and
repost the series.

Thanks!

-- 
tejun

Re: [PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init failure path

[Vladimir Davydov]

On 12/06/2013 08:25 PM, Tejun Heo wrote:
> On Fri, Dec 06, 2013 at 11:13:12AM -0500, Tejun Heo wrote:
>>> Second, it's not clear to me why we need the CSS_ONLINE flag at all if
>>> we never assign css's that we fail to online to a cgroup. AFAIU we will
>>> never see such css's, because in all places we call offline_css(),
>>> namely cgroup_destroy_locked() (via kill_css()) and
>>> cgroup_unload_subsys(), we use cgroup_css() which will return NULL for them.
>> The whole thing is in flux and will look very different in near
>> future.  I actually had patches queued which deal with the issue you
>> spotted but they are being blocked on other changes ATM.  So, yeah,
>> there are some spurious stuff now.
> LOL, I found the patch.  It was posted and acked I just forgot to
> apply the whole series.  I'm a moron.
>
>   http://permalink.gmane.org/gmane.linux.kernel.containers/26804
>
> This should do it, right?  I'll update the patch description and
> repost the series.

If combined with the patch you've sent recently, this should do the trick.

Thanks.

Re: [PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init failure path

[Vladimir Davydov]

On 12/06/2013 08:13 PM, Tejun Heo wrote:
> Hello, Vladimir.
>
> On Fri, Dec 06, 2013 at 11:02:07AM +0400, Vladimir Davydov wrote:
>> This patch fixes this bug, but I have a couple of questions regarding it.
>>
>> First, cgroup_load_subsys() also calls css_online(), and if it fails, it
>> calls cgroup_unload_subsys() to rollback. The latter function executes
>> the following command:
>>
>>     offline_css(cgroup_css(cgroup_dummy_top, ss));
>>
>> But since we failed to online_css(), cgroup_css() will return NULL
>> resulting in another oops.
> I don't think the root css onlining fails for any existing controllers
> but yeah that looks wrong.  Can you please send a patch?

Sure, I will send it soon.

Thanks.

Re: [PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init failure path

[Li Zefan]

On 2013/12/6 5:18, Tejun Heo wrote:
> Hello, Vladimir.
> 
> Thanks a lot for the report and fix; however, I really wanna make sure
> that only online css's become visible, so I wrote up a different fix.
> Can you please test this one?
> 

Oh, I spotted this bug when reviewing a bug fix months ago.

http://lkml.org/lkml/2013/9/2/135

but that bug fix didn't go into mainline and it still exists even after
the below fix.

This is your updated fix that fixes both bugs.

http://lkml.org/lkml/2013/9/3/502