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>
 <CAN-5tyHgYpGBaJYB932VAqyMGSMikexA=0uKTzROtP9nw=Nu-w@mail.gmail.com>
 <944bf7f3e6956989933d07aabd4a632de2ec4667.camel@kernel.org>
 <CAN-5tyHnhk9sV-jfnDvQ66brYtqY7NPvsq3D1-hWe7vYUxjgUQ@mail.gmail.com> <51D89D69-F23D-44CA-81B1-B7ECE34F9D34@oracle.com>
In-Reply-To: <51D89D69-F23D-44CA-81B1-B7ECE34F9D34@oracle.com>
From:   Olga Kornievskaia <aglo@umich.edu>
Date:   Wed, 18 Jan 2023 20:46:31 -0500
Message-ID: <CAN-5tyFOy-Qhu3JYF0KRm=QK7qv0KbWEFeyOQpGLbzx9=BD1Uw@mail.gmail.com>
Subject: Re: [PATCH 1/6] nfsd: don't take nfsd4_copy ref for OP_OFFLOAD_STATUS
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 18, 2023 at 1:53 PM Chuck Lever III <chuck.lever@oracle.com> wrote:
>
>
>
> > On Jan 18, 2023, at 1:08 PM, Olga Kornievskaia <aglo@umich.edu> wrote:
> >
> > On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <jlayton@kernel.org> wrote:
> >>
> >> On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
> >>> 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?
> >>>
> >>
> >> No. The existing code finds this object, takes a reference to it,
> >> fetches a single integer out of it and then puts the reference. This
> >> patch just has it avoid the reference altogether and fetch the value
> >> while we still hold the spinlock. This should be completely safe
> >> (assuming the locking around the existing list handling is correct,
> >> which it does seem to be).
> >
> > Thank you for the explanation. I see it now.
>
> May I add Reviewed-by: Olga Kornievskaia <kolga@netapp.com> ?

Of course.

> >>>> -               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
> >>>>
> >>
> >> --
> >> Jeff Layton <jlayton@kernel.org>
>
> --
> Chuck Lever
>
>
>