race condition in procfs

Hello,

I found a race condition in procfs on SMP systems. The result is an oops in processes like pidof. Apparently ->proc_read() gets passed a potentially NULL pointer.

The following micro-patch seems to fix it.

Quoted 3 lines

I found a race condition in procfs on SMP systems. The result is an oops in processes like pidof. Apparently ->proc_read() gets passed a potentially NULL pointer.

Do you know what the race is?

How does one reproduce it?

Quoted 2 lines

The following micro-patch seems to fix it.

It might be right, or it might be a workaround..

Quoted 2 lines

Do you know what the race is?

Apparently it's a race between deleting a process and accessing its /proc/pid entries. It came out in pidof while it was accessing /proc/pid/stat (fs/proc/array.c:do_task_stat crashed on first instruction - it was an inline function accessing task->state, get_task_state IIRC). oops (with vserver history data - I'm using a patch mentioned below) is attached.

Quoted 2 lines

How does one reproduce it?

I managed to reproduce it (although not reliably) during high CPU load and I/O (parallel kernel compiles) on SMP systems with the vserver patch (http://linux-vserver.org, the exact patch is
http://vserver.13thfloor.at/Experimental/patch-2.6.14.2-vs2.1.0-rc8.diff),
but the vserver maintainer pointed out that it probably is a mainline issue. We're not using 2.6 systems too much except for the vserver test beds so I cannot tell if it happens on vanilla kernels.

Quoted 3 lines

Quoted 2 lines

The following micro-patch seems to fix it.

Quoted 2 lines

It might be right, or it might be a workaround..

I'm not a kernel guru so it's just my proposal. Can it break anything? An alternative _might_ be somewhat coarser task_struct locking (do_task_stat grabs a spinlock but then it's already too late). However, if no "right" solution appears, I'll keep using my two-liner because it seems to help, at least in my setup.

Oh well, I got another oops in the very same place with the patch applied. So now I surrounded the check with read_[un]lock(&tasklist_lock) and added a check to do_task_stat (both now have a printk). If it builds, boots and doesn't crash, I'll post the patch.

2005/11/29, Grzegorz Nosek <[email protected]>:

Quoted 6 lines

Oh well, I got another oops in the very same place with the patch applied. So now I surrounded the check with read_[un]lock(&tasklist_lock) and added a check to do_task_stat (both now have a printk). If it builds, boots and doesn't crash, I'll post the patch.

Froze solid a minute after booting :( netconsole didn't log anything either. Back to the drawing board.

Have you seen this crash the vanilla kernel? What exactly are you doing to see the crash? If you have a script or something, could you post it. I could spend some time helping you debug it too on one of my SMP boxes.

I'm not really using vanilla 2.6 kernels and my setup would be quite hard to run on a vanilla kernel.

The reproduceability of this bug varies. Sometimes it'll go for a few days without happening, sometimes it's a matter of a few minutes. I'm beginning to feel it's a vserver issue after all, somehow related to pid virtualisation (it maps some vxi->vx_initpid to 1).

Thus I cannot provide a simple script to trigger the bug (I wish I could) but often doing a -j8 kernel compile in a vserver is enough.

What you are showing, would probably show up by others if this were a vanilla kernel issue. I don't have an 8 way machine, just 2 way, but the vanilla kernel is being used on many 8 ways out there, so I think you are right that this _is_ a vserver issue.

Unless, of course, that the vserver is producing an obscure race in the vanilla kernel that normal operations would seldom have. Just like the PREEMPT_RT patch has discovered many race conditions that were in the vanilla kernel but were not often a problem.

I'm lost. Any assistance will be invaluable.

OK, Remove your patches, run the system where you can capture the log, and provide a full output of the oops. Make sure you have CONFIG_KALLSYMS set.

2005/11/30, Steven Rostedt <[email protected]>:

Quoted 5 lines

OK, Remove your patches, run the system where you can capture the log, and provide a full output of the oops. Make sure you have CONFIG_KALLSYMS set.

OK, attached an oops from netconsole.

(Andrew, this will be the last email that I include you on. I'm taking you off unless you want to stay on this thread, and say so. I figure that you get enough spam without having to read through this. I'll obviously add you back if this results in a patch.)

Quoted 7 lines

2005/11/30, Steven Rostedt <[email protected]>:

Quoted 5 lines

OK, Remove your patches, run the system where you can capture the log, and provide a full output of the oops. Make sure you have CONFIG_KALLSYMS set.

Quoted 2 lines

OK, attached an oops from netconsole.

The oops happened at address a01b50eb. Could you go into the compiled directory run gdb on vmlinux and type li *0xa01b50eb and show what you get.

Quoted 5 lines

2005/11/30, Steven Rostedt <[email protected]>:

The oops happened at address a01b50eb. Could you go into the compiled directory run gdb on vmlinux and type li *0xa01b50eb and show what you get.

Would the following accomplish the same thing as your patch, to shrink UP dentry structs back to 128 bytes, with a smaller and less intrusive patch?

...
- struct list_head d_child; /* child of parent list */
+ union { /* Fit 32 bit UP dentry in 128 bytes */
+ struct list_head du_child; /* child of parent list */
+ struct rcu_head du_rcu;
+ } d_du;
...

+#define d_child d_du.du_child
+#define d_rcu d_du.du_rcu

Yes, but it's better to just do the big edit, rather than letting these little namespace crufties accumulate over time.

Yes, but it's better to just do the big edit, rather than letting these little namespace crufties accumulate over time.

Even better would be to ditch gcc-2.95.x and use an anonymous union, but Hugh won't let me ;)

Hi Andrew

Could you add this patch to mm ?

Thank you

[PATCH] shrinks dentry struct

Some long time ago, dentry struct was carefully tuned so that on 32 bits UP, sizeof(struct dentry) was exactly 128, ie a power of 2, and a multiple of memory cache lines.

Then RCU was added and dentry struct enlarged by two pointers, with nice results for SMP, but not so good on UP, because breaking the above tuning (128 + 8 = 136 bytes)

This patch reverts this unwanted side effect, by using an union (d_u), where d_rcu and d_child are placed so that these two fields can share their memory needs.

At the time d_free() is called (and d_rcu is really used), d_child is known to be empty and not touched by the dentry freeing.

Lockless lookups only access d_name, d_parent, d_lock, d_op, d_flags (so the previous content of d_child is not needed if said dentry was unhashed but still accessed by a CPU because of RCU constraints)

As dentry cache easily contains millions of entries, a size reduction is worth the extra complexity of the ugly C union.

Signed-off-by Eric Dumazet <[email protected]>

include/linux/dcache.h +7 -2

@@ -95,14 +95,19 @@
 	struct qstr d_name;
 
 	struct list_head d_lru;		/* LRU list */
-	struct list_head d_child;	/* child of parent list */
+	/*
+	 * d_child and d_rcu can share memory
+	 */
+	union {
+		struct list_head d_child;	/* child of parent list */
+	 	struct rcu_head d_rcu;
+	} d_u;
 	struct list_head d_subdirs;	/* our children */
 	struct list_head d_alias;	/* inode alias list */
 	unsigned long d_time;		/* used by d_revalidate */
 	struct dentry_operations *d_op;
 	struct super_block *d_sb;	/* The root of the dentry tree */
 	void *d_fsdata;			/* fs-specific data */
- 	struct rcu_head d_rcu;
 	struct dcookie_struct *d_cookie; /* cookie, if any */
 	int d_mounted;
 	unsigned char d_iname[DNAME_INLINE_LEN_MIN];	/* small names */

fs/dcache.c +17 -17

@@ -71,7 +71,7 @@
 
 static void d_callback(struct rcu_head *head)
 {
-	struct dentry * dentry = container_of(head, struct dentry, d_rcu);
+	struct dentry * dentry = container_of(head, struct dentry, d_u.d_rcu);
 
 	if (dname_external(dentry))
 		kfree(dentry->d_name.name);

@@ -86,7 +86,7 @@
 {
 	if (dentry->d_op && dentry->d_op->d_release)
 		dentry->d_op->d_release(dentry);
- 	call_rcu(&dentry->d_rcu, d_callback);
+ 	call_rcu(&dentry->d_u.d_rcu, d_callback);
 }
 
 /*

@@ -193,7 +193,7 @@
   			list_del(&dentry->d_lru);
   			dentry_stat.nr_unused--;
   		}
-  		list_del(&dentry->d_child);
+  		list_del(&dentry->d_u.d_child);
 		dentry_stat.nr_dentry--;	/* For d_free, below */
 		/*drops the locks, at that point nobody can reach this dentry */
 		dentry_iput(dentry);

@@ -367,7 +367,7 @@
 	struct dentry * parent;
 
 	__d_drop(dentry);
-	list_del(&dentry->d_child);
+	list_del(&dentry->d_u.d_child);
 	dentry_stat.nr_dentry--;	/* For d_free, below */
 	dentry_iput(dentry);
 	parent = dentry->d_parent;

@@ -518,7 +518,7 @@
 resume:
 	while (next != &this_parent->d_subdirs) {
 		struct list_head *tmp = next;
-		struct dentry *dentry = list_entry(tmp, struct dentry, d_child);
+		struct dentry *dentry = list_entry(tmp, struct dentry, d_u.d_child);
 		next = tmp->next;
 		/* Have we found a mount point ? */
 		if (d_mountpoint(dentry))

@@ -532,7 +532,7 @@
 	 * All done at this level ... ascend and resume the search.
 	 */
 	if (this_parent != parent) {
-		next = this_parent->d_child.next; 
+		next = this_parent->d_u.d_child.next; 
 		this_parent = this_parent->d_parent;
 		goto resume;
 	}

@@ -569,7 +569,7 @@
 resume:
 	while (next != &this_parent->d_subdirs) {
 		struct list_head *tmp = next;
-		struct dentry *dentry = list_entry(tmp, struct dentry, d_child);
+		struct dentry *dentry = list_entry(tmp, struct dentry, d_u.d_child);
 		next = tmp->next;
 
 		if (!list_empty(&dentry->d_lru)) {

@@ -610,7 +610,7 @@
 	 * All done at this level ... ascend and resume the search.
 	 */
 	if (this_parent != parent) {
-		next = this_parent->d_child.next; 
+		next = this_parent->d_u.d_child.next; 
 		this_parent = this_parent->d_parent;
 #ifdef DCACHE_DEBUG
 printk(KERN_DEBUG "select_parent: ascending to %s/%s, found=%d\n",

@@ -753,12 +753,12 @@
 		dentry->d_parent = dget(parent);
 		dentry->d_sb = parent->d_sb;
 	} else {
-		INIT_LIST_HEAD(&dentry->d_child);
+		INIT_LIST_HEAD(&dentry->d_u.d_child);
 	}
 
 	spin_lock(&dcache_lock);
 	if (parent)
-		list_add(&dentry->d_child, &parent->d_subdirs);
+		list_add(&dentry->d_u.d_child, &parent->d_subdirs);
 	dentry_stat.nr_dentry++;
 	spin_unlock(&dcache_lock);
 

@@ -1310,8 +1310,8 @@
 	/* Unhash the target: dput() will then get rid of it */
 	__d_drop(target);
 
-	list_del(&dentry->d_child);
-	list_del(&target->d_child);
+	list_del(&dentry->d_u.d_child);
+	list_del(&target->d_u.d_child);
 
 	/* Switch the names.. */
 	switch_names(dentry, target);

@@ -1322,15 +1322,15 @@
 	if (IS_ROOT(dentry)) {
 		dentry->d_parent = target->d_parent;
 		target->d_parent = target;
-		INIT_LIST_HEAD(&target->d_child);
+		INIT_LIST_HEAD(&target->d_u.d_child);
 	} else {
 		do_switch(dentry->d_parent, target->d_parent);
 
 		/* And add them back to the (new) parent lists */
-		list_add(&target->d_child, &target->d_parent->d_subdirs);
+		list_add(&target->d_u.d_child, &target->d_parent->d_subdirs);
 	}
 
-	list_add(&dentry->d_child, &dentry->d_parent->d_subdirs);
+	list_add(&dentry->d_u.d_child, &dentry->d_parent->d_subdirs);
 	spin_unlock(&target->d_lock);
 	spin_unlock(&dentry->d_lock);
 	write_sequnlock(&rename_lock);

@@ -1568,7 +1568,7 @@
 resume:
 	while (next != &this_parent->d_subdirs) {
 		struct list_head *tmp = next;
-		struct dentry *dentry = list_entry(tmp, struct dentry, d_child);
+		struct dentry *dentry = list_entry(tmp, struct dentry, d_u.d_child);
 		next = tmp->next;
 		if (d_unhashed(dentry)||!dentry->d_inode)
 			continue;

@@ -1579,7 +1579,7 @@
 		atomic_dec(&dentry->d_count);
 	}
 	if (this_parent != root) {
-		next = this_parent->d_child.next; 
+		next = this_parent->d_u.d_child.next; 
 		atomic_dec(&this_parent->d_count);
 		this_parent = this_parent->d_parent;
 		goto resume;

fs/autofs4/autofs_i.h +1 -1

@@ -209,7 +209,7 @@
 	struct dentry *child;
 	int ret = 0;
 
-	list_for_each_entry(child, &dentry->d_subdirs, d_child)
+	list_for_each_entry(child, &dentry->d_subdirs, d_u.d_child)
 		if (simple_positive(child))
 			goto out;
 	ret = 1;

fs/autofs4/expire.c +6 -6

@@ -105,7 +105,7 @@
 	next = this_parent->d_subdirs.next;
 resume:
 	while (next != &this_parent->d_subdirs) {
-		struct dentry *dentry = list_entry(next, struct dentry, d_child);
+		struct dentry *dentry = list_entry(next, struct dentry, d_u.d_child);
 
 		/* Negative dentry - give up */
 		if (!simple_positive(dentry)) {

@@ -138,7 +138,7 @@
 	}
 
 	if (this_parent != top) {
-		next = this_parent->d_child.next;
+		next = this_parent->d_u.d_child.next;
 		this_parent = this_parent->d_parent;
 		goto resume;
 	}

@@ -163,7 +163,7 @@
 	next = this_parent->d_subdirs.next;
 resume:
 	while (next != &this_parent->d_subdirs) {
-		struct dentry *dentry = list_entry(next, struct dentry, d_child);
+		struct dentry *dentry = list_entry(next, struct dentry, d_u.d_child);
 
 		/* Negative dentry - give up */
 		if (!simple_positive(dentry)) {

@@ -199,7 +199,7 @@
 	}
 
 	if (this_parent != parent) {
-		next = this_parent->d_child.next;
+		next = this_parent->d_u.d_child.next;
 		this_parent = this_parent->d_parent;
 		goto resume;
 	}

@@ -238,7 +238,7 @@
 	/* On exit from the loop expire is set to a dgot dentry
 	 * to expire or it's NULL */
 	while ( next != &root->d_subdirs ) {
-		struct dentry *dentry = list_entry(next, struct dentry, d_child);
+		struct dentry *dentry = list_entry(next, struct dentry, d_u.d_child);
 
 		/* Negative dentry - give up */
 		if ( !simple_positive(dentry) ) {

@@ -302,7 +302,7 @@
 			expired, (int)expired->d_name.len, expired->d_name.name);
 		spin_lock(&dcache_lock);
 		list_del(&expired->d_parent->d_subdirs);
-		list_add(&expired->d_parent->d_subdirs, &expired->d_child);
+		list_add(&expired->d_parent->d_subdirs, &expired->d_u.d_child);
 		spin_unlock(&dcache_lock);
 		return expired;
 	}

fs/autofs4/inode.c +2 -2

@@ -91,7 +91,7 @@
 	next = this_parent->d_subdirs.next;
 resume:
 	while (next != &this_parent->d_subdirs) {
-		struct dentry *dentry = list_entry(next, struct dentry, d_child);
+		struct dentry *dentry = list_entry(next, struct dentry, d_u.d_child);
 
 		/* Negative dentry - don`t care */
 		if (!simple_positive(dentry)) {

@@ -117,7 +117,7 @@
 	if (this_parent != sbi->root) {
 		struct dentry *dentry = this_parent;
 
-		next = this_parent->d_child.next;
+		next = this_parent->d_u.d_child.next;
 		this_parent = this_parent->d_parent;
 		spin_unlock(&dcache_lock);
 		DPRINTK("parent dentry %p %.*s",

fs/coda/cache.c +0 -0

 		/* don't know what to do with negative dentries */
 		if ( ! de->d_inode ) 
 			continue;

fs/libfs.c +2 -2

 				if (!d_unhashed(next) && next->d_inode)
 					n--;
 				p = p->next;
 			}
-			list_add_tail(&cursor->d_child, p);
+			list_add_tail(&cursor->d_u.d_child, p);
 			spin_unlock(&dcache_lock);
 		}
 	}

@@ -126,7 +126,7 @@
 {
 	struct dentry *dentry = filp->f_dentry;
 	struct dentry *cursor = filp->private_data;
-	struct list_head *p, *q = &cursor->d_child;
+	struct list_head *p, *q = &cursor->d_u.d_child;
 	ino_t ino;
 	int i = filp->f_pos;
 

fs/libfs.c +1 -1

 				if (d_unhashed(next) || !next->d_inode)
 					continue;
 

@@ -261,7 +261,7 @@
 	int ret = 0;
 
 	spin_lock(&dcache_lock);
-	list_for_each_entry(child, &dentry->d_subdirs, d_child)
+	list_for_each_entry(child, &dentry->d_subdirs, d_u.d_child)
 		if (simple_positive(child))
 			goto out;
 	ret = 1;

kernel/cpuset.c +1 -1

 		list_del_init(node);
 		if (d->d_inode) {
 			d = dget_locked(d);

@@ -316,7 +316,7 @@
 		}
 		node = dentry->d_subdirs.next;
 	}
-	list_del_init(&dentry->d_child);
+	list_del_init(&dentry->d_u.d_child);
 	spin_unlock(&dcache_lock);
 	remove_dir(dentry);
 }

drivers/usb/core/inode.c +2 -2

@@ -186,7 +186,7 @@
 
 	down(&bus->d_inode->i_sem);
 
-	list_for_each_entry(dev, &bus->d_subdirs, d_child)
+	list_for_each_entry(dev, &bus->d_subdirs, d_u.d_child)
 		if (dev->d_inode)
 			update_dev(dev);
 

@@ -203,7 +203,7 @@
 
 	down(&root->d_inode->i_sem);
 
-	list_for_each_entry(bus, &root->d_subdirs, d_child) {
+	list_for_each_entry(bus, &root->d_subdirs, d_u.d_child) {
 		if (bus->d_inode) {
 			switch (S_IFMT & bus->d_inode->i_mode) {
 			case S_IFDIR:

drivers/usb/core/inode.c +0 -0

 		if (usbfs_positive(de)) {
 			spin_unlock(&dcache_lock);
 			return 0;

net/sunrpc/rpc_pipe.c +1 -1

@@ -492,7 +492,7 @@
 repeat:
 	spin_lock(&dcache_lock);
 	list_for_each_safe(pos, next, &parent->d_subdirs) {
-		dentry = list_entry(pos, struct dentry, d_child);
+		dentry = list_entry(pos, struct dentry, d_u.d_child);
 		spin_lock(&dentry->d_lock);
 		if (!d_unhashed(dentry)) {
 			dget_locked(dentry);

security/selinux/selinuxfs.c +0 -0

 		list_del_init(node);
 
 		if (d->d_inode) {

Quoted 12 lines · Eric Dumazet

Hi Andrew

Could you add this patch to mm ?

Thank you

[PATCH] shrinks dentry struct

Some long time ago, dentry struct was carefully tuned so that on 32 bits UP, sizeof(struct dentry) was exactly 128, ie a power of 2, and a multiple of memory cache lines.

Then RCU was added and dentry struct enlarged by two pointers, with nice results for SMP, but not so good on UP, because breaking the above tuning (128 + 8 = 136 bytes)

Quoted 7 lines

This patch reverts this unwanted side effect, by using an union (d_u), where d_rcu and d_child are placed so that these two fields can share their memory needs.

At the time d_free() is called (and d_rcu is really used), d_child is known to be empty and not touched by the dentry freeing.

Lockless lookups only access d_name, d_parent, d_lock, d_op, d_flags (so the previous content of d_child is not needed if said dentry was unhashed but still accessed by a CPU because of RCU constraints)

As dentry cache easily contains millions of entries, a size reduction is worth the extra complexity of the ugly C union.

Looks sound to me! Some opportunities for simplification below.

(Please accept my apologies for the delay -- some diversions turned out to be more consuming than I had expected.)

Thanx, Paul