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 <[email protected]>
---
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);
- 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
On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <[email protected]> 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 <[email protected]>
> ---
> 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?
> - 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
>
On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
> On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <[email protected]> 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 <[email protected]>
> > ---
> > 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).
> > - 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 <[email protected]>
On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <[email protected]> wrote:
>
> On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
> > On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
>
>
> > > - 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 <[email protected]>
> On Jan 18, 2023, at 1:08 PM, Olga Kornievskaia <[email protected]> wrote:
>
> On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <[email protected]> wrote:
>>
>> On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
>>> On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <[email protected]> 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 <[email protected]>
>>>> ---
>>>> 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 <[email protected]> ?
>>>> - 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 <[email protected]>
--
Chuck Lever
On Wed, Jan 18, 2023 at 1:53 PM Chuck Lever III <[email protected]> wrote:
>
>
>
> > On Jan 18, 2023, at 1:08 PM, Olga Kornievskaia <[email protected]> wrote:
> >
> > On Wed, Jan 18, 2023 at 12:49 PM Jeff Layton <[email protected]> wrote:
> >>
> >> On Wed, 2023-01-18 at 12:43 -0500, Olga Kornievskaia wrote:
> >>> On Wed, Jan 18, 2023 at 12:35 PM Jeff Layton <[email protected]> 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 <[email protected]>
> >>>> ---
> >>>> 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 <[email protected]> ?
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(©->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 <[email protected]>
>
> --
> Chuck Lever
>
>
>