[PATCH RFCv2] NFS: Fix an LOCK/OPEN race when unlinking an open file

[PATCH RFCv2] NFS: Fix an LOCK/OPEN race when unlinking an open file

[Chuck Lever]

At Connectathon 2016, we found that recent upstream Linux clients
would occasionally send a LOCK operation with a zero stateid. This
appeared to happen in close proximity to another thread returning
a delegation before unlinking the same file while it remained open.

Earlier, the client received a write delegation on this file and
returned the open stateid. Now, as it is getting ready to unlink the
file, it returns the write delegation. But there is still an open
file descriptor on that file, so the client must OPEN the file
again before it returns the delegation.

To eliminate this race, serialize delegation return with the
acquisition of a file lock on the same file. Adopt the same approach
as is used in the unlock path.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfs/nfs4proc.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 01bef06..dd435f4 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6054,6 +6054,7 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques
 static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
 {
 	struct nfs_inode *nfsi = NFS_I(state->inode);
+	struct nfs4_state_owner *sp = state->owner;
 	unsigned char fl_flags = request->fl_flags;
 	int status = -ENOLCK;
 
@@ -6065,9 +6066,12 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
 	if (status != 0)
 		goto out;
 	request->fl_flags |= FL_ACCESS;
+	mutex_lock(&sp->so_delegreturn_mutex);
 	status = do_vfs_lock(state->inode, request);
-	if (status < 0)
+	if (status < 0) {
+		mutex_unlock(&sp->so_delegreturn_mutex);
 		goto out;
+	}
 	down_read(&nfsi->rwsem);
 	if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
 		/* Yes: cache locks! */
@@ -6075,9 +6079,11 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
 		request->fl_flags = fl_flags & ~FL_SLEEP;
 		status = do_vfs_lock(state->inode, request);
 		up_read(&nfsi->rwsem);
+		mutex_unlock(&sp->so_delegreturn_mutex);
 		goto out;
 	}
 	up_read(&nfsi->rwsem);
+	mutex_unlock(&sp->so_delegreturn_mutex);
 	status = _nfs4_do_setlk(state, cmd, request, NFS_LOCK_NEW);
 out:
 	request->fl_flags = fl_flags;

Re: [PATCH RFCv2] NFS: Fix an LOCK/OPEN race when unlinking an open file

[Chuck Lever]

> On Mar 28, 2016, at 5:19 PM, Chuck Lever <chuck.lever@oracle.com> wrote:
> 
> At Connectathon 2016, we found that recent upstream Linux clients
> would occasionally send a LOCK operation with a zero stateid. This
> appeared to happen in close proximity to another thread returning
> a delegation before unlinking the same file while it remained open.
> 
> Earlier, the client received a write delegation on this file and
> returned the open stateid. Now, as it is getting ready to unlink the
> file, it returns the write delegation. But there is still an open
> file descriptor on that file, so the client must OPEN the file
> again before it returns the delegation.
> 
> To eliminate this race, serialize delegation return with the
> acquisition of a file lock on the same file. Adopt the same approach
> as is used in the unlock path.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---

I see two new behaviors when this patch is applied:

1. With NFSv4.0, the cthon04 lock test #7 hangs after
"unlock region so child will unblock". ^C then unmount
works as expected.

2. With NFSv4.0, xfstests generic/089 fails because one
of the three test processes is starved out of locking
the test file (t_mtab). The test fails, there are no
other hangs or error messages.

I'm starting to look into the first issue.


> fs/nfs/nfs4proc.c |    8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 01bef06..dd435f4 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -6054,6 +6054,7 @@ static int nfs41_lock_expired(struct nfs4_state *state, struct file_lock *reques
> static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock *request)
> {
> 	struct nfs_inode *nfsi = NFS_I(state->inode);
> +	struct nfs4_state_owner *sp = state->owner;
> 	unsigned char fl_flags = request->fl_flags;
> 	int status = -ENOLCK;
> 
> @@ -6065,9 +6066,12 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
> 	if (status != 0)
> 		goto out;
> 	request->fl_flags |= FL_ACCESS;
> +	mutex_lock(&sp->so_delegreturn_mutex);
> 	status = do_vfs_lock(state->inode, request);
> -	if (status < 0)
> +	if (status < 0) {
> +		mutex_unlock(&sp->so_delegreturn_mutex);
> 		goto out;
> +	}
> 	down_read(&nfsi->rwsem);
> 	if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
> 		/* Yes: cache locks! */
> @@ -6075,9 +6079,11 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
> 		request->fl_flags = fl_flags & ~FL_SLEEP;
> 		status = do_vfs_lock(state->inode, request);
> 		up_read(&nfsi->rwsem);
> +		mutex_unlock(&sp->so_delegreturn_mutex);
> 		goto out;
> 	}
> 	up_read(&nfsi->rwsem);
> +	mutex_unlock(&sp->so_delegreturn_mutex);
> 	status = _nfs4_do_setlk(state, cmd, request, NFS_LOCK_NEW);
> out:
> 	request->fl_flags = fl_flags;
> 

--
Chuck Lever