[PATCH cgroup/for-3.12 1/2] cgroup: fix subsystem file accesses on the root cgroup

[PATCH cgroup/for-3.12 1/2] cgroup: fix subsystem file accesses on the root cgroup

[Tejun Heo]

From af8e64db96406f91746711260717c11584f90efd Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Thu, 15 Aug 2013 11:37:54 -0400

105347ba5 ("cgroup: make cgroup_file_open() rcu_read_lock() around
cgroup_css() and add cfent->css") added cfent->css to cache the
associted cgroup_subsys_state across file operations.

A cfent is associated with single css throughout its lifetime and the
origimal commit initialized the cache pointer during cgroup_add_file()
and verified that it matches the actual one in cgroup_file_open().
While this works fine for !root cgroups, it's broken for root cgroups
as files in a root cgroup are created before the css's are associated
with the cgroup and thus cgroup_css() call in cgroup_add_file()
returns NULL associating all cfents in the root cgroup with NULL css.
This makes cgroup_file_open() trigger WARN and fail with -ENODEV for
all !core subsystem files in the root cgroups.

There's no reason to initialize cfent->css separately from
cgroup_add_file().  As the association never changes,
cgroup_file_open() can set it unconditionally every time and
containing the logic in cgroup_file_open() makes more sense anyway as
the only reason it's necessary is file->private_data being already
occupied.

Fix it by setting cfent->css unconditionally from cgroup_file_open().

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 kernel/cgroup.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 66d0107..ab2a23f 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -2490,10 +2490,18 @@ static int cgroup_file_open(struct inode *inode, struct file *file)
 	}
 	rcu_read_unlock();
 
-	/* css should match @cfe->css, see cgroup_add_file() for details */
-	if (!css || WARN_ON_ONCE(css != cfe->css))
+	if (!css)
 		return -ENODEV;
 
+	/*
+	 * @cfe->css is used by read/write/close to determine the
+	 * associated css.  @file->private_data would be a better place but
+	 * that's already used by seqfile.  Multiple accessors may use it
+	 * simultaneously which is okay as the association never changes.
+	 */
+	WARN_ON_ONCE(cfe->css && cfe->css != css);
+	cfe->css = css;
+
 	if (cft->read_map || cft->read_seq_string) {
 		file->f_op = &cgroup_seqfile_operations;
 		err = single_open(file, cgroup_seqfile_show, cfe);
@@ -2772,18 +2780,6 @@ static int cgroup_add_file(struct cgroup *cgrp, struct cftype *cft)
 	dentry->d_fsdata = cfe;
 	simple_xattrs_init(&cfe->xattrs);
 
-	/*
-	 * cfe->css is used by read/write/close to determine the associated
-	 * css.  file->private_data would be a better place but that's
-	 * already used by seqfile.  Note that open will use the usual
-	 * cgroup_css() and css_tryget() to acquire the css and this
-	 * caching doesn't affect css lifetime management.
-	 */
-	if (cft->ss)
-		cfe->css = cgroup_css(cgrp, cft->ss->subsys_id);
-	else
-		cfe->css = &cgrp->dummy_css;
-
 	mode = cgroup_file_mode(cft);
 	error = cgroup_create_file(dentry, mode | S_IFREG, cgrp->root->sb);
 	if (!error) {
-- 
1.8.3.1

[PATCH cgroup/for-3.12 2/2] cgroup: fix cgroup_write_event_control()

[Tejun Heo]

From ced043264c9c3f03ab3c992af17461db50421dd8 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Date: Thu, 15 Aug 2013 11:37:54 -0400

81eeaf0411 ("cgroup: make cftype->[un]register_event() deal with
cgroup_subsys_state inst ead of cgroup") updated the cftype event
methods to take @css (cgroup_subsys_state) instead of @cgroup;
however, it incorrectly used @css passed to
cgroup_write_event_control(), which the dummy_css for the cgroup as
the file is a cgroup core file.  This leads to oops on event
registration.

Fix it by using the css matching the event target file.  Note that
cgroup_write_event_control() now disallows cgroup core files from
being event sources.  This is for simplicity and doesn't matter as
cgroup_event will be moved and made specific to memcg.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 kernel/cgroup.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index ab2a23f..6499004 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -4040,10 +4040,10 @@ static void cgroup_event_ptable_queue_proc(struct file *file,
  * Input must be in format '<event_fd> <control_fd> <args>'.
  * Interpretation of args is defined by control file implementation.
  */
-static int cgroup_write_event_control(struct cgroup_subsys_state *css,
+static int cgroup_write_event_control(struct cgroup_subsys_state *dummy_css,
 				      struct cftype *cft, const char *buffer)
 {
-	struct cgroup *cgrp = css->cgroup;
+	struct cgroup *cgrp = dummy_css->cgroup;
 	struct cgroup_event *event;
 	struct cgroup *cgrp_cfile;
 	unsigned int efd, cfd;
@@ -4065,7 +4065,7 @@ static int cgroup_write_event_control(struct cgroup_subsys_state *css,
 	event = kzalloc(sizeof(*event), GFP_KERNEL);
 	if (!event)
 		return -ENOMEM;
-	event->css = css;
+
 	INIT_LIST_HEAD(&event->list);
 	init_poll_funcptr(&event->pt, cgroup_event_ptable_queue_proc);
 	init_waitqueue_func_entry(&event->wait, cgroup_event_wake);
@@ -4101,6 +4101,23 @@ static int cgroup_write_event_control(struct cgroup_subsys_state *css,
 		goto out_put_cfile;
 	}
 
+	if (!event->cft->ss) {
+		ret = -EBADF;
+		goto out_put_cfile;
+	}
+
+	/* determine the css of @cfile and associate @event with it */
+	rcu_read_lock();
+
+	ret = -EINVAL;
+	event->css = cgroup_css(cgrp, event->cft->ss->subsys_id);
+	if (event->css)
+		ret = 0;
+
+	rcu_read_unlock();
+	if (ret)
+		goto out_put_cfile;
+
 	/*
 	 * The file to be monitored must be in the same cgroup as
 	 * cgroup.event_control is.
@@ -4116,7 +4133,7 @@ static int cgroup_write_event_control(struct cgroup_subsys_state *css,
 		goto out_put_cfile;
 	}
 
-	ret = event->cft->register_event(css, event->cft,
+	ret = event->cft->register_event(event->css, event->cft,
 			event->eventfd, buffer);
 	if (ret)
 		goto out_put_cfile;
-- 
1.8.3.1

Re: [PATCH cgroup/for-3.12 2/2] cgroup: fix cgroup_write_event_control()

[Li Zefan]

On 2013/8/15 23:43, Tejun Heo wrote:
>>From ced043264c9c3f03ab3c992af17461db50421dd8 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: Thu, 15 Aug 2013 11:37:54 -0400
> 
> 81eeaf0411 ("cgroup: make cftype->[un]register_event() deal with
> cgroup_subsys_state inst ead of cgroup") updated the cftype event
> methods to take @css (cgroup_subsys_state) instead of @cgroup;
> however, it incorrectly used @css passed to
> cgroup_write_event_control(), which the dummy_css for the cgroup as
> the file is a cgroup core file.  This leads to oops on event
> registration.
> 
> Fix it by using the css matching the event target file.  Note that
> cgroup_write_event_control() now disallows cgroup core files from
> being event sources.  This is for simplicity and doesn't matter as
> cgroup_event will be moved and made specific to memcg.
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

Re: [PATCH cgroup/for-3.12 2/2] cgroup: fix cgroup_write_event_control()

[Tejun Heo]

On Mon, Aug 19, 2013 at 10:41:55AM +0800, Li Zefan wrote:
> On 2013/8/15 23:43, Tejun Heo wrote:
> >>From ced043264c9c3f03ab3c992af17461db50421dd8 Mon Sep 17 00:00:00 2001
> > From: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > Date: Thu, 15 Aug 2013 11:37:54 -0400
> > 
> > 81eeaf0411 ("cgroup: make cftype->[un]register_event() deal with
> > cgroup_subsys_state inst ead of cgroup") updated the cftype event
> > methods to take @css (cgroup_subsys_state) instead of @cgroup;
> > however, it incorrectly used @css passed to
> > cgroup_write_event_control(), which the dummy_css for the cgroup as
> > the file is a cgroup core file.  This leads to oops on event
> > registration.
> > 
> > Fix it by using the css matching the event target file.  Note that
> > cgroup_write_event_control() now disallows cgroup core files from
> > being event sources.  This is for simplicity and doesn't matter as
> > cgroup_event will be moved and made specific to memcg.
> > 
> > Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-3.12.  Thanks.

-- 
tejun

Re: [PATCH cgroup/for-3.12 1/2] cgroup: fix subsystem file accesses on the root cgroup

[Li Zefan]

> 105347ba5 ("cgroup: make cgroup_file_open() rcu_read_lock() around
> cgroup_css() and add cfent->css") added cfent->css to cache the
> associted cgroup_subsys_state across file operations.
> 
> A cfent is associated with single css throughout its lifetime and the
> origimal commit initialized the cache pointer during cgroup_add_file()
> and verified that it matches the actual one in cgroup_file_open().
> While this works fine for !root cgroups, it's broken for root cgroups
> as files in a root cgroup are created before the css's are associated
> with the cgroup and thus cgroup_css() call in cgroup_add_file()
> returns NULL associating all cfents in the root cgroup with NULL css.
> This makes cgroup_file_open() trigger WARN and fail with -ENODEV for
> all !core subsystem files in the root cgroups.
> 
> There's no reason to initialize cfent->css separately from
> cgroup_add_file().  As the association never changes,
> cgroup_file_open() can set it unconditionally every time and
> containing the logic in cgroup_file_open() makes more sense anyway as
> the only reason it's necessary is file->private_data being already
> occupied.
> 
> Fix it by setting cfent->css unconditionally from cgroup_file_open().
> 
> Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

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

Re: [PATCH cgroup/for-3.12 1/2] cgroup: fix subsystem file accesses on the root cgroup

[Tejun Heo]

On Mon, Aug 19, 2013 at 10:23:41AM +0800, Li Zefan wrote:
> > 105347ba5 ("cgroup: make cgroup_file_open() rcu_read_lock() around
> > cgroup_css() and add cfent->css") added cfent->css to cache the
> > associted cgroup_subsys_state across file operations.
> > 
> > A cfent is associated with single css throughout its lifetime and the
> > origimal commit initialized the cache pointer during cgroup_add_file()
> > and verified that it matches the actual one in cgroup_file_open().
> > While this works fine for !root cgroups, it's broken for root cgroups
> > as files in a root cgroup are created before the css's are associated
> > with the cgroup and thus cgroup_css() call in cgroup_add_file()
> > returns NULL associating all cfents in the root cgroup with NULL css.
> > This makes cgroup_file_open() trigger WARN and fail with -ENODEV for
> > all !core subsystem files in the root cgroups.
> > 
> > There's no reason to initialize cfent->css separately from
> > cgroup_add_file().  As the association never changes,
> > cgroup_file_open() can set it unconditionally every time and
> > containing the logic in cgroup_file_open() makes more sense anyway as
> > the only reason it's necessary is file->private_data being already
> > occupied.
> > 
> > Fix it by setting cfent->css unconditionally from cgroup_file_open().
> > 
> > Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Acked-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-3.12.

Thanks.

-- 
tejun