Re: [PATCH] nfsd4: Prevent the reuse of a closed stateid

["Benjamin Coddington" <bcodding@redhat.com>]

On 17 Oct 2017, at 14:19, Jeff Layton wrote:

> On Tue, 2017-10-17 at 13:46 -0400, Benjamin Coddington wrote:
>> On 17 Oct 2017, at 12:39, Jeff Layton wrote:
>>
>>> On Tue, 2017-10-17 at 09:55 -0400, Benjamin Coddington wrote:
>>>> While running generic/089 on NFSv4.1, the following on-the-wire
>>>> exchange
>>>> occurs:
>>>>
>>>> Client                  Server
>>>> ----------              ----------
>>>> OPEN (owner A)  ->
>>>>                     <-  OPEN response: state A1
>>>> CLOSE (state A1)->
>>>> OPEN (owner A)  ->
>>>>                     <-  CLOSE response: state A2
>>>>                     <-  OPEN response: state A3
>>>> LOCK (state A3) ->
>>>>                     <-  LOCK response: NFS4_BAD_STATEID
>>>>
>>>> The server should not be returning state A3 in response to the second
>>>> OPEN
>>>> after processing the CLOSE and sending back state A2.  The problem is
>>>> that
>>>> nfsd4_process_open2() is able to find an existing open state just
>>>> after
>>>> nfsd4_close() has incremented the state's sequence number but before
>>>> calling unhash_ol_stateid().
>>>>
>>>> Fix this by using the state's sc_type field to identify closed state,
>>>> and
>>>> protect the update of that field with st_mutex.
>>>> nfsd4_find_existing_open()
>>>> will return with the st_mutex held, so that the state will not
>>>> transition
>>>> between being looked up and then updated in nfsd4_process_open2().
>>>>
>>>> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
>>>> ---
>>>>  fs/nfsd/nfs4state.c | 29 +++++++++++++++++++----------
>>>>  1 file changed, 19 insertions(+), 10 deletions(-)
>>>>
>>>
>>> The problem is real, but I'm not thrilled with the fix. It seems more
>>> lock thrashy...
>>
>> Really?  I don't think I increased any call counts to spinlocks or mutex
>> locks.  The locking overhead should be equivalent.
>>
>
> init_open_stateid will now end up taking fi_lock twice.

Yes, that's true unless there's an existing state.

> Also we now have to take the st_mutex in nfsd4_find_existing_open, just to
> check sc_type.  Neither of those are probably unreasonable, it's just
> messier than I'd like.

It is indeed messy.. no argument.  I'll spin up your suggestion to unhash
the stateid before updating and take it for a ride and let you know the
results.  Thanks for looking at this.

Ben