Revert "cgroup: fix cgroup_path() vs rename() race"

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.

---
 kernel/cgroup.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

index 776ff75..5d4c92e 100644
@@ -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

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

---
 include/linux/cgroup.h |  4 ++-
 kernel/cgroup.c        | 94 +++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 76 insertions(+), 22 deletions(-)

index 900af59..8b2ce2a 100644
@@ -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);
 

1.8.0.2

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.

Hello, Li.

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