[PATCH v2 0/2] nfs_complete_rename() calls d_move() without i_mutex

[PATCH v2 0/2] nfs_complete_rename() calls d_move() without i_mutex

[Benjamin Coddington]

Once commit 920b4530fb80430ff30ef83efe21ba1fa5623731 "NFS: nfs_rename()
handle -ERESTARTSYS dentry left behind" moved the local d_move() into the
RPC asyncronous context, d_move() could be called without holding the
directories' i_mutex.

Let's revert that commit, and a follow-up fix for it in 1/2, and then fix
the original problem once more by forcing a revalidation of the old and new
directories if we notice that the rename was interrupted in 2/2.

v2:  Add memory barrier, hold directory i_locks while revalidating, specify
     single-bit field width for cancelled flag.

Benjamin Coddington (2):
  Revert "NFS: nfs_rename() handle -ERESTARTSYS dentry left behind"
  NFS: nfs_rename() - revalidate directories on -ERESTARTSYS

 fs/nfs/dir.c            | 50 ++++++++++++++++++++++---------------------------
 fs/nfs/unlink.c         | 11 +++++++++++
 include/linux/nfs_xdr.h |  1 +
 3 files changed, 34 insertions(+), 28 deletions(-)

-- 
2.9.3

[PATCH v2 1/2] Revert "NFS: nfs_rename() handle -ERESTARTSYS dentry left behind"

[Benjamin Coddington]

This reverts commit 920b4530fb80430ff30ef83efe21ba1fa5623731 which could
call d_move() without holding the directory's i_mutex, and reverts commit
d4ea7e3c5c0e341c15b073016dbf3ab6c65f12f3 "NFS: Fix old dentry rehash after
move", which was a follow-up fix.

Fixes: 920b4530fb80 ("NFS: nfs_rename() handle -ERESTARTSYS dentry left behind")
Cc: stable@vger.kernel.org # v4.10+
---
 fs/nfs/dir.c | 45 ++++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 27 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 32ccd7754f8a..1faf337b316f 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1946,29 +1946,6 @@ nfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
 }
 EXPORT_SYMBOL_GPL(nfs_link);
 
-static void
-nfs_complete_rename(struct rpc_task *task, struct nfs_renamedata *data)
-{
-	struct dentry *old_dentry = data->old_dentry;
-	struct dentry *new_dentry = data->new_dentry;
-	struct inode *old_inode = d_inode(old_dentry);
-	struct inode *new_inode = d_inode(new_dentry);
-
-	nfs_mark_for_revalidate(old_inode);
-
-	switch (task->tk_status) {
-	case 0:
-		if (new_inode != NULL)
-			nfs_drop_nlink(new_inode);
-		d_move(old_dentry, new_dentry);
-		nfs_set_verifier(new_dentry,
-					nfs_save_change_attribute(data->new_dir));
-		break;
-	case -ENOENT:
-		nfs_dentry_handle_enoent(old_dentry);
-	}
-}
-
 /*
  * RENAME
  * FIXME: Some nfsds, like the Linux user space nfsd, may generate a
@@ -1999,7 +1976,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 {
 	struct inode *old_inode = d_inode(old_dentry);
 	struct inode *new_inode = d_inode(new_dentry);
-	struct dentry *dentry = NULL;
+	struct dentry *dentry = NULL, *rehash = NULL;
 	struct rpc_task *task;
 	int error = -EBUSY;
 
@@ -2022,8 +1999,10 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 		 * To prevent any new references to the target during the
 		 * rename, we unhash the dentry in advance.
 		 */
-		if (!d_unhashed(new_dentry))
+		if (!d_unhashed(new_dentry)) {
 			d_drop(new_dentry);
+			rehash = new_dentry;
+		}
 
 		if (d_count(new_dentry) > 2) {
 			int err;
@@ -2040,6 +2019,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 				goto out;
 
 			new_dentry = dentry;
+			rehash = NULL;
 			new_inode = NULL;
 		}
 	}
@@ -2048,8 +2028,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (new_inode != NULL)
 		NFS_PROTO(new_inode)->return_delegation(new_inode);
 
-	task = nfs_async_rename(old_dir, new_dir, old_dentry, new_dentry,
-					nfs_complete_rename);
+	task = nfs_async_rename(old_dir, new_dir, old_dentry, new_dentry, NULL);
 	if (IS_ERR(task)) {
 		error = PTR_ERR(task);
 		goto out;
@@ -2059,9 +2038,21 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	if (error == 0)
 		error = task->tk_status;
 	rpc_put_task(task);
+	nfs_mark_for_revalidate(old_inode);
 out:
+	if (rehash)
+		d_rehash(rehash);
 	trace_nfs_rename_exit(old_dir, old_dentry,
 			new_dir, new_dentry, error);
+	if (!error) {
+		if (new_inode != NULL)
+			nfs_drop_nlink(new_inode);
+		d_move(old_dentry, new_dentry);
+		nfs_set_verifier(new_dentry,
+					nfs_save_change_attribute(new_dir));
+	} else if (error == -ENOENT)
+		nfs_dentry_handle_enoent(old_dentry);
+
 	/* new dentry created? */
 	if (dentry)
 		dput(dentry);
-- 
2.9.3

Re: [PATCH v2 1/2] Revert "NFS: nfs_rename() handle -ERESTARTSYS dentry left behind"

[Jeff Layton]

On Thu, 2017-06-15 at 16:59 -0400, Benjamin Coddington wrote:
> This reverts commit 920b4530fb80430ff30ef83efe21ba1fa5623731 which could
> call d_move() without holding the directory's i_mutex, and reverts commit
> d4ea7e3c5c0e341c15b073016dbf3ab6c65f12f3 "NFS: Fix old dentry rehash after
> move", which was a follow-up fix.
> 
> Fixes: 920b4530fb80 ("NFS: nfs_rename() handle -ERESTARTSYS dentry left behind")
> Cc: stable@vger.kernel.org # v4.10+
> ---
>  fs/nfs/dir.c | 45 ++++++++++++++++++---------------------------
>  1 file changed, 18 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 32ccd7754f8a..1faf337b316f 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -1946,29 +1946,6 @@ nfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *dentry)
>  }
>  EXPORT_SYMBOL_GPL(nfs_link);
>  
> -static void
> -nfs_complete_rename(struct rpc_task *task, struct nfs_renamedata *data)
> -{
> -	struct dentry *old_dentry = data->old_dentry;
> -	struct dentry *new_dentry = data->new_dentry;
> -	struct inode *old_inode = d_inode(old_dentry);
> -	struct inode *new_inode = d_inode(new_dentry);
> -
> -	nfs_mark_for_revalidate(old_inode);
> -
> -	switch (task->tk_status) {
> -	case 0:
> -		if (new_inode != NULL)
> -			nfs_drop_nlink(new_inode);
> -		d_move(old_dentry, new_dentry);
> -		nfs_set_verifier(new_dentry,
> -					nfs_save_change_attribute(data->new_dir));
> -		break;
> -	case -ENOENT:
> -		nfs_dentry_handle_enoent(old_dentry);
> -	}
> -}
> -
>  /*
>   * RENAME
>   * FIXME: Some nfsds, like the Linux user space nfsd, may generate a
> @@ -1999,7 +1976,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  {
>  	struct inode *old_inode = d_inode(old_dentry);
>  	struct inode *new_inode = d_inode(new_dentry);
> -	struct dentry *dentry = NULL;
> +	struct dentry *dentry = NULL, *rehash = NULL;
>  	struct rpc_task *task;
>  	int error = -EBUSY;
>  
> @@ -2022,8 +1999,10 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  		 * To prevent any new references to the target during the
>  		 * rename, we unhash the dentry in advance.
>  		 */
> -		if (!d_unhashed(new_dentry))
> +		if (!d_unhashed(new_dentry)) {
>  			d_drop(new_dentry);
> +			rehash = new_dentry;
> +		}
>  
>  		if (d_count(new_dentry) > 2) {
>  			int err;
> @@ -2040,6 +2019,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  				goto out;
>  
>  			new_dentry = dentry;
> +			rehash = NULL;
>  			new_inode = NULL;
>  		}
>  	}
> @@ -2048,8 +2028,7 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (new_inode != NULL)
>  		NFS_PROTO(new_inode)->return_delegation(new_inode);
>  
> -	task = nfs_async_rename(old_dir, new_dir, old_dentry, new_dentry,
> -					nfs_complete_rename);
> +	task = nfs_async_rename(old_dir, new_dir, old_dentry, new_dentry, NULL);
>  	if (IS_ERR(task)) {
>  		error = PTR_ERR(task);
>  		goto out;
> @@ -2059,9 +2038,21 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (error == 0)
>  		error = task->tk_status;
>  	rpc_put_task(task);
> +	nfs_mark_for_revalidate(old_inode);
>  out:
> +	if (rehash)
> +		d_rehash(rehash);
>  	trace_nfs_rename_exit(old_dir, old_dentry,
>  			new_dir, new_dentry, error);
> +	if (!error) {
> +		if (new_inode != NULL)
> +			nfs_drop_nlink(new_inode);
> +		d_move(old_dentry, new_dentry);
> +		nfs_set_verifier(new_dentry,
> +					nfs_save_change_attribute(new_dir));
> +	} else if (error == -ENOENT)
> +		nfs_dentry_handle_enoent(old_dentry);
> +

Could you add a comment about why we need to do the d_move here instead
of in the completion handler? I worry that someone else may make the
same mistake later if we don't leave some breadcrumbs.

>  	/* new dentry created? */
>  	if (dentry)
>  		dput(dentry);

You can add this to both. Thanks for turning this around quickly:

Reviewed-by: Jeff Layton <jlayton@redhat.com>

[PATCH v2 2/2] NFS: nfs_rename() - revalidate directories on -ERESTARTSYS

[Benjamin Coddington]

An interrupted rename will leave the old dentry behind if the rename
succeeds.  Fix this by forcing a lookup the next time through
->d_revalidate.

A previous attempt at solving this problem took the approach to complete
the work of the rename asynchronously, however that approach was wrong
since it would allow the d_move() to occur after the directory's i_mutex
had been dropped by the original process.

v2:  Add memory barrier, hold directory i_locks while revalidating, specify
     single-bit field width for cancelled flag.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/nfs/dir.c            |  5 ++++-
 fs/nfs/unlink.c         | 11 +++++++++++
 include/linux/nfs_xdr.h |  1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 1faf337b316f..94b05d0eec06 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2035,7 +2035,10 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
 	}
 
 	error = rpc_wait_for_completion_task(task);
-	if (error == 0)
+	if (error != 0) {
+		((struct nfs_renamedata *)task->tk_calldata)->cancelled = 1;
+		smp_wmb();
+	} else
 		error = task->tk_status;
 	rpc_put_task(task);
 	nfs_mark_for_revalidate(old_inode);
diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 191aa577dd1f..28628dde38b9 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -288,6 +288,17 @@ static void nfs_async_rename_release(void *calldata)
 	if (d_really_is_positive(data->old_dentry))
 		nfs_mark_for_revalidate(d_inode(data->old_dentry));
 
+	/* The result of the rename is unknown. Play it safe by
+	 * forcing a new lookup */
+	if (data->cancelled) {
+		spin_lock(&data->old_dir->i_lock);
+		nfs_force_lookup_revalidate(data->old_dir);
+		spin_unlock(&data->old_dir->i_lock);
+		spin_lock(&data->new_dir->i_lock);
+		nfs_force_lookup_revalidate(data->new_dir);
+		spin_unlock(&data->new_dir->i_lock);
+	}
+
 	dput(data->old_dentry);
 	dput(data->new_dentry);
 	iput(data->old_dir);
diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
index b28c83475ee8..0c32443d29a7 100644
--- a/include/linux/nfs_xdr.h
+++ b/include/linux/nfs_xdr.h
@@ -1533,6 +1533,7 @@ struct nfs_renamedata {
 	struct nfs_fattr	new_fattr;
 	void (*complete)(struct rpc_task *, struct nfs_renamedata *);
 	long timeout;
+	unsigned int cancelled : 1;
 };
 
 struct nfs_access_entry;
-- 
2.9.3

Re: [PATCH v2 2/2] NFS: nfs_rename() - revalidate directories on -ERESTARTSYS

[Jeff Layton]

On Thu, 2017-06-15 at 16:59 -0400, Benjamin Coddington wrote:
> An interrupted rename will leave the old dentry behind if the rename
> succeeds.  Fix this by forcing a lookup the next time through
> ->d_revalidate.
> 
> A previous attempt at solving this problem took the approach to complete
> the work of the rename asynchronously, however that approach was wrong
> since it would allow the d_move() to occur after the directory's i_mutex
> had been dropped by the original process.
> 
> v2:  Add memory barrier, hold directory i_locks while revalidating, specify
>      single-bit field width for cancelled flag.
> 
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/dir.c            |  5 ++++-
>  fs/nfs/unlink.c         | 11 +++++++++++
>  include/linux/nfs_xdr.h |  1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index 1faf337b316f..94b05d0eec06 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -2035,7 +2035,10 @@ int nfs_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	}
>  
>  	error = rpc_wait_for_completion_task(task);
> -	if (error == 0)
> +	if (error != 0) {
> +		((struct nfs_renamedata *)task->tk_calldata)->cancelled = 1;
> +		smp_wmb();

Looks good, a comment here about how this barrier is paired would be
nice. We'll be scratching our heads on this in another year or two.

> +	} else
>  		error = task->tk_status;
>  	rpc_put_task(task);
>  	nfs_mark_for_revalidate(old_inode);
> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> index 191aa577dd1f..28628dde38b9 100644
> --- a/fs/nfs/unlink.c
> +++ b/fs/nfs/unlink.c
> @@ -288,6 +288,17 @@ static void nfs_async_rename_release(void *calldata)
>  	if (d_really_is_positive(data->old_dentry))
>  		nfs_mark_for_revalidate(d_inode(data->old_dentry));
>  
> +	/* The result of the rename is unknown. Play it safe by
> +	 * forcing a new lookup */
> +	if (data->cancelled) {
> +		spin_lock(&data->old_dir->i_lock);
> +		nfs_force_lookup_revalidate(data->old_dir);
> +		spin_unlock(&data->old_dir->i_lock);
> +		spin_lock(&data->new_dir->i_lock);
> +		nfs_force_lookup_revalidate(data->new_dir);
> +		spin_unlock(&data->new_dir->i_lock);
> +	}
> +
>  	dput(data->old_dentry);
>  	dput(data->new_dentry);
>  	iput(data->old_dir);
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index b28c83475ee8..0c32443d29a7 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -1533,6 +1533,7 @@ struct nfs_renamedata {
>  	struct nfs_fattr	new_fattr;
>  	void (*complete)(struct rpc_task *, struct nfs_renamedata *);
>  	long timeout;
> +	unsigned int cancelled : 1;
> };
>  
>  struct nfs_access_entry;

nitpick:

I'd suggest a bool. You're going to get at least an extra byte on the
allocation anyway. Might as well make it easier to read. We can always
move to bitfields if we need another flag here later.

-- 
Jeff Layton <jlayton@redhat.com>