cgroup: fix cgroup_path() vs rename() race, take 2

PATCH Superseded Li Zefan 2 participants 3 messages Feb 18, 2013 – Feb 19, 2013

Subsystems (2)

Block layer cgroup

lore

Share & tools

lore

Li Zefan

author

2013-02-18 01:16 UTC #0 lore raw

PATCH v2 2/2 cgroup: fix cgroup_path() vs rename() race, take 2

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

v2: make cgrp->name RCU safe.

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

---
 block/blk-cgroup.h     |   2 -
 include/linux/cgroup.h |   1 +
 kernel/cgroup.c        | 113 ++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 85 insertions(+), 31 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 +1 -0

index 900af59..a3d87d9 100644
@@ -171,6 +171,7 @@ struct cgroup {
 
 	struct cgroup *parent;		/* my parent */
 	struct dentry *dentry;		/* cgroup fs entry, RCU protected */
+	char __rcu *name;		/* a copy of dentry->d_name */
 
 	/* Private pointers for each registered subsystem */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];

kernel/cgroup.c +84 ~~-29~~

206 lines not on this page · raw message

Signature · 1 line

1.8.0.2

Tejun Heo

maintainer

2013-02-18 17:30 UTC #1 lore raw

Hello, Li.

include/linux/cgroup.h struct cgroup { · line 174

Quoted 5 lines · Li Zefan

include/linux/cgroup.h +1 -0

@@ -171,6 +171,7 @@ struct cgroup {
 
 	struct cgroup *parent;		/* my parent */
 	struct dentry *dentry;		/* cgroup fs entry, RCU protected */
+	char __rcu *name;		/* a copy of dentry->d_name */

A brief explanation of why this is necessary and how rcu is used would be nice.

kernel/cgroup.c static int cgroup_get_rootdir(struct super_block *sb) · line 1579

Quoted 11 lines

kernel/cgroup.c +11 -0

+static char *cgroup_alloc_name(struct dentry *dentry)
+{
+	char *name;
+
+	name = kmalloc(dentry->d_name.len + 1, GFP_KERNEL);
+	if (!name)
+		return NULL;
+	memcpy(name, dentry->d_name.name, dentry->d_name.len);
+	name[dentry->d_name.len] = '\0';
+	return name;
+}

While d_name has length field, it's always properly NULL terminated, so kstrdup() should suffice here. Right, Al?

Quoted 1 line

(quoted diff) +0 -0

@@ -1613,13 +1626,19 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,

...

kernel/cgroup.c static struct dentry *cgroup_mount(struct file_system_type *fs_type, · line 1641

Quoted 7 lines

kernel/cgroup.c +6 -1

-		inode = sb->s_root->d_inode;
+		dentry = sb->s_root;
+		inode = dentry->d_inode;
+
+		root_cgrp->name = cgroup_alloc_name(dentry);
+		if (!root_cgrp->name)
+			goto drop_new_super;

Don't we need an RCU assignment? Is it safe because it isn't online yet? But wouldn't this still trigger sparse warning?

kernel/cgroup.c static void cgroup_kill_sb(struct super_block *sb) { · line 1773

Quoted 5 lines

kernel/cgroup.c +1 -0

@@ -1751,6 +1770,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
 	mutex_unlock(&cgroup_root_mutex);
 	mutex_unlock(&cgroup_mutex);
 
+	synchronize_rcu();

An explanation on what we're synchronizing would be nice. Barriers without explanation sucks because there's nothing directly linking the barriers to the things which are being protected.

Quoted 4 lines

kernel/cgroup.c +0 -0

@@ -2539,13 +2558,41 @@ 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)
 {

...

kernel/cgroup.c static int cgroup_file_release(struct inode *inode, struct file *file) · line 2593

Quoted 4 lines

kernel/cgroup.c +4 -0

+	old_name = cgrp->name;
+	rcu_assign_pointer(cgrp->name, name);
+
+	synchronize_rcu();

Please don't call synchronize_rcu() from interface which is directly visible to userland. It leads to sporadic difficult-to-reproduce latencies which hurt enough in corner cases and this is kmalloc memory. It's not like kfree_rcu() is difficult to use or anything.

kernel/cgroup.c static long cgroup_create(struct cgroup *parent, struct dentry *dentry, · line 4196

Quoted 12 lines

kernel/cgroup.c +5 -0

+	kfree(old_name);
+	return 0;
 }
 
 static struct simple_xattrs *__d_xattrs(struct dentry *dentry)

@@ -4144,9 +4191,13 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 	if (!cgrp)
 		return -ENOMEM;
 
+	cgrp->name = cgroup_alloc_name(dentry);
+	if (!cgrp->name)
+		goto err_free_cgrp;

Ditto with assignment.

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2013-02-19 01:44 UTC #2 lore raw

Quoted 12 lines · Tejun Heo

Hello, Li.
include/linux/cgroup.h struct cgroup { · line 174
Quoted 5 lines · Li Zefan
include/linux/cgroup.h +1 -0
@@ -171,6 +171,7 @@ struct cgroup {
 
 	struct cgroup *parent;		/* my parent */
 	struct dentry *dentry;		/* cgroup fs entry, RCU protected */
+	char __rcu *name;		/* a copy of dentry->d_name */
A brief explanation of why this is necessary and how rcu is used would be nice.

The comments in cgroup_path() explains why we can't use dentry->d_name, which suggests why cgrp->name is needed. I'll revise the comments there, and add comments on the rcu thing.

Quoted 6 lines

kernel/cgroup.c +6 -0

+static char *cgroup_alloc_name(struct dentry *dentry)
+{
+	char *name;
+
+	name = kmalloc(dentry->d_name.len + 1, GFP_KERNEL);
+	if (!name)

Quoted 9 lines

kernel/cgroup.c static int cgroup_get_rootdir(struct super_block *sb) · line 1579
Quoted 5 lines
kernel/cgroup.c +5 -0
+		return NULL;
+	memcpy(name, dentry->d_name.name, dentry->d_name.len);
+	name[dentry->d_name.len] = '\0';
+	return name;
+}
While d_name has length field, it's always properly NULL terminated, so kstrdup() should suffice here. Right, Al?

Oh you're right. We pass dentry->d_name.name to printk %s in other places.

Quoted 13 lines

Quoted 1 line
(quoted diff) +0 -0
@@ -1613,13 +1626,19 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
...
kernel/cgroup.c static struct dentry *cgroup_mount(struct file_system_type *fs_type, · line 1641
Quoted 7 lines
kernel/cgroup.c +6 -1
-		inode = sb->s_root->d_inode;
+		dentry = sb->s_root;
+		inode = dentry->d_inode;
+
+		root_cgrp->name = cgroup_alloc_name(dentry);
+		if (!root_cgrp->name)
+			goto drop_new_super;
Don't we need an RCU assignment? Is it safe because it isn't online yet? But wouldn't this still trigger sparse warning?

Yeah, it's safe.

To be frank, I haven't used sparse for years. Will check.

Quoted 15 lines

kernel/cgroup.c static void cgroup_kill_sb(struct super_block *sb) { · line 1773
Quoted 5 lines
kernel/cgroup.c +1 -0
@@ -1751,6 +1770,8 @@ static void cgroup_kill_sb(struct super_block *sb) {
 	mutex_unlock(&cgroup_root_mutex);
 	mutex_unlock(&cgroup_mutex);
 
+	synchronize_rcu();
An explanation on what we're synchronizing would be nice. Barriers without explanation sucks because there's nothing directly linking the barriers to the things which are being protected.
Quoted 4 lines
kernel/cgroup.c +0 -0
@@ -2539,13 +2558,41 @@ 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)
 {
...

Quoted 10 lines

kernel/cgroup.c static int cgroup_file_release(struct inode *inode, struct file *file) · line 2593
Quoted 4 lines
kernel/cgroup.c +4 -0
+	old_name = cgrp->name;
+	rcu_assign_pointer(cgrp->name, name);
+
+	synchronize_rcu();
Please don't call synchronize_rcu() from interface which is directly visible to userland. It leads to sporadic difficult-to-reproduce latencies which hurt enough in corner cases and this is kmalloc memory. It's not like kfree_rcu() is difficult to use or anything.

Quoted 17 lines

kernel/cgroup.c static long cgroup_create(struct cgroup *parent, struct dentry *dentry, · line 4196

Quoted 12 lines

kernel/cgroup.c +5 -0

+	kfree(old_name);
+	return 0;
 }
 
 static struct simple_xattrs *__d_xattrs(struct dentry *dentry)

@@ -4144,9 +4191,13 @@ static long cgroup_create(struct cgroup *parent, struct dentry *dentry,
 	if (!cgrp)
 		return -ENOMEM;
 
+	cgrp->name = cgroup_alloc_name(dentry);
+	if (!cgrp->name)
+		goto err_free_cgrp;

Ditto with assignment.

Thanks.