Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230118173139.71846-1-jlayton@kernel.org> <20230118173139.71846-2-jlayton@kernel.org>
In-Reply-To: <20230118173139.71846-2-jlayton@kernel.org>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Wed, 18 Jan 2023 12:43:53 -0500
Message-ID: <CAN-5tyHgYpGBaJYB932VAqyMGSMikexA=0uKTzROtP9nw=Nu-w@mail.gmail.com>
Subject: Re: [PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS
To:     Jeff Layton <jlayton@kernel.org>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> We're not doing any blocking operations for OP_OFFLOAD_STATUS, so taking
> and putting a reference is a waste of effort. Take the client lock,
> search for the copy and fetch the wr_bytes_written field and return.
>
> Also, make find_async_copy a static function.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/nfsd/nfs4proc.c | 35 ++++++++++++++++++++++++-----------
>  fs/nfsd/state.h    |  2 --
>  2 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 62b9d6c1b18b..731c2b22f163 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1823,23 +1823,34 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         goto out;
>  }
>
> -struct nfsd4_copy *
> -find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> +static struct nfsd4_copy *
> +find_async_copy_locked(struct nfs4_client *clp, stateid_t *stateid)
>  {
>         struct nfsd4_copy *copy;
>
> -       spin_lock(&clp->async_lock);
> +       lockdep_assert_held(&clp->async_lock);
> +
>         list_for_each_entry(copy, &clp->async_copies, copies) {
>                 if (memcmp(&copy->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
>                         continue;
> -               refcount_inc(&copy->refcount);

If we don't take a refcount on the copy, this copy could be removed
between the time we found it in the list of copies and when we then
look inside to check the amount written so far. This would lead to a
null (or bad) pointer dereference?

> -               spin_unlock(&clp->async_lock);
>                 return copy;
>         }
> -       spin_unlock(&clp->async_lock);
>         return NULL;
>  }
>
> +static struct nfsd4_copy *
> +find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
> +{
> +       struct nfsd4_copy *copy;
> +
> +       spin_lock(&clp->async_lock);
> +       copy = find_async_copy_locked(clp, stateid);
> +       if (copy)
> +               refcount_inc(&copy->refcount);
> +       spin_unlock(&clp->async_lock);
> +       return copy;
> +}
> +
>  static __be32
>  nfsd4_offload_cancel(struct svc_rqst *rqstp,
>                      struct nfsd4_compound_state *cstate,
> @@ -1924,22 +1935,24 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>         nfsd_file_put(nf);
>         return status;
>  }
> +
>  static __be32
>  nfsd4_offload_status(struct svc_rqst *rqstp,
>                      struct nfsd4_compound_state *cstate,
>                      union nfsd4_op_u *u)
>  {
>         struct nfsd4_offload_status *os = &u->offload_status;
> -       __be32 status = 0;
> +       __be32 status = nfs_ok;
>         struct nfsd4_copy *copy;
>         struct nfs4_client *clp = cstate->clp;
>
> -       copy = find_async_copy(clp, &os->stateid);
> -       if (copy) {
> +       spin_lock(&clp->async_lock);
> +       copy = find_async_copy_locked(clp, &os->stateid);
> +       if (copy)
>                 os->count = copy->cp_res.wr_bytes_written;
> -               nfs4_put_copy(copy);
> -       } else
> +       else
>                 status = nfserr_bad_stateid;
> +       spin_unlock(&clp->async_lock);
>
>         return status;
>  }
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e94634d30591..d49d3060ed4f 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -705,8 +705,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(struct xdr_netobj name
>  extern bool nfs4_has_reclaimed_state(struct xdr_netobj name, struct nfsd_net *nn);
>
>  void put_nfs4_file(struct nfs4_file *fi);
> -extern struct nfsd4_copy *
> -find_async_copy(struct nfs4_client *clp, stateid_t *staetid);
>  extern void nfs4_put_cpntf_state(struct nfsd_net *nn,
>                                  struct nfs4_cpntf_state *cps);
>  extern __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> --
> 2.39.0
>