Re: [PATCH 1/1] NFSv4: can_open_cached needs to be called with so_lock

From: dai.ngo@oracle.com
To: Trond Myklebust <trondmy@hammerspace.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 1/1] NFSv4: can_open_cached needs to be called with so_lock
Date: Fri, 30 Apr 2021 09:19:04 -0700	[thread overview]
Message-ID: <3eb40de3-7e7f-8164-0abf-f5355c8e70ca@oracle.com> (raw)
In-Reply-To: <8fadf7c12b188eacf5c2bb577a2fbf938e51ebaa.camel@hammerspace.com>

On 4/30/21 5:42 AM, Trond Myklebust wrote:
> On Fri, 2021-04-30 at 01:09 -0400, Dai Ngo wrote:
>> Currently can_open_cached accesses the openstate's flags without the
>> so_lock and also does not update the flags of the cached state. This
>> results in the openstate's flags be out of sync which can cause the
>> file to be closed prematurely.
>>
>> This patch adds the missing so_lock around the call to
>> can_open_cached
>> and also updates the openstate's flags if the cached openstate is
>> used.
>>
>> Signed-off-by: Dai Ngo <dai.ngo@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 c65c4b41e2c1..2464e77c51f9 100644
>> --- a/fs/nfs/nfs4proc.c
>> +++ b/fs/nfs/nfs4proc.c
>> @@ -2410,9 +2410,15 @@ static void nfs4_open_prepare(struct rpc_task
>> *task, void *calldata)
>>          if (data->state != NULL) {
>>                  struct nfs_delegation *delegation;
>>   
>> +               spin_lock(&data->state->owner->so_lock);
>>                  if (can_open_cached(data->state, data->o_arg.fmode,
>> -                                       data->o_arg.open_flags,
>> claim))
>> +                               data->o_arg.open_flags, claim)) {
>> +                       update_open_stateflags(data->state, data-
>>> o_arg.fmode);
>> +                       spin_unlock(&data->state->owner->so_lock);
>>                          goto out_no_action;
>> +               }
>> +               spin_unlock(&data->state->owner->so_lock);
>> +
>>                  rcu_read_lock();
>>                  delegation = nfs4_get_valid_delegation(data->state-
>>> inode);
>>                  if (can_open_delegated(delegation, data->o_arg.fmode,
>> claim))
> This is going to introduce stateid leaks. The actual update of the open
> state flags happens in nfs4_try_open_cached(), which is called from
> nfs4_opendata_to_nfs4_state().

Right, the actual update is done by _nfs4_opendata_to_nfs4_state called
from _nfs4_do_open/_nfs4_open_and_get_state. I missed the check of
data->cancelled in nfs4_open_release and just keying in on rpc_done not
set path which skips the call to nfs4_opendata_to_nfs4_state.

Thanks Trond!

-Dai

>
> While we could put spinlocks around the call to can_open_cached() here,
> there is little point in doing so, since this is just a read-only
> advisory check. The real check is performed, as I said, in
> nfs4_try_open_cached().
>