[PATCH 1/2] Revert "cgroup: fix cgroup_path() vs rename() race"

[PATCH 1/2] Revert "cgroup: fix cgroup_path() vs rename() race"

[Li Zefan]

This reverts commit 299772fab304ab3a36b22b5d28ed81f9408972e7

Sasha reported this:

[  313.262599] ======================================================
[  313.271340] [ INFO: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected ]
[  313.277542] 3.8.0-rc6-next-20130208-sasha-00028-ge4e162d #278 Tainted: G       W
[  313.277542] ------------------------------------------------------
[  313.277542] kworker/u:3/4490 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
[  313.277542]  (rename_lock){+.+...}, at: [<ffffffff812a11f9>] dentry_path_raw+0x29/0x70
[  313.277542]
[  313.277542] and this task is already holding:
[  313.277542]  (&(&q->__queue_lock)->rlock){-.-...}, at: [<ffffffff819e78f3>] put_io_context_active+0x63/0x100
[  313.277542] which would create a new lock dependency:
[  313.277542]  (&(&q->__queue_lock)->rlock){-.-...} -> (rename_lock){+.+...}
[  313.277542]
[  313.277542] but this new dependency connects a HARDIRQ-irq-safe lock:
[  313.277542]  (&(&q->__queue_lock)->rlock){-.-...}

dentry_path_raw() grabs rename_lock and dentry->d_lock without disabling
irqs, which means cgroup_path() can't be called if the caller has already
held a spinlock with irq disabled.

Reported-by: Sasha Levin <levinsasha928@gmail.com>
Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 776ff75..5d4c92e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1792,10 +1792,26 @@ int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen)
 		return 0;
 	}
 
-	start = dentry_path_raw(dentry, buf, buflen);
-	if (IS_ERR(start))
-		return PTR_ERR(start);
+	start = buf + buflen - 1;
 
+	*start = '\0';
+	for (;;) {
+		int len = dentry->d_name.len;
+
+		if ((start -= len) < buf)
+			return -ENAMETOOLONG;
+		memcpy(start, dentry->d_name.name, len);
+		cgrp = cgrp->parent;
+		if (!cgrp)
+			break;
+
+		dentry = cgrp->dentry;
+		if (!cgrp->parent)
+			continue;
+		if (--start < buf)
+			return -ENAMETOOLONG;
+		*start = '/';
+	}
 	memmove(buf, start, buf + buflen - start);
 	return 0;
 }
-- 
1.8.0.2

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

[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_lock and save it in
cgrp->name when a cgroup is created, and update cgrp->name at
rename().

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 include/linux/cgroup.h |  4 ++-
 kernel/cgroup.c        | 94 +++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 76 insertions(+), 22 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 900af59..8b2ce2a 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -171,6 +171,8 @@ struct cgroup {
 
 	struct cgroup *parent;		/* my parent */
 	struct dentry *dentry;		/* cgroup fs entry, RCU protected */
+	char *name;			/* a copy of dentry->d_name */
+	spinlock_t name_lock;
 
 	/* Private pointers for each registered subsystem */
 	struct cgroup_subsys_state *subsys[CGROUP_SUBSYS_COUNT];
@@ -409,7 +411,7 @@ int cgroup_rm_cftypes(struct cgroup_subsys *ss, struct cftype *cfts);
 
 int cgroup_is_removed(const struct cgroup *cgrp);
 
-int cgroup_path(const struct cgroup *cgrp, char *buf, int buflen);
+int cgroup_path(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 5d4c92e..cea6a88 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -890,6 +890,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(cgrp->name);
 	kfree(cgrp);
 }
 
@@ -1410,6 +1411,7 @@ static void init_cgroup_housekeeping(struct cgroup *cgrp)
 	mutex_init(&cgrp->pidlist_mutex);
 	INIT_LIST_HEAD(&cgrp->event_list);
 	spin_lock_init(&cgrp->event_list_lock);
+	spin_lock_init(&cgrp->name_lock);
 	simple_xattrs_init(&cgrp->xattrs);
 }
 
@@ -1565,6 +1567,18 @@ static int cgroup_get_rootdir(struct super_block *sb)
 	return 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;
+}
+
 static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 			 int flags, const char *unused_dev_name,
 			 void *data)
@@ -1613,13 +1627,19 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 		int i;
 		struct hlist_node *node;
 		struct css_set *cg;
+		struct dentry *dentry;
 
 		BUG_ON(sb->s_root != NULL);
 
 		ret = cgroup_get_rootdir(sb);
 		if (ret)
 			goto drop_new_super;
-		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;
 
 		mutex_lock(&inode->i_mutex);
 		mutex_lock(&cgroup_mutex);
@@ -1660,8 +1680,8 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 		list_add(&root->root_list, &roots);
 		root_count++;
 
-		sb->s_root->d_fsdata = root_cgrp;
-		root->top_cgroup.dentry = sb->s_root;
+		dentry->d_fsdata = root_cgrp;
+		root->top_cgroup.dentry = dentry;
 
 		/* Link the top cgroup in this hierarchy into all
 		 * the css_set objects */
@@ -1751,6 +1771,7 @@ static void cgroup_kill_sb(struct super_block *sb) {
 	mutex_unlock(&cgroup_root_mutex);
 	mutex_unlock(&cgroup_mutex);
 
+	kfree(cgrp->name);
 	simple_xattrs_free(&cgrp->xattrs);
 
 	kill_litter_super(sb);
@@ -1774,38 +1795,39 @@ static struct kobject *cgroup_kobj;
  * 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.
+ *
+ *  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)
+int cgroup_path(struct cgroup *cgrp, char *buf, int buflen)
 {
-	struct dentry *dentry = cgrp->dentry;
 	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
-		 */
-		strcpy(buf, "/");
-		return 0;
-	}
-
 	start = buf + buflen - 1;
 
 	*start = '\0';
-	for (;;) {
-		int len = dentry->d_name.len;
+	while (true) {
+		int len;
+		unsigned long flags;
 
-		if ((start -= len) < buf)
+		spin_lock_irqsave(&cgrp->name_lock, flags);
+		len = strlen(cgrp->name);
+		if ((start -= len) < buf) {
+			spin_unlock_irqrestore(&cgrp->name_lock, flags);
 			return -ENAMETOOLONG;
-		memcpy(start, dentry->d_name.name, len);
+		}
+		memcpy(start, cgrp->name, len);
+		spin_unlock_irqrestore(&cgrp->name_lock, flags);
+
 		cgrp = cgrp->parent;
 		if (!cgrp)
 			break;
 
-		dentry = cgrp->dentry;
 		if (!cgrp->parent)
 			continue;
 		if (--start < buf)
@@ -2539,13 +2561,35 @@ 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)
 {
+	unsigned long flags;
+	int ret;
+	char *name;
+	struct cgroup *cgrp;
+
 	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;
+	}
+
+	spin_lock_irqsave(&cgrp->name_lock, flags);
+	kfree(cgrp->name);
+	cgrp->name = name;
+	spin_unlock_irqrestore(&cgrp->name_lock, flags);
+	return 0;
 }
 
 static struct simple_xattrs *__d_xattrs(struct dentry *dentry)
@@ -4144,9 +4188,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;
+
 	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
@@ -4252,6 +4300,8 @@ err_free_all:
 	deactivate_super(sb);
 err_free_id:
 	ida_simple_remove(&root->cgroup_ida, cgrp->id);
+err_free_name:
+	kfree(cgrp->name);
 err_free_cgrp:
 	kfree(cgrp);
 	return err;
@@ -4656,6 +4706,8 @@ int __init cgroup_init_early(void)
 	root_count = 1;
 	init_task.cgroups = &init_css_set;
 
+	dummytop->name = "/";
+
 	init_css_set_link.cg = &init_css_set;
 	init_css_set_link.cgrp = dummytop;
 	list_add(&init_css_set_link.cgrp_link_list,
-- 
1.8.0.2

Re: [PATCH 2/2] cgroup: fix cgroup_path() vs rename() race, take 2

[Al Viro]

On Sun, Feb 17, 2013 at 12:03:37PM +0800, Li Zefan wrote:
> 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_lock and save it in
> cgrp->name when a cgroup is created, and update cgrp->name at
> rename().

Yecchhh...  This is just plain wrong.  If nothing else, your locking
is way too heavy for your uses.  Just put freeing those separately
allocated names in RCU callback and assign them on rename with
rcu_assign_pointer().  Then put cgroup_path() under rcu_read_lock()
(most of the callers already are) and be done with that.

Re: [PATCH 1/2] Revert "cgroup: fix cgroup_path() vs rename() race"

[Tejun Heo]

Hello, Li.

On Sun, Feb 17, 2013 at 12:03:16PM +0800, Li Zefan wrote:
> This reverts commit 299772fab304ab3a36b22b5d28ed81f9408972e7

I didn't push 299772 to 3.8.  I forgot about it for a couple weeks and
then it felt too late and was planning to push it through for-3.9.
I'm just gonna drop for-3.8-fixes and cherr-pick the following two
commits into for-3.9.

 * 7ccff5e0e9 ("cgroup: fix exit() vs rmdir() race")
 * 0844ab1e68 ("cpuset: fix cpuset_print_task_mems_allowed() vs rename() race")

Thanks.

-- 
tejun