> On Jan 18, 2023, at 1:08 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>
> On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <jlayton@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>> ---
>>>> 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(©->cp_stateid.cs_stid, stateid, NFS4_STATEID_SIZE))
>>>> continue;
>>>> - refcount_inc(©->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@xxxxxxxxxx> ?
>>>> - 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(©->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@xxxxxxxxxx>
--
Chuck Lever
