[PATCH] nfs: don't use d_move in nfs_async_rename_done

[PATCH] nfs: don't use d_move in nfs_async_rename_done

[Jeff Layton]

If the task that initiated the sillyrename ends up being killed by a
fatal signal, then it will eventually return back to userspace and end
up releasing the i_mutex. d_move however needs to be done while holding
the i_mutex.

Instead of using d_move here, just unhash the old and new dentries to
prevent them from being found by lookups. With this change though, the
dentries are now incorrect post-rename and do not reflect the actual
name of the file on the server. I'm proceeding under the assumption
that since they are unhashed that this isn't really a problem.

In order for the sillydelete to still work though, the dname must be
copied earlier when setting up the sillydelete info, and the name must
be recopied if the sillydelete info has to be moved to a new dentry.

Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 fs/nfs/unlink.c |   29 +++++++++++++++++++----------
 1 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 8d6864c..7687f4f 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -147,7 +147,7 @@ static int nfs_do_call_unlink(struct dentry *parent, struct inode *dir, struct n
 
 	alias = d_lookup(parent, &data->args.name);
 	if (alias != NULL) {
-		int ret = 0;
+		int ret;
 		void *devname_garbage = NULL;
 
 		/*
@@ -155,14 +155,16 @@ static int nfs_do_call_unlink(struct dentry *parent, struct inode *dir, struct n
 		 * the sillyrename information to the aliased dentry.
 		 */
 		nfs_free_dname(data);
+		ret = nfs_copy_dname(alias, data);
 		spin_lock(&alias->d_lock);
-		if (alias->d_inode != NULL &&
+		if (ret == 0 && alias->d_inode != NULL &&
 		    !(alias->d_flags & DCACHE_NFSFS_RENAMED)) {
 			devname_garbage = alias->d_fsdata;
 			alias->d_fsdata = data;
 			alias->d_flags |= DCACHE_NFSFS_RENAMED;
 			ret = 1;
-		}
+		} else
+			ret = 0;
 		spin_unlock(&alias->d_lock);
 		nfs_dec_sillycount(dir);
 		dput(alias);
@@ -171,8 +173,7 @@ static int nfs_do_call_unlink(struct dentry *parent, struct inode *dir, struct n
 		 * point dentry is definitely not a root, so we won't need
 		 * that anymore.
 		 */
-		if (devname_garbage)
-			kfree(devname_garbage);
+		kfree(devname_garbage);
 		return ret;
 	}
 	data->dir = igrab(dir);
@@ -204,8 +205,6 @@ static int nfs_call_unlink(struct dentry *dentry, struct nfs_unlinkdata *data)
 	if (parent == NULL)
 		goto out_free;
 	dir = parent->d_inode;
-	if (nfs_copy_dname(dentry, data) != 0)
-		goto out_dput;
 	/* Non-exclusive lock protects against concurrent lookup() calls */
 	spin_lock(&dir->i_lock);
 	if (atomic_inc_not_zero(&NFS_I(dir)->silly_count) == 0) {
@@ -366,6 +365,8 @@ static void nfs_async_rename_done(struct rpc_task *task, void *calldata)
 	struct nfs_renamedata *data = calldata;
 	struct inode *old_dir = data->old_dir;
 	struct inode *new_dir = data->new_dir;
+	struct dentry *old_dentry = data->old_dentry;
+	struct dentry *new_dentry = data->new_dentry;
 
 	if (!NFS_PROTO(old_dir)->rename_done(task, old_dir, new_dir)) {
 		nfs_restart_rpc(task, NFS_SERVER(old_dir)->nfs_client);
@@ -373,12 +374,12 @@ static void nfs_async_rename_done(struct rpc_task *task, void *calldata)
 	}
 
 	if (task->tk_status != 0) {
-		nfs_cancel_async_unlink(data->old_dentry);
+		nfs_cancel_async_unlink(old_dentry);
 		return;
 	}
 
-	nfs_set_verifier(data->old_dentry, nfs_save_change_attribute(old_dir));
-	d_move(data->old_dentry, data->new_dentry);
+	d_drop(old_dentry);
+	d_drop(new_dentry);
 }
 
 /**
@@ -560,6 +561,14 @@ nfs_sillyrename(struct inode *dir, struct dentry *dentry)
 	if (error)
 		goto out_dput;
 
+	/* populate unlinkdata with the right dname */
+	error = nfs_copy_dname(sdentry,
+				(struct nfs_unlinkdata *)dentry->d_fsdata);
+	if (error) {
+		nfs_cancel_async_unlink(dentry);
+		goto out_dput;
+	}
+
 	/* run the rename task, undo unlink if it fails */
 	task = nfs_async_rename(dir, dir, dentry, sdentry);
 	if (IS_ERR(task)) {
-- 
1.7.6

Re: [PATCH] nfs: don't use d_move in nfs_async_rename_done

[Trond Myklebust]

On Mon, 2011-07-18 at 11:26 -0400, Jeff Layton wrote: 
> If the task that initiated the sillyrename ends up being killed by a
> fatal signal, then it will eventually return back to userspace and end
> up releasing the i_mutex. d_move however needs to be done while holding
> the i_mutex.

Umm... Where is this requirement documented? I thought the rename_lock
was there to protect against lookup races etc with d_move.

Besides, NFS already has
nfs_block_sillyrename()/nfs_unblock_sillyrename() to provide further
exclusion between dentry lookups and revalidations and the silly-unlink
code.

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH] nfs: don't use d_move in nfs_async_rename_done

[Al Viro]

On Mon, Jul 18, 2011 at 01:35:14PM -0400, Trond Myklebust wrote:
> On Mon, 2011-07-18 at 11:26 -0400, Jeff Layton wrote: 
> > If the task that initiated the sillyrename ends up being killed by a
> > fatal signal, then it will eventually return back to userspace and end
> > up releasing the i_mutex. d_move however needs to be done while holding
> > the i_mutex.
> 
> Umm... Where is this requirement documented? I thought the rename_lock
> was there to protect against lookup races etc with d_move.

It protects lookup against d_move().  It does *NOT* protect the i_mutex
locking scheme from deadlocks a-sodding-plenty and it does not protect
->d_parent/->d_name accesses in directory methods (->i_mutex does).  The
latter is not a big deal, but the former is.

> Besides, NFS already has
> nfs_block_sillyrename()/nfs_unblock_sillyrename() to provide further
> exclusion between dentry lookups and revalidations and the silly-unlink
> code.

It's broken.  We are dealing with more than just NFS data structures.
Don't change ->d_parent unless you hold ->i_mutex on parent(s) involved
and if they are different you need ->s_vfs_rename_mutex as well.  See
lock_rename() in fs/namei.c and Documentation/filesystems/directory-locking.

Moreover, I would be very sceptical about the code trying to grap ->i_mutex
on ->d_parent of preexisting dentry, unless you have very good reasons to
be sure that it couldn't be moved around in the meanwhile.

d_move() in async rename is really broken...