[PATCH] fix for nfs_unlink and vfs_unlink

Ugh.  nfs_unlink() is actually racy as hell - look what happens if
we enter it with ->d_count == 1, see that nfs_sillyrename() doesn't
need to do anything and call nfs_safe_remove().  In the meanwhile
somebody does dcache lookup (without going into NFS code - plain
and simple cache hit) and increments ->d_count.  nfs_safe_remove()
decides that something is very rotten and fails.

AFAICS we should take the test for ->d_count + d_drop if it's 1
under dcache_lock (and in one place).  Comments?

Proposed fix follows:
	* dget/dput killed in vfs_unlink() (safely)
	* nfs_unlink() starts with check for ->d_count (under dcache_lock)
	* if it's > 1 - sillyrename
	* if it is 1 - immediately unhash, then drop dcache_lock.
		after that we do as in old variant, except that
		rehashing is done in nfs_unlink() and only if there
		was an error - if there was none we simply leave
		d_delete() to vfs_unlink().

fs/namei.c

@@ -1590,21 +1590,16 @@ int vfs_unlink(struct inode *dir, struct dentry *dentry)
 
 	DQUOT_INIT(dir);
 
-	dget(dentry);
 	down(&dentry->d_inode->i_sem);
 	if (d_mountpoint(dentry))
 		error = -EBUSY;
-	else {
+	else
 		error = dir->i_op->unlink(dir, dentry);
-		if (!error)
-			d_delete(dentry);
-	}
 	up(&dentry->d_inode->i_sem);
-	dput(dentry);
-
-	if (!error)
+	if (!error) {
+		d_delete(dentry);
 		inode_dir_notify(dir, DN_DELETE);
-
+	}
 	return error;
 }
 

fs/nfs/dir.c

@@ -772,10 +772,6 @@ static int nfs_sillyrename(struct inode *dir, struct dentry *dentry)
 		dentry->d_parent->d_name.name, dentry->d_name.name, 
 		atomic_read(&dentry->d_count));
 
-	if (atomic_read(&dentry->d_count) == 1)
-		goto out;  /* No need to silly rename. */
-
-
 #ifdef NFS_PARANOIA
 if (!dentry->d_inode)
 printk("NFS: silly-renaming %s/%s, negative dentry??\n",
@@ -842,26 +838,10 @@ static int nfs_safe_remove(struct dentry *dentry)
 	dfprintk(VFS, "NFS: safe_remove(%s/%s)\n",
 		dentry->d_parent->d_name.name, dentry->d_name.name);
 
-	/*
-	 * Unhash the dentry while we remove the file ...
-	 */
-	if (!d_unhashed(dentry)) {
-		d_drop(dentry);
-		rehash = 1;
-	}
-	if (atomic_read(&dentry->d_count) > 1) {
-#ifdef NFS_PARANOIA
-		printk("nfs_safe_remove: %s/%s busy, d_count=%d\n",
-			dentry->d_parent->d_name.name, dentry->d_name.name,
-			atomic_read(&dentry->d_count));
-#endif
-		goto out;
-	}
-
 	/* If the dentry was sillyrenamed, we simply call d_delete() */
 	if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
 		error = 0;
-		goto out_delete;
+		goto out;
 	}
 
 	nfs_zap_caches(dir);
@@ -872,15 +852,7 @@ static int nfs_safe_remove(struct dentry *dentry)
 		goto out;
 	if (inode)
 		inode->i_nlink--;
-
- out_delete:
-	/*
-	 * Free the inode
-	 */
-	d_delete(dentry);
 out:
-	if (rehash)
-		d_rehash(dentry);
 	return error;
 }
 
@@ -892,18 +864,29 @@ static int nfs_safe_remove(struct dentry *dentry)
 static int nfs_unlink(struct inode *dir, struct dentry *dentry)
 {
 	int error;
+	int need_rehash = 0;
 
 	dfprintk(VFS, "NFS: unlink(%s/%ld, %s)\n", dir->i_sb->s_id,
 		dir->i_ino, dentry->d_name.name);
 
 	lock_kernel();
+	spin_lock(&dcache_lock);
+	if (atomic_read(&dentry->d_count) > 1) {
+		spin_unlock(&dcache_lock);
 		error = nfs_sillyrename(dir, dentry);
-	if (error && error != -EBUSY) {
-		error = nfs_safe_remove(dentry);
-		if (!error) {
-			nfs_renew_times(dentry);
+		unlock_kernel();
+		return error;
 	}
+	if (!d_unhashed(dentry)) {
+		d_drop(dentry);
+		need_rehash = 1;
 	}
+	spin_unlock(&dcache_lock);
+	error = nfs_safe_remove(dentry);
+	if (!error)
+		nfs_renew_times(dentry);
+	else if (need_rehash)
+		d_rehash(dentry);
 	unlock_kernel();
 	return error;
 }