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

[Tejun Heo <tj@kernel.org>]

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