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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 6 Dec 2013 11:13:12 -0500
From:	Tejun Heo <tj@...nel.org>
To:	Vladimir Davydov <vdavydov@...allels.com>
Cc:	Li Zefan <lizefan@...wei.com>, linux-kernel@...r.kernel.org,
	cgroups@...r.kernel.org, devel@...nvz.org
Subject: Re: [PATCH cgroup/for-3.13-fixes] cgroup: fix oops in cgroup init
 failure path

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/