[PATCH] cgroup: fix fail path in cgroup_load_subsys()

[PATCH] cgroup: fix fail path in cgroup_load_subsys()

[Vladimir Davydov]

We should not call cgroup_unload_subsys() if online_css() fails, because
online_css() does not assign css to cgroup on failure, while
offline_css() called from cgroup_unload_subsys() expects it is assigned.
So let's call everything we need to rollback inline without involving
cgroup_unload_subsys().

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 8b729c2..3cd7247 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4861,10 +4861,8 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	 */
 	css = ss->css_alloc(cgroup_css(cgroup_dummy_top, ss));
 	if (IS_ERR(css)) {
-		/* failure case - need to deassign the cgroup_subsys[] slot. */
-		cgroup_subsys[ss->subsys_id] = NULL;
-		mutex_unlock(&cgroup_mutex);
-		return PTR_ERR(css);
+		ret = PTR_ERR(css);
+		goto out_err;
 	}
 
 	list_add(&ss->sibling, &cgroup_dummy_root.subsys_list);
@@ -4873,6 +4871,10 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	/* our new subsystem will be attached to the dummy hierarchy. */
 	init_css(css, ss, cgroup_dummy_top);
 
+	ret = online_css(css);
+	if (ret)
+		goto free_css;
+
 	/*
 	 * Now we need to entangle the css into the existing css_sets. unlike
 	 * in cgroup_init_subsys, there are now multiple css_sets, so each one
@@ -4896,18 +4898,17 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
 	}
 	write_unlock(&css_set_lock);
 
-	ret = online_css(css);
-	if (ret)
-		goto err_unload;
-
 	/* success! */
 	mutex_unlock(&cgroup_mutex);
 	return 0;
 
-err_unload:
+free_css:
+	list_del(&ss->sibling);
+	ss->css_free(css);
+out_err:
+	/* failure case - need to deassign the cgroup_subsys[] slot. */
+	cgroup_subsys[ss->subsys_id] = NULL;
 	mutex_unlock(&cgroup_mutex);
-	/* @ss can't be mounted here as try_module_get() would fail */
-	cgroup_unload_subsys(ss);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cgroup_load_subsys);
-- 
1.7.10.4

Re: [PATCH] cgroup: fix fail path in cgroup_load_subsys()

[Li Zefan]

> @@ -4861,10 +4861,8 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
>  	 */
>  	css = ss->css_alloc(cgroup_css(cgroup_dummy_top, ss));
>  	if (IS_ERR(css)) {
> -		/* failure case - need to deassign the cgroup_subsys[] slot. */
> -		cgroup_subsys[ss->subsys_id] = NULL;
> -		mutex_unlock(&cgroup_mutex);
> -		return PTR_ERR(css);
> +		ret = PTR_ERR(css);
> +		goto out_err;
>  	}
>  
>  	list_add(&ss->sibling, &cgroup_dummy_root.subsys_list);
> @@ -4873,6 +4871,10 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
>  	/* our new subsystem will be attached to the dummy hierarchy. */
>  	init_css(css, ss, cgroup_dummy_top);
>  
> +	ret = online_css(css);
> +	if (ret)
> +		goto free_css;
> +
>  	/*
>  	 * Now we need to entangle the css into the existing css_sets. unlike
>  	 * in cgroup_init_subsys, there are now multiple css_sets, so each one
> @@ -4896,18 +4898,17 @@ int __init_or_module cgroup_load_subsys(struct cgroup_subsys *ss)
>  	}
>  	write_unlock(&css_set_lock);
>  
> -	ret = online_css(css);
> -	if (ret)
> -		goto err_unload;
> -

Moving online_css() upwards should be fine.

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

>  	/* success! */
>  	mutex_unlock(&cgroup_mutex);
>  	return 0;
>  
> -err_unload:
> +free_css:
> +	list_del(&ss->sibling);
> +	ss->css_free(css);
> +out_err:
> +	/* failure case - need to deassign the cgroup_subsys[] slot. */
> +	cgroup_subsys[ss->subsys_id] = NULL;
>  	mutex_unlock(&cgroup_mutex);
> -	/* @ss can't be mounted here as try_module_get() would fail */
> -	cgroup_unload_subsys(ss);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(cgroup_load_subsys);
> 

Re: [PATCH] cgroup: fix fail path in cgroup_load_subsys()

[Tejun Heo]

On Sat, Dec 07, 2013 at 11:58:05AM +0400, Vladimir Davydov wrote:
> We should not call cgroup_unload_subsys() if online_css() fails, because
> online_css() does not assign css to cgroup on failure, while
> offline_css() called from cgroup_unload_subsys() expects it is assigned.
> So let's call everything we need to rollback inline without involving
> cgroup_unload_subsys().
> 
> Signed-off-by: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-3.13-fixes.

Thanks.

-- 
tejun

Re: [PATCH] cgroup: fix fail path in cgroup_load_subsys()

[Tejun Heo]

On Thu, Dec 12, 2013 at 10:35:36AM -0500, Tejun Heo wrote:
> On Sat, Dec 07, 2013 at 11:58:05AM +0400, Vladimir Davydov wrote:
> > We should not call cgroup_unload_subsys() if online_css() fails, because
> > online_css() does not assign css to cgroup on failure, while
> > offline_css() called from cgroup_unload_subsys() expects it is assigned.
> > So let's call everything we need to rollback inline without involving
> > cgroup_unload_subsys().
> > 
> > Signed-off-by: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> > Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Applied to cgroup/for-3.13-fixes.

Scrap that.  I don't think we can fail css_online for the root css in
3.13-fixes and this would cause conflict with 3.14, so let's not
commit it to 3.13-fixes.  Vladimir, can you please respin the patch on
top of cgroup/for-3.14 but make cgroup_unload_subsys() check whether
css is actually there before calling offline so that it matches the
usual offline path better?

Thanks.

-- 
tejun

Re: [PATCH] cgroup: fix fail path in cgroup_load_subsys()

[Vladimir Davydov]

On 12/12/2013 07:55 PM, Tejun Heo wrote:
> On Thu, Dec 12, 2013 at 10:35:36AM -0500, Tejun Heo wrote:
>> On Sat, Dec 07, 2013 at 11:58:05AM +0400, Vladimir Davydov wrote:
>>> We should not call cgroup_unload_subsys() if online_css() fails, because
>>> online_css() does not assign css to cgroup on failure, while
>>> offline_css() called from cgroup_unload_subsys() expects it is assigned.
>>> So let's call everything we need to rollback inline without involving
>>> cgroup_unload_subsys().
>>>
>>> Signed-off-by: Vladimir Davydov <vdavydov-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>>> Cc: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> Applied to cgroup/for-3.13-fixes.
> Scrap that.  I don't think we can fail css_online for the root css in
> 3.13-fixes and this would cause conflict with 3.14, so let's not
> commit it to 3.13-fixes.  Vladimir, can you please respin the patch on
> top of cgroup/for-3.14

No problem.

> but make cgroup_unload_subsys() check whether
> css is actually there before calling offline so that it matches the
> usual offline path better?

You want me to add something like this to the patch, don't you?

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 402f7aa..bc03b97 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4680,6 +4680,7 @@ EXPORT_SYMBOL_GPL(cgroup_load_subsys);
 void cgroup_unload_subsys(struct cgroup_subsys *ss)
 {
     struct cgrp_cset_link *link;
+    struct cgroup_subsys_state *css;
 
     BUG_ON(ss->module == NULL);
 
@@ -4693,7 +4694,9 @@ void cgroup_unload_subsys(struct cgroup_subsys *ss)
     mutex_lock(&cgroup_mutex);
     mutex_lock(&cgroup_root_mutex);
 
-    offline_css(cgroup_css(cgroup_dummy_top, ss));
+    css = cgroup_css(cgroup_dummy_top, ss);
+    if (css)
+        offline_css(css);
 
     /* deassign the subsys_id */
     cgroup_subsys[ss->subsys_id] = NULL;


If so, I don't mind, but why? cgroup_unload_subsys() should never be
called without properly initialized css.

Thanks.

Re: [PATCH] cgroup: fix fail path in cgroup_load_subsys()

[Tejun Heo]

Hello,

On Thu, Dec 12, 2013 at 10:37:32PM +0400, Vladimir Davydov wrote:
> If so, I don't mind, but why? cgroup_unload_subsys() should never be
> called without properly initialized css.

Hmm.... I think it's generally preferable to reuse normal exit path
for error handling as much as possible.  Plus, the normal cgroup init
failure path behaves that way.  That said, it's more of a preference
and if you have technical argument, I'd be happy to give in. :)

Thanks.

-- 
tejun

Re: [PATCH] cgroup: fix fail path in cgroup_load_subsys()

[Vladimir Davydov]

On 12/12/2013 10:45 PM, Tejun Heo wrote:
> Hello,
>
> On Thu, Dec 12, 2013 at 10:37:32PM +0400, Vladimir Davydov wrote:
>> If so, I don't mind, but why? cgroup_unload_subsys() should never be
>> called without properly initialized css.
> Hmm.... I think it's generally preferable to reuse normal exit path
> for error handling as much as possible.  Plus, the normal cgroup init
> failure path behaves that way.  That said, it's more of a preference
> and if you have technical argument, I'd be happy to give in. :)

Got it, will do.

Thanks.