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

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

[Li Zefan]

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 <lizefan@huawei.com>
---
 block/blk-cgroup.h     |   2 -
 include/linux/cgroup.h |  17 +++++++++
 kernel/cgroup.c        | 101 ++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 91 insertions(+), 29 deletions(-)

diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
index 2459730..e2e3404 100644
--- a/block/blk-cgroup.h
+++ b/block/blk-cgroup.h
@@ -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;
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 900af59..7f21bad 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -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];
 
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index b5c6432..eb3c280 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -860,6 +860,17 @@ static struct inode *cgroup_new_inode(umode_t mode, struct super_block *sb)
 	return inode;
 }
 
+static struct cgroup_name *cgroup_alloc_name(struct dentry *dentry)
+{
+	struct cgroup_name *name;
+
+	name = kmalloc(sizeof(*name) + dentry->d_name.len + 1, GFP_KERNEL);
+	if (!name)
+		return NULL;
+	strcpy(name->name, dentry->d_name.name);
+	return name;
+}
+
 static void cgroup_free_fn(struct work_struct *work)
 {
 	struct cgroup *cgrp = container_of(work, struct cgroup, free_work);
@@ -890,6 +901,7 @@ static void cgroup_free_fn(struct work_struct *work)
 	simple_xattrs_free(&cgrp->xattrs);
 
 	ida_simple_remove(&cgrp->root->cgroup_ida, cgrp->id);
+	kfree(rcu_dereference_raw(cgrp->name));
 	kfree(cgrp);
 }
 
@@ -1771,49 +1783,49 @@ static struct kobject *cgroup_kobj;
  * @buf: the buffer to write the path into
  * @buflen: the length of the buffer
  *
- * Called with cgroup_mutex held or else with an RCU-protected cgroup
- * reference.  Writes path of cgroup into buf.  Returns 0 on success,
- * -errno on error.
+ * Writes path of cgroup into buf.  Returns 0 on success, -errno on error.
+ *
+ * We can't generate cgroup path using dentry->d_name, as accessing
+ * dentry->name must be protected by irq-unsafe 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.
  */
 int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen)
 {
-	struct dentry *dentry = cgrp->dentry;
+	int ret = -ENAMETOOLONG;
 	char *start;
 
-	rcu_lockdep_assert(rcu_read_lock_held() || cgroup_lock_is_held(),
-			   "cgroup_path() called without proper locking");
-
-	if (cgrp == dummytop) {
-		/*
-		 * Inactive subsystems have no dentry for their root
-		 * cgroup
-		 */
+	if (!cgrp->parent) {
 		strcpy(buf, "/");
 		return 0;
 	}
 
 	start = buf + buflen - 1;
-
 	*start = '\0';
-	for (;;) {
-		int len = dentry->d_name.len;
 
+	rcu_read_lock();
+	while (cgrp->parent) {
+		struct cgroup_name *cname = rcu_dereference(cgrp->name);
+		char *name;
+		int len;
+
+		name = cname->name;
+		len = strlen(name);
 		if ((start -= len) < buf)
-			return -ENAMETOOLONG;
-		memcpy(start, dentry->d_name.name, len);
-		cgrp = cgrp->parent;
-		if (!cgrp)
-			break;
+			goto out;
+		memcpy(start, name, len);
 
-		dentry = cgrp->dentry;
-		if (!cgrp->parent)
-			continue;
 		if (--start < buf)
-			return -ENAMETOOLONG;
+			goto out;
 		*start = '/';
+
+		cgrp = cgrp->parent;
 	}
+	ret = 0;
 	memmove(buf, start, buf + buflen - start);
-	return 0;
+out:
+	rcu_read_unlock();
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cgroup_path);
 
@@ -2539,13 +2551,40 @@ static int cgroup_file_release(struct inode *inode, struct file *file)
 static int cgroup_rename(struct inode *old_dir, struct dentry *old_dentry,
 			    struct inode *new_dir, struct dentry *new_dentry)
 {
+	int ret;
+	struct cgroup_name *name, *old_name;
+	struct cgroup *cgrp;
+
+	/*
+	 * It's convinient to use parent dir's i_mutex to protected
+	 * cgrp->name.
+	 */
+	lockdep_assert_held(&old_dir->i_mutex);
+
 	if (!S_ISDIR(old_dentry->d_inode->i_mode))
 		return -ENOTDIR;
 	if (new_dentry->d_inode)
 		return -EEXIST;
 	if (old_dir != new_dir)
 		return -EIO;
-	return simple_rename(old_dir, old_dentry, new_dir, new_dentry);
+
+	cgrp = __d_cgrp(old_dentry);
+
+	name = cgroup_alloc_name(new_dentry);
+	if (!name)
+		return -ENOMEM;
+
+	ret = simple_rename(old_dir, old_dentry, new_dir, new_dentry);
+	if (ret) {
+		kfree(name);
+		return ret;
+	}
+
+	old_name = cgrp->name;
+	rcu_assign_pointer(cgrp->name, name);
+
+	kfree_rcu(old_name, rcu_head);
+	return 0;
 }
 
 static struct simple_xattrs *__d_xattrs(struct dentry *dentry)
@@ -4160,6 +4199,7 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 			     umode_t mode)
 {
 	struct cgroup *cgrp;
+	struct cgroup_name *name;
 	struct cgroupfs_root *root = parent->root;
 	int err = 0;
 	struct cgroup_subsys *ss;
@@ -4170,9 +4210,14 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 	if (!cgrp)
 		return -ENOMEM;
 
+	name = cgroup_alloc_name(dentry);
+	if (!name)
+		goto err_free_cgrp;
+	rcu_assign_pointer(cgrp->name, name);
+
 	cgrp->id = ida_simple_get(&root->cgroup_ida, 1, 0, GFP_KERNEL);
 	if (cgrp->id < 0)
-		goto err_free_cgrp;
+		goto err_free_name;
 
 	/*
 	 * Only live parents can have children.  Note that the liveliness
@@ -4278,6 +4323,8 @@ err_free_all:
 	deactivate_super(sb);
 err_free_id:
 	ida_simple_remove(&root->cgroup_ida, cgrp->id);
+err_free_name:
+	kfree(rcu_dereference_raw(cgrp->name));
 err_free_cgrp:
 	kfree(cgrp);
 	return err;
-- 
1.8.0.2

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

[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 <lizefan@huawei.com>
---
 include/linux/cgroup.h |  1 +
 kernel/cgroup.c        | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 7f21bad..70676eb 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -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);
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index eb3c280..abbb097 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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
-- 
1.8.0.2

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

[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 <lizefan@huawei.com>
---
 kernel/cpuset.c | 32 +++++++++-----------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 4f9dfe4..ace5bfc 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -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);
 }
 
-- 
1.8.0.2

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

[Tejun Heo]

On Mon, Feb 25, 2013 at 02:17:49PM +0800, Li Zefan wrote:
> 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 <lizefan@huawei.com>
...
>  /**
> + * 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;
> +}

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.

-- 
tejun

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

[Li Zefan]

On 2013/2/26 10:27, Tejun Heo wrote:
> On Mon, Feb 25, 2013 at 02:17:49PM +0800, Li Zefan wrote:
>> 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 <lizefan@huawei.com>
> ...
>>  /**
>> + * 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;
>> +}
> 
> 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.

> 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.

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

[Tejun Heo]

Hello,

On Tue, Feb 26, 2013 at 2:25 AM, Li Zefan <lizefan@huawei.com> wrote:
> 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.

-- 
tejun

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

[Li Zefan]

On 2013/2/26 21:26, Tejun Heo wrote:
> Hello,
> 
> On Tue, Feb 26, 2013 at 2:25 AM, Li Zefan <lizefan@huawei.com> wrote:
>> 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.

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

[Tejun Heo]

On Wed, Feb 27, 2013 at 2:49 AM, Li Zefan <lizefan@huawei.com> wrote:
> 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.

-- 
tejun

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

[Li Zefan]

On 2013/2/27 21:23, Tejun Heo wrote:
> On Wed, Feb 27, 2013 at 2:49 AM, Li Zefan <lizefan@huawei.com> wrote:
>> 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.

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

[Tejun Heo]

On Wed, Feb 27, 2013 at 10:53 PM, Li Zefan <lizefan@huawei.com> wrote:
>> 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.

-- 
tejun

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

[Li Zefan]

On 2013/2/28 22:49, Tejun Heo wrote:
> On Wed, Feb 27, 2013 at 10:53 PM, Li Zefan <lizefan@huawei.com> wrote:
>>> 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.

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

[Al Viro]

On Fri, Mar 01, 2013 at 02:36:29PM +0800, Li Zefan wrote:
> On 2013/2/28 22:49, Tejun Heo wrote:
> > On Wed, Feb 27, 2013 at 10:53 PM, Li Zefan <lizefan@huawei.com> wrote:
> >>> 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.

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

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

[Tejun Heo]

Hello, Al.

On Fri, Mar 1, 2013 at 12:39 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> > 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.

-- 
tejun

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

[Li Zefan]

On 2013/3/2 4:45, Tejun Heo wrote:
> Hello, Al.
> 
> On Fri, Mar 1, 2013 at 12:39 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>> 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?
> 

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

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

[Tejun Heo]

Hello,

On Mon, Mar 04, 2013 at 11:02:37AM +0800, Li Zefan wrote:
> > 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.

-- 
tejun