Skip to contentSkip to conversation

cgroup: fix cgroup_path() vs rename() race

PATCH Superseded Li Zefan 3 participants 15 messages Feb 25, 2013 – Mar 4, 2013

Subsystems (2)
Block layer cgroup

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Li Zefan

author
#0 lore raw

PATCH v3 1/3 cgroup: fix cgroup_path() vs rename() race

rename() will change dentry->d_name. The result of this race can be worse than seeing partially rewritten name, but we might access a stale pointer because rename() will re-allocate memory to hold a longer name.

As accessing dentry->name must be protected by dentry->d_lock or parent inode's i_mutex, while on the other hand cgroup-path() can be called with some irq-safe spinlocks held, we can't generate cgroup path using dentry->d_name.

Alternatively we make a copy of dentry->d_name and save it in cgrp->name when a cgroup is created, and update cgrp->name at rename().

v3: use kfree_rcu() instead of synchronize_rcu() in user-visible path
v2: make cgrp->name RCU safe.

Signed-off-by Li Zefan <[email protected]>
---
 block/blk-cgroup.h     |   2 -
 include/linux/cgroup.h |  17 +++++++++
 kernel/cgroup.c        | 101 ++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 91 insertions(+), 29 deletions(-)
block/blk-cgroup.h +0 -2
index 2459730..e2e3404 100644
@@ -216,9 +216,7 @@ static inline int blkg_path(struct blkcg_gq *blkg, char *buf, int buflen)
 {
 	int ret;
 
-	rcu_read_lock();
 	ret = cgroup_path(blkg->blkcg->css.cgroup, buf, buflen);
-	rcu_read_unlock();
 	if (ret)
 		strncpy(buf, "<unavailable>", buflen);
 	return ret;
include/linux/cgroup.h +17 -0
index 900af59..7f21bad 100644
@@ -150,6 +150,11 @@ enum {
 	CGRP_CPUSET_CLONE_CHILDREN,
 };
 
+struct cgroup_name {
+	struct rcu_head rcu_head;
+	char name[0];
+};
+
 struct cgroup {
 	unsigned long flags;		/* "unsigned long" so bitops work */
 

@@ -172,6 +177,18 @@ struct cgroup {
 	struct cgroup *parent;		/* my parent */
 	struct dentry *dentry;		/* cgroup fs entry, RCU protected */
 
+	/*
+	 * This is a copy of dentry->d_name, and it's needed because
+	 * we can't use dentry->d_name in cgroup_path().
+	 *
+	 * You must acquire rcu_read_lock() to access cgrp->name, and
+	 * the only place that can change it is rename(), which is
+	 * protected by parent dir's i_mutex.
+	 *
+	 * The name of the root cgroup is "/", and @name == NULL.
+	 */
+	struct cgroup_name __rcu *name;
+
 	/* Private pointers for each registered subsystem */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];
kernel/cgroup.c +74 -27

169 lines not on this page · raw message

Signature · 1 line
1.8.0.2
author
#1 lore raw

PATCH 2/3 cgroup: add cgroup_name() API

cgroup_name() returns the name of a cgroup and it must be called with rcu_read_lock() held.

This will be used by cpuset.

Signed-off-by Li Zefan <[email protected]>
---
 include/linux/cgroup.h |  1 +
 kernel/cgroup.c        | 14 ++++++++++++++
 2 files changed, 15 insertions(+)
include/linux/cgroup.h +1 -0
index 7f21bad..70676eb 100644
@@ -426,6 +426,7 @@ int cgroup_rm_cftypes(struct cgroup_subsys *ss, struct cftype *cfts);
 
 int cgroup_is_removed(const struct cgroup *cgrp);
 
+char *cgroup_name(const struct cgroup *cgrp);
 int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
 
 int cgroup_task_count(const struct cgroup *cgrp);
kernel/cgroup.c +14 -0
index eb3c280..abbb097 100644
@@ -1778,6 +1778,20 @@ static struct file_system_type cgroup_fs_type = {
 static struct kobject *cgroup_kobj;
 
 /**
+ * cgroup_name - get the name of a cgroup
+ * @cgrp: the cgroup in question
+ *
+ * Must be called with rcu_read_lock() held.
+ */
+char *cgroup_name(const struct cgroup *cgrp)
+{
+	if (!cgrp->parent)
+		return "/";
+	else
+		return rcu_dereference(cgrp->name)->name;
+}
+
+/**
  * cgroup_path - generate the path of a cgroup
  * @cgrp: the cgroup in question
  * @buf: the buffer to write the path into
Signature · 1 line
1.8.0.2
maintainer
#2 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 6 lines · Li Zefan

cgroup_name() returns the name of a cgroup and it must be called with rcu_read_lock() held.

This will be used by cpuset.

Signed-off-by Li Zefan <[email protected]>

...

kernel/cgroup.c static struct file_system_type cgroup_fs_type = { · line 1792

Quoted 13 lines

/**
+ * cgroup_name - get the name of a cgroup
+ * @cgrp: the cgroup in question
+ *
+ * Must be called with rcu_read_lock() held.
+ */

kernel/cgroup.c +7 -0
+char *cgroup_name(const struct cgroup *cgrp)
+{
+	if (!cgrp->parent)
+		return "/";
+	else
+		return rcu_dereference(cgrp->name)->name;
+}

Can't we initialize ->name of root cgroup to "/" and lose the conditional? We can lose the wrapper altogether but if you're worried that sparse check isn't enough, we can have trivial inline wrapper, but in that case it probably would help to rename cgrp->name to, say, cgrp->__name and put a comment directing people to the accessing wrapper which should probably return const char *.

Thanks.

Signature · 1 line
tejun
author
#3 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 8 lines · Tejun Heo
Quoted 6 lines · Li Zefan

cgroup_name() returns the name of a cgroup and it must be called with rcu_read_lock() held.

This will be used by cpuset.

Signed-off-by Li Zefan <[email protected]>

...

Quoted 16 lines

kernel/cgroup.c static struct file_system_type cgroup_fs_type = { · line 1792

Quoted 13 lines

/**
+ * cgroup_name - get the name of a cgroup
+ * @cgrp: the cgroup in question
+ *
+ * Must be called with rcu_read_lock() held.
+ */

kernel/cgroup.c +7 -0
+char *cgroup_name(const struct cgroup *cgrp)
+{
+	if (!cgrp->parent)
+		return "/";
+	else
+		return rcu_dereference(cgrp->name)->name;
+}

Can't we initialize ->name of root cgroup to "/" and lose the conditional?

Sure we can. We'll have to allocate cgrp->name in cgroup_remount() and cgroup_init(), and free cgrp->name in cgroup_kill_sb(). It looks to me the current version is a bit simpler.

That said, I don't have strong preference. I'll revise the patchset if you still prefer to init root_cgrp->name.

Quoted 6 lines

We can lose the wrapper altogether but if you're worried that sparse check isn't enough, we can have trivial inline wrapper, but in that case it probably would help to rename cgrp->name to, say, cgrp->__name and put a comment directing people to the accessing wrapper which should probably return const char *.

I do expect people always use cgroup_name(). Should anyone access cgrp->name directly and doesn't notice cgrp->name can be NULL, he'll get NULL ptr crash and turn to cgroup_name(), and a comment to guide people to cgroup_name() is helpful too.

maintainer
#4 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Hello,

Quoted 3 lines · Li Zefan

Sure we can. We'll have to allocate cgrp->name in cgroup_remount() and cgroup_init(), and free cgrp->name in cgroup_kill_sb(). It looks to me the current version is a bit simpler.

Can't we just set it to constant "/"? Root cgroup init is a separate path anyway.

Thanks.

Signature · 1 line
tejun
author
#5 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 10 lines · Tejun Heo

Hello,

Quoted 3 lines · Li Zefan

Sure we can. We'll have to allocate cgrp->name in cgroup_remount() and cgroup_init(), and free cgrp->name in cgroup_kill_sb(). It looks to me the current version is a bit simpler.

Can't we just set it to constant "/"? Root cgroup init is a separate path anyway.

Well, cgrp->name is a pointer to struct cgroup_name.

At first I tried to declare cgrp->name as char *, and use container_of() to get struct cgroup_name, but it didn't result in simpler code.

So seems there's no way that is a pure win over another.

maintainer
#6 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 4 lines · Li Zefan

Well, cgrp->name is a pointer to struct cgroup_name.

At first I tried to declare cgrp->name as char *, and use container_of() to get struct cgroup_name, but it didn't result in simpler code.

Hmmm? But then what prevents defining const struct cgroup_name?

static const struct cgroup_name root_cgroup_name = { .name = "/" };

Thanks.

Signature · 1 line
tejun
author
#7 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 9 lines · Tejun Heo
Quoted 4 lines · Li Zefan

Well, cgrp->name is a pointer to struct cgroup_name.

At first I tried to declare cgrp->name as char *, and use container_of() to get struct cgroup_name, but it didn't result in simpler code.

Hmmm? But then what prevents defining const struct cgroup_name?

static const struct cgroup_name root_cgroup_name = { .name = "/" };

Can't... That's char name[0] not char *name.

I'll allocate a global struct cgroup_name *root_cgroup_name, and use it for all root_cgroup->name and dummytop->name.

maintainer
#8 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 3 lines · Li Zefan
Quoted 1 line

static const struct cgroup_name root_cgroup_name = { .name = "/" };

Can't... That's char name[0] not char *name.

Flexible array members can be statically initialized. If you wanna be really anal about it, you can do it manually with a wrapping struct but I don't think that would be necessary.

Thanks.

Signature · 1 line
tejun
author
#9 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 9 lines · Tejun Heo
Quoted 3 lines · Li Zefan
Quoted 1 line

static const struct cgroup_name root_cgroup_name = { .name = "/" };

Can't... That's char name[0] not char *name.

Flexible array members can be statically initialized. If you wanna be really anal about it, you can do it manually with a wrapping struct but I don't think that would be necessary.

I didn't know this difference between flexible array and zero-size array. Thanks.

#10 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 6 lines · Li Zefan
Quoted 5 lines · Tejun Heo
Quoted 3 lines · Li Zefan
Quoted 1 line

static const struct cgroup_name root_cgroup_name = { .name = "/" };

Can't... That's char name[0] not char *name.

Quoted 7 lines
Quoted 4 lines

Flexible array members can be statically initialized. If you wanna be really anal about it, you can do it manually with a wrapping struct but I don't think that would be necessary.

I didn't know this difference between flexible array and zero-size array. Thanks.

Mind you, initializing flex array member is explicitly invalid per C99; it's a GNU extension...

maintainer
#11 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Hello, Al.

Quoted 10 lines · Al Viro
Quoted 7 lines
Quoted 4 lines

Flexible array members can be statically initialized. If you wanna be really anal about it, you can do it manually with a wrapping struct but I don't think that would be necessary.

I didn't know this difference between flexible array and zero-size array. Thanks.

Mind you, initializing flex array member is explicitly invalid per C99; it's a GNU extension...

Yeah, that's what I meant by the "anal" part although it seems like c99 doesn't even allow that. Do we care tho? It seems like a logical feature which should show up in the standard eventually. Maybe it could be a problem for clang?

Thanks.

Signature · 1 line
tejun
author
#12 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Quoted 11 lines · Tejun Heo

Hello, Al.

Quoted 8 lines · Al Viro
Quoted 7 lines
Quoted 4 lines

Flexible array members can be statically initialized. If you wanna be really anal about it, you can do it manually with a wrapping struct but I don't think that would be necessary.

I didn't know this difference between flexible array and zero-size array. Thanks.

Quoted 8 lines
Quoted 2 lines

Mind you, initializing flex array member is explicitly invalid per C99; it's a GNU extension...

Yeah, that's what I meant by the "anal" part although it seems like c99 doesn't even allow that. Do we care tho? It seems like a logical feature which should show up in the standard eventually. Maybe it could be a problem for clang?

Do we have a decision now? Please pick v4 if this GNU extension can't be used in kernel code, otherwise v5.

maintainer
#13 lore raw

Re: [PATCH 2/3] cgroup: add cgroup_name() API

Hello,

Quoted 8 lines · Li Zefan
Quoted 5 lines

Yeah, that's what I meant by the "anal" part although it seems like c99 doesn't even allow that. Do we care tho? It seems like a logical feature which should show up in the standard eventually. Maybe it could be a problem for clang?

Do we have a decision now? Please pick v4 if this GNU extension can't be used in kernel code, otherwise v5.

I'm just gonna apply the one with the initializer. It's simpler and not like we can't back out easily if it ever becomes a problem.

Thanks.

Signature · 1 line
tejun
author
#14 lore raw

PATCH 3/3 cpuset: use cgroup_name() in cpuset_print_task_mems_allowed()

In reply to Li Zefan

Use cgroup_name() instead of cgrp->dentry->name. This makes the code simpler.

While at it, remove cpuset_name and make cpuset_nodelist a local variable to cpuset_print_task_mems_allowed().

Signed-off-by Li Zefan <[email protected]>
---
 kernel/cpuset.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)
kernel/cpuset.c +9 -23
index 4f9dfe4..ace5bfc 100644
@@ -265,17 +265,6 @@ static DEFINE_MUTEX(cpuset_mutex);
 static DEFINE_MUTEX(callback_mutex);
 
 /*
- * cpuset_buffer_lock protects both the cpuset_name and cpuset_nodelist
- * buffers.  They are statically allocated to prevent using excess stack
- * when calling cpuset_print_task_mems_allowed().
- */
-#define CPUSET_NAME_LEN		(128)
-#define	CPUSET_NODELIST_LEN	(256)
-static char cpuset_name[CPUSET_NAME_LEN];
-static char cpuset_nodelist[CPUSET_NODELIST_LEN];
-static DEFINE_SPINLOCK(cpuset_buffer_lock);
-
-/*
  * CPU / memory hotplug is handled asynchronously.
  */
 static struct workqueue_struct *cpuset_propagate_hotplug_wq;

@@ -2592,6 +2581,8 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
 	return nodes_intersects(tsk1->mems_allowed, tsk2->mems_allowed);
 }
 
+#define CPUSET_NODELIST_LEN	(256)
+
 /**
  * cpuset_print_task_mems_allowed - prints task's cpuset and mems_allowed
  * @task: pointer to task_struct of some task.

@@ -2602,24 +2593,19 @@ int cpuset_mems_allowed_intersects(const struct task_struct *tsk1,
  */
 void cpuset_print_task_mems_allowed(struct task_struct *tsk)
 {
-	struct dentry *dentry;
+	 /* Statically allocated to prevent using excess stack. */
+	static char cpuset_nodelist[CPUSET_NODELIST_LEN];
+	static DEFINE_SPINLOCK(cpuset_buffer_lock);
 
-	dentry = task_cs(tsk)->css.cgroup->dentry;
-	spin_lock(&cpuset_buffer_lock);
+	struct cgroup *cgrp = task_cs(tsk)->css.cgroup;
 
-	if (!dentry) {
-		strcpy(cpuset_name, "/");
-	} else {
-		spin_lock(&dentry->d_lock);
-		strlcpy(cpuset_name, (const char *)dentry->d_name.name,
-			CPUSET_NAME_LEN);
-		spin_unlock(&dentry->d_lock);
-	}
+	spin_lock(&cpuset_buffer_lock);
 
 	nodelist_scnprintf(cpuset_nodelist, CPUSET_NODELIST_LEN,
 			   tsk->mems_allowed);
 	printk(KERN_INFO "%s cpuset=%s mems_allowed=%s\n",
-	       tsk->comm, cpuset_name, cpuset_nodelist);
+	       tsk->comm, cgroup_name(cgrp), cpuset_nodelist);
+
 	spin_unlock(&cpuset_buffer_lock);
 }
Signature · 1 line
1.8.0.2