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.
Since commit 24311f884189 ('NFSv4: Recovery of recalled read
delegations is broken'), nfs_open_delegation_recall() clears the
NFS_DELEGATED_STATE flag _before_ it sends the OPEN. This allows a
racing LOCK on the same inode to be put on the wire before the OPEN
operation has returned a valid open stateid.
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.
Fixes: 24311f884189 ('NFSv4: Recovery of recalled read ... ')
Signed-off-by: Chuck Lever <[email protected]>
---
fs/nfs/nfs4proc.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 01bef06..c40f1b6 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;
@@ -6068,6 +6069,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
status = do_vfs_lock(state->inode, request);
if (status < 0)
goto out;
+ mutex_lock(&sp->so_delegreturn_mutex);
down_read(&nfsi->rwsem);
if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
/* Yes: cache locks! */
@@ -6075,9 +6077,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;
> On Mar 31, 2016, at 12:38 PM, Chuck Lever <[email protected]> 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.
>
> Since commit 24311f884189 ('NFSv4: Recovery of recalled read
> delegations is broken'), nfs_open_delegation_recall() clears the
> NFS_DELEGATED_STATE flag _before_ it sends the OPEN. This allows a
> racing LOCK on the same inode to be put on the wire before the OPEN
> operation has returned a valid open stateid.
>
> 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.
>
> Fixes: 24311f884189 ('NFSv4: Recovery of recalled read ... ')
> Signed-off-by: Chuck Lever <[email protected]>
> ---
Previously reported issues have been addressed.
> fs/nfs/nfs4proc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 01bef06..c40f1b6 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;
>
> @@ -6068,6 +6069,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
> status = do_vfs_lock(state->inode, request);
> if (status < 0)
> goto out;
> + mutex_lock(&sp->so_delegreturn_mutex);
> down_read(&nfsi->rwsem);
> if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
> /* Yes: cache locks! */
> @@ -6075,9 +6077,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;
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chuck Lever
Hi Chuck,
Any news about this patch?
On Thu, Mar 31, 2016 at 6:40 PM, Chuck Lever <[email protected]> wrote:
>
>> On Mar 31, 2016, at 12:38 PM, Chuck Lever <[email protected]> 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.
>>
>> Since commit 24311f884189 ('NFSv4: Recovery of recalled read
>> delegations is broken'), nfs_open_delegation_recall() clears the
>> NFS_DELEGATED_STATE flag _before_ it sends the OPEN. This allows a
>> racing LOCK on the same inode to be put on the wire before the OPEN
>> operation has returned a valid open stateid.
>>
>> 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.
>>
>> Fixes: 24311f884189 ('NFSv4: Recovery of recalled read ... ')
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>
> Previously reported issues have been addressed.
>
>
>> fs/nfs/nfs4proc.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
>> index 01bef06..c40f1b6 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;
>>
>> @@ -6068,6 +6069,7 @@ static int _nfs4_proc_setlk(struct nfs4_state *state, int cmd, struct file_lock
>> status = do_vfs_lock(state->inode, request);
>> if (status < 0)
>> goto out;
>> + mutex_lock(&sp->so_delegreturn_mutex);
>> down_read(&nfsi->rwsem);
>> if (test_bit(NFS_DELEGATED_STATE, &state->flags)) {
>> /* Yes: cache locks! */
>> @@ -6075,9 +6077,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;
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> --
> Chuck Lever
>
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
William