Re: [PATCH 3/3] nfsd4: fix delegation-unlink/rename race

["J. Bruce Fields" <bfields@fieldses.org>]

On Sun, Jan 26, 2014 at 03:59:21PM -0500, J. Bruce Fields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> If a file is unlinked or renamed between the time when we do the local
> open and the time when we get the delegation, then we will return to the
> client indicating that it holds a delegation even though the file no
> longer exists under the name it was open under.
> 
> But a client performing an open-by-name, when it is returned a
> delegation, must be able to assume that the file is still linked at the
> name it was opened under.
> 
> So, hold the parent i_mutex for longer to prevent concurrent renames or
> unlinks.
> 
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
>  fs/nfsd/nfs4proc.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 844813a..ef76ba6 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -279,11 +279,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
>  		if (open->op_createmode == NFS4_CREATE_EXCLUSIVE && status == 0)
>  			open->op_bmval[1] = (FATTR4_WORD1_TIME_ACCESS |
>  							FATTR4_WORD1_TIME_MODIFY);
> -	} else {
> +	} else
> +		/*
> +		 * Note this may exit with the parent still locked.
> +		 * We will hold the lock until nfsd4_open's final
> +		 * lookup, to prevent renames or unlinks until we've had
> +		 * a chance to an acquire a delegation if appropriate.
> +		 */
>  		status = nfsd_lookup(rqstp, current_fh,
>  				     open->op_fname.data, open->op_fname.len, *resfh);
> -		fh_unlock(current_fh);
> -	}
>  	if (status)
>  		goto out;
>  	status = nfsd_check_obj_isreg(*resfh);

One last-minute fix: we can now end up taking two i_mutexes.  The
locking's still correct but we need the following annotation on the
parent directory.

(I haven't actually seen any lockdep warning trigger here.)

--b.

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index e85b463..a41302a 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -207,7 +207,12 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				goto out_nfserr;
 		}
 	} else {
-		fh_lock(fhp);
+		/*
+		 * In the nfsd4_open() case, this may be held across
+		 * subsequent open and delegation acquisition which may
+		 * need to take the child's i_mutex:
+		 */
+		fh_lock_nested(fhp, I_MUTEX_PARENT);
 		dentry = lookup_one_len(name, dparent, len);
 		host_err = PTR_ERR(dentry);
 		if (IS_ERR(dentry))