Re: PROBLEM: /proc (procfs) task exit race condition causes a kernel crash

[Tony Griffiths <tonyg@agile.tv>]

Eric,

I've attached a patch file which rolls up a set of patches to dcache.c 
along with a few changed I made to locking symantics.  So far a 2.6.16 
(+ -mm2) with the patch applied survives the harshest testing I can 
throw at it!

Note that I've also made some minor changes to exit.c [preempt 
disable/enable during task exit] and truncate.c [BUG_ON check of 
'private' page].  The testing I've done is with a kernel built without 
preemption, and one built with voluntary preemption.  A kernel built 
with forced preemption and with spinlock debugging enabled did NOT work 
very well!!!  Also, the performance of a kernel  built with voluntary 
preemption and with the cond_resched_lock() calls in dcache.c was 
surprisingly *BAD*, with the system going into a wheel-spin for a long 
period [high system cpu of 87%+] when I fired up a number of large 
cpu+memory hungry tasks.  This might need to be looked at more closely?!

Eric W. Biederman wrote:

> I have tried to reproduce this.  The circumstances weren't the most
> controlled but they did overlap with what you described and I haven't seen
> anything.
>
> So I am guessing that you are having memory corruption from some source.
> Either bad ram or a bad module.
>
> I'm off on vacation for a week, so I won't be able to follow up.
>
>
> Eric
>  

diff -urpN linux-2.6.16-mm2/fs/dcache.c linux-2.6.16/fs/dcache.c
--- linux-2.6.16-mm2/fs/dcache.c	2006-05-31 16:30:13.000000000 +1000
+++ linux-2.6.16/fs/dcache.c	2006-05-31 16:25:53.000000000 +1000
@@ -36,12 +36,10 @@
 
 
 int sysctl_vfs_cache_pressure __read_mostly = 100;
-EXPORT_SYMBOL_GPL(sysctl_vfs_cache_pressure);
 
  __cacheline_aligned_in_smp DEFINE_SPINLOCK(dcache_lock);
 static seqlock_t rename_lock __cacheline_aligned_in_smp = SEQLOCK_UNLOCKED;
 
-EXPORT_SYMBOL(dcache_lock);
 
 static kmem_cache_t *dentry_cache __read_mostly;
 
@@ -142,21 +140,18 @@ static void dentry_iput(struct dentry * 
  * no dcache lock, please.
  */
 
-void dput(struct dentry *dentry)
+static void dput_locked(struct dentry *dentry, struct list_head *list)
 {
 	if (!dentry)
 		return;
 
-repeat:
-	if (atomic_read(&dentry->d_count) == 1)
-		might_sleep();
-	if (!atomic_dec_and_lock(&dentry->d_count, &dcache_lock))
+	if (!atomic_dec_and_test(&dentry->d_count))
 		return;
 
+repeat:
 	spin_lock(&dentry->d_lock);
 	if (atomic_read(&dentry->d_count)) {
 		spin_unlock(&dentry->d_lock);
-		spin_unlock(&dcache_lock);
 		return;
 	}
 
@@ -176,33 +171,59 @@ repeat:
   		dentry_stat.nr_unused++;
   	}
  	spin_unlock(&dentry->d_lock);
-	spin_unlock(&dcache_lock);
 	return;
 
 unhash_it:
 	__d_drop(dentry);
 
 kill_it: {
-		struct dentry *parent;
-
 		/* If dentry was on d_lru list
 		 * delete it from there
 		 */
   		if (!list_empty(&dentry->d_lru)) {
-  			list_del(&dentry->d_lru);
+  			list_del_init(&dentry->d_lru);
   			dentry_stat.nr_unused--;
   		}
   		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);
-		parent = dentry->d_parent;
-		d_free(dentry);
-		if (dentry == parent)
+		/* at this point nobody can reach this dentry */
+		list_add(&dentry->d_lru, list);
+		spin_unlock(&dentry->d_lock);
+		if (dentry == dentry->d_parent)
 			return;
-		dentry = parent;
-		goto repeat;
+		dentry = dentry->d_parent;
+		if (atomic_dec_and_test(&dentry->d_count))
+			goto repeat;
+		/* out */
+	}
+}
+
+void dput(struct dentry *dentry)
+{
+	LIST_HEAD(free_list);
+
+	if (!dentry)
+		goto do_return;
+
+	if (atomic_add_unless(&dentry->d_count, -1, 1))
+		goto do_return;
+
+	spin_lock(&dcache_lock);			/* While we hold the dcache_lock */
+	dput_locked(dentry, &free_list);		/* Put ALL free-able dentry's onto 'free_list' */
+
+	if (!list_empty(&free_list)) {			/* Then process as a single batch! */
+		struct dentry *dentry, *p;
+		list_for_each_entry_safe(dentry, p, &free_list, d_lru) {
+			spin_lock(&dentry->d_lock);	/* Lock dentry while also holding dcache_lock! */
+			list_del(&dentry->d_lru);
+			dentry_iput(dentry);		/* Enter with locks held; Exit with no locks! */
+			d_free(dentry);
+			spin_lock(&dcache_lock);	/* Assume we will iterate again so ... */
+		}
 	}
+	spin_unlock(&dcache_lock);			/* There *MUST* be a better way of doing this?! */
+do_return:
+	return;
 }
 
 /**
@@ -219,13 +240,15 @@ kill_it: {
  
 int d_invalidate(struct dentry * dentry)
 {
+	int	ret = 0;
+
 	/*
 	 * If it's already been dropped, return OK.
 	 */
 	spin_lock(&dcache_lock);
 	if (d_unhashed(dentry)) {
 		spin_unlock(&dcache_lock);
-		return 0;
+		goto do_return;
 	}
 	/*
 	 * Check whether to do a partial shrink_dcache
@@ -252,14 +275,16 @@ int d_invalidate(struct dentry * dentry)
 		if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode)) {
 			spin_unlock(&dentry->d_lock);
 			spin_unlock(&dcache_lock);
-			return -EBUSY;
+			ret = -EBUSY;
+			goto do_return;
 		}
 	}
 
 	__d_drop(dentry);
 	spin_unlock(&dentry->d_lock);
 	spin_unlock(&dcache_lock);
-	return 0;
+do_return:
+	return ret;
 }
 
 /* This should be called _only_ with dcache_lock held */
@@ -276,7 +301,10 @@ static inline struct dentry * __dget_loc
 
 struct dentry * dget_locked(struct dentry *dentry)
 {
-	return __dget_locked(dentry);
+	struct dentry*	ret;
+
+	ret = __dget_locked(dentry);
+	return ret;
 }
 
 /**
@@ -366,22 +394,39 @@ restart:
  */
 static inline void prune_one_dentry(struct dentry * dentry)
 {
-	struct dentry * parent;
+	LIST_HEAD(free_list);
 
 	__d_drop(dentry);
 	list_del(&dentry->d_u.d_child);
 	dentry_stat.nr_dentry--;	/* For d_free, below */
-	dentry_iput(dentry);
-	parent = dentry->d_parent;
+
+	/* dput the parent here before we release dcache_lock */
+	if (dentry != dentry->d_parent)
+		dput_locked(dentry->d_parent, &free_list);
+
+	dentry_iput(dentry);		/* drop locks */
 	d_free(dentry);
-	if (parent != dentry)
-		dput(parent);
+
+	if (!list_empty(&free_list)) {
+		struct dentry *tmp, *p;
+
+		list_for_each_entry_safe(tmp, p, &free_list, d_lru) {
+			spin_lock(&dcache_lock);	/* All of this locking/unlocking  */
+			spin_lock(&tmp->d_lock);	/* is so incredibly UGLY!!! */
+			list_del(&tmp->d_lru);
+			dentry_iput(tmp);
+			d_free(tmp);
+		}
+	}
+
 	spin_lock(&dcache_lock);
 }
 
 /**
  * prune_dcache - shrink the dcache
  * @count: number of entries to try and free
+ * @sb: if given, ignore dentries for other superblocks
+ *         which are being unmounted.
  *
  * Shrink the dcache. This is done when we need
  * more memory, or simply when we need to unmount
@@ -392,16 +437,30 @@ static inline void prune_one_dentry(stru
  * all the dentries are in use.
  */
  
-static void prune_dcache(int count)
+static void prune_dcache(int count, struct super_block *sb)
 {
 	spin_lock(&dcache_lock);
 	for (; count ; count--) {
 		struct dentry *dentry;
 		struct list_head *tmp;
+		struct rw_semaphore *s_umount;
 
-		cond_resched_lock(&dcache_lock);
+		/*cond_resched_lock(&dcache_lock);	** ?BAD PERFORMANCE? **   */
 
 		tmp = dentry_unused.prev;
+		if (unlikely(sb)) {
+			/* Try to find a dentry for this sb, but don't try
+			 * too hard, if they aren't near the tail they will
+			 * be moved down again soon
+			 */
+			int skip = count;
+			while (skip &&
+			       tmp != &dentry_unused &&
+			       list_entry(tmp, struct dentry, d_lru)->d_sb != sb) {
+				skip--;
+				tmp = tmp->prev;
+			}
+		}
 		if (tmp == &dentry_unused)
 			break;
 		list_del_init(tmp);
@@ -427,7 +486,45 @@ static void prune_dcache(int count)
  			spin_unlock(&dentry->d_lock);
 			continue;
 		}
-		prune_one_dentry(dentry);
+		/*
+		 * If the dentry is not DCACHED_REFERENCED, it is time
+		 * to remove it from the dcache, provided the super block is
+		 * NULL (which means we are trying to reclaim memory)
+		 * or this dentry belongs to the same super block that
+		 * we want to shrink.
+		 */
+		/*
+		 * If this dentry is for "my" filesystem, then I can prune it
+		 * without taking the s_umount lock (I already hold it).
+		 */
+		if (sb && dentry->d_sb == sb) {
+			prune_one_dentry(dentry);
+			continue;
+		}
+		/*
+		 * ...otherwise we need to be sure this filesystem isn't being
+		 * unmounted, otherwise we could race with
+		 * generic_shutdown_super(), and end up holding a reference to
+		 * an inode while the filesystem is unmounted.
+		 * So we try to get s_umount, and make sure s_root isn't NULL.
+		 * (Take a local copy of s_umount to avoid a use-after-free of
+		 * `dentry').
+		 */
+		s_umount = &dentry->d_sb->s_umount;
+		if (down_read_trylock(s_umount)) {
+			if (dentry->d_sb->s_root != NULL) {
+				prune_one_dentry(dentry);
+				up_read(s_umount);
+				continue;
+			}
+			up_read(s_umount);
+		}
+		spin_unlock(&dentry->d_lock);
+		/* Cannot remove the first dentry, and it isn't appropriate
+		 * to move it to the head of the list, so give up, and try
+		 * later
+		 */
+		break;
 	}
 	spin_unlock(&dcache_lock);
 }
@@ -481,14 +578,14 @@ repeat:
 		if (dentry->d_sb != sb)
 			continue;
 		dentry_stat.nr_unused--;
-		list_del_init(tmp);
 		spin_lock(&dentry->d_lock);
+		list_del_init(tmp);
 		if (atomic_read(&dentry->d_count)) {
 			spin_unlock(&dentry->d_lock);
 			continue;
 		}
 		prune_one_dentry(dentry);
-		cond_resched_lock(&dcache_lock);
+		/*cond_resched_lock(&dcache_lock);	** ?BAD PERFORMANCE? **   */
 		goto repeat;
 	}
 	spin_unlock(&dcache_lock);
@@ -512,6 +609,7 @@ int have_submounts(struct dentry *parent
 {
 	struct dentry *this_parent = parent;
 	struct list_head *next;
+	int	ret = 1;
 
 	spin_lock(&dcache_lock);
 	if (d_mountpoint(parent))
@@ -539,11 +637,10 @@ resume:
 		this_parent = this_parent->d_parent;
 		goto resume;
 	}
-	spin_unlock(&dcache_lock);
-	return 0; /* No mount points found in tree */
+	ret = 0; /* No mount points found in tree */
 positive:
 	spin_unlock(&dcache_lock);
-	return 1;
+	return ret;
 }
 
 /*
@@ -630,7 +727,7 @@ void shrink_dcache_parent(struct dentry 
 	int found;
 
 	while ((found = select_parent(parent)) != 0)
-		prune_dcache(found);
+		prune_dcache(found, parent->d_sb);
 }
 
 /**
@@ -643,9 +740,10 @@ void shrink_dcache_parent(struct dentry 
  * done under dcache_lock.
  *
  */
-void shrink_dcache_anon(struct hlist_head *head)
+void shrink_dcache_anon(struct super_block *sb)
 {
 	struct hlist_node *lp;
+	struct hlist_head *head = &sb->s_anon;
 	int found;
 	do {
 		found = 0;
@@ -668,7 +766,7 @@ void shrink_dcache_anon(struct hlist_hea
 			}
 		}
 		spin_unlock(&dcache_lock);
-		prune_dcache(found);
+		prune_dcache(found, sb);
 	} while(found);
 }
 
@@ -686,12 +784,16 @@ void shrink_dcache_anon(struct hlist_hea
  */
 static int shrink_dcache_memory(int nr, gfp_t gfp_mask)
 {
+	int	ret = -1;
+
 	if (nr) {
 		if (!(gfp_mask & __GFP_FS))
-			return -1;
-		prune_dcache(nr);
+			goto do_return;
+		prune_dcache(nr, NULL);
 	}
-	return (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
+	ret = (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
+do_return:
+	return ret;
 }
 
 /**
@@ -711,13 +813,14 @@ struct dentry *d_alloc(struct dentry * p
 
 	dentry = kmem_cache_alloc(dentry_cache, GFP_KERNEL); 
 	if (!dentry)
-		return NULL;
+		goto do_return;
 
 	if (name->len > DNAME_INLINE_LEN-1) {
 		dname = kmalloc(name->len + 1, GFP_KERNEL);
 		if (!dname) {
 			kmem_cache_free(dentry_cache, dentry); 
-			return NULL;
+			dentry = NULL;
+			goto do_return;
 		}
 	} else  {
 		dname = dentry->d_iname;
@@ -758,18 +861,20 @@ struct dentry *d_alloc(struct dentry * p
 		list_add(&dentry->d_u.d_child, &parent->d_subdirs);
 	dentry_stat.nr_dentry++;
 	spin_unlock(&dcache_lock);
-
+do_return:
 	return dentry;
 }
 
 struct dentry *d_alloc_name(struct dentry *parent, const char *name)
 {
 	struct qstr q;
+	struct dentry * ret;
 
 	q.name = name;
 	q.len = strlen(name);
 	q.hash = full_name_hash(q.name, q.len);
-	return d_alloc(parent, &q);
+	ret = d_alloc(parent, &q);
+	return ret;
 }
 
 /**
@@ -817,7 +922,7 @@ void d_instantiate(struct dentry *entry,
  */
 struct dentry *d_instantiate_unique(struct dentry *entry, struct inode *inode)
 {
-	struct dentry *alias;
+	struct dentry *alias = NULL;
 	int len = entry->d_name.len;
 	const char *name = entry->d_name.name;
 	unsigned int hash = entry->d_name.hash;
@@ -841,7 +946,7 @@ struct dentry *d_instantiate_unique(stru
 		spin_unlock(&dcache_lock);
 		BUG_ON(!d_unhashed(alias));
 		iput(inode);
-		return alias;
+		goto do_return;
 	}
 	list_add(&entry->d_alias, &inode->i_dentry);
 do_negative:
@@ -849,9 +954,10 @@ do_negative:
 	fsnotify_d_instantiate(entry, inode);
 	spin_unlock(&dcache_lock);
 	security_d_instantiate(entry, inode);
-	return NULL;
+	alias = NULL;
+do_return:
+	return alias;
 }
-EXPORT_SYMBOL(d_instantiate_unique);
 
 /**
  * d_alloc_root - allocate root dentry
@@ -915,12 +1021,14 @@ struct dentry * d_alloc_anon(struct inod
 
 	if ((res = d_find_alias(inode))) {
 		iput(inode);
-		return res;
+		goto do_return;
 	}
 
 	tmp = d_alloc(NULL, &anonstring);
-	if (!tmp)
-		return NULL;
+	if (!tmp) {
+		res = NULL;
+		goto do_return;
+	}
 
 	tmp->d_parent = tmp; /* make sure dput doesn't croak */
 	
@@ -948,6 +1056,7 @@ struct dentry * d_alloc_anon(struct inod
 		iput(inode);
 	if (tmp)
 		dput(tmp);
+do_return:
 	return res;
 }
 
@@ -1142,6 +1251,7 @@ int d_validate(struct dentry *dentry, st
 {
 	struct hlist_head *base;
 	struct hlist_node *lhp;
+	int		   ret = 0;
 
 	/* Check whether the ptr might be valid at all.. */
 	if (!kmem_ptr_validate(dentry_cache, dentry))
@@ -1159,12 +1269,13 @@ int d_validate(struct dentry *dentry, st
 		if (dentry == hlist_entry(lhp, struct dentry, d_hash)) {
 			__dget_locked(dentry);
 			spin_unlock(&dcache_lock);
-			return 1;
+			ret = 1;
+			goto out;
 		}
 	}
 	spin_unlock(&dcache_lock);
 out:
-	return 0;
+	return ret;
 }
 
 /*
@@ -1191,6 +1302,7 @@ out:
 void d_delete(struct dentry * dentry)
 {
 	int isdir = 0;
+
 	/*
 	 * Are we the only user?
 	 */
@@ -1203,7 +1315,7 @@ void d_delete(struct dentry * dentry)
 
 		dentry_iput(dentry);
 		fsnotify_nameremove(dentry, isdir);
-		return;
+		goto do_return;
 	}
 
 	if (!d_unhashed(dentry))
@@ -1213,6 +1325,8 @@ void d_delete(struct dentry * dentry)
 	spin_unlock(&dcache_lock);
 
 	fsnotify_nameremove(dentry, isdir);
+do_return:
+	return;
 }
 
 static void __d_rehash(struct dentry * entry, struct hlist_head *list)
@@ -1497,13 +1611,13 @@ char * d_path(struct dentry *dentry, str
  */
 asmlinkage long sys_getcwd(char __user *buf, unsigned long size)
 {
-	int error;
+	int error = -ENOMEM;
 	struct vfsmount *pwdmnt, *rootmnt;
 	struct dentry *pwd, *root;
 	char *page = (char *) __get_free_page(GFP_USER);
 
 	if (!page)
-		return -ENOMEM;
+		goto do_return;
 
 	read_lock(&current->fs->lock);
 	pwdmnt = mntget(current->fs->pwdmnt);
@@ -1542,6 +1656,7 @@ out:
 	dput(root);
 	mntput(rootmnt);
 	free_page((unsigned long) page);
+do_return:
 	return error;
 }
 
@@ -1729,7 +1844,6 @@ kmem_cache_t *names_cachep __read_mostly
 /* SLAB cache for file structures */
 kmem_cache_t *filp_cachep __read_mostly;
 
-EXPORT_SYMBOL(d_genocide);
 
 extern void bdev_cache_init(void);
 extern void chrdev_init(void);
@@ -1764,6 +1878,10 @@ void __init vfs_caches_init(unsigned lon
 	chrdev_init();
 }
 
+EXPORT_SYMBOL_GPL(sysctl_vfs_cache_pressure);
+EXPORT_SYMBOL(dcache_lock);
+EXPORT_SYMBOL(d_instantiate_unique);
+EXPORT_SYMBOL(d_genocide);
 EXPORT_SYMBOL(d_alloc);
 EXPORT_SYMBOL(d_alloc_anon);
 EXPORT_SYMBOL(d_alloc_root);
diff -urpN linux-2.6.16-mm2/kernel/exit.c linux-2.6.16/kernel/exit.c
--- linux-2.6.16-mm2/kernel/exit.c	2006-05-31 16:30:14.000000000 +1000
+++ linux-2.6.16/kernel/exit.c	2006-05-30 14:49:35.000000000 +1000
@@ -136,6 +136,7 @@ void release_task(struct task_struct * p
 {
 	int zap_leader;
 	task_t *leader;
+	preempt_disable();	// ** Cleanup as fast as we can! **
 repeat:
 	atomic_dec(&p->user->processes);
 	write_lock_irq(&tasklist_lock);
@@ -173,6 +174,7 @@ repeat:
 	p = leader;
 	if (unlikely(zap_leader))
 		goto repeat;
+	preempt_enable();	// ** OK to give other tasks some cycles now **
 }
 
 /*
diff -urpN linux-2.6.16-mm2/mm/truncate.c linux-2.6.16/mm/truncate.c
--- linux-2.6.16-mm2/mm/truncate.c	2006-05-31 16:30:14.000000000 +1000
+++ linux-2.6.16/mm/truncate.c	2006-05-28 17:22:46.000000000 +1000
@@ -80,7 +80,7 @@ invalidate_complete_page(struct address_
 		return 0;
 	}
 
-	BUG_ON(PagePrivate(page));
+	BUG_ON(PagePrivate(page) && (page_private(page) != 0));
 	__remove_from_page_cache(page);
 	write_unlock_irq(&mapping->tree_lock);
 	ClearPageUptodate(page);