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

[Trond Myklebust <trondmy@hammerspace.com>]

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().

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().

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com