LinuxLists.cc - [PATCH 2/2] nfsd: clean up potential nfsd

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
>
> There are two different flavors of the nfsd4_copy struct. One is
> embedded in the compound and is used directly in synchronous copies. The
> other is dynamically allocated, refcounted and tracked in the client
> struture. For the embedded one, the cleanup just involves releasing any
> nfsd_files held on its behalf. For the async one, the cleanup is a bit
> more involved, and we need to dequeue it from lists, unhash it, etc.
>
> There is at least one potential refcount leak in this code now. If the
> kthread_create call fails, then both the src and dst nfsd_files in the
> original nfsd4_copy object are leaked.

I don't believe that's true. If kthread_create thread fails we call
cleanup_async_copy() that does a put on the file descriptors.

> The cleanup in this codepath is also sort of weird. In the async copy
> case, we'll have up to four nfsd_file references (src and dst for both
> flavors of copy structure).

That's not true. There is a careful distinction between intra -- which
had 2 valid file pointers and does a get on both as they both point to
something that's opened on this server--- but inter -- only does a get
on the dst file descriptor, the src doesn't exit. And yes I realize
the code checks for nfs_src being null which it should be but it makes
the code less clear and at some point somebody might want to decide to
really do a put on it.

> They are both put at the end of
> nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> one outlive that structure.
>
> Change it so that we always clean up the nfsd_file refs held by the
> embedded copy structure before nfsd4_copy returns. Rework
> cleanup_async_copy to handle both inter and intra copies. Eliminate
> nfsd4_cleanup_intra_ssc since it now becomes a no-op.

I feel by combining the cleanup for both it obscures a very important
destication that src filehandle doesn't exist for inter.

> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 37a9cc8ae7ae..62b9d6c1b18b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>
> nfs42_ssc_close(filp);
> - nfsd_file_put(dst);
> fput(filp);
>
> spin_lock(&nn->nfsd_ssc_lock);
> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> &copy->nf_dst);
> }
>
> -static void
> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> -{
> - nfsd_file_put(src);
> - nfsd_file_put(dst);
> -}
> -
> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> {
> struct nfsd4_cb_offload *cbo =
> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> dst->ss_nsui = src->ss_nsui;
> }
>
> +static void release_copy_files(struct nfsd4_copy *copy)
> +{
> + if (copy->nf_src)
> + nfsd_file_put(copy->nf_src);
> + if (copy->nf_dst)
> + nfsd_file_put(copy->nf_dst);
> +}
> +
> static void cleanup_async_copy(struct nfsd4_copy *copy)
> {
> nfs4_free_copy_state(copy);
> - nfsd_file_put(copy->nf_dst);
> - if (!nfsd4_ssc_is_inter(copy))
> - nfsd_file_put(copy->nf_src);
> + release_copy_files(copy);
> spin_lock(&copy->cp_clp->async_lock);
> list_del(&copy->copies);
> spin_unlock(&copy->cp_clp->async_lock);
> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> } else {
> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> copy->nf_dst->nf_file, false);
> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> }
>
> do_callback:
> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> } else {
> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> copy->nf_dst->nf_file, true);
> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> }
> out:
> + release_copy_files(copy);
> return status;
> out_err:
> if (async_copy)
> --
> 2.39.0
>

2023-01-18 15:38:44

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> >
> > There are two different flavors of the nfsd4_copy struct. One is
> > embedded in the compound and is used directly in synchronous copies. The
> > other is dynamically allocated, refcounted and tracked in the client
> > struture. For the embedded one, the cleanup just involves releasing any
> > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > more involved, and we need to dequeue it from lists, unhash it, etc.
> >
> > There is at least one potential refcount leak in this code now. If the
> > kthread_create call fails, then both the src and dst nfsd_files in the
> > original nfsd4_copy object are leaked.
>
> I don't believe that's true. If kthread_create thread fails we call
> cleanup_async_copy() that does a put on the file descriptors.
>

You mean this?

out_err:
if (async_copy)
cleanup_async_copy(async_copy);

That puts the references that were taken in dup_copy_fields, but the
original (embedded) nfsd4_copy also holds references and those are not
being put in this codepath.

> > The cleanup in this codepath is also sort of weird. In the async copy
> > case, we'll have up to four nfsd_file references (src and dst for both
> > flavors of copy structure).
>
> That's not true. There is a careful distinction between intra -- which
> had 2 valid file pointers and does a get on both as they both point to
> something that's opened on this server--- but inter -- only does a get
> on the dst file descriptor, the src doesn't exit. And yes I realize
> the code checks for nfs_src being null which it should be but it makes
> the code less clear and at some point somebody might want to decide to
> really do a put on it.
>

This is part of the problem here. We have a nfsd4_copy structure, and
depending on what has been done to it, you need to call different
methods to clean it up. That seems like a real antipattern to me.

> > They are both put at the end of
> > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > one outlive that structure.
> >
> > Change it so that we always clean up the nfsd_file refs held by the
> > embedded copy structure before nfsd4_copy returns. Rework
> > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>
> I feel by combining the cleanup for both it obscures a very important
> destication that src filehandle doesn't exist for inter.

If the src filehandle doesn't exist, then the pointer to it will be
NULL. I don't see what we gain by keeping these two distinct, other than
avoiding a NULL pointer check.

>
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> >
> > nfs42_ssc_close(filp);
> > - nfsd_file_put(dst);
> > fput(filp);
> >
> > spin_lock(&nn->nfsd_ssc_lock);
> > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > &copy->nf_dst);
> > }
> >
> > -static void
> > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > -{
> > - nfsd_file_put(src);
> > - nfsd_file_put(dst);
> > -}
> > -
> > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > {
> > struct nfsd4_cb_offload *cbo =
> > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > dst->ss_nsui = src->ss_nsui;
> > }
> >
> > +static void release_copy_files(struct nfsd4_copy *copy)
> > +{
> > + if (copy->nf_src)
> > + nfsd_file_put(copy->nf_src);
> > + if (copy->nf_dst)
> > + nfsd_file_put(copy->nf_dst);
> > +}
> > +
> > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > {
> > nfs4_free_copy_state(copy);
> > - nfsd_file_put(copy->nf_dst);
> > - if (!nfsd4_ssc_is_inter(copy))
> > - nfsd_file_put(copy->nf_src);
> > + release_copy_files(copy);
> > spin_lock(&copy->cp_clp->async_lock);
> > list_del(&copy->copies);
> > spin_unlock(&copy->cp_clp->async_lock);
> > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > } else {
> > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > copy->nf_dst->nf_file, false);
> > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > }
> >
> > do_callback:
> > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > } else {
> > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > copy->nf_dst->nf_file, true);
> > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > }
> > out:
> > + release_copy_files(copy);
> > return status;
> > out_err:
> > if (async_copy)
> > --
> > 2.39.0
> >

--
Jeff Layton <[email protected]>

2023-01-18 16:34:04

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
>
> On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > >
> > > There are two different flavors of the nfsd4_copy struct. One is
> > > embedded in the compound and is used directly in synchronous copies. The
> > > other is dynamically allocated, refcounted and tracked in the client
> > > struture. For the embedded one, the cleanup just involves releasing any
> > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > >
> > > There is at least one potential refcount leak in this code now. If the
> > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > original nfsd4_copy object are leaked.
> >
> > I don't believe that's true. If kthread_create thread fails we call
> > cleanup_async_copy() that does a put on the file descriptors.
> >
>
> You mean this?
>
> out_err:
> if (async_copy)
> cleanup_async_copy(async_copy);
>
> That puts the references that were taken in dup_copy_fields, but the
> original (embedded) nfsd4_copy also holds references and those are not
> being put in this codepath.

Can you please point out where do we take a reference on the original copy?

> > > The cleanup in this codepath is also sort of weird. In the async copy
> > > case, we'll have up to four nfsd_file references (src and dst for both
> > > flavors of copy structure).
> >
> > That's not true. There is a careful distinction between intra -- which
> > had 2 valid file pointers and does a get on both as they both point to
> > something that's opened on this server--- but inter -- only does a get
> > on the dst file descriptor, the src doesn't exit. And yes I realize
> > the code checks for nfs_src being null which it should be but it makes
> > the code less clear and at some point somebody might want to decide to
> > really do a put on it.
> >
>
> This is part of the problem here. We have a nfsd4_copy structure, and
> depending on what has been done to it, you need to call different
> methods to clean it up. That seems like a real antipattern to me.

But they call different methods because different things need to be
done there and it makes it clear what needs to be for what type of
copy.

> > > They are both put at the end of
> > > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > > one outlive that structure.
> > >
> > > Change it so that we always clean up the nfsd_file refs held by the
> > > embedded copy structure before nfsd4_copy returns. Rework
> > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> >
> > I feel by combining the cleanup for both it obscures a very important
> > destication that src filehandle doesn't exist for inter.
>
> If the src filehandle doesn't exist, then the pointer to it will be
> NULL. I don't see what we gain by keeping these two distinct, other than
> avoiding a NULL pointer check.

My reason would be for code clarity because different things are
supposed to happen for intra and inter. Difference of opinion it
seems.

>
> >
> > > Signed-off-by: Jeff Layton <[email protected]>
> > > ---
> > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > >
> > > nfs42_ssc_close(filp);
> > > - nfsd_file_put(dst);
> > > fput(filp);
> > >
> > > spin_lock(&nn->nfsd_ssc_lock);
> > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > &copy->nf_dst);
> > > }
> > >
> > > -static void
> > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > > -{
> > > - nfsd_file_put(src);
> > > - nfsd_file_put(dst);
> > > -}
> > > -
> > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > {
> > > struct nfsd4_cb_offload *cbo =
> > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > > dst->ss_nsui = src->ss_nsui;
> > > }
> > >
> > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > +{
> > > + if (copy->nf_src)
> > > + nfsd_file_put(copy->nf_src);
> > > + if (copy->nf_dst)
> > > + nfsd_file_put(copy->nf_dst);
> > > +}
> > > +
> > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > {
> > > nfs4_free_copy_state(copy);
> > > - nfsd_file_put(copy->nf_dst);
> > > - if (!nfsd4_ssc_is_inter(copy))
> > > - nfsd_file_put(copy->nf_src);
> > > + release_copy_files(copy);
> > > spin_lock(&copy->cp_clp->async_lock);
> > > list_del(&copy->copies);
> > > spin_unlock(&copy->cp_clp->async_lock);
> > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > } else {
> > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > copy->nf_dst->nf_file, false);
> > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > }
> > >
> > > do_callback:
> > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > } else {
> > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > copy->nf_dst->nf_file, true);
> > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > }
> > > out:
> > > + release_copy_files(copy);
> > > return status;
> > > out_err:
> > > if (async_copy)
> > > --
> > > 2.39.0
> > >
>
> --
> Jeff Layton <[email protected]>

2023-01-18 16:48:22

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

> On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <[email protected]> wrote:
>
> On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
>>
>> On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
>>> On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
>>>>
>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>> embedded in the compound and is used directly in synchronous copies. The
>>>> other is dynamically allocated, refcounted and tracked in the client
>>>> struture. For the embedded one, the cleanup just involves releasing any
>>>> nfsd_files held on its behalf. For the async one, the cleanup is a bit
>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>
>>>> There is at least one potential refcount leak in this code now. If the
>>>> kthread_create call fails, then both the src and dst nfsd_files in the
>>>> original nfsd4_copy object are leaked.
>>>
>>> I don't believe that's true. If kthread_create thread fails we call
>>> cleanup_async_copy() that does a put on the file descriptors.
>>>
>>
>> You mean this?
>>
>> out_err:
>> if (async_copy)
>> cleanup_async_copy(async_copy);
>>
>> That puts the references that were taken in dup_copy_fields, but the
>> original (embedded) nfsd4_copy also holds references and those are not
>> being put in this codepath.
>
> Can you please point out where do we take a reference on the original copy?
>
>>>> The cleanup in this codepath is also sort of weird. In the async copy
>>>> case, we'll have up to four nfsd_file references (src and dst for both
>>>> flavors of copy structure).
>>>
>>> That's not true. There is a careful distinction between intra -- which
>>> had 2 valid file pointers and does a get on both as they both point to
>>> something that's opened on this server--- but inter -- only does a get
>>> on the dst file descriptor, the src doesn't exit. And yes I realize
>>> the code checks for nfs_src being null which it should be but it makes
>>> the code less clear and at some point somebody might want to decide to
>>> really do a put on it.
>>>
>>
>> This is part of the problem here. We have a nfsd4_copy structure, and
>> depending on what has been done to it, you need to call different
>> methods to clean it up. That seems like a real antipattern to me.
>
> But they call different methods because different things need to be
> done there and it makes it clear what needs to be for what type of
> copy.

In cases like this, it makes sense to consider using types to
ensure the code can't do the wrong thing. So you might want to
have a struct nfs4_copy_A for the inter code to use, and a struct
nfs4_copy_B for the intra code to use. Sharing the same struct
for both use cases is probably what's confusing to human readers.

I've never been a stickler for removing every last ounce of code
duplication. Here, it might help to have a little duplication
just to make it easier to reason about the reference counting in
the two use cases.

That's my view from the mountain top, worth every penny you paid
for it.

>>>> They are both put at the end of
>>>> nfsd4_do_async_copy, even though the ones held on behalf of the embedded
>>>> one outlive that structure.
>>>>
>>>> Change it so that we always clean up the nfsd_file refs held by the
>>>> embedded copy structure before nfsd4_copy returns. Rework
>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>
>>> I feel by combining the cleanup for both it obscures a very important
>>> destication that src filehandle doesn't exist for inter.
>>
>> If the src filehandle doesn't exist, then the pointer to it will be
>> NULL. I don't see what we gain by keeping these two distinct, other than
>> avoiding a NULL pointer check.
>
> My reason would be for code clarity because different things are
> supposed to happen for intra and inter. Difference of opinion it
> seems.

--
Chuck Lever

2023-01-18 16:58:36

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, 2023-01-18 at 11:29 -0500, Olga Kornievskaia wrote:
> On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> >
> > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > >
> > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > embedded in the compound and is used directly in synchronous copies. The
> > > > other is dynamically allocated, refcounted and tracked in the client
> > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > >
> > > > There is at least one potential refcount leak in this code now. If the
> > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > original nfsd4_copy object are leaked.
> > >
> > > I don't believe that's true. If kthread_create thread fails we call
> > > cleanup_async_copy() that does a put on the file descriptors.
> > >
> >
> > You mean this?
> >
> > out_err:
> > if (async_copy)
> > cleanup_async_copy(async_copy);
> >
> > That puts the references that were taken in dup_copy_fields, but the
> > original (embedded) nfsd4_copy also holds references and those are not
> > being put in this codepath.
>
> Can you please point out where do we take a reference on the original copy?
>

In the case of an inter-server copy, nf_dst is set in
nfsd4_setup_inter_ssc. For intraserver copy, both pointers are set via
the call to nfsd4_verify_copy. Both functions call
nfs4_preprocess_stateid_op, which returns a reference to the nfsd_file
in the second to last arg.

> > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > flavors of copy structure).
> > >
> > > That's not true. There is a careful distinction between intra -- which
> > > had 2 valid file pointers and does a get on both as they both point to
> > > something that's opened on this server--- but inter -- only does a get
> > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > the code checks for nfs_src being null which it should be but it makes
> > > the code less clear and at some point somebody might want to decide to
> > > really do a put on it.
> > >
> >
> > This is part of the problem here. We have a nfsd4_copy structure, and
> > depending on what has been done to it, you need to call different
> > methods to clean it up. That seems like a real antipattern to me.
>
> But they call different methods because different things need to be
> done there and it makes it clear what needs to be for what type of
> copy.
>

I sure as hell had a hard time dissecting how all of that was supposed
to work. There is clear bug here, and I think this patch makes the
result clearer and more robust in the face of changes.

There are actually 4 different cases here: sync vs. async, alongside
intra vs. interserver copy. These are all overloaded onto a nfsd4_copy
structure, seemingly for no good reason.

The cleanup, in particular seems quite fragile to me, and there is a
dearth of defensive coding measures. If you subtly call the "wrong"
cleanup function at the wrong point in time, then things may go awry.

I'll leave it up to Chuck to make the final determination, but I see
this patch as an improvement.

> > > > They are both put at the end of
> > > > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > > > one outlive that structure.
> > > >
> > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > >
> > > I feel by combining the cleanup for both it obscures a very important
> > > destication that src filehandle doesn't exist for inter.
> >
> > If the src filehandle doesn't exist, then the pointer to it will be
> > NULL. I don't see what we gain by keeping these two distinct, other than
> > avoiding a NULL pointer check.
>
> My reason would be for code clarity because different things are
> supposed to happen for intra and inter. Difference of opinion it
> seems.
>
> >
> > >
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > ---
> > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > --- a/fs/nfsd/nfs4proc.c
> > > > +++ b/fs/nfsd/nfs4proc.c
> > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > >
> > > > nfs42_ssc_close(filp);
> > > > - nfsd_file_put(dst);
> > > > fput(filp);
> > > >
> > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > &copy->nf_dst);
> > > > }
> > > >
> > > > -static void
> > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > > > -{
> > > > - nfsd_file_put(src);
> > > > - nfsd_file_put(dst);
> > > > -}
> > > > -
> > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > {
> > > > struct nfsd4_cb_offload *cbo =
> > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > dst->ss_nsui = src->ss_nsui;
> > > > }
> > > >
> > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > +{
> > > > + if (copy->nf_src)
> > > > + nfsd_file_put(copy->nf_src);
> > > > + if (copy->nf_dst)
> > > > + nfsd_file_put(copy->nf_dst);
> > > > +}
> > > > +
> > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > {
> > > > nfs4_free_copy_state(copy);
> > > > - nfsd_file_put(copy->nf_dst);
> > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > - nfsd_file_put(copy->nf_src);
> > > > + release_copy_files(copy);
> > > > spin_lock(&copy->cp_clp->async_lock);
> > > > list_del(&copy->copies);
> > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > } else {
> > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > copy->nf_dst->nf_file, false);
> > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > }
> > > >
> > > > do_callback:
> > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > } else {
> > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > copy->nf_dst->nf_file, true);
> > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > }
> > > > out:
> > > > + release_copy_files(copy);
> > > > return status;
> > > > out_err:
> > > > if (async_copy)
> > > > --
> > > > 2.39.0
> > > >
> >
> > --
> > Jeff Layton <[email protected]>

--
Jeff Layton <[email protected]>

2023-01-18 17:17:19

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

> On Jan 18, 2023, at 12:06 PM, Jeff Layton <[email protected]> wrote:
>
> On Wed, 2023-01-18 at 16:39 +0000, Chuck Lever III wrote:
>>
>>> On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <[email protected]> wrote:
>>>
>>> On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
>>>>
>>>> On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
>>>>> On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
>>>>>>
>>>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>>>> embedded in the compound and is used directly in synchronous copies. The
>>>>>> other is dynamically allocated, refcounted and tracked in the client
>>>>>> struture. For the embedded one, the cleanup just involves releasing any
>>>>>> nfsd_files held on its behalf. For the async one, the cleanup is a bit
>>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>>>
>>>>>> There is at least one potential refcount leak in this code now. If the
>>>>>> kthread_create call fails, then both the src and dst nfsd_files in the
>>>>>> original nfsd4_copy object are leaked.
>>>>>
>>>>> I don't believe that's true. If kthread_create thread fails we call
>>>>> cleanup_async_copy() that does a put on the file descriptors.
>>>>>
>>>>
>>>> You mean this?
>>>>
>>>> out_err:
>>>> if (async_copy)
>>>> cleanup_async_copy(async_copy);
>>>>
>>>> That puts the references that were taken in dup_copy_fields, but the
>>>> original (embedded) nfsd4_copy also holds references and those are not
>>>> being put in this codepath.
>>>
>>> Can you please point out where do we take a reference on the original copy?
>>>
>>>>>> The cleanup in this codepath is also sort of weird. In the async copy
>>>>>> case, we'll have up to four nfsd_file references (src and dst for both
>>>>>> flavors of copy structure).
>>>>>
>>>>> That's not true. There is a careful distinction between intra -- which
>>>>> had 2 valid file pointers and does a get on both as they both point to
>>>>> something that's opened on this server--- but inter -- only does a get
>>>>> on the dst file descriptor, the src doesn't exit. And yes I realize
>>>>> the code checks for nfs_src being null which it should be but it makes
>>>>> the code less clear and at some point somebody might want to decide to
>>>>> really do a put on it.
>>>>>
>>>>
>>>> This is part of the problem here. We have a nfsd4_copy structure, and
>>>> depending on what has been done to it, you need to call different
>>>> methods to clean it up. That seems like a real antipattern to me.
>>>
>>> But they call different methods because different things need to be
>>> done there and it makes it clear what needs to be for what type of
>>> copy.
>>
>> In cases like this, it makes sense to consider using types to
>> ensure the code can't do the wrong thing. So you might want to
>> have a struct nfs4_copy_A for the inter code to use, and a struct
>> nfs4_copy_B for the intra code to use. Sharing the same struct
>> for both use cases is probably what's confusing to human readers.
>>
>> I've never been a stickler for removing every last ounce of code
>> duplication. Here, it might help to have a little duplication
>> just to make it easier to reason about the reference counting in
>> the two use cases.
>>
>> That's my view from the mountain top, worth every penny you paid
>> for it.
>>
>
> +1
>
> The nfsd4_copy structure has a lot of fields in it that only matter for
> the async copy case. ISTM that nfsd4_copy (the function) should
> dynamically allocate a struct nfsd4_async_copy that contains a
> nfsd4_copy and whatever other fields are needed.
>
> Then, we could trim down struct nfsd4_copy to just the info needed.

Yeah, some of those fields are actually quite large, like filehandles.

> For instance, the nf_src and nf_dst fields really don't need to be in
> nfsd4_copy. For the synchronous copy case, we can just keep those
> pointers on the stack, and for the async case they would be inside the
> larger structure.
>
> That would allow us to trim down the footprint of the compound union
> too.

That seems sensible. Do you feel like redriving this clean-up series
with the changes you describe above?

--
Chuck Lever

2023-01-18 17:23:16

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, Jan 18, 2023 at 11:57 AM Jeff Layton <[email protected]> wrote:
>
> On Wed, 2023-01-18 at 11:29 -0500, Olga Kornievskaia wrote:
> > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> > >
> > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > > >
> > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > embedded in the compound and is used directly in synchronous copies. The
> > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > >
> > > > > There is at least one potential refcount leak in this code now. If the
> > > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > > original nfsd4_copy object are leaked.
> > > >
> > > > I don't believe that's true. If kthread_create thread fails we call
> > > > cleanup_async_copy() that does a put on the file descriptors.
> > > >
> > >
> > > You mean this?
> > >
> > > out_err:
> > > if (async_copy)
> > > cleanup_async_copy(async_copy);
> > >
> > > That puts the references that were taken in dup_copy_fields, but the
> > > original (embedded) nfsd4_copy also holds references and those are not
> > > being put in this codepath.
> >
> > Can you please point out where do we take a reference on the original copy?
> >
>
> In the case of an inter-server copy, nf_dst is set in
> nfsd4_setup_inter_ssc. For intraserver copy, both pointers are set via
> the call to nfsd4_verify_copy. Both functions call
> nfs4_preprocess_stateid_op, which returns a reference to the nfsd_file
> in the second to last arg.

Ah. Thank you. I didn't know that nfs4_preprocess_stateid_op() takes a
reference on it's 5th argument. I think I was previously looking at
nfsd4_read() function which calls nfs4_preprocess_stateid_op() and
gets back read->rd_nf but it never does a put on it when it returns.
However, I now looked at other functions that call
nfs4_preproess_stateid_op() such as nfsd4_fallocate() and I see that
it does a put.
>
> > > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > > flavors of copy structure).
> > > >
> > > > That's not true. There is a careful distinction between intra -- which
> > > > had 2 valid file pointers and does a get on both as they both point to
> > > > something that's opened on this server--- but inter -- only does a get
> > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > the code checks for nfs_src being null which it should be but it makes
> > > > the code less clear and at some point somebody might want to decide to
> > > > really do a put on it.
> > > >
> > >
> > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > depending on what has been done to it, you need to call different
> > > methods to clean it up. That seems like a real antipattern to me.
> >
> > But they call different methods because different things need to be
> > done there and it makes it clear what needs to be for what type of
> > copy.
> >
>
>
> I sure as hell had a hard time dissecting how all of that was supposed
> to work. There is clear bug here, and I think this patch makes the
> result clearer and more robust in the face of changes.
>
> There are actually 4 different cases here: sync vs. async, alongside
> intra vs. interserver copy. These are all overloaded onto a nfsd4_copy
> structure, seemingly for no good reason.
>
> The cleanup, in particular seems quite fragile to me, and there is a
> dearth of defensive coding measures. If you subtly call the "wrong"
> cleanup function at the wrong point in time, then things may go awry.
>
> I'll leave it up to Chuck to make the final determination, but I see
> this patch as an improvement.
>
> > > > > They are both put at the end of
> > > > > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > > > > one outlive that structure.
> > > > >
> > > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > >
> > > > I feel by combining the cleanup for both it obscures a very important
> > > > destication that src filehandle doesn't exist for inter.
> > >
> > > If the src filehandle doesn't exist, then the pointer to it will be
> > > NULL. I don't see what we gain by keeping these two distinct, other than
> > > avoiding a NULL pointer check.
> >
> > My reason would be for code clarity because different things are
> > supposed to happen for intra and inter. Difference of opinion it
> > seems.
> >
> > >
> > > >
> > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > ---
> > > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > >
> > > > > nfs42_ssc_close(filp);
> > > > > - nfsd_file_put(dst);
> > > > > fput(filp);
> > > > >
> > > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > > &copy->nf_dst);
> > > > > }
> > > > >
> > > > > -static void
> > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > > > > -{
> > > > > - nfsd_file_put(src);
> > > > > - nfsd_file_put(dst);
> > > > > -}
> > > > > -
> > > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > > {
> > > > > struct nfsd4_cb_offload *cbo =
> > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > > dst->ss_nsui = src->ss_nsui;
> > > > > }
> > > > >
> > > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > > +{
> > > > > + if (copy->nf_src)
> > > > > + nfsd_file_put(copy->nf_src);
> > > > > + if (copy->nf_dst)
> > > > > + nfsd_file_put(copy->nf_dst);
> > > > > +}
> > > > > +
> > > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > > {
> > > > > nfs4_free_copy_state(copy);
> > > > > - nfsd_file_put(copy->nf_dst);
> > > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > > - nfsd_file_put(copy->nf_src);
> > > > > + release_copy_files(copy);
> > > > > spin_lock(&copy->cp_clp->async_lock);
> > > > > list_del(&copy->copies);
> > > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > > } else {
> > > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > copy->nf_dst->nf_file, false);
> > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > }
> > > > >
> > > > > do_callback:
> > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > > } else {
> > > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > copy->nf_dst->nf_file, true);
> > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > }
> > > > > out:
> > > > > + release_copy_files(copy);
> > > > > return status;
> > > > > out_err:
> > > > > if (async_copy)
> > > > > --
> > > > > 2.39.0
> > > > >
> > >
> > > --
> > > Jeff Layton <[email protected]>
>
> --
> Jeff Layton <[email protected]>

2023-01-18 17:23:30

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, 2023-01-18 at 16:39 +0000, Chuck Lever III wrote:
>
> > On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <[email protected]> wrote:
> >
> > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> > >
> > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > > >
> > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > embedded in the compound and is used directly in synchronous copies. The
> > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > >
> > > > > There is at least one potential refcount leak in this code now. If the
> > > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > > original nfsd4_copy object are leaked.
> > > >
> > > > I don't believe that's true. If kthread_create thread fails we call
> > > > cleanup_async_copy() that does a put on the file descriptors.
> > > >
> > >
> > > You mean this?
> > >
> > > out_err:
> > > if (async_copy)
> > > cleanup_async_copy(async_copy);
> > >
> > > That puts the references that were taken in dup_copy_fields, but the
> > > original (embedded) nfsd4_copy also holds references and those are not
> > > being put in this codepath.
> >
> > Can you please point out where do we take a reference on the original copy?
> >
> > > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > > flavors of copy structure).
> > > >
> > > > That's not true. There is a careful distinction between intra -- which
> > > > had 2 valid file pointers and does a get on both as they both point to
> > > > something that's opened on this server--- but inter -- only does a get
> > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > the code checks for nfs_src being null which it should be but it makes
> > > > the code less clear and at some point somebody might want to decide to
> > > > really do a put on it.
> > > >
> > >
> > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > depending on what has been done to it, you need to call different
> > > methods to clean it up. That seems like a real antipattern to me.
> >
> > But they call different methods because different things need to be
> > done there and it makes it clear what needs to be for what type of
> > copy.
>
> In cases like this, it makes sense to consider using types to
> ensure the code can't do the wrong thing. So you might want to
> have a struct nfs4_copy_A for the inter code to use, and a struct
> nfs4_copy_B for the intra code to use. Sharing the same struct
> for both use cases is probably what's confusing to human readers.
>
> I've never been a stickler for removing every last ounce of code
> duplication. Here, it might help to have a little duplication
> just to make it easier to reason about the reference counting in
> the two use cases.
>
> That's my view from the mountain top, worth every penny you paid
> for it.
>

+1

The nfsd4_copy structure has a lot of fields in it that only matter for
the async copy case. ISTM that nfsd4_copy (the function) should
dynamically allocate a struct nfsd4_async_copy that contains a
nfsd4_copy and whatever other fields are needed.

Then, we could trim down struct nfsd4_copy to just the info needed.

For instance, the nf_src and nf_dst fields really don't need to be in
nfsd4_copy. For the synchronous copy case, we can just keep those
pointers on the stack, and for the async case they would be inside the
larger structure.

That would allow us to trim down the footprint of the compound union
too.

--
Jeff Layton <[email protected]>

2023-01-18 17:31:52

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, 2023-01-18 at 17:11 +0000, Chuck Lever III wrote:
>
> > On Jan 18, 2023, at 12:06 PM, Jeff Layton <[email protected]> wrote:
> >
> > On Wed, 2023-01-18 at 16:39 +0000, Chuck Lever III wrote:
> > >
> > > > On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <[email protected]> wrote:
> > > >
> > > > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> > > > >
> > > > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > > > > >
> > > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > > embedded in the compound and is used directly in synchronous copies. The
> > > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > > >
> > > > > > > There is at least one potential refcount leak in this code now. If the
> > > > > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > > > > original nfsd4_copy object are leaked.
> > > > > >
> > > > > > I don't believe that's true. If kthread_create thread fails we call
> > > > > > cleanup_async_copy() that does a put on the file descriptors.
> > > > > >
> > > > >
> > > > > You mean this?
> > > > >
> > > > > out_err:
> > > > > if (async_copy)
> > > > > cleanup_async_copy(async_copy);
> > > > >
> > > > > That puts the references that were taken in dup_copy_fields, but the
> > > > > original (embedded) nfsd4_copy also holds references and those are not
> > > > > being put in this codepath.
> > > >
> > > > Can you please point out where do we take a reference on the original copy?
> > > >
> > > > > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > > > > flavors of copy structure).
> > > > > >
> > > > > > That's not true. There is a careful distinction between intra -- which
> > > > > > had 2 valid file pointers and does a get on both as they both point to
> > > > > > something that's opened on this server--- but inter -- only does a get
> > > > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > > > the code checks for nfs_src being null which it should be but it makes
> > > > > > the code less clear and at some point somebody might want to decide to
> > > > > > really do a put on it.
> > > > > >
> > > > >
> > > > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > > > depending on what has been done to it, you need to call different
> > > > > methods to clean it up. That seems like a real antipattern to me.
> > > >
> > > > But they call different methods because different things need to be
> > > > done there and it makes it clear what needs to be for what type of
> > > > copy.
> > >
> > > In cases like this, it makes sense to consider using types to
> > > ensure the code can't do the wrong thing. So you might want to
> > > have a struct nfs4_copy_A for the inter code to use, and a struct
> > > nfs4_copy_B for the intra code to use. Sharing the same struct
> > > for both use cases is probably what's confusing to human readers.
> > >
> > > I've never been a stickler for removing every last ounce of code
> > > duplication. Here, it might help to have a little duplication
> > > just to make it easier to reason about the reference counting in
> > > the two use cases.
> > >
> > > That's my view from the mountain top, worth every penny you paid
> > > for it.
> > >
> >
> > +1
> >
> > The nfsd4_copy structure has a lot of fields in it that only matter for
> > the async copy case. ISTM that nfsd4_copy (the function) should
> > dynamically allocate a struct nfsd4_async_copy that contains a
> > nfsd4_copy and whatever other fields are needed.
> >
> > Then, we could trim down struct nfsd4_copy to just the info needed.
>
> Yeah, some of those fields are actually quite large, like filehandles.
>
>
> > For instance, the nf_src and nf_dst fields really don't need to be in
> > nfsd4_copy. For the synchronous copy case, we can just keep those
> > pointers on the stack, and for the async case they would be inside the
> > larger structure.
> >
> > That would allow us to trim down the footprint of the compound union
> > too.
>
> That seems sensible. Do you feel like redriving this clean-up series
> with the changes you describe above?
>

I can, unless Olga, Dai or someone else would rather do it. Not sure how
soon I can get to it though.
--
Jeff Layton <[email protected]>

2023-01-18 18:01:32

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, Jan 18, 2023 at 12:26 PM Jeff Layton <[email protected]> wrote:
>
> On Wed, 2023-01-18 at 17:11 +0000, Chuck Lever III wrote:
> >
> > > On Jan 18, 2023, at 12:06 PM, Jeff Layton <[email protected]> wrote:
> > >
> > > On Wed, 2023-01-18 at 16:39 +0000, Chuck Lever III wrote:
> > > >
> > > > > On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > > > > > >
> > > > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > > > embedded in the compound and is used directly in synchronous copies. The
> > > > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > > > >
> > > > > > > > There is at least one potential refcount leak in this code now. If the
> > > > > > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > > > > > original nfsd4_copy object are leaked.
> > > > > > >
> > > > > > > I don't believe that's true. If kthread_create thread fails we call
> > > > > > > cleanup_async_copy() that does a put on the file descriptors.
> > > > > > >
> > > > > >
> > > > > > You mean this?
> > > > > >
> > > > > > out_err:
> > > > > > if (async_copy)
> > > > > > cleanup_async_copy(async_copy);
> > > > > >
> > > > > > That puts the references that were taken in dup_copy_fields, but the
> > > > > > original (embedded) nfsd4_copy also holds references and those are not
> > > > > > being put in this codepath.
> > > > >
> > > > > Can you please point out where do we take a reference on the original copy?
> > > > >
> > > > > > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > > > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > > > > > flavors of copy structure).
> > > > > > >
> > > > > > > That's not true. There is a careful distinction between intra -- which
> > > > > > > had 2 valid file pointers and does a get on both as they both point to
> > > > > > > something that's opened on this server--- but inter -- only does a get
> > > > > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > > > > the code checks for nfs_src being null which it should be but it makes
> > > > > > > the code less clear and at some point somebody might want to decide to
> > > > > > > really do a put on it.
> > > > > > >
> > > > > >
> > > > > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > > > > depending on what has been done to it, you need to call different
> > > > > > methods to clean it up. That seems like a real antipattern to me.
> > > > >
> > > > > But they call different methods because different things need to be
> > > > > done there and it makes it clear what needs to be for what type of
> > > > > copy.
> > > >
> > > > In cases like this, it makes sense to consider using types to
> > > > ensure the code can't do the wrong thing. So you might want to
> > > > have a struct nfs4_copy_A for the inter code to use, and a struct
> > > > nfs4_copy_B for the intra code to use. Sharing the same struct
> > > > for both use cases is probably what's confusing to human readers.
> > > >
> > > > I've never been a stickler for removing every last ounce of code
> > > > duplication. Here, it might help to have a little duplication
> > > > just to make it easier to reason about the reference counting in
> > > > the two use cases.
> > > >
> > > > That's my view from the mountain top, worth every penny you paid
> > > > for it.
> > > >
> > >
> > > +1
> > >
> > > The nfsd4_copy structure has a lot of fields in it that only matter for
> > > the async copy case. ISTM that nfsd4_copy (the function) should
> > > dynamically allocate a struct nfsd4_async_copy that contains a
> > > nfsd4_copy and whatever other fields are needed.
> > >
> > > Then, we could trim down struct nfsd4_copy to just the info needed.
> >
> > Yeah, some of those fields are actually quite large, like filehandles.
> >
> >
> > > For instance, the nf_src and nf_dst fields really don't need to be in
> > > nfsd4_copy. For the synchronous copy case, we can just keep those
> > > pointers on the stack, and for the async case they would be inside the
> > > larger structure.
> > >
> > > That would allow us to trim down the footprint of the compound union
> > > too.
> >
> > That seems sensible. Do you feel like redriving this clean-up series
> > with the changes you describe above?
> >
>
> I can, unless Olga, Dai or someone else would rather do it. Not sure how
> soon I can get to it though.

I'm not going to volunteer as I don't believe in the suggested change.

I think there is a performance advantage to having this structure
preallocated and ready for use.

> --
> Jeff Layton <[email protected]>

2023-01-18 18:27:37

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, Jan 18, 2023 at 12:07 PM Olga Kornievskaia <[email protected]> wrote:
>
> On Wed, Jan 18, 2023 at 11:57 AM Jeff Layton <[email protected]> wrote:
> >
> > On Wed, 2023-01-18 at 11:29 -0500, Olga Kornievskaia wrote:
> > > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> > > >
> > > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > > > >
> > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > embedded in the compound and is used directly in synchronous copies. The
> > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > >
> > > > > > There is at least one potential refcount leak in this code now. If the
> > > > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > > > original nfsd4_copy object are leaked.
> > > > >
> > > > > I don't believe that's true. If kthread_create thread fails we call
> > > > > cleanup_async_copy() that does a put on the file descriptors.
> > > > >
> > > >
> > > > You mean this?
> > > >
> > > > out_err:
> > > > if (async_copy)
> > > > cleanup_async_copy(async_copy);
> > > >
> > > > That puts the references that were taken in dup_copy_fields, but the
> > > > original (embedded) nfsd4_copy also holds references and those are not
> > > > being put in this codepath.
> > >
> > > Can you please point out where do we take a reference on the original copy?
> > >
> >
> > In the case of an inter-server copy, nf_dst is set in
> > nfsd4_setup_inter_ssc. For intraserver copy, both pointers are set via
> > the call to nfsd4_verify_copy. Both functions call
> > nfs4_preprocess_stateid_op, which returns a reference to the nfsd_file
> > in the second to last arg.
>
> Ah. Thank you. I didn't know that nfs4_preprocess_stateid_op() takes a
> reference on it's 5th argument. I think I was previously looking at
> nfsd4_read() function which calls nfs4_preprocess_stateid_op() and
> gets back read->rd_nf but it never does a put on it when it returns.
> However, I now looked at other functions that call
> nfs4_preproess_stateid_op() such as nfsd4_fallocate() and I see that
> it does a put.

So is there a refcount leak in the nfsd4_read() then since it doesn't
do a put? Or the internals obscure and that even though it calls the
same function and passes that parameter no refcount was increased. Is
it based on the "WR_STATE, RD_STATE" parameter. I see that
nfsd4_write() does do a put. For copy, we call the src_fd with
RD_STATE and dst_fd with WR_STATE. If I were to follow the logic of
nfsd4_read/nfsd4_write, the the copy doesn't need to do a put for src
but will need it for the dst. The proposed patch does it for both.

So I'm still confused if this patch is the correct solution.

> >
> > > > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > > > flavors of copy structure).
> > > > >
> > > > > That's not true. There is a careful distinction between intra -- which
> > > > > had 2 valid file pointers and does a get on both as they both point to
> > > > > something that's opened on this server--- but inter -- only does a get
> > > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > > the code checks for nfs_src being null which it should be but it makes
> > > > > the code less clear and at some point somebody might want to decide to
> > > > > really do a put on it.
> > > > >
> > > >
> > > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > > depending on what has been done to it, you need to call different
> > > > methods to clean it up. That seems like a real antipattern to me.
> > >
> > > But they call different methods because different things need to be
> > > done there and it makes it clear what needs to be for what type of
> > > copy.
> > >
> >
> >
> > I sure as hell had a hard time dissecting how all of that was supposed
> > to work. There is clear bug here, and I think this patch makes the
> > result clearer and more robust in the face of changes.
> >
> > There are actually 4 different cases here: sync vs. async, alongside
> > intra vs. interserver copy. These are all overloaded onto a nfsd4_copy
> > structure, seemingly for no good reason.
> >
> > The cleanup, in particular seems quite fragile to me, and there is a
> > dearth of defensive coding measures. If you subtly call the "wrong"
> > cleanup function at the wrong point in time, then things may go awry.
> >
> > I'll leave it up to Chuck to make the final determination, but I see
> > this patch as an improvement.
> >
> > > > > > They are both put at the end of
> > > > > > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > > > > > one outlive that structure.
> > > > > >
> > > > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > > >
> > > > > I feel by combining the cleanup for both it obscures a very important
> > > > > destication that src filehandle doesn't exist for inter.
> > > >
> > > > If the src filehandle doesn't exist, then the pointer to it will be
> > > > NULL. I don't see what we gain by keeping these two distinct, other than
> > > > avoiding a NULL pointer check.
> > >
> > > My reason would be for code clarity because different things are
> > > supposed to happen for intra and inter. Difference of opinion it
> > > seems.
> > >
> > > >
> > > > >
> > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > ---
> > > > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > >
> > > > > > nfs42_ssc_close(filp);
> > > > > > - nfsd_file_put(dst);
> > > > > > fput(filp);
> > > > > >
> > > > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > > > &copy->nf_dst);
> > > > > > }
> > > > > >
> > > > > > -static void
> > > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > > > > > -{
> > > > > > - nfsd_file_put(src);
> > > > > > - nfsd_file_put(dst);
> > > > > > -}
> > > > > > -
> > > > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > > > {
> > > > > > struct nfsd4_cb_offload *cbo =
> > > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > > > dst->ss_nsui = src->ss_nsui;
> > > > > > }
> > > > > >
> > > > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > > > +{
> > > > > > + if (copy->nf_src)
> > > > > > + nfsd_file_put(copy->nf_src);
> > > > > > + if (copy->nf_dst)
> > > > > > + nfsd_file_put(copy->nf_dst);
> > > > > > +}
> > > > > > +
> > > > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > > > {
> > > > > > nfs4_free_copy_state(copy);
> > > > > > - nfsd_file_put(copy->nf_dst);
> > > > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > > > - nfsd_file_put(copy->nf_src);
> > > > > > + release_copy_files(copy);
> > > > > > spin_lock(&copy->cp_clp->async_lock);
> > > > > > list_del(&copy->copies);
> > > > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > > > } else {
> > > > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > copy->nf_dst->nf_file, false);
> > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > }
> > > > > >
> > > > > > do_callback:
> > > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > > > } else {
> > > > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > copy->nf_dst->nf_file, true);
> > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > }
> > > > > > out:
> > > > > > + release_copy_files(copy);
> > > > > > return status;
> > > > > > out_err:
> > > > > > if (async_copy)
> > > > > > --
> > > > > > 2.39.0
> > > > > >
> > > >
> > > > --
> > > > Jeff Layton <[email protected]>
> >
> > --
> > Jeff Layton <[email protected]>

2023-01-18 18:46:54

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, 2023-01-18 at 13:16 -0500, Olga Kornievskaia wrote:
> On Wed, Jan 18, 2023 at 12:07 PM Olga Kornievskaia <[email protected]> wrote:
> >
> > On Wed, Jan 18, 2023 at 11:57 AM Jeff Layton <[email protected]> wrote:
> > >
> > > On Wed, 2023-01-18 at 11:29 -0500, Olga Kornievskaia wrote:
> > > > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> > > > >
> > > > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > > > > >
> > > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > > embedded in the compound and is used directly in synchronous copies. The
> > > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > > >
> > > > > > > There is at least one potential refcount leak in this code now. If the
> > > > > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > > > > original nfsd4_copy object are leaked.
> > > > > >
> > > > > > I don't believe that's true. If kthread_create thread fails we call
> > > > > > cleanup_async_copy() that does a put on the file descriptors.
> > > > > >
> > > > >
> > > > > You mean this?
> > > > >
> > > > > out_err:
> > > > > if (async_copy)
> > > > > cleanup_async_copy(async_copy);
> > > > >
> > > > > That puts the references that were taken in dup_copy_fields, but the
> > > > > original (embedded) nfsd4_copy also holds references and those are not
> > > > > being put in this codepath.
> > > >
> > > > Can you please point out where do we take a reference on the original copy?
> > > >
> > >
> > > In the case of an inter-server copy, nf_dst is set in
> > > nfsd4_setup_inter_ssc. For intraserver copy, both pointers are set via
> > > the call to nfsd4_verify_copy. Both functions call
> > > nfs4_preprocess_stateid_op, which returns a reference to the nfsd_file
> > > in the second to last arg.
> >
> > Ah. Thank you. I didn't know that nfs4_preprocess_stateid_op() takes a
> > reference on it's 5th argument. I think I was previously looking at
> > nfsd4_read() function which calls nfs4_preprocess_stateid_op() and
> > gets back read->rd_nf but it never does a put on it when it returns.
> > However, I now looked at other functions that call
> > nfs4_preproess_stateid_op() such as nfsd4_fallocate() and I see that
> > it does a put.
>
> So is there a refcount leak in the nfsd4_read() then since it doesn't
> do a put? Or the internals obscure and that even though it calls the
> same function and passes that parameter no refcount was increased. Is
> it based on the "WR_STATE, RD_STATE" parameter.
>

I don't think so. The put is done just below there, in
nfsd4_read_release:

if (u->read.rd_nf)
nfsd_file_put(u->read.rd_nf);

That said, I am hunting a refcount overput with nfsd_files that I've not
been able to nail down yet (which is why I've been auditing the
nfsd_file refcounting). If you see anything that looks hinky, please do
point it out.

> I see that
> nfsd4_write() does do a put. For copy, we call the src_fd with
> RD_STATE and dst_fd with WR_STATE. If I were to follow the logic of
> nfsd4_read/nfsd4_write, the the copy doesn't need to do a put for src
> but will need it for the dst. The proposed patch does it for both.
>

That'd be wrong. READs have to hold a ref to the open file while the
reply is being marshalled. A WRITE can release it once the data has been
written to the file. Maybe that's worth a comment.

Note that I just sent a patch to the list to add a comment that
(hopefully) makes it clear that nfs4_preprocess_stateid_op returns a
reference in that field.

FWIW, it wouldn't be safe for it to do anything else. Returning a
pointer to a refcounted object without taking a reference would be very
problematic.

> So I'm still confused if this patch is the correct solution.
>

Fair enough. I'm not sure I understand the pushback, as the result seems
clearer to me. If you want to propose an alternate fix, I'm happy to
take a look.

> > >
> > > > > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > > > > flavors of copy structure).
> > > > > >
> > > > > > That's not true. There is a careful distinction between intra -- which
> > > > > > had 2 valid file pointers and does a get on both as they both point to
> > > > > > something that's opened on this server--- but inter -- only does a get
> > > > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > > > the code checks for nfs_src being null which it should be but it makes
> > > > > > the code less clear and at some point somebody might want to decide to
> > > > > > really do a put on it.
> > > > > >
> > > > >
> > > > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > > > depending on what has been done to it, you need to call different
> > > > > methods to clean it up. That seems like a real antipattern to me.
> > > >
> > > > But they call different methods because different things need to be
> > > > done there and it makes it clear what needs to be for what type of
> > > > copy.
> > > >
> > >
> > >
> > > I sure as hell had a hard time dissecting how all of that was supposed
> > > to work. There is clear bug here, and I think this patch makes the
> > > result clearer and more robust in the face of changes.
> > >
> > > There are actually 4 different cases here: sync vs. async, alongside
> > > intra vs. interserver copy. These are all overloaded onto a nfsd4_copy
> > > structure, seemingly for no good reason.
> > >
> > > The cleanup, in particular seems quite fragile to me, and there is a
> > > dearth of defensive coding measures. If you subtly call the "wrong"
> > > cleanup function at the wrong point in time, then things may go awry.
> > >
> > > I'll leave it up to Chuck to make the final determination, but I see
> > > this patch as an improvement.
> > >
> > > > > > > They are both put at the end of
> > > > > > > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > > > > > > one outlive that structure.
> > > > > > >
> > > > > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > > > >
> > > > > > I feel by combining the cleanup for both it obscures a very important
> > > > > > destication that src filehandle doesn't exist for inter.
> > > > >
> > > > > If the src filehandle doesn't exist, then the pointer to it will be
> > > > > NULL. I don't see what we gain by keeping these two distinct, other than
> > > > > avoiding a NULL pointer check.
> > > >
> > > > My reason would be for code clarity because different things are
> > > > supposed to happen for intra and inter. Difference of opinion it
> > > > seems.
> > > >
> > > > >
> > > > > >
> > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > > ---
> > > > > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > > >
> > > > > > > nfs42_ssc_close(filp);
> > > > > > > - nfsd_file_put(dst);
> > > > > > > fput(filp);
> > > > > > >
> > > > > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > > > > &copy->nf_dst);
> > > > > > > }
> > > > > > >
> > > > > > > -static void
> > > > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > > > > > > -{
> > > > > > > - nfsd_file_put(src);
> > > > > > > - nfsd_file_put(dst);
> > > > > > > -}
> > > > > > > -
> > > > > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > > > > {
> > > > > > > struct nfsd4_cb_offload *cbo =
> > > > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > > > > dst->ss_nsui = src->ss_nsui;
> > > > > > > }
> > > > > > >
> > > > > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > > > > +{
> > > > > > > + if (copy->nf_src)
> > > > > > > + nfsd_file_put(copy->nf_src);
> > > > > > > + if (copy->nf_dst)
> > > > > > > + nfsd_file_put(copy->nf_dst);
> > > > > > > +}
> > > > > > > +
> > > > > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > > > > {
> > > > > > > nfs4_free_copy_state(copy);
> > > > > > > - nfsd_file_put(copy->nf_dst);
> > > > > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > > > > - nfsd_file_put(copy->nf_src);
> > > > > > > + release_copy_files(copy);
> > > > > > > spin_lock(&copy->cp_clp->async_lock);
> > > > > > > list_del(&copy->copies);
> > > > > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > > > > } else {
> > > > > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > copy->nf_dst->nf_file, false);
> > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > }
> > > > > > >
> > > > > > > do_callback:
> > > > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > > > > } else {
> > > > > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > copy->nf_dst->nf_file, true);
> > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > }
> > > > > > > out:
> > > > > > > + release_copy_files(copy);
> > > > > > > return status;
> > > > > > > out_err:
> > > > > > > if (async_copy)
> > > > > > > --
> > > > > > > 2.39.0
> > > > > > >
> > > > >
> > > > > --
> > > > > Jeff Layton <[email protected]>
> > >
> > > --
> > > Jeff Layton <[email protected]>

--
Jeff Layton <[email protected]>

2023-01-19 02:01:30

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, Jan 18, 2023 at 1:35 PM Jeff Layton <[email protected]> wrote:
>
> On Wed, 2023-01-18 at 13:16 -0500, Olga Kornievskaia wrote:
> > On Wed, Jan 18, 2023 at 12:07 PM Olga Kornievskaia <[email protected]> wrote:
> > >
> > > On Wed, Jan 18, 2023 at 11:57 AM Jeff Layton <[email protected]> wrote:
> > > >
> > > > On Wed, 2023-01-18 at 11:29 -0500, Olga Kornievskaia wrote:
> > > > > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <[email protected]> wrote:
> > > > > > > >
> > > > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > > > embedded in the compound and is used directly in synchronous copies. The
> > > > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > > > >
> > > > > > > > There is at least one potential refcount leak in this code now. If the
> > > > > > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > > > > > original nfsd4_copy object are leaked.
> > > > > > >
> > > > > > > I don't believe that's true. If kthread_create thread fails we call
> > > > > > > cleanup_async_copy() that does a put on the file descriptors.
> > > > > > >
> > > > > >
> > > > > > You mean this?
> > > > > >
> > > > > > out_err:
> > > > > > if (async_copy)
> > > > > > cleanup_async_copy(async_copy);
> > > > > >
> > > > > > That puts the references that were taken in dup_copy_fields, but the
> > > > > > original (embedded) nfsd4_copy also holds references and those are not
> > > > > > being put in this codepath.
> > > > >
> > > > > Can you please point out where do we take a reference on the original copy?
> > > > >
> > > >
> > > > In the case of an inter-server copy, nf_dst is set in
> > > > nfsd4_setup_inter_ssc. For intraserver copy, both pointers are set via
> > > > the call to nfsd4_verify_copy. Both functions call
> > > > nfs4_preprocess_stateid_op, which returns a reference to the nfsd_file
> > > > in the second to last arg.
> > >
> > > Ah. Thank you. I didn't know that nfs4_preprocess_stateid_op() takes a
> > > reference on it's 5th argument. I think I was previously looking at
> > > nfsd4_read() function which calls nfs4_preprocess_stateid_op() and
> > > gets back read->rd_nf but it never does a put on it when it returns.
> > > However, I now looked at other functions that call
> > > nfs4_preproess_stateid_op() such as nfsd4_fallocate() and I see that
> > > it does a put.
> >
> > So is there a refcount leak in the nfsd4_read() then since it doesn't
> > do a put? Or the internals obscure and that even though it calls the
> > same function and passes that parameter no refcount was increased. Is
> > it based on the "WR_STATE, RD_STATE" parameter.
> >
>
> I don't think so. The put is done just below there, in
> nfsd4_read_release:
>
> if (u->read.rd_nf)
> nfsd_file_put(u->read.rd_nf);
>
> That said, I am hunting a refcount overput with nfsd_files that I've not
> been able to nail down yet (which is why I've been auditing the
> nfsd_file refcounting). If you see anything that looks hinky, please do
> point it out.

Thank you for the explanation about the read. I didn't know about the
op_release.

> > I see that
> > nfsd4_write() does do a put. For copy, we call the src_fd with
> > RD_STATE and dst_fd with WR_STATE. If I were to follow the logic of
> > nfsd4_read/nfsd4_write, the the copy doesn't need to do a put for src
> > but will need it for the dst. The proposed patch does it for both.
> >
>
> That'd be wrong. READs have to hold a ref to the open file while the
> reply is being marshalled. A WRITE can release it once the data has been
> written to the file. Maybe that's worth a comment.
>
> Note that I just sent a patch to the list to add a comment that
> (hopefully) makes it clear that nfs4_preprocess_stateid_op returns a
> reference in that field.
>
> FWIW, it wouldn't be safe for it to do anything else. Returning a
> pointer to a refcounted object without taking a reference would be very
> problematic.
>
> > So I'm still confused if this patch is the correct solution.
> >
>
> Fair enough. I'm not sure I understand the pushback, as the result seems
> clearer to me. If you want to propose an alternate fix, I'm happy to
> take a look.

After your explanation about the read, things make sense to me so, no pushback.

> > > >
> > > > > > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > > > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > > > > > flavors of copy structure).
> > > > > > >
> > > > > > > That's not true. There is a careful distinction between intra -- which
> > > > > > > had 2 valid file pointers and does a get on both as they both point to
> > > > > > > something that's opened on this server--- but inter -- only does a get
> > > > > > > on the dst file descriptor, the src doesn't exit. And yes I realize
> > > > > > > the code checks for nfs_src being null which it should be but it makes
> > > > > > > the code less clear and at some point somebody might want to decide to
> > > > > > > really do a put on it.
> > > > > > >
> > > > > >
> > > > > > This is part of the problem here. We have a nfsd4_copy structure, and
> > > > > > depending on what has been done to it, you need to call different
> > > > > > methods to clean it up. That seems like a real antipattern to me.
> > > > >
> > > > > But they call different methods because different things need to be
> > > > > done there and it makes it clear what needs to be for what type of
> > > > > copy.
> > > > >
> > > >
> > > >
> > > > I sure as hell had a hard time dissecting how all of that was supposed
> > > > to work. There is clear bug here, and I think this patch makes the
> > > > result clearer and more robust in the face of changes.
> > > >
> > > > There are actually 4 different cases here: sync vs. async, alongside
> > > > intra vs. interserver copy. These are all overloaded onto a nfsd4_copy
> > > > structure, seemingly for no good reason.
> > > >
> > > > The cleanup, in particular seems quite fragile to me, and there is a
> > > > dearth of defensive coding measures. If you subtly call the "wrong"
> > > > cleanup function at the wrong point in time, then things may go awry.
> > > >
> > > > I'll leave it up to Chuck to make the final determination, but I see
> > > > this patch as an improvement.
> > > >
> > > > > > > > They are both put at the end of
> > > > > > > > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > > > > > > > one outlive that structure.
> > > > > > > >
> > > > > > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > > > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > > > > >
> > > > > > > I feel by combining the cleanup for both it obscures a very important
> > > > > > > destication that src filehandle doesn't exist for inter.
> > > > > >
> > > > > > If the src filehandle doesn't exist, then the pointer to it will be
> > > > > > NULL. I don't see what we gain by keeping these two distinct, other than
> > > > > > avoiding a NULL pointer check.
> > > > >
> > > > > My reason would be for code clarity because different things are
> > > > > supposed to happen for intra and inter. Difference of opinion it
> > > > > seems.
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > > > ---
> > > > > > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > > > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > > > >
> > > > > > > > nfs42_ssc_close(filp);
> > > > > > > > - nfsd_file_put(dst);
> > > > > > > > fput(filp);
> > > > > > > >
> > > > > > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > > > > > &copy->nf_dst);
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static void
> > > > > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > > > > > > > -{
> > > > > > > > - nfsd_file_put(src);
> > > > > > > > - nfsd_file_put(dst);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > > > > > {
> > > > > > > > struct nfsd4_cb_offload *cbo =
> > > > > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > > > > > dst->ss_nsui = src->ss_nsui;
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > > > > > +{
> > > > > > > > + if (copy->nf_src)
> > > > > > > > + nfsd_file_put(copy->nf_src);
> > > > > > > > + if (copy->nf_dst)
> > > > > > > > + nfsd_file_put(copy->nf_dst);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > > > > > {
> > > > > > > > nfs4_free_copy_state(copy);
> > > > > > > > - nfsd_file_put(copy->nf_dst);
> > > > > > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > > > > > - nfsd_file_put(copy->nf_src);
> > > > > > > > + release_copy_files(copy);
> > > > > > > > spin_lock(&copy->cp_clp->async_lock);
> > > > > > > > list_del(&copy->copies);
> > > > > > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > > > > > } else {
> > > > > > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > > copy->nf_dst->nf_file, false);
> > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > > }
> > > > > > > >
> > > > > > > > do_callback:
> > > > > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > > > > > } else {
> > > > > > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > > copy->nf_dst->nf_file, true);
> > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > > }
> > > > > > > > out:
> > > > > > > > + release_copy_files(copy);
> > > > > > > > return status;
> > > > > > > > out_err:
> > > > > > > > if (async_copy)
> > > > > > > > --
> > > > > > > > 2.39.0
> > > > > > > >
> > > > > >
> > > > > > --
> > > > > > Jeff Layton <[email protected]>
> > > >
> > > > --
> > > > Jeff Layton <[email protected]>
>
> --
> Jeff Layton <[email protected]>

2023-01-19 05:18:36

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On 1/17/23 11:38 AM, Jeff Layton wrote:
> There are two different flavors of the nfsd4_copy struct. One is
> embedded in the compound and is used directly in synchronous copies. The
> other is dynamically allocated, refcounted and tracked in the client
> struture. For the embedded one, the cleanup just involves releasing any
> nfsd_files held on its behalf. For the async one, the cleanup is a bit
> more involved, and we need to dequeue it from lists, unhash it, etc.
>
> There is at least one potential refcount leak in this code now. If the
> kthread_create call fails, then both the src and dst nfsd_files in the
> original nfsd4_copy object are leaked.
>
> The cleanup in this codepath is also sort of weird. In the async copy
> case, we'll have up to four nfsd_file references (src and dst for both
> flavors of copy structure). They are both put at the end of
> nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> one outlive that structure.
>
> Change it so that we always clean up the nfsd_file refs held by the
> embedded copy structure before nfsd4_copy returns. Rework
> cleanup_async_copy to handle both inter and intra copies. Eliminate
> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>
> Signed-off-by: Jeff Layton <[email protected]>
> ---
> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> 1 file changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 37a9cc8ae7ae..62b9d6c1b18b 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>
> nfs42_ssc_close(filp);
> - nfsd_file_put(dst);

I think we still need this, in addition to release_copy_files called
from cleanup_async_copy. For async inter-copy, there are 2 reference
count added to the destination file, one from nfsd4_setup_inter_ssc
and the other one from dup_copy_fields. The above nfsd_file_put is for
the count added by dup_copy_fields.

> fput(filp);
>
> spin_lock(&nn->nfsd_ssc_lock);
> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> &copy->nf_dst);
> }
>
> -static void
> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> -{
> - nfsd_file_put(src);
> - nfsd_file_put(dst);
> -}
> -
> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> {
> struct nfsd4_cb_offload *cbo =
> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> dst->ss_nsui = src->ss_nsui;
> }
>
> +static void release_copy_files(struct nfsd4_copy *copy)
> +{
> + if (copy->nf_src)
> + nfsd_file_put(copy->nf_src);
> + if (copy->nf_dst)
> + nfsd_file_put(copy->nf_dst);
> +}
> +
> static void cleanup_async_copy(struct nfsd4_copy *copy)
> {
> nfs4_free_copy_state(copy);
> - nfsd_file_put(copy->nf_dst);
> - if (!nfsd4_ssc_is_inter(copy))
> - nfsd_file_put(copy->nf_src);
> + release_copy_files(copy);
> spin_lock(&copy->cp_clp->async_lock);
> list_del(&copy->copies);
> spin_unlock(&copy->cp_clp->async_lock);
> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> } else {
> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> copy->nf_dst->nf_file, false);
> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> }
>
> do_callback:
> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> } else {
> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> copy->nf_dst->nf_file, true);
> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> }
> out:
> + release_copy_files(copy);
> return status;
> out_err:

This is unrelated to the reference count issue.

Here if this is an inter-copy then we need to decrement the reference
count of the nfsd4_ssc_umount_item so that the vfsmount can be unmounted
later.

-Dai

> if (async_copy)

2023-01-19 11:03:22

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
> On 1/17/23 11:38 AM, Jeff Layton wrote:
> > There are two different flavors of the nfsd4_copy struct. One is
> > embedded in the compound and is used directly in synchronous copies. The
> > other is dynamically allocated, refcounted and tracked in the client
> > struture. For the embedded one, the cleanup just involves releasing any
> > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > more involved, and we need to dequeue it from lists, unhash it, etc.
> >
> > There is at least one potential refcount leak in this code now. If the
> > kthread_create call fails, then both the src and dst nfsd_files in the
> > original nfsd4_copy object are leaked.
> >
> > The cleanup in this codepath is also sort of weird. In the async copy
> > case, we'll have up to four nfsd_file references (src and dst for both
> > flavors of copy structure). They are both put at the end of
> > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > one outlive that structure.
> >
> > Change it so that we always clean up the nfsd_file refs held by the
> > embedded copy structure before nfsd4_copy returns. Rework
> > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > 1 file changed, 10 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> >
> > nfs42_ssc_close(filp);
> > - nfsd_file_put(dst);
>
> I think we still need this, in addition to release_copy_files called
> from cleanup_async_copy. For async inter-copy, there are 2 reference
> count added to the destination file, one from nfsd4_setup_inter_ssc
> and the other one from dup_copy_fields. The above nfsd_file_put is for
> the count added by dup_copy_fields.
>

With this patch, the references held by the original copy structure are
put by the call to release_copy_files at the end of nfsd4_copy. That
means that the kthread task is only responsible for putting the
references held by the (kmalloc'ed) async_copy structure. So, I think
this gets the nfsd_file refcounting right.

> > fput(filp);
> >
> > spin_lock(&nn->nfsd_ssc_lock);
> > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > &copy->nf_dst);
> > }
> >
> > -static void
> > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > -{
> > - nfsd_file_put(src);
> > - nfsd_file_put(dst);
> > -}
> > -
> > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > {
> > struct nfsd4_cb_offload *cbo =
> > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > dst->ss_nsui = src->ss_nsui;
> > }
> >
> > +static void release_copy_files(struct nfsd4_copy *copy)
> > +{
> > + if (copy->nf_src)
> > + nfsd_file_put(copy->nf_src);
> > + if (copy->nf_dst)
> > + nfsd_file_put(copy->nf_dst);
> > +}
> > +
> > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > {
> > nfs4_free_copy_state(copy);
> > - nfsd_file_put(copy->nf_dst);
> > - if (!nfsd4_ssc_is_inter(copy))
> > - nfsd_file_put(copy->nf_src);
> > + release_copy_files(copy);
> > spin_lock(&copy->cp_clp->async_lock);
> > list_del(&copy->copies);
> > spin_unlock(&copy->cp_clp->async_lock);
> > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > } else {
> > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > copy->nf_dst->nf_file, false);
> > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > }
> >
> > do_callback:
> > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > } else {
> > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > copy->nf_dst->nf_file, true);
> > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > }
> > out:
> > + release_copy_files(copy);
> > return status;
> > out_err:
>
> This is unrelated to the reference count issue.
>
> Here if this is an inter-copy then we need to decrement the reference
> count of the nfsd4_ssc_umount_item so that the vfsmount can be unmounted
> later.
>

Oh, I think I see what you mean. Maybe something like the (untested)
patch below on top of the original patch would fix that?

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index c9057462b973..7475c593553c 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);

- nfs42_ssc_close(filp);
- fput(filp);
+ if (filp) {
+ nfs42_ssc_close(filp);
+ fput(filp);
+ }

spin_lock(&nn->nfsd_ssc_lo
list_del(&nsui->nsui_list);
@@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
release_copy_files(copy);
return status;
out_err:
- if (async_copy)
+ if (async_copy) {
cleanup_async_copy(async_copy);
+ if (nfsd4_ssc_is_inter(async_copy))
+ nfsd4_cleanup_inter_ssc(copy->ss_nsui, NULL,
+ copy->nf_dst);
+
+ }
status = nfserrno(-ENOMEM);
/*
* source's vfsmount of inter-copy will be unmounted

--
Jeff Layton <[email protected]>

2023-01-19 18:50:31

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On 1/19/23 2:56 AM, Jeff Layton wrote:
> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>> There are two different flavors of the nfsd4_copy struct. One is
>>> embedded in the compound and is used directly in synchronous copies. The
>>> other is dynamically allocated, refcounted and tracked in the client
>>> struture. For the embedded one, the cleanup just involves releasing any
>>> nfsd_files held on its behalf. For the async one, the cleanup is a bit
>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>
>>> There is at least one potential refcount leak in this code now. If the
>>> kthread_create call fails, then both the src and dst nfsd_files in the
>>> original nfsd4_copy object are leaked.
>>>
>>> The cleanup in this codepath is also sort of weird. In the async copy
>>> case, we'll have up to four nfsd_file references (src and dst for both
>>> flavors of copy structure). They are both put at the end of
>>> nfsd4_do_async_copy, even though the ones held on behalf of the embedded
>>> one outlive that structure.
>>>
>>> Change it so that we always clean up the nfsd_file refs held by the
>>> embedded copy structure before nfsd4_copy returns. Rework
>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>
>>> Signed-off-by: Jeff Layton <[email protected]>
>>> ---
>>> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>
>>> nfs42_ssc_close(filp);
>>> - nfsd_file_put(dst);
>> I think we still need this, in addition to release_copy_files called
>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>> count added to the destination file, one from nfsd4_setup_inter_ssc
>> and the other one from dup_copy_fields. The above nfsd_file_put is for
>> the count added by dup_copy_fields.
>>
> With this patch, the references held by the original copy structure are
> put by the call to release_copy_files at the end of nfsd4_copy. That
> means that the kthread task is only responsible for putting the
> references held by the (kmalloc'ed) async_copy structure. So, I think
> this gets the nfsd_file refcounting right.

Yes, I see. One refcount is decremented by release_copy_files at end
of nfsd4_copy and another is decremented by release_copy_files in
cleanup_async_copy.

>
>
>>> fput(filp);
>>>
>>> spin_lock(&nn->nfsd_ssc_lock);
>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>> &copy->nf_dst);
>>> }
>>>
>>> -static void
>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
>>> -{
>>> - nfsd_file_put(src);
>>> - nfsd_file_put(dst);
>>> -}
>>> -
>>> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>> {
>>> struct nfsd4_cb_offload *cbo =
>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>>> dst->ss_nsui = src->ss_nsui;
>>> }
>>>
>>> +static void release_copy_files(struct nfsd4_copy *copy)
>>> +{
>>> + if (copy->nf_src)
>>> + nfsd_file_put(copy->nf_src);
>>> + if (copy->nf_dst)
>>> + nfsd_file_put(copy->nf_dst);
>>> +}
>>> +
>>> static void cleanup_async_copy(struct nfsd4_copy *copy)
>>> {
>>> nfs4_free_copy_state(copy);
>>> - nfsd_file_put(copy->nf_dst);
>>> - if (!nfsd4_ssc_is_inter(copy))
>>> - nfsd_file_put(copy->nf_src);
>>> + release_copy_files(copy);
>>> spin_lock(&copy->cp_clp->async_lock);
>>> list_del(&copy->copies);
>>> spin_unlock(&copy->cp_clp->async_lock);
>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>> } else {
>>> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>> copy->nf_dst->nf_file, false);
>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>> }
>>>
>>> do_callback:
>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> } else {
>>> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>> copy->nf_dst->nf_file, true);
>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>> }
>>> out:
>>> + release_copy_files(copy);
>>> return status;
>>> out_err:
>> This is unrelated to the reference count issue.
>>
>> Here if this is an inter-copy then we need to decrement the reference
>> count of the nfsd4_ssc_umount_item so that the vfsmount can be unmounted
>> later.
>>
>
> Oh, I think I see what you mean. Maybe something like the (untested)
> patch below on top of the original patch would fix that?
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index c9057462b973..7475c593553c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>
> - nfs42_ssc_close(filp);
> - fput(filp);
> + if (filp) {
> + nfs42_ssc_close(filp);
> + fput(filp);
> + }
>
> spin_lock(&nn->nfsd_ssc_lo
> list_del(&nsui->nsui_list);
> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> release_copy_files(copy);
> return status;
> out_err:
> - if (async_copy)
> + if (async_copy) {
> cleanup_async_copy(async_copy);
> + if (nfsd4_ssc_is_inter(async_copy))

We don't need to call nfsd4_cleanup_inter_ssc since the thread
nfsd4_do_async_copy has not started yet so the file is not opened.
We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
you want to change nfsd4_cleanup_inter_ssc to detect this error
condition and only decrement the reference count.

-Dai

> + nfsd4_cleanup_inter_ssc(copy->ss_nsui, NULL,
> + copy->nf_dst);
> +
> + }
> status = nfserrno(-ENOMEM);
> /*
> * source's vfsmount of inter-copy will be unmounted
>

2023-01-20 11:47:44

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
> On 1/19/23 2:56 AM, Jeff Layton wrote:
> > On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
> > > On 1/17/23 11:38 AM, Jeff Layton wrote:
> > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > embedded in the compound and is used directly in synchronous copies. The
> > > > other is dynamically allocated, refcounted and tracked in the client
> > > > struture. For the embedded one, the cleanup just involves releasing any
> > > > nfsd_files held on its behalf. For the async one, the cleanup is a bit
> > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > >
> > > > There is at least one potential refcount leak in this code now. If the
> > > > kthread_create call fails, then both the src and dst nfsd_files in the
> > > > original nfsd4_copy object are leaked.
> > > >
> > > > The cleanup in this codepath is also sort of weird. In the async copy
> > > > case, we'll have up to four nfsd_file references (src and dst for both
> > > > flavors of copy structure). They are both put at the end of
> > > > nfsd4_do_async_copy, even though the ones held on behalf of the embedded
> > > > one outlive that structure.
> > > >
> > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > >
> > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > ---
> > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > --- a/fs/nfsd/nfs4proc.c
> > > > +++ b/fs/nfsd/nfs4proc.c
> > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > >
> > > > nfs42_ssc_close(filp);
> > > > - nfsd_file_put(dst);
> > > I think we still need this, in addition to release_copy_files called
> > > from cleanup_async_copy. For async inter-copy, there are 2 reference
> > > count added to the destination file, one from nfsd4_setup_inter_ssc
> > > and the other one from dup_copy_fields. The above nfsd_file_put is for
> > > the count added by dup_copy_fields.
> > >
> > With this patch, the references held by the original copy structure are
> > put by the call to release_copy_files at the end of nfsd4_copy. That
> > means that the kthread task is only responsible for putting the
> > references held by the (kmalloc'ed) async_copy structure. So, I think
> > this gets the nfsd_file refcounting right.
>
> Yes, I see. One refcount is decremented by release_copy_files at end
> of nfsd4_copy and another is decremented by release_copy_files in
> cleanup_async_copy.
>
> >
> >
> > > > fput(filp);
> > > >
> > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > &copy->nf_dst);
> > > > }
> > > >
> > > > -static void
> > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
> > > > -{
> > > > - nfsd_file_put(src);
> > > > - nfsd_file_put(dst);
> > > > -}
> > > > -
> > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > {
> > > > struct nfsd4_cb_offload *cbo =
> > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > dst->ss_nsui = src->ss_nsui;
> > > > }
> > > >
> > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > +{
> > > > + if (copy->nf_src)
> > > > + nfsd_file_put(copy->nf_src);
> > > > + if (copy->nf_dst)
> > > > + nfsd_file_put(copy->nf_dst);
> > > > +}
> > > > +
> > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > {
> > > > nfs4_free_copy_state(copy);
> > > > - nfsd_file_put(copy->nf_dst);
> > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > - nfsd_file_put(copy->nf_src);
> > > > + release_copy_files(copy);
> > > > spin_lock(&copy->cp_clp->async_lock);
> > > > list_del(&copy->copies);
> > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > } else {
> > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > copy->nf_dst->nf_file, false);
> > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > }
> > > >
> > > > do_callback:
> > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > > > } else {
> > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > copy->nf_dst->nf_file, true);
> > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > }
> > > > out:
> > > > + release_copy_files(copy);
> > > > return status;
> > > > out_err:
> > > This is unrelated to the reference count issue.
> > >
> > > Here if this is an inter-copy then we need to decrement the reference
> > > count of the nfsd4_ssc_umount_item so that the vfsmount can be unmounted
> > > later.
> > >
> >
> > Oh, I think I see what you mean. Maybe something like the (untested)
> > patch below on top of the original patch would fix that?
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index c9057462b973..7475c593553c 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
> > struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
> > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> >
> > - nfs42_ssc_close(filp);
> > - fput(filp);
> > + if (filp) {
> > + nfs42_ssc_close(filp);
> > + fput(filp);
> > + }
> >
> > spin_lock(&nn->nfsd_ssc_lo
> > list_del(&nsui->nsui_list);
> > @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> > release_copy_files(copy);
> > return status;
> > out_err:
> > - if (async_copy)
> > + if (async_copy) {
> > cleanup_async_copy(async_copy);
> > + if (nfsd4_ssc_is_inter(async_copy))
>
> We don't need to call nfsd4_cleanup_inter_ssc since the thread
> nfsd4_do_async_copy has not started yet so the file is not opened.
> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
> you want to change nfsd4_cleanup_inter_ssc to detect this error
> condition and only decrement the reference count.
>

Oh yeah, and this would break anyway since the nsui_list head is not
being initialized. Dai, would you mind spinning up a patch for this
since you're more familiar with the cleanup here?

--
Jeff Layton <[email protected]>

2023-01-21 18:59:32

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On 1/20/23 3:43 AM, Jeff Layton wrote:
> On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
>> On 1/19/23 2:56 AM, Jeff Layton wrote:
>>> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
>>>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>>> embedded in the compound and is used directly in synchronous copies. The
>>>>> other is dynamically allocated, refcounted and tracked in the client
>>>>> struture. For the embedded one, the cleanup just involves releasing any
>>>>> nfsd_files held on its behalf. For the async one, the cleanup is a bit
>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>>
>>>>> There is at least one potential refcount leak in this code now. If the
>>>>> kthread_create call fails, then both the src and dst nfsd_files in the
>>>>> original nfsd4_copy object are leaked.
>>>>>
>>>>> The cleanup in this codepath is also sort of weird. In the async copy
>>>>> case, we'll have up to four nfsd_file references (src and dst for both
>>>>> flavors of copy structure). They are both put at the end of
>>>>> nfsd4_do_async_copy, even though the ones held on behalf of the embedded
>>>>> one outlive that structure.
>>>>>
>>>>> Change it so that we always clean up the nfsd_file refs held by the
>>>>> embedded copy structure before nfsd4_copy returns. Rework
>>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>>>
>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>> ---
>>>>> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>
>>>>> nfs42_ssc_close(filp);
>>>>> - nfsd_file_put(dst);
>>>> I think we still need this, in addition to release_copy_files called
>>>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>>>> count added to the destination file, one from nfsd4_setup_inter_ssc
>>>> and the other one from dup_copy_fields. The above nfsd_file_put is for
>>>> the count added by dup_copy_fields.
>>>>
>>> With this patch, the references held by the original copy structure are
>>> put by the call to release_copy_files at the end of nfsd4_copy. That
>>> means that the kthread task is only responsible for putting the
>>> references held by the (kmalloc'ed) async_copy structure. So, I think
>>> this gets the nfsd_file refcounting right.
>> Yes, I see. One refcount is decremented by release_copy_files at end
>> of nfsd4_copy and another is decremented by release_copy_files in
>> cleanup_async_copy.
>>
>>>
>>>>> fput(filp);
>>>>>
>>>>> spin_lock(&nn->nfsd_ssc_lock);
>>>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>>>> &copy->nf_dst);
>>>>> }
>>>>>
>>>>> -static void
>>>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file *dst)
>>>>> -{
>>>>> - nfsd_file_put(src);
>>>>> - nfsd_file_put(dst);
>>>>> -}
>>>>> -
>>>>> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>>>> {
>>>>> struct nfsd4_cb_offload *cbo =
>>>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct nfsd4_copy *src, struct nfsd4_copy *dst)
>>>>> dst->ss_nsui = src->ss_nsui;
>>>>> }
>>>>>
>>>>> +static void release_copy_files(struct nfsd4_copy *copy)
>>>>> +{
>>>>> + if (copy->nf_src)
>>>>> + nfsd_file_put(copy->nf_src);
>>>>> + if (copy->nf_dst)
>>>>> + nfsd_file_put(copy->nf_dst);
>>>>> +}
>>>>> +
>>>>> static void cleanup_async_copy(struct nfsd4_copy *copy)
>>>>> {
>>>>> nfs4_free_copy_state(copy);
>>>>> - nfsd_file_put(copy->nf_dst);
>>>>> - if (!nfsd4_ssc_is_inter(copy))
>>>>> - nfsd_file_put(copy->nf_src);
>>>>> + release_copy_files(copy);
>>>>> spin_lock(&copy->cp_clp->async_lock);
>>>>> list_del(&copy->copies);
>>>>> spin_unlock(&copy->cp_clp->async_lock);
>>>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>>>> } else {
>>>>> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>> copy->nf_dst->nf_file, false);
>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>> }
>>>>>
>>>>> do_callback:
>>>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>>>> } else {
>>>>> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>> copy->nf_dst->nf_file, true);
>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>> }
>>>>> out:
>>>>> + release_copy_files(copy);
>>>>> return status;
>>>>> out_err:
>>>> This is unrelated to the reference count issue.
>>>>
>>>> Here if this is an inter-copy then we need to decrement the reference
>>>> count of the nfsd4_ssc_umount_item so that the vfsmount can be unmounted
>>>> later.
>>>>
>>> Oh, I think I see what you mean. Maybe something like the (untested)
>>> patch below on top of the original patch would fix that?
>>>
>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>> index c9057462b973..7475c593553c 100644
>>> --- a/fs/nfsd/nfs4proc.c
>>> +++ b/fs/nfsd/nfs4proc.c
>>> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct nfsd4_ssc_umount_item *nsui, struct file *filp,
>>> struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>
>>> - nfs42_ssc_close(filp);
>>> - fput(filp);
>>> + if (filp) {
>>> + nfs42_ssc_close(filp);
>>> + fput(filp);
>>> + }
>>>
>>> spin_lock(&nn->nfsd_ssc_lo
>>> list_del(&nsui->nsui_list);
>>> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>> release_copy_files(copy);
>>> return status;
>>> out_err:
>>> - if (async_copy)
>>> + if (async_copy) {
>>> cleanup_async_copy(async_copy);
>>> + if (nfsd4_ssc_is_inter(async_copy))
>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
>> nfsd4_do_async_copy has not started yet so the file is not opened.
>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
>> you want to change nfsd4_cleanup_inter_ssc to detect this error
>> condition and only decrement the reference count.
>>
> Oh yeah, and this would break anyway since the nsui_list head is not
> being initialized. Dai, would you mind spinning up a patch for this
> since you're more familiar with the cleanup here?

Will do. My patch will only fix the unmount issue. Your patch does
the clean up potential nfsd_file refcount leaks in COPY codepath.

Thanks,
-Dai

>

2023-01-21 20:06:02

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On 1/21/23 10:56 AM, [email protected] wrote:
>
> On 1/20/23 3:43 AM, Jeff Layton wrote:
>> On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
>>> On 1/19/23 2:56 AM, Jeff Layton wrote:
>>>> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
>>>>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>>>> embedded in the compound and is used directly in synchronous
>>>>>> copies. The
>>>>>> other is dynamically allocated, refcounted and tracked in the client
>>>>>> struture. For the embedded one, the cleanup just involves
>>>>>> releasing any
>>>>>> nfsd_files held on its behalf. For the async one, the cleanup is
>>>>>> a bit
>>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>>>
>>>>>> There is at least one potential refcount leak in this code now.
>>>>>> If the
>>>>>> kthread_create call fails, then both the src and dst nfsd_files
>>>>>> in the
>>>>>> original nfsd4_copy object are leaked.
>>>>>>
>>>>>> The cleanup in this codepath is also sort of weird. In the async
>>>>>> copy
>>>>>> case, we'll have up to four nfsd_file references (src and dst for
>>>>>> both
>>>>>> flavors of copy structure). They are both put at the end of
>>>>>> nfsd4_do_async_copy, even though the ones held on behalf of the
>>>>>> embedded
>>>>>> one outlive that structure.
>>>>>>
>>>>>> Change it so that we always clean up the nfsd_file refs held by the
>>>>>> embedded copy structure before nfsd4_copy returns. Rework
>>>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>>>>
>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>>> ---
>>>>>>     fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>>>>>     1 file changed, 10 insertions(+), 13 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>         long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>             nfs42_ssc_close(filp);
>>>>>> -    nfsd_file_put(dst);
>>>>> I think we still need this, in addition to release_copy_files called
>>>>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>>>>> count added to the destination file, one from nfsd4_setup_inter_ssc
>>>>> and the other one from dup_copy_fields. The above nfsd_file_put is
>>>>> for
>>>>> the count added by dup_copy_fields.
>>>>>
>>>> With this patch, the references held by the original copy structure
>>>> are
>>>> put by the call to release_copy_files at the end of nfsd4_copy. That
>>>> means that the kthread task is only responsible for putting the
>>>> references held by the (kmalloc'ed) async_copy structure. So, I think
>>>> this gets the nfsd_file refcounting right.
>>> Yes, I see. One refcount is decremented by release_copy_files at end
>>> of nfsd4_copy and another is decremented by release_copy_files in
>>> cleanup_async_copy.
>>>
>>>>
>>>>>>         fput(filp);
>>>>>>             spin_lock(&nn->nfsd_ssc_lock);
>>>>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>>>>>                      &copy->nf_dst);
>>>>>>     }
>>>>>>     -static void
>>>>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
>>>>>> *dst)
>>>>>> -{
>>>>>> -    nfsd_file_put(src);
>>>>>> -    nfsd_file_put(dst);
>>>>>> -}
>>>>>> -
>>>>>>     static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>>>>>     {
>>>>>>         struct nfsd4_cb_offload *cbo =
>>>>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
>>>>>> nfsd4_copy *src, struct nfsd4_copy *dst)
>>>>>>         dst->ss_nsui = src->ss_nsui;
>>>>>>     }
>>>>>>     +static void release_copy_files(struct nfsd4_copy *copy)
>>>>>> +{
>>>>>> +    if (copy->nf_src)
>>>>>> +        nfsd_file_put(copy->nf_src);
>>>>>> +    if (copy->nf_dst)
>>>>>> +        nfsd_file_put(copy->nf_dst);
>>>>>> +}
>>>>>> +
>>>>>>     static void cleanup_async_copy(struct nfsd4_copy *copy)
>>>>>>     {
>>>>>>         nfs4_free_copy_state(copy);
>>>>>> -    nfsd_file_put(copy->nf_dst);
>>>>>> -    if (!nfsd4_ssc_is_inter(copy))
>>>>>> -        nfsd_file_put(copy->nf_src);
>>>>>> +    release_copy_files(copy);
>>>>>>         spin_lock(&copy->cp_clp->async_lock);
>>>>>>         list_del(&copy->copies);
>>>>>> spin_unlock(&copy->cp_clp->async_lock);
>>>>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>>>>>         } else {
>>>>>>             nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>                            copy->nf_dst->nf_file, false);
>>>>>> -        nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>         }
>>>>>>         do_callback:
>>>>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>> nfsd4_compound_state *cstate,
>>>>>>         } else {
>>>>>>             status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>                            copy->nf_dst->nf_file, true);
>>>>>> -        nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>         }
>>>>>>     out:
>>>>>> +    release_copy_files(copy);
>>>>>>         return status;
>>>>>>     out_err:
>>>>> This is unrelated to the reference count issue.
>>>>>
>>>>> Here if this is an inter-copy then we need to decrement the reference
>>>>> count of the nfsd4_ssc_umount_item so that the vfsmount can be
>>>>> unmounted
>>>>> later.
>>>>>
>>>> Oh, I think I see what you mean. Maybe something like the (untested)
>>>> patch below on top of the original patch would fix that?
>>>>
>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>> index c9057462b973..7475c593553c 100644
>>>> --- a/fs/nfsd/nfs4proc.c
>>>> +++ b/fs/nfsd/nfs4proc.c
>>>> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>           struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
>>>>           long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>    -       nfs42_ssc_close(filp);
>>>> -       fput(filp);
>>>> +       if (filp) {
>>>> +               nfs42_ssc_close(filp);
>>>> +               fput(filp);
>>>> +       }
>>>>              spin_lock(&nn->nfsd_ssc_lo
>>>>           list_del(&nsui->nsui_list);
>>>> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>> nfsd4_compound_state *cstate,
>>>>           release_copy_files(copy);
>>>>           return status;
>>>>    out_err:
>>>> -       if (async_copy)
>>>> +       if (async_copy) {
>>>>                   cleanup_async_copy(async_copy);
>>>> +               if (nfsd4_ssc_is_inter(async_copy))
>>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
>>> nfsd4_do_async_copy has not started yet so the file is not opened.
>>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
>>> you want to change nfsd4_cleanup_inter_ssc to detect this error
>>> condition and only decrement the reference count.
>>>
>> Oh yeah, and this would break anyway since the nsui_list head is not
>> being initialized. Dai, would you mind spinning up a patch for this
>> since you're more familiar with the cleanup here?
>
> Will do. My patch will only fix the unmount issue. Your patch does
> the clean up potential nfsd_file refcount leaks in COPY codepath.

Or do you want me to merge your patch and mine into one?

I think we need a bit more cleanup in addition to your patch. When
kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
fails, the async_copy is not initialized yet so calling cleanup_async_copy
can be a problem.

-Dai

2023-01-21 20:18:11

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
> On 1/21/23 10:56 AM, [email protected] wrote:
> >
> > On 1/20/23 3:43 AM, Jeff Layton wrote:
> > > On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
> > > > On 1/19/23 2:56 AM, Jeff Layton wrote:
> > > > > On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
> > > > > > On 1/17/23 11:38 AM, Jeff Layton wrote:
> > > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > > embedded in the compound and is used directly in synchronous
> > > > > > > copies. The
> > > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > > struture. For the embedded one, the cleanup just involves
> > > > > > > releasing any
> > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is
> > > > > > > a bit
> > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > > >
> > > > > > > There is at least one potential refcount leak in this code now.
> > > > > > > If the
> > > > > > > kthread_create call fails, then both the src and dst nfsd_files
> > > > > > > in the
> > > > > > > original nfsd4_copy object are leaked.
> > > > > > >
> > > > > > > The cleanup in this codepath is also sort of weird. In the async
> > > > > > > copy
> > > > > > > case, we'll have up to four nfsd_file references (src and dst for
> > > > > > > both
> > > > > > > flavors of copy structure). They are both put at the end of
> > > > > > > nfsd4_do_async_copy, even though the ones held on behalf of the
> > > > > > > embedded
> > > > > > > one outlive that structure.
> > > > > > >
> > > > > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > > > > >
> > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > > ---
> > > > > > > ??? fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > > > > ??? 1 file changed, 10 insertions(+), 13 deletions(-)
> > > > > > >
> > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
> > > > > > > nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > > ??????? long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > > > ??? ??????? nfs42_ssc_close(filp);
> > > > > > > -??? nfsd_file_put(dst);
> > > > > > I think we still need this, in addition to release_copy_files called
> > > > > > from cleanup_async_copy. For async inter-copy, there are 2 reference
> > > > > > count added to the destination file, one from nfsd4_setup_inter_ssc
> > > > > > and the other one from dup_copy_fields. The above nfsd_file_put is
> > > > > > for
> > > > > > the count added by dup_copy_fields.
> > > > > >
> > > > > With this patch, the references held by the original copy structure
> > > > > are
> > > > > put by the call to release_copy_files at the end of nfsd4_copy. That
> > > > > means that the kthread task is only responsible for putting the
> > > > > references held by the (kmalloc'ed) async_copy structure. So, I think
> > > > > this gets the nfsd_file refcounting right.
> > > > Yes, I see. One refcount is decremented by release_copy_files at end
> > > > of nfsd4_copy and another is decremented by release_copy_files in
> > > > cleanup_async_copy.
> > > >
> > > > >
> > > > > > > ??????? fput(filp);
> > > > > > > ??? ??????? spin_lock(&nn->nfsd_ssc_lock);
> > > > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > > > > ???????????????????? &copy->nf_dst);
> > > > > > > ??? }
> > > > > > > ??? -static void
> > > > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
> > > > > > > *dst)
> > > > > > > -{
> > > > > > > -??? nfsd_file_put(src);
> > > > > > > -??? nfsd_file_put(dst);
> > > > > > > -}
> > > > > > > -
> > > > > > > ??? static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > > > > ??? {
> > > > > > > ??????? struct nfsd4_cb_offload *cbo =
> > > > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
> > > > > > > nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > > > > ??????? dst->ss_nsui = src->ss_nsui;
> > > > > > > ??? }
> > > > > > > ??? +static void release_copy_files(struct nfsd4_copy *copy)
> > > > > > > +{
> > > > > > > +??? if (copy->nf_src)
> > > > > > > +??????? nfsd_file_put(copy->nf_src);
> > > > > > > +??? if (copy->nf_dst)
> > > > > > > +??????? nfsd_file_put(copy->nf_dst);
> > > > > > > +}
> > > > > > > +
> > > > > > > ??? static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > > > > ??? {
> > > > > > > ??????? nfs4_free_copy_state(copy);
> > > > > > > -??? nfsd_file_put(copy->nf_dst);
> > > > > > > -??? if (!nfsd4_ssc_is_inter(copy))
> > > > > > > -??????? nfsd_file_put(copy->nf_src);
> > > > > > > +??? release_copy_files(copy);
> > > > > > > ??????? spin_lock(&copy->cp_clp->async_lock);
> > > > > > > ??????? list_del(&copy->copies);
> > > > > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > > > > ??????? } else {
> > > > > > > ??????????? nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > ?????????????????????????? copy->nf_dst->nf_file, false);
> > > > > > > -??????? nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > ??????? }
> > > > > > > ??? ??? do_callback:
> > > > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > > > > > > nfsd4_compound_state *cstate,
> > > > > > > ??????? } else {
> > > > > > > ??????????? status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > ?????????????????????????? copy->nf_dst->nf_file, true);
> > > > > > > -??????? nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > ??????? }
> > > > > > > ??? out:
> > > > > > > +??? release_copy_files(copy);
> > > > > > > ??????? return status;
> > > > > > > ??? out_err:
> > > > > > This is unrelated to the reference count issue.
> > > > > >
> > > > > > Here if this is an inter-copy then we need to decrement the reference
> > > > > > count of the nfsd4_ssc_umount_item so that the vfsmount can be
> > > > > > unmounted
> > > > > > later.
> > > > > >
> > > > > Oh, I think I see what you mean. Maybe something like the (untested)
> > > > > patch below on top of the original patch would fix that?
> > > > >
> > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > index c9057462b973..7475c593553c 100644
> > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
> > > > > nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > ????????? struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
> > > > > ????????? long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > ?? -?????? nfs42_ssc_close(filp);
> > > > > -?????? fput(filp);
> > > > > +?????? if (filp) {
> > > > > +?????????????? nfs42_ssc_close(filp);
> > > > > +?????????????? fput(filp);
> > > > > +?????? }
> > > > > ?? ????????? spin_lock(&nn->nfsd_ssc_lo
> > > > > ????????? list_del(&nsui->nsui_list);
> > > > > @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > > > > nfsd4_compound_state *cstate,
> > > > > ????????? release_copy_files(copy);
> > > > > ????????? return status;
> > > > > ?? out_err:
> > > > > -?????? if (async_copy)
> > > > > +?????? if (async_copy) {
> > > > > ????????????????? cleanup_async_copy(async_copy);
> > > > > +?????????????? if (nfsd4_ssc_is_inter(async_copy))
> > > > We don't need to call nfsd4_cleanup_inter_ssc since the thread
> > > > nfsd4_do_async_copy has not started yet so the file is not opened.
> > > > We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
> > > > you want to change nfsd4_cleanup_inter_ssc to detect this error
> > > > condition and only decrement the reference count.
> > > >
> > > Oh yeah, and this would break anyway since the nsui_list head is not
> > > being initialized. Dai, would you mind spinning up a patch for this
> > > since you're more familiar with the cleanup here?
> >
> > Will do. My patch will only fix the unmount issue. Your patch does
> > the clean up potential nfsd_file refcount leaks in COPY codepath.
>
> Or do you want me to merge your patch and mine into one?
>

It probably is best to merge them, since backporters will probably want
both patches anyway. Just make yourself the patch author and keep my S-
o-b line.

> I think we need a bit more cleanup in addition to your patch. When
> kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
> fails, the async_copy is not initialized yet so calling cleanup_async_copy
> can be a problem.
>

Yeah.

It may even be best to ensure that the list_head and such are fully
initialized for both allocated and embedded struct nfsd4_copy's. You
might shave off a few cpu cycles by not doing that, but it makes things
more fragile.

Even better, we really ought to split a lot of the fields in nfsd4_copy
into a different structure (maybe nfsd4_async_copy). Trimming down
struct nfsd4_copy would cut down the size of nfsd4_compound as well
since it has a union that contains it. I was planning on doing that
eventually, but if you want to take that on, then that would be fine
too.

--
Jeff Layton <[email protected]>

2023-01-21 20:19:40

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

> On Jan 21, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
>
> On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
>> On 1/21/23 10:56 AM, [email protected] wrote:
>>>
>>> On 1/20/23 3:43 AM, Jeff Layton wrote:
>>>> On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
>>>>> On 1/19/23 2:56 AM, Jeff Layton wrote:
>>>>>> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
>>>>>>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>>>>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>>>>>> embedded in the compound and is used directly in synchronous
>>>>>>>> copies. The
>>>>>>>> other is dynamically allocated, refcounted and tracked in the client
>>>>>>>> struture. For the embedded one, the cleanup just involves
>>>>>>>> releasing any
>>>>>>>> nfsd_files held on its behalf. For the async one, the cleanup is
>>>>>>>> a bit
>>>>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>>>>>
>>>>>>>> There is at least one potential refcount leak in this code now.
>>>>>>>> If the
>>>>>>>> kthread_create call fails, then both the src and dst nfsd_files
>>>>>>>> in the
>>>>>>>> original nfsd4_copy object are leaked.
>>>>>>>>
>>>>>>>> The cleanup in this codepath is also sort of weird. In the async
>>>>>>>> copy
>>>>>>>> case, we'll have up to four nfsd_file references (src and dst for
>>>>>>>> both
>>>>>>>> flavors of copy structure). They are both put at the end of
>>>>>>>> nfsd4_do_async_copy, even though the ones held on behalf of the
>>>>>>>> embedded
>>>>>>>> one outlive that structure.
>>>>>>>>
>>>>>>>> Change it so that we always clean up the nfsd_file refs held by the
>>>>>>>> embedded copy structure before nfsd4_copy returns. Rework
>>>>>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>>>>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>>>>>>
>>>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>>>>> ---
>>>>>>>> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>>>>>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>>> nfs42_ssc_close(filp);
>>>>>>>> - nfsd_file_put(dst);
>>>>>>> I think we still need this, in addition to release_copy_files called
>>>>>>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>>>>>>> count added to the destination file, one from nfsd4_setup_inter_ssc
>>>>>>> and the other one from dup_copy_fields. The above nfsd_file_put is
>>>>>>> for
>>>>>>> the count added by dup_copy_fields.
>>>>>>>
>>>>>> With this patch, the references held by the original copy structure
>>>>>> are
>>>>>> put by the call to release_copy_files at the end of nfsd4_copy. That
>>>>>> means that the kthread task is only responsible for putting the
>>>>>> references held by the (kmalloc'ed) async_copy structure. So, I think
>>>>>> this gets the nfsd_file refcounting right.
>>>>> Yes, I see. One refcount is decremented by release_copy_files at end
>>>>> of nfsd4_copy and another is decremented by release_copy_files in
>>>>> cleanup_async_copy.
>>>>>
>>>>>>
>>>>>>>> fput(filp);
>>>>>>>> spin_lock(&nn->nfsd_ssc_lock);
>>>>>>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>>>>>>> &copy->nf_dst);
>>>>>>>> }
>>>>>>>> -static void
>>>>>>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
>>>>>>>> *dst)
>>>>>>>> -{
>>>>>>>> - nfsd_file_put(src);
>>>>>>>> - nfsd_file_put(dst);
>>>>>>>> -}
>>>>>>>> -
>>>>>>>> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>>>>>>> {
>>>>>>>> struct nfsd4_cb_offload *cbo =
>>>>>>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
>>>>>>>> nfsd4_copy *src, struct nfsd4_copy *dst)
>>>>>>>> dst->ss_nsui = src->ss_nsui;
>>>>>>>> }
>>>>>>>> +static void release_copy_files(struct nfsd4_copy *copy)
>>>>>>>> +{
>>>>>>>> + if (copy->nf_src)
>>>>>>>> + nfsd_file_put(copy->nf_src);
>>>>>>>> + if (copy->nf_dst)
>>>>>>>> + nfsd_file_put(copy->nf_dst);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> static void cleanup_async_copy(struct nfsd4_copy *copy)
>>>>>>>> {
>>>>>>>> nfs4_free_copy_state(copy);
>>>>>>>> - nfsd_file_put(copy->nf_dst);
>>>>>>>> - if (!nfsd4_ssc_is_inter(copy))
>>>>>>>> - nfsd_file_put(copy->nf_src);
>>>>>>>> + release_copy_files(copy);
>>>>>>>> spin_lock(&copy->cp_clp->async_lock);
>>>>>>>> list_del(&copy->copies);
>>>>>>>> spin_unlock(&copy->cp_clp->async_lock);
>>>>>>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>>>>>>> } else {
>>>>>>>> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>> copy->nf_dst->nf_file, false);
>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>> }
>>>>>>>> do_callback:
>>>>>>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>>>> nfsd4_compound_state *cstate,
>>>>>>>> } else {
>>>>>>>> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>> copy->nf_dst->nf_file, true);
>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>> }
>>>>>>>> out:
>>>>>>>> + release_copy_files(copy);
>>>>>>>> return status;
>>>>>>>> out_err:
>>>>>>> This is unrelated to the reference count issue.
>>>>>>>
>>>>>>> Here if this is an inter-copy then we need to decrement the reference
>>>>>>> count of the nfsd4_ssc_umount_item so that the vfsmount can be
>>>>>>> unmounted
>>>>>>> later.
>>>>>>>
>>>>>> Oh, I think I see what you mean. Maybe something like the (untested)
>>>>>> patch below on top of the original patch would fix that?
>>>>>>
>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>> index c9057462b973..7475c593553c 100644
>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>> struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>> - nfs42_ssc_close(filp);
>>>>>> - fput(filp);
>>>>>> + if (filp) {
>>>>>> + nfs42_ssc_close(filp);
>>>>>> + fput(filp);
>>>>>> + }
>>>>>> spin_lock(&nn->nfsd_ssc_lo
>>>>>> list_del(&nsui->nsui_list);
>>>>>> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>> nfsd4_compound_state *cstate,
>>>>>> release_copy_files(copy);
>>>>>> return status;
>>>>>> out_err:
>>>>>> - if (async_copy)
>>>>>> + if (async_copy) {
>>>>>> cleanup_async_copy(async_copy);
>>>>>> + if (nfsd4_ssc_is_inter(async_copy))
>>>>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
>>>>> nfsd4_do_async_copy has not started yet so the file is not opened.
>>>>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
>>>>> you want to change nfsd4_cleanup_inter_ssc to detect this error
>>>>> condition and only decrement the reference count.
>>>>>
>>>> Oh yeah, and this would break anyway since the nsui_list head is not
>>>> being initialized. Dai, would you mind spinning up a patch for this
>>>> since you're more familiar with the cleanup here?
>>>
>>> Will do. My patch will only fix the unmount issue. Your patch does
>>> the clean up potential nfsd_file refcount leaks in COPY codepath.
>>
>> Or do you want me to merge your patch and mine into one?
>>
>
> It probably is best to merge them, since backporters will probably want
> both patches anyway.

Unless these two changes are somehow interdependent, I'd like to keep
them separate. They address two separate issues, yes?

And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
for nfsd-next. I'd rather not mix the two types of change.

> Just make yourself the patch author and keep my S-o-b line.
>
>> I think we need a bit more cleanup in addition to your patch. When
>> kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
>> fails, the async_copy is not initialized yet so calling cleanup_async_copy
>> can be a problem.
>>
>
> Yeah.
>
> It may even be best to ensure that the list_head and such are fully
> initialized for both allocated and embedded struct nfsd4_copy's. You
> might shave off a few cpu cycles by not doing that, but it makes things
> more fragile.
>
> Even better, we really ought to split a lot of the fields in nfsd4_copy
> into a different structure (maybe nfsd4_async_copy). Trimming down
> struct nfsd4_copy would cut down the size of nfsd4_compound as well
> since it has a union that contains it. I was planning on doing that
> eventually, but if you want to take that on, then that would be fine
> too.
>
> --
> Jeff Layton <[email protected]>

--
Chuck Lever

2023-01-21 21:36:05

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On 1/21/23 12:12 PM, Chuck Lever III wrote:
>
>> On Jan 21, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
>>
>> On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
>>> On 1/21/23 10:56 AM, [email protected] wrote:
>>>> On 1/20/23 3:43 AM, Jeff Layton wrote:
>>>>> On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
>>>>>> On 1/19/23 2:56 AM, Jeff Layton wrote:
>>>>>>> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
>>>>>>>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>>>>>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>>>>>>> embedded in the compound and is used directly in synchronous
>>>>>>>>> copies. The
>>>>>>>>> other is dynamically allocated, refcounted and tracked in the client
>>>>>>>>> struture. For the embedded one, the cleanup just involves
>>>>>>>>> releasing any
>>>>>>>>> nfsd_files held on its behalf. For the async one, the cleanup is
>>>>>>>>> a bit
>>>>>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>>>>>>
>>>>>>>>> There is at least one potential refcount leak in this code now.
>>>>>>>>> If the
>>>>>>>>> kthread_create call fails, then both the src and dst nfsd_files
>>>>>>>>> in the
>>>>>>>>> original nfsd4_copy object are leaked.
>>>>>>>>>
>>>>>>>>> The cleanup in this codepath is also sort of weird. In the async
>>>>>>>>> copy
>>>>>>>>> case, we'll have up to four nfsd_file references (src and dst for
>>>>>>>>> both
>>>>>>>>> flavors of copy structure). They are both put at the end of
>>>>>>>>> nfsd4_do_async_copy, even though the ones held on behalf of the
>>>>>>>>> embedded
>>>>>>>>> one outlive that structure.
>>>>>>>>>
>>>>>>>>> Change it so that we always clean up the nfsd_file refs held by the
>>>>>>>>> embedded copy structure before nfsd4_copy returns. Rework
>>>>>>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>>>>>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>>>>>> ---
>>>>>>>>> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>>>>>>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>>>> nfs42_ssc_close(filp);
>>>>>>>>> - nfsd_file_put(dst);
>>>>>>>> I think we still need this, in addition to release_copy_files called
>>>>>>>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>>>>>>>> count added to the destination file, one from nfsd4_setup_inter_ssc
>>>>>>>> and the other one from dup_copy_fields. The above nfsd_file_put is
>>>>>>>> for
>>>>>>>> the count added by dup_copy_fields.
>>>>>>>>
>>>>>>> With this patch, the references held by the original copy structure
>>>>>>> are
>>>>>>> put by the call to release_copy_files at the end of nfsd4_copy. That
>>>>>>> means that the kthread task is only responsible for putting the
>>>>>>> references held by the (kmalloc'ed) async_copy structure. So, I think
>>>>>>> this gets the nfsd_file refcounting right.
>>>>>> Yes, I see. One refcount is decremented by release_copy_files at end
>>>>>> of nfsd4_copy and another is decremented by release_copy_files in
>>>>>> cleanup_async_copy.
>>>>>>
>>>>>>>>> fput(filp);
>>>>>>>>> spin_lock(&nn->nfsd_ssc_lock);
>>>>>>>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>>>>>>>> &copy->nf_dst);
>>>>>>>>> }
>>>>>>>>> -static void
>>>>>>>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
>>>>>>>>> *dst)
>>>>>>>>> -{
>>>>>>>>> - nfsd_file_put(src);
>>>>>>>>> - nfsd_file_put(dst);
>>>>>>>>> -}
>>>>>>>>> -
>>>>>>>>> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>>>>>>>> {
>>>>>>>>> struct nfsd4_cb_offload *cbo =
>>>>>>>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
>>>>>>>>> nfsd4_copy *src, struct nfsd4_copy *dst)
>>>>>>>>> dst->ss_nsui = src->ss_nsui;
>>>>>>>>> }
>>>>>>>>> +static void release_copy_files(struct nfsd4_copy *copy)
>>>>>>>>> +{
>>>>>>>>> + if (copy->nf_src)
>>>>>>>>> + nfsd_file_put(copy->nf_src);
>>>>>>>>> + if (copy->nf_dst)
>>>>>>>>> + nfsd_file_put(copy->nf_dst);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> static void cleanup_async_copy(struct nfsd4_copy *copy)
>>>>>>>>> {
>>>>>>>>> nfs4_free_copy_state(copy);
>>>>>>>>> - nfsd_file_put(copy->nf_dst);
>>>>>>>>> - if (!nfsd4_ssc_is_inter(copy))
>>>>>>>>> - nfsd_file_put(copy->nf_src);
>>>>>>>>> + release_copy_files(copy);
>>>>>>>>> spin_lock(&copy->cp_clp->async_lock);
>>>>>>>>> list_del(&copy->copies);
>>>>>>>>> spin_unlock(&copy->cp_clp->async_lock);
>>>>>>>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>>>>>>>> } else {
>>>>>>>>> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>>> copy->nf_dst->nf_file, false);
>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>>> }
>>>>>>>>> do_callback:
>>>>>>>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>>>>> nfsd4_compound_state *cstate,
>>>>>>>>> } else {
>>>>>>>>> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>>> copy->nf_dst->nf_file, true);
>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>>> }
>>>>>>>>> out:
>>>>>>>>> + release_copy_files(copy);
>>>>>>>>> return status;
>>>>>>>>> out_err:
>>>>>>>> This is unrelated to the reference count issue.
>>>>>>>>
>>>>>>>> Here if this is an inter-copy then we need to decrement the reference
>>>>>>>> count of the nfsd4_ssc_umount_item so that the vfsmount can be
>>>>>>>> unmounted
>>>>>>>> later.
>>>>>>>>
>>>>>>> Oh, I think I see what you mean. Maybe something like the (untested)
>>>>>>> patch below on top of the original patch would fix that?
>>>>>>>
>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>> index c9057462b973..7475c593553c 100644
>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>> struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>> - nfs42_ssc_close(filp);
>>>>>>> - fput(filp);
>>>>>>> + if (filp) {
>>>>>>> + nfs42_ssc_close(filp);
>>>>>>> + fput(filp);
>>>>>>> + }
>>>>>>> spin_lock(&nn->nfsd_ssc_lo
>>>>>>> list_del(&nsui->nsui_list);
>>>>>>> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>>> nfsd4_compound_state *cstate,
>>>>>>> release_copy_files(copy);
>>>>>>> return status;
>>>>>>> out_err:
>>>>>>> - if (async_copy)
>>>>>>> + if (async_copy) {
>>>>>>> cleanup_async_copy(async_copy);
>>>>>>> + if (nfsd4_ssc_is_inter(async_copy))
>>>>>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
>>>>>> nfsd4_do_async_copy has not started yet so the file is not opened.
>>>>>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
>>>>>> you want to change nfsd4_cleanup_inter_ssc to detect this error
>>>>>> condition and only decrement the reference count.
>>>>>>
>>>>> Oh yeah, and this would break anyway since the nsui_list head is not
>>>>> being initialized. Dai, would you mind spinning up a patch for this
>>>>> since you're more familiar with the cleanup here?
>>>> Will do. My patch will only fix the unmount issue. Your patch does
>>>> the clean up potential nfsd_file refcount leaks in COPY codepath.
>>> Or do you want me to merge your patch and mine into one?
>>>
>> It probably is best to merge them, since backporters will probably want
>> both patches anyway.
> Unless these two changes are somehow interdependent, I'd like to keep
> them separate. They address two separate issues, yes?

Yes.

>
> And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
> for nfsd-next. I'd rather not mix the two types of change.

Ok. Can we do this:

1. Jeff's patch goes to nfsd-fixes since it has the fix for missing
reference count.

2. My fix for the cleanup of allocated memory goes to nfsd-fixes.

3. I will do the optimization Jeff proposed about list_head and
nfsd4_compound in a separate patch that goes into nfsd-next.

-Dai

>> Just make yourself the patch author and keep my S-o-b line.
>>
>>> I think we need a bit more cleanup in addition to your patch. When
>>> kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
>>> fails, the async_copy is not initialized yet so calling cleanup_async_copy
>>> can be a problem.
>>>
>> Yeah.
>>
>> It may even be best to ensure that the list_head and such are fully
>> initialized for both allocated and embedded struct nfsd4_copy's. You
>> might shave off a few cpu cycles by not doing that, but it makes things
>> more fragile.
>>
>> Even better, we really ought to split a lot of the fields in nfsd4_copy
>> into a different structure (maybe nfsd4_async_copy). Trimming down
>> struct nfsd4_copy would cut down the size of nfsd4_compound as well
>> since it has a union that contains it. I was planning on doing that
>> eventually, but if you want to take that on, then that would be fine
>> too.
>>
>> --
>> Jeff Layton <[email protected]>
> --
> Chuck Lever
>
>
>

2023-01-22 16:46:17

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

> On Jan 21, 2023, at 4:28 PM, Dai Ngo <[email protected]> wrote:
>
>
> On 1/21/23 12:12 PM, Chuck Lever III wrote:
>>
>>> On Jan 21, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
>>>
>>> On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
>>>> On 1/21/23 10:56 AM, [email protected] wrote:
>>>>> On 1/20/23 3:43 AM, Jeff Layton wrote:
>>>>>> On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
>>>>>>> On 1/19/23 2:56 AM, Jeff Layton wrote:
>>>>>>>> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
>>>>>>>>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>>>>>>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>>>>>>>> embedded in the compound and is used directly in synchronous
>>>>>>>>>> copies. The
>>>>>>>>>> other is dynamically allocated, refcounted and tracked in the client
>>>>>>>>>> struture. For the embedded one, the cleanup just involves
>>>>>>>>>> releasing any
>>>>>>>>>> nfsd_files held on its behalf. For the async one, the cleanup is
>>>>>>>>>> a bit
>>>>>>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>>>>>>>
>>>>>>>>>> There is at least one potential refcount leak in this code now.
>>>>>>>>>> If the
>>>>>>>>>> kthread_create call fails, then both the src and dst nfsd_files
>>>>>>>>>> in the
>>>>>>>>>> original nfsd4_copy object are leaked.
>>>>>>>>>>
>>>>>>>>>> The cleanup in this codepath is also sort of weird. In the async
>>>>>>>>>> copy
>>>>>>>>>> case, we'll have up to four nfsd_file references (src and dst for
>>>>>>>>>> both
>>>>>>>>>> flavors of copy structure). They are both put at the end of
>>>>>>>>>> nfsd4_do_async_copy, even though the ones held on behalf of the
>>>>>>>>>> embedded
>>>>>>>>>> one outlive that structure.
>>>>>>>>>>
>>>>>>>>>> Change it so that we always clean up the nfsd_file refs held by the
>>>>>>>>>> embedded copy structure before nfsd4_copy returns. Rework
>>>>>>>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>>>>>>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>>>>>>> ---
>>>>>>>>>> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>>>>>>>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>>>>> nfs42_ssc_close(filp);
>>>>>>>>>> - nfsd_file_put(dst);
>>>>>>>>> I think we still need this, in addition to release_copy_files called
>>>>>>>>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>>>>>>>>> count added to the destination file, one from nfsd4_setup_inter_ssc
>>>>>>>>> and the other one from dup_copy_fields. The above nfsd_file_put is
>>>>>>>>> for
>>>>>>>>> the count added by dup_copy_fields.
>>>>>>>>>
>>>>>>>> With this patch, the references held by the original copy structure
>>>>>>>> are
>>>>>>>> put by the call to release_copy_files at the end of nfsd4_copy. That
>>>>>>>> means that the kthread task is only responsible for putting the
>>>>>>>> references held by the (kmalloc'ed) async_copy structure. So, I think
>>>>>>>> this gets the nfsd_file refcounting right.
>>>>>>> Yes, I see. One refcount is decremented by release_copy_files at end
>>>>>>> of nfsd4_copy and another is decremented by release_copy_files in
>>>>>>> cleanup_async_copy.
>>>>>>>
>>>>>>>>>> fput(filp);
>>>>>>>>>> spin_lock(&nn->nfsd_ssc_lock);
>>>>>>>>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>>>>>>>>> &copy->nf_dst);
>>>>>>>>>> }
>>>>>>>>>> -static void
>>>>>>>>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
>>>>>>>>>> *dst)
>>>>>>>>>> -{
>>>>>>>>>> - nfsd_file_put(src);
>>>>>>>>>> - nfsd_file_put(dst);
>>>>>>>>>> -}
>>>>>>>>>> -
>>>>>>>>>> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>>>>>>>>> {
>>>>>>>>>> struct nfsd4_cb_offload *cbo =
>>>>>>>>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
>>>>>>>>>> nfsd4_copy *src, struct nfsd4_copy *dst)
>>>>>>>>>> dst->ss_nsui = src->ss_nsui;
>>>>>>>>>> }
>>>>>>>>>> +static void release_copy_files(struct nfsd4_copy *copy)
>>>>>>>>>> +{
>>>>>>>>>> + if (copy->nf_src)
>>>>>>>>>> + nfsd_file_put(copy->nf_src);
>>>>>>>>>> + if (copy->nf_dst)
>>>>>>>>>> + nfsd_file_put(copy->nf_dst);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> static void cleanup_async_copy(struct nfsd4_copy *copy)
>>>>>>>>>> {
>>>>>>>>>> nfs4_free_copy_state(copy);
>>>>>>>>>> - nfsd_file_put(copy->nf_dst);
>>>>>>>>>> - if (!nfsd4_ssc_is_inter(copy))
>>>>>>>>>> - nfsd_file_put(copy->nf_src);
>>>>>>>>>> + release_copy_files(copy);
>>>>>>>>>> spin_lock(&copy->cp_clp->async_lock);
>>>>>>>>>> list_del(&copy->copies);
>>>>>>>>>> spin_unlock(&copy->cp_clp->async_lock);
>>>>>>>>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>>>>>>>>> } else {
>>>>>>>>>> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>>>> copy->nf_dst->nf_file, false);
>>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>>>> }
>>>>>>>>>> do_callback:
>>>>>>>>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>>>>>> nfsd4_compound_state *cstate,
>>>>>>>>>> } else {
>>>>>>>>>> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>>>> copy->nf_dst->nf_file, true);
>>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>>>> }
>>>>>>>>>> out:
>>>>>>>>>> + release_copy_files(copy);
>>>>>>>>>> return status;
>>>>>>>>>> out_err:
>>>>>>>>> This is unrelated to the reference count issue.
>>>>>>>>>
>>>>>>>>> Here if this is an inter-copy then we need to decrement the reference
>>>>>>>>> count of the nfsd4_ssc_umount_item so that the vfsmount can be
>>>>>>>>> unmounted
>>>>>>>>> later.
>>>>>>>>>
>>>>>>>> Oh, I think I see what you mean. Maybe something like the (untested)
>>>>>>>> patch below on top of the original patch would fix that?
>>>>>>>>
>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>> index c9057462b973..7475c593553c 100644
>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>>> struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
>>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>>> - nfs42_ssc_close(filp);
>>>>>>>> - fput(filp);
>>>>>>>> + if (filp) {
>>>>>>>> + nfs42_ssc_close(filp);
>>>>>>>> + fput(filp);
>>>>>>>> + }
>>>>>>>> spin_lock(&nn->nfsd_ssc_lo
>>>>>>>> list_del(&nsui->nsui_list);
>>>>>>>> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>>>> nfsd4_compound_state *cstate,
>>>>>>>> release_copy_files(copy);
>>>>>>>> return status;
>>>>>>>> out_err:
>>>>>>>> - if (async_copy)
>>>>>>>> + if (async_copy) {
>>>>>>>> cleanup_async_copy(async_copy);
>>>>>>>> + if (nfsd4_ssc_is_inter(async_copy))
>>>>>>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
>>>>>>> nfsd4_do_async_copy has not started yet so the file is not opened.
>>>>>>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
>>>>>>> you want to change nfsd4_cleanup_inter_ssc to detect this error
>>>>>>> condition and only decrement the reference count.
>>>>>>>
>>>>>> Oh yeah, and this would break anyway since the nsui_list head is not
>>>>>> being initialized. Dai, would you mind spinning up a patch for this
>>>>>> since you're more familiar with the cleanup here?
>>>>> Will do. My patch will only fix the unmount issue. Your patch does
>>>>> the clean up potential nfsd_file refcount leaks in COPY codepath.
>>>> Or do you want me to merge your patch and mine into one?
>>>>
>>> It probably is best to merge them, since backporters will probably want
>>> both patches anyway.
>> Unless these two changes are somehow interdependent, I'd like to keep
>> them separate. They address two separate issues, yes?
>
> Yes.
>
>>
>> And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
>> for nfsd-next. I'd rather not mix the two types of change.
>
> Ok. Can we do this:
>
> 1. Jeff's patch goes to nfsd-fixes since it has the fix for missing
> reference count.

To make sure I haven't lost track of anything:

The patch you refer to here is this one:

https://lore.kernel.org/linux-nfs/[email protected]/

Correct?

(I was waiting for Jeff and Olga to come to consensus, and I think
they have, so I can apply it to nfsd-fixes now).

> 2. My fix for the cleanup of allocated memory goes to nfsd-fixes.

And this one hasn't been posted yet, right? Or did I miss it?

> 3. I will do the optimization Jeff proposed about list_head and
> nfsd4_compound in a separate patch that goes into nfsd-next.

That should be fine.

> -Dai
>
>>> Just make yourself the patch author and keep my S-o-b line.
>>>
>>>> I think we need a bit more cleanup in addition to your patch. When
>>>> kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
>>>> fails, the async_copy is not initialized yet so calling cleanup_async_copy
>>>> can be a problem.
>>>>
>>> Yeah.
>>>
>>> It may even be best to ensure that the list_head and such are fully
>>> initialized for both allocated and embedded struct nfsd4_copy's. You
>>> might shave off a few cpu cycles by not doing that, but it makes things
>>> more fragile.
>>>
>>> Even better, we really ought to split a lot of the fields in nfsd4_copy
>>> into a different structure (maybe nfsd4_async_copy). Trimming down
>>> struct nfsd4_copy would cut down the size of nfsd4_compound as well
>>> since it has a union that contains it. I was planning on doing that
>>> eventually, but if you want to take that on, then that would be fine
>>> too.
>>>
>>> --
>>> Jeff Layton <[email protected]>
>> --
>> Chuck Lever

--
Chuck Lever

2023-01-22 17:11:08

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

> On Jan 22, 2023, at 11:45 AM, Chuck Lever III <[email protected]> wrote:
>
>
>
>> On Jan 21, 2023, at 4:28 PM, Dai Ngo <[email protected]> wrote:
>>
>>
>> On 1/21/23 12:12 PM, Chuck Lever III wrote:
>>>
>>>> On Jan 21, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
>>>>
>>>> On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
>>>>> On 1/21/23 10:56 AM, [email protected] wrote:
>>>>>> On 1/20/23 3:43 AM, Jeff Layton wrote:
>>>>>>> On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
>>>>>>>> On 1/19/23 2:56 AM, Jeff Layton wrote:
>>>>>>>>> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
>>>>>>>>>> On 1/17/23 11:38 AM, Jeff Layton wrote:
>>>>>>>>>>> There are two different flavors of the nfsd4_copy struct. One is
>>>>>>>>>>> embedded in the compound and is used directly in synchronous
>>>>>>>>>>> copies. The
>>>>>>>>>>> other is dynamically allocated, refcounted and tracked in the client
>>>>>>>>>>> struture. For the embedded one, the cleanup just involves
>>>>>>>>>>> releasing any
>>>>>>>>>>> nfsd_files held on its behalf. For the async one, the cleanup is
>>>>>>>>>>> a bit
>>>>>>>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
>>>>>>>>>>>
>>>>>>>>>>> There is at least one potential refcount leak in this code now.
>>>>>>>>>>> If the
>>>>>>>>>>> kthread_create call fails, then both the src and dst nfsd_files
>>>>>>>>>>> in the
>>>>>>>>>>> original nfsd4_copy object are leaked.
>>>>>>>>>>>
>>>>>>>>>>> The cleanup in this codepath is also sort of weird. In the async
>>>>>>>>>>> copy
>>>>>>>>>>> case, we'll have up to four nfsd_file references (src and dst for
>>>>>>>>>>> both
>>>>>>>>>>> flavors of copy structure). They are both put at the end of
>>>>>>>>>>> nfsd4_do_async_copy, even though the ones held on behalf of the
>>>>>>>>>>> embedded
>>>>>>>>>>> one outlive that structure.
>>>>>>>>>>>
>>>>>>>>>>> Change it so that we always clean up the nfsd_file refs held by the
>>>>>>>>>>> embedded copy structure before nfsd4_copy returns. Rework
>>>>>>>>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
>>>>>>>>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Jeff Layton <[email protected]>
>>>>>>>>>>> ---
>>>>>>>>>>> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
>>>>>>>>>>> 1 file changed, 10 insertions(+), 13 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>>>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
>>>>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>>>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>>>>>> nfs42_ssc_close(filp);
>>>>>>>>>>> - nfsd_file_put(dst);
>>>>>>>>>> I think we still need this, in addition to release_copy_files called
>>>>>>>>>> from cleanup_async_copy. For async inter-copy, there are 2 reference
>>>>>>>>>> count added to the destination file, one from nfsd4_setup_inter_ssc
>>>>>>>>>> and the other one from dup_copy_fields. The above nfsd_file_put is
>>>>>>>>>> for
>>>>>>>>>> the count added by dup_copy_fields.
>>>>>>>>>>
>>>>>>>>> With this patch, the references held by the original copy structure
>>>>>>>>> are
>>>>>>>>> put by the call to release_copy_files at the end of nfsd4_copy. That
>>>>>>>>> means that the kthread task is only responsible for putting the
>>>>>>>>> references held by the (kmalloc'ed) async_copy structure. So, I think
>>>>>>>>> this gets the nfsd_file refcounting right.
>>>>>>>> Yes, I see. One refcount is decremented by release_copy_files at end
>>>>>>>> of nfsd4_copy and another is decremented by release_copy_files in
>>>>>>>> cleanup_async_copy.
>>>>>>>>
>>>>>>>>>>> fput(filp);
>>>>>>>>>>> spin_lock(&nn->nfsd_ssc_lock);
>>>>>>>>>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
>>>>>>>>>>> &copy->nf_dst);
>>>>>>>>>>> }
>>>>>>>>>>> -static void
>>>>>>>>>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
>>>>>>>>>>> *dst)
>>>>>>>>>>> -{
>>>>>>>>>>> - nfsd_file_put(src);
>>>>>>>>>>> - nfsd_file_put(dst);
>>>>>>>>>>> -}
>>>>>>>>>>> -
>>>>>>>>>>> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
>>>>>>>>>>> {
>>>>>>>>>>> struct nfsd4_cb_offload *cbo =
>>>>>>>>>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
>>>>>>>>>>> nfsd4_copy *src, struct nfsd4_copy *dst)
>>>>>>>>>>> dst->ss_nsui = src->ss_nsui;
>>>>>>>>>>> }
>>>>>>>>>>> +static void release_copy_files(struct nfsd4_copy *copy)
>>>>>>>>>>> +{
>>>>>>>>>>> + if (copy->nf_src)
>>>>>>>>>>> + nfsd_file_put(copy->nf_src);
>>>>>>>>>>> + if (copy->nf_dst)
>>>>>>>>>>> + nfsd_file_put(copy->nf_dst);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> static void cleanup_async_copy(struct nfsd4_copy *copy)
>>>>>>>>>>> {
>>>>>>>>>>> nfs4_free_copy_state(copy);
>>>>>>>>>>> - nfsd_file_put(copy->nf_dst);
>>>>>>>>>>> - if (!nfsd4_ssc_is_inter(copy))
>>>>>>>>>>> - nfsd_file_put(copy->nf_src);
>>>>>>>>>>> + release_copy_files(copy);
>>>>>>>>>>> spin_lock(&copy->cp_clp->async_lock);
>>>>>>>>>>> list_del(&copy->copies);
>>>>>>>>>>> spin_unlock(&copy->cp_clp->async_lock);
>>>>>>>>>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
>>>>>>>>>>> } else {
>>>>>>>>>>> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>>>>> copy->nf_dst->nf_file, false);
>>>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>>>>> }
>>>>>>>>>>> do_callback:
>>>>>>>>>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>>>>>>> nfsd4_compound_state *cstate,
>>>>>>>>>>> } else {
>>>>>>>>>>> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>>>>>>>>>>> copy->nf_dst->nf_file, true);
>>>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
>>>>>>>>>>> }
>>>>>>>>>>> out:
>>>>>>>>>>> + release_copy_files(copy);
>>>>>>>>>>> return status;
>>>>>>>>>>> out_err:
>>>>>>>>>> This is unrelated to the reference count issue.
>>>>>>>>>>
>>>>>>>>>> Here if this is an inter-copy then we need to decrement the reference
>>>>>>>>>> count of the nfsd4_ssc_umount_item so that the vfsmount can be
>>>>>>>>>> unmounted
>>>>>>>>>> later.
>>>>>>>>>>
>>>>>>>>> Oh, I think I see what you mean. Maybe something like the (untested)
>>>>>>>>> patch below on top of the original patch would fix that?
>>>>>>>>>
>>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>>>> index c9057462b973..7475c593553c 100644
>>>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>>>> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
>>>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
>>>>>>>>> struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
>>>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
>>>>>>>>> - nfs42_ssc_close(filp);
>>>>>>>>> - fput(filp);
>>>>>>>>> + if (filp) {
>>>>>>>>> + nfs42_ssc_close(filp);
>>>>>>>>> + fput(filp);
>>>>>>>>> + }
>>>>>>>>> spin_lock(&nn->nfsd_ssc_lo
>>>>>>>>> list_del(&nsui->nsui_list);
>>>>>>>>> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
>>>>>>>>> nfsd4_compound_state *cstate,
>>>>>>>>> release_copy_files(copy);
>>>>>>>>> return status;
>>>>>>>>> out_err:
>>>>>>>>> - if (async_copy)
>>>>>>>>> + if (async_copy) {
>>>>>>>>> cleanup_async_copy(async_copy);
>>>>>>>>> + if (nfsd4_ssc_is_inter(async_copy))
>>>>>>>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
>>>>>>>> nfsd4_do_async_copy has not started yet so the file is not opened.
>>>>>>>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
>>>>>>>> you want to change nfsd4_cleanup_inter_ssc to detect this error
>>>>>>>> condition and only decrement the reference count.
>>>>>>>>
>>>>>>> Oh yeah, and this would break anyway since the nsui_list head is not
>>>>>>> being initialized. Dai, would you mind spinning up a patch for this
>>>>>>> since you're more familiar with the cleanup here?
>>>>>> Will do. My patch will only fix the unmount issue. Your patch does
>>>>>> the clean up potential nfsd_file refcount leaks in COPY codepath.
>>>>> Or do you want me to merge your patch and mine into one?
>>>>>
>>>> It probably is best to merge them, since backporters will probably want
>>>> both patches anyway.
>>> Unless these two changes are somehow interdependent, I'd like to keep
>>> them separate. They address two separate issues, yes?
>>
>> Yes.
>>
>>>
>>> And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
>>> for nfsd-next. I'd rather not mix the two types of change.
>>
>> Ok. Can we do this:
>>
>> 1. Jeff's patch goes to nfsd-fixes since it has the fix for missing
>> reference count.
>
> To make sure I haven't lost track of anything:
>
> The patch you refer to here is this one:
>
> https://lore.kernel.org/linux-nfs/[email protected]/
>
> Correct?
>
> (I was waiting for Jeff and Olga to come to consensus, and I think
> they have, so I can apply it to nfsd-fixes now).

Or not...

This one does not apply cleanly to nfsd-fixes, but does apply to nfsd-next.
Also, the patch description says "clean up" and does not provide a Fixes:
tag. So, either:

- Jeff needs to test and redrive this patch against nfsd-fixes if we all
agree that it fixes a real and urgent bug, not a potential one; or

- I will apply it as it stands to nfsd-next; or

- You were referring to something else in 1. above.

Let me know how you'd both like to proceed.

>> 2. My fix for the cleanup of allocated memory goes to nfsd-fixes.
>
> And this one hasn't been posted yet, right? Or did I miss it?
>
>
>> 3. I will do the optimization Jeff proposed about list_head and
>> nfsd4_compound in a separate patch that goes into nfsd-next.
>
> That should be fine.
>
>
>> -Dai
>>
>>>> Just make yourself the patch author and keep my S-o-b line.
>>>>
>>>>> I think we need a bit more cleanup in addition to your patch. When
>>>>> kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
>>>>> fails, the async_copy is not initialized yet so calling cleanup_async_copy
>>>>> can be a problem.
>>>>>
>>>> Yeah.
>>>>
>>>> It may even be best to ensure that the list_head and such are fully
>>>> initialized for both allocated and embedded struct nfsd4_copy's. You
>>>> might shave off a few cpu cycles by not doing that, but it makes things
>>>> more fragile.
>>>>
>>>> Even better, we really ought to split a lot of the fields in nfsd4_copy
>>>> into a different structure (maybe nfsd4_async_copy). Trimming down
>>>> struct nfsd4_copy would cut down the size of nfsd4_compound as well
>>>> since it has a union that contains it. I was planning on doing that
>>>> eventually, but if you want to take that on, then that would be fine
>>>> too.
>>>>
>>>> --
>>>> Jeff Layton <[email protected]>
>>> --
>>> Chuck Lever
>
> --
> Chuck Lever
>
>
>

--
Chuck Lever

2023-01-23 12:17:18

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Sun, 2023-01-22 at 17:10 +0000, Chuck Lever III wrote:
>
> > On Jan 22, 2023, at 11:45 AM, Chuck Lever III <[email protected]> wrote:
> >
> >
> >
> > > On Jan 21, 2023, at 4:28 PM, Dai Ngo <[email protected]> wrote:
> > >
> > >
> > > On 1/21/23 12:12 PM, Chuck Lever III wrote:
> > > >
> > > > > On Jan 21, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
> > > > >
> > > > > On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
> > > > > > On 1/21/23 10:56 AM, [email protected] wrote:
> > > > > > > On 1/20/23 3:43 AM, Jeff Layton wrote:
> > > > > > > > On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
> > > > > > > > > On 1/19/23 2:56 AM, Jeff Layton wrote:
> > > > > > > > > > On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
> > > > > > > > > > > On 1/17/23 11:38 AM, Jeff Layton wrote:
> > > > > > > > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > > > > > > > embedded in the compound and is used directly in synchronous
> > > > > > > > > > > > copies. The
> > > > > > > > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > > > > > > > struture. For the embedded one, the cleanup just involves
> > > > > > > > > > > > releasing any
> > > > > > > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is
> > > > > > > > > > > > a bit
> > > > > > > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > > > > > > > >
> > > > > > > > > > > > There is at least one potential refcount leak in this code now.
> > > > > > > > > > > > If the
> > > > > > > > > > > > kthread_create call fails, then both the src and dst nfsd_files
> > > > > > > > > > > > in the
> > > > > > > > > > > > original nfsd4_copy object are leaked.
> > > > > > > > > > > >
> > > > > > > > > > > > The cleanup in this codepath is also sort of weird. In the async
> > > > > > > > > > > > copy
> > > > > > > > > > > > case, we'll have up to four nfsd_file references (src and dst for
> > > > > > > > > > > > both
> > > > > > > > > > > > flavors of copy structure). They are both put at the end of
> > > > > > > > > > > > nfsd4_do_async_copy, even though the ones held on behalf of the
> > > > > > > > > > > > embedded
> > > > > > > > > > > > one outlive that structure.
> > > > > > > > > > > >
> > > > > > > > > > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > > > > > > > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > > > > > > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > > > > > > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > > > > > > > ---
> > > > > > > > > > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > > > > > > > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > > > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > > > > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > > > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
> > > > > > > > > > > > nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > > > > > > > > nfs42_ssc_close(filp);
> > > > > > > > > > > > - nfsd_file_put(dst);
> > > > > > > > > > > I think we still need this, in addition to release_copy_files called
> > > > > > > > > > > from cleanup_async_copy. For async inter-copy, there are 2 reference
> > > > > > > > > > > count added to the destination file, one from nfsd4_setup_inter_ssc
> > > > > > > > > > > and the other one from dup_copy_fields. The above nfsd_file_put is
> > > > > > > > > > > for
> > > > > > > > > > > the count added by dup_copy_fields.
> > > > > > > > > > >
> > > > > > > > > > With this patch, the references held by the original copy structure
> > > > > > > > > > are
> > > > > > > > > > put by the call to release_copy_files at the end of nfsd4_copy. That
> > > > > > > > > > means that the kthread task is only responsible for putting the
> > > > > > > > > > references held by the (kmalloc'ed) async_copy structure. So, I think
> > > > > > > > > > this gets the nfsd_file refcounting right.
> > > > > > > > > Yes, I see. One refcount is decremented by release_copy_files at end
> > > > > > > > > of nfsd4_copy and another is decremented by release_copy_files in
> > > > > > > > > cleanup_async_copy.
> > > > > > > > >
> > > > > > > > > > > > fput(filp);
> > > > > > > > > > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > > > > > > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > > > > > > > > > &copy->nf_dst);
> > > > > > > > > > > > }
> > > > > > > > > > > > -static void
> > > > > > > > > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
> > > > > > > > > > > > *dst)
> > > > > > > > > > > > -{
> > > > > > > > > > > > - nfsd_file_put(src);
> > > > > > > > > > > > - nfsd_file_put(dst);
> > > > > > > > > > > > -}
> > > > > > > > > > > > -
> > > > > > > > > > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > > > > > > > > > {
> > > > > > > > > > > > struct nfsd4_cb_offload *cbo =
> > > > > > > > > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
> > > > > > > > > > > > nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > > > > > > > > > dst->ss_nsui = src->ss_nsui;
> > > > > > > > > > > > }
> > > > > > > > > > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + if (copy->nf_src)
> > > > > > > > > > > > + nfsd_file_put(copy->nf_src);
> > > > > > > > > > > > + if (copy->nf_dst)
> > > > > > > > > > > > + nfsd_file_put(copy->nf_dst);
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > > > > > > > > > {
> > > > > > > > > > > > nfs4_free_copy_state(copy);
> > > > > > > > > > > > - nfsd_file_put(copy->nf_dst);
> > > > > > > > > > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > > > > > > > > > - nfsd_file_put(copy->nf_src);
> > > > > > > > > > > > + release_copy_files(copy);
> > > > > > > > > > > > spin_lock(&copy->cp_clp->async_lock);
> > > > > > > > > > > > list_del(&copy->copies);
> > > > > > > > > > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > > > > > > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > > > > > > > > > } else {
> > > > > > > > > > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > > > > > > copy->nf_dst->nf_file, false);
> > > > > > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > > > > > > }
> > > > > > > > > > > > do_callback:
> > > > > > > > > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > > > > > > > > > > > nfsd4_compound_state *cstate,
> > > > > > > > > > > > } else {
> > > > > > > > > > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > > > > > > copy->nf_dst->nf_file, true);
> > > > > > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > > > > > > }
> > > > > > > > > > > > out:
> > > > > > > > > > > > + release_copy_files(copy);
> > > > > > > > > > > > return status;
> > > > > > > > > > > > out_err:
> > > > > > > > > > > This is unrelated to the reference count issue.
> > > > > > > > > > >
> > > > > > > > > > > Here if this is an inter-copy then we need to decrement the reference
> > > > > > > > > > > count of the nfsd4_ssc_umount_item so that the vfsmount can be
> > > > > > > > > > > unmounted
> > > > > > > > > > > later.
> > > > > > > > > > >
> > > > > > > > > > Oh, I think I see what you mean. Maybe something like the (untested)
> > > > > > > > > > patch below on top of the original patch would fix that?
> > > > > > > > > >
> > > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > > > > index c9057462b973..7475c593553c 100644
> > > > > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > > > > @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
> > > > > > > > > > nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > > > > > struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
> > > > > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > > > > > > - nfs42_ssc_close(filp);
> > > > > > > > > > - fput(filp);
> > > > > > > > > > + if (filp) {
> > > > > > > > > > + nfs42_ssc_close(filp);
> > > > > > > > > > + fput(filp);
> > > > > > > > > > + }
> > > > > > > > > > spin_lock(&nn->nfsd_ssc_lo
> > > > > > > > > > list_del(&nsui->nsui_list);
> > > > > > > > > > @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > > > > > > > > > nfsd4_compound_state *cstate,
> > > > > > > > > > release_copy_files(copy);
> > > > > > > > > > return status;
> > > > > > > > > > out_err:
> > > > > > > > > > - if (async_copy)
> > > > > > > > > > + if (async_copy) {
> > > > > > > > > > cleanup_async_copy(async_copy);
> > > > > > > > > > + if (nfsd4_ssc_is_inter(async_copy))
> > > > > > > > > We don't need to call nfsd4_cleanup_inter_ssc since the thread
> > > > > > > > > nfsd4_do_async_copy has not started yet so the file is not opened.
> > > > > > > > > We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
> > > > > > > > > you want to change nfsd4_cleanup_inter_ssc to detect this error
> > > > > > > > > condition and only decrement the reference count.
> > > > > > > > >
> > > > > > > > Oh yeah, and this would break anyway since the nsui_list head is not
> > > > > > > > being initialized. Dai, would you mind spinning up a patch for this
> > > > > > > > since you're more familiar with the cleanup here?
> > > > > > > Will do. My patch will only fix the unmount issue. Your patch does
> > > > > > > the clean up potential nfsd_file refcount leaks in COPY codepath.
> > > > > > Or do you want me to merge your patch and mine into one?
> > > > > >
> > > > > It probably is best to merge them, since backporters will probably want
> > > > > both patches anyway.
> > > > Unless these two changes are somehow interdependent, I'd like to keep
> > > > them separate. They address two separate issues, yes?
> > >
> > > Yes.
> > >
> > > >
> > > > And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
> > > > for nfsd-next. I'd rather not mix the two types of change.
> > >
> > > Ok. Can we do this:
> > >
> > > 1. Jeff's patch goes to nfsd-fixes since it has the fix for missing
> > > reference count.
> >
> > To make sure I haven't lost track of anything:
> >
> > The patch you refer to here is this one:
> >
> > https://lore.kernel.org/linux-nfs/[email protected]/
> >
> > Correct?
> >
> > (I was waiting for Jeff and Olga to come to consensus, and I think
> > they have, so I can apply it to nfsd-fixes now).
>
> Or not...
>
> This one does not apply cleanly to nfsd-fixes, but does apply to nfsd-next.
> Also, the patch description says "clean up" and does not provide a Fixes:
> tag. So, either:
>
> - Jeff needs to test and redrive this patch against nfsd-fixes if we all
> agree that it fixes a real and urgent bug, not a potential one; or
>
> - I will apply it as it stands to nfsd-next; or
>
> - You were referring to something else in 1. above.
>
> Let me know how you'd both like to proceed.
>

I'm fine with nfsd-next here. These are not a bugs that people are going
to hit under normal circumstances. It's something we need to fix, but
it's not urgent.
--
Jeff Layton <[email protected]>

2023-01-23 15:23:37

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Sun, Jan 22, 2023 at 11:46 AM Chuck Lever III <[email protected]> wrote:
>
>
>
> > On Jan 21, 2023, at 4:28 PM, Dai Ngo <[email protected]> wrote:
> >
> >
> > On 1/21/23 12:12 PM, Chuck Lever III wrote:
> >>
> >>> On Jan 21, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
> >>>
> >>> On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
> >>>> On 1/21/23 10:56 AM, [email protected] wrote:
> >>>>> On 1/20/23 3:43 AM, Jeff Layton wrote:
> >>>>>> On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
> >>>>>>> On 1/19/23 2:56 AM, Jeff Layton wrote:
> >>>>>>>> On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
> >>>>>>>>> On 1/17/23 11:38 AM, Jeff Layton wrote:
> >>>>>>>>>> There are two different flavors of the nfsd4_copy struct. One is
> >>>>>>>>>> embedded in the compound and is used directly in synchronous
> >>>>>>>>>> copies. The
> >>>>>>>>>> other is dynamically allocated, refcounted and tracked in the client
> >>>>>>>>>> struture. For the embedded one, the cleanup just involves
> >>>>>>>>>> releasing any
> >>>>>>>>>> nfsd_files held on its behalf. For the async one, the cleanup is
> >>>>>>>>>> a bit
> >>>>>>>>>> more involved, and we need to dequeue it from lists, unhash it, etc.
> >>>>>>>>>>
> >>>>>>>>>> There is at least one potential refcount leak in this code now.
> >>>>>>>>>> If the
> >>>>>>>>>> kthread_create call fails, then both the src and dst nfsd_files
> >>>>>>>>>> in the
> >>>>>>>>>> original nfsd4_copy object are leaked.
> >>>>>>>>>>
> >>>>>>>>>> The cleanup in this codepath is also sort of weird. In the async
> >>>>>>>>>> copy
> >>>>>>>>>> case, we'll have up to four nfsd_file references (src and dst for
> >>>>>>>>>> both
> >>>>>>>>>> flavors of copy structure). They are both put at the end of
> >>>>>>>>>> nfsd4_do_async_copy, even though the ones held on behalf of the
> >>>>>>>>>> embedded
> >>>>>>>>>> one outlive that structure.
> >>>>>>>>>>
> >>>>>>>>>> Change it so that we always clean up the nfsd_file refs held by the
> >>>>>>>>>> embedded copy structure before nfsd4_copy returns. Rework
> >>>>>>>>>> cleanup_async_copy to handle both inter and intra copies. Eliminate
> >>>>>>>>>> nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Jeff Layton <[email protected]>
> >>>>>>>>>> ---
> >>>>>>>>>> fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> >>>>>>>>>> 1 file changed, 10 insertions(+), 13 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>>>>>>>>> index 37a9cc8ae7ae..62b9d6c1b18b 100644
> >>>>>>>>>> --- a/fs/nfsd/nfs4proc.c
> >>>>>>>>>> +++ b/fs/nfsd/nfs4proc.c
> >>>>>>>>>> @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
> >>>>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
> >>>>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> >>>>>>>>>> nfs42_ssc_close(filp);
> >>>>>>>>>> - nfsd_file_put(dst);
> >>>>>>>>> I think we still need this, in addition to release_copy_files called
> >>>>>>>>> from cleanup_async_copy. For async inter-copy, there are 2 reference
> >>>>>>>>> count added to the destination file, one from nfsd4_setup_inter_ssc
> >>>>>>>>> and the other one from dup_copy_fields. The above nfsd_file_put is
> >>>>>>>>> for
> >>>>>>>>> the count added by dup_copy_fields.
> >>>>>>>>>
> >>>>>>>> With this patch, the references held by the original copy structure
> >>>>>>>> are
> >>>>>>>> put by the call to release_copy_files at the end of nfsd4_copy. That
> >>>>>>>> means that the kthread task is only responsible for putting the
> >>>>>>>> references held by the (kmalloc'ed) async_copy structure. So, I think
> >>>>>>>> this gets the nfsd_file refcounting right.
> >>>>>>> Yes, I see. One refcount is decremented by release_copy_files at end
> >>>>>>> of nfsd4_copy and another is decremented by release_copy_files in
> >>>>>>> cleanup_async_copy.
> >>>>>>>
> >>>>>>>>>> fput(filp);
> >>>>>>>>>> spin_lock(&nn->nfsd_ssc_lock);
> >>>>>>>>>> @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> >>>>>>>>>> &copy->nf_dst);
> >>>>>>>>>> }
> >>>>>>>>>> -static void
> >>>>>>>>>> -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
> >>>>>>>>>> *dst)
> >>>>>>>>>> -{
> >>>>>>>>>> - nfsd_file_put(src);
> >>>>>>>>>> - nfsd_file_put(dst);
> >>>>>>>>>> -}
> >>>>>>>>>> -
> >>>>>>>>>> static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> >>>>>>>>>> {
> >>>>>>>>>> struct nfsd4_cb_offload *cbo =
> >>>>>>>>>> @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
> >>>>>>>>>> nfsd4_copy *src, struct nfsd4_copy *dst)
> >>>>>>>>>> dst->ss_nsui = src->ss_nsui;
> >>>>>>>>>> }
> >>>>>>>>>> +static void release_copy_files(struct nfsd4_copy *copy)
> >>>>>>>>>> +{
> >>>>>>>>>> + if (copy->nf_src)
> >>>>>>>>>> + nfsd_file_put(copy->nf_src);
> >>>>>>>>>> + if (copy->nf_dst)
> >>>>>>>>>> + nfsd_file_put(copy->nf_dst);
> >>>>>>>>>> +}
> >>>>>>>>>> +
> >>>>>>>>>> static void cleanup_async_copy(struct nfsd4_copy *copy)
> >>>>>>>>>> {
> >>>>>>>>>> nfs4_free_copy_state(copy);
> >>>>>>>>>> - nfsd_file_put(copy->nf_dst);
> >>>>>>>>>> - if (!nfsd4_ssc_is_inter(copy))
> >>>>>>>>>> - nfsd_file_put(copy->nf_src);
> >>>>>>>>>> + release_copy_files(copy);
> >>>>>>>>>> spin_lock(&copy->cp_clp->async_lock);
> >>>>>>>>>> list_del(&copy->copies);
> >>>>>>>>>> spin_unlock(&copy->cp_clp->async_lock);
> >>>>>>>>>> @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> >>>>>>>>>> } else {
> >>>>>>>>>> nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> >>>>>>>>>> copy->nf_dst->nf_file, false);
> >>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> >>>>>>>>>> }
> >>>>>>>>>> do_callback:
> >>>>>>>>>> @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> >>>>>>>>>> nfsd4_compound_state *cstate,
> >>>>>>>>>> } else {
> >>>>>>>>>> status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> >>>>>>>>>> copy->nf_dst->nf_file, true);
> >>>>>>>>>> - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> >>>>>>>>>> }
> >>>>>>>>>> out:
> >>>>>>>>>> + release_copy_files(copy);
> >>>>>>>>>> return status;
> >>>>>>>>>> out_err:
> >>>>>>>>> This is unrelated to the reference count issue.
> >>>>>>>>>
> >>>>>>>>> Here if this is an inter-copy then we need to decrement the reference
> >>>>>>>>> count of the nfsd4_ssc_umount_item so that the vfsmount can be
> >>>>>>>>> unmounted
> >>>>>>>>> later.
> >>>>>>>>>
> >>>>>>>> Oh, I think I see what you mean. Maybe something like the (untested)
> >>>>>>>> patch below on top of the original patch would fix that?
> >>>>>>>>
> >>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> >>>>>>>> index c9057462b973..7475c593553c 100644
> >>>>>>>> --- a/fs/nfsd/nfs4proc.c
> >>>>>>>> +++ b/fs/nfsd/nfs4proc.c
> >>>>>>>> @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
> >>>>>>>> nfsd4_ssc_umount_item *nsui, struct file *filp,
> >>>>>>>> struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
> >>>>>>>> long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> >>>>>>>> - nfs42_ssc_close(filp);
> >>>>>>>> - fput(filp);
> >>>>>>>> + if (filp) {
> >>>>>>>> + nfs42_ssc_close(filp);
> >>>>>>>> + fput(filp);
> >>>>>>>> + }
> >>>>>>>> spin_lock(&nn->nfsd_ssc_lo
> >>>>>>>> list_del(&nsui->nsui_list);
> >>>>>>>> @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> >>>>>>>> nfsd4_compound_state *cstate,
> >>>>>>>> release_copy_files(copy);
> >>>>>>>> return status;
> >>>>>>>> out_err:
> >>>>>>>> - if (async_copy)
> >>>>>>>> + if (async_copy) {
> >>>>>>>> cleanup_async_copy(async_copy);
> >>>>>>>> + if (nfsd4_ssc_is_inter(async_copy))
> >>>>>>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
> >>>>>>> nfsd4_do_async_copy has not started yet so the file is not opened.
> >>>>>>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
> >>>>>>> you want to change nfsd4_cleanup_inter_ssc to detect this error
> >>>>>>> condition and only decrement the reference count.
> >>>>>>>
> >>>>>> Oh yeah, and this would break anyway since the nsui_list head is not
> >>>>>> being initialized. Dai, would you mind spinning up a patch for this
> >>>>>> since you're more familiar with the cleanup here?
> >>>>> Will do. My patch will only fix the unmount issue. Your patch does
> >>>>> the clean up potential nfsd_file refcount leaks in COPY codepath.
> >>>> Or do you want me to merge your patch and mine into one?
> >>>>
> >>> It probably is best to merge them, since backporters will probably want
> >>> both patches anyway.
> >> Unless these two changes are somehow interdependent, I'd like to keep
> >> them separate. They address two separate issues, yes?
> >
> > Yes.
> >
> >>
> >> And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
> >> for nfsd-next. I'd rather not mix the two types of change.
> >
> > Ok. Can we do this:
> >
> > 1. Jeff's patch goes to nfsd-fixes since it has the fix for missing
> > reference count.
>
> To make sure I haven't lost track of anything:
>
> The patch you refer to here is this one:
>
> https://lore.kernel.org/linux-nfs/[email protected]/
>
> Correct?
>
> (I was waiting for Jeff and Olga to come to consensus, and I think
> they have, so I can apply it to nfsd-fixes now).

Sorry folks but I got a bit lost in the thread. I thought Dai pointed
out that we can't remove the put from the nfsd4_cleanup_inter_ssc()
because that's the put for the copied structure and not the original
file references which is what Jeff's patch is trying to address.

> > 2. My fix for the cleanup of allocated memory goes to nfsd-fixes.
>
> And this one hasn't been posted yet, right? Or did I miss it?
>
>
> > 3. I will do the optimization Jeff proposed about list_head and
> > nfsd4_compound in a separate patch that goes into nfsd-next.
>
> That should be fine.
>
>
> > -Dai
> >
> >>> Just make yourself the patch author and keep my S-o-b line.
> >>>
> >>>> I think we need a bit more cleanup in addition to your patch. When
> >>>> kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
> >>>> fails, the async_copy is not initialized yet so calling cleanup_async_copy
> >>>> can be a problem.
> >>>>
> >>> Yeah.
> >>>
> >>> It may even be best to ensure that the list_head and such are fully
> >>> initialized for both allocated and embedded struct nfsd4_copy's. You
> >>> might shave off a few cpu cycles by not doing that, but it makes things
> >>> more fragile.
> >>>
> >>> Even better, we really ought to split a lot of the fields in nfsd4_copy
> >>> into a different structure (maybe nfsd4_async_copy). Trimming down
> >>> struct nfsd4_copy would cut down the size of nfsd4_compound as well
> >>> since it has a union that contains it. I was planning on doing that
> >>> eventually, but if you want to take that on, then that would be fine
> >>> too.
> >>>
> >>> --
> >>> Jeff Layton <[email protected]>
> >> --
> >> Chuck Lever
>
> --
> Chuck Lever
>
>
>

2023-01-23 15:32:51

[permalink] [raw]

Subject: Re: [PATCH 2/2] nfsd: clean up potential nfsd_file refcount leaks in COPY codepath

On Mon, 2023-01-23 at 10:22 -0500, Olga Kornievskaia wrote:
> On Sun, Jan 22, 2023 at 11:46 AM Chuck Lever III <[email protected]> wrote:
> >
> >
> >
> > > On Jan 21, 2023, at 4:28 PM, Dai Ngo <[email protected]> wrote:
> > >
> > >
> > > On 1/21/23 12:12 PM, Chuck Lever III wrote:
> > > >
> > > > > On Jan 21, 2023, at 3:05 PM, Jeff Layton <[email protected]> wrote:
> > > > >
> > > > > On Sat, 2023-01-21 at 11:50 -0800, [email protected] wrote:
> > > > > > On 1/21/23 10:56 AM, [email protected] wrote:
> > > > > > > On 1/20/23 3:43 AM, Jeff Layton wrote:
> > > > > > > > On Thu, 2023-01-19 at 10:38 -0800, [email protected] wrote:
> > > > > > > > > On 1/19/23 2:56 AM, Jeff Layton wrote:
> > > > > > > > > > On Wed, 2023-01-18 at 21:05 -0800, [email protected] wrote:
> > > > > > > > > > > On 1/17/23 11:38 AM, Jeff Layton wrote:
> > > > > > > > > > > > There are two different flavors of the nfsd4_copy struct. One is
> > > > > > > > > > > > embedded in the compound and is used directly in synchronous
> > > > > > > > > > > > copies. The
> > > > > > > > > > > > other is dynamically allocated, refcounted and tracked in the client
> > > > > > > > > > > > struture. For the embedded one, the cleanup just involves
> > > > > > > > > > > > releasing any
> > > > > > > > > > > > nfsd_files held on its behalf. For the async one, the cleanup is
> > > > > > > > > > > > a bit
> > > > > > > > > > > > more involved, and we need to dequeue it from lists, unhash it, etc.
> > > > > > > > > > > >
> > > > > > > > > > > > There is at least one potential refcount leak in this code now.
> > > > > > > > > > > > If the
> > > > > > > > > > > > kthread_create call fails, then both the src and dst nfsd_files
> > > > > > > > > > > > in the
> > > > > > > > > > > > original nfsd4_copy object are leaked.
> > > > > > > > > > > >
> > > > > > > > > > > > The cleanup in this codepath is also sort of weird. In the async
> > > > > > > > > > > > copy
> > > > > > > > > > > > case, we'll have up to four nfsd_file references (src and dst for
> > > > > > > > > > > > both
> > > > > > > > > > > > flavors of copy structure). They are both put at the end of
> > > > > > > > > > > > nfsd4_do_async_copy, even though the ones held on behalf of the
> > > > > > > > > > > > embedded
> > > > > > > > > > > > one outlive that structure.
> > > > > > > > > > > >
> > > > > > > > > > > > Change it so that we always clean up the nfsd_file refs held by the
> > > > > > > > > > > > embedded copy structure before nfsd4_copy returns. Rework
> > > > > > > > > > > > cleanup_async_copy to handle both inter and intra copies. Eliminate
> > > > > > > > > > > > nfsd4_cleanup_intra_ssc since it now becomes a no-op.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Jeff Layton <[email protected]>
> > > > > > > > > > > > ---
> > > > > > > > > > > > fs/nfsd/nfs4proc.c | 23 ++++++++++-------------
> > > > > > > > > > > > 1 file changed, 10 insertions(+), 13 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > > > > > > index 37a9cc8ae7ae..62b9d6c1b18b 100644
> > > > > > > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > > > > > > @@ -1512,7 +1512,6 @@ nfsd4_cleanup_inter_ssc(struct
> > > > > > > > > > > > nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > > > > > > > > nfs42_ssc_close(filp);
> > > > > > > > > > > > - nfsd_file_put(dst);
> > > > > > > > > > > I think we still need this, in addition to release_copy_files called
> > > > > > > > > > > from cleanup_async_copy. For async inter-copy, there are 2 reference
> > > > > > > > > > > count added to the destination file, one from nfsd4_setup_inter_ssc
> > > > > > > > > > > and the other one from dup_copy_fields. The above nfsd_file_put is
> > > > > > > > > > > for
> > > > > > > > > > > the count added by dup_copy_fields.
> > > > > > > > > > >
> > > > > > > > > > With this patch, the references held by the original copy structure
> > > > > > > > > > are
> > > > > > > > > > put by the call to release_copy_files at the end of nfsd4_copy. That
> > > > > > > > > > means that the kthread task is only responsible for putting the
> > > > > > > > > > references held by the (kmalloc'ed) async_copy structure. So, I think
> > > > > > > > > > this gets the nfsd_file refcounting right.
> > > > > > > > > Yes, I see. One refcount is decremented by release_copy_files at end
> > > > > > > > > of nfsd4_copy and another is decremented by release_copy_files in
> > > > > > > > > cleanup_async_copy.
> > > > > > > > >
> > > > > > > > > > > > fput(filp);
> > > > > > > > > > > > spin_lock(&nn->nfsd_ssc_lock);
> > > > > > > > > > > > @@ -1562,13 +1561,6 @@ nfsd4_setup_intra_ssc(struct svc_rqst *rqstp,
> > > > > > > > > > > > &copy->nf_dst);
> > > > > > > > > > > > }
> > > > > > > > > > > > -static void
> > > > > > > > > > > > -nfsd4_cleanup_intra_ssc(struct nfsd_file *src, struct nfsd_file
> > > > > > > > > > > > *dst)
> > > > > > > > > > > > -{
> > > > > > > > > > > > - nfsd_file_put(src);
> > > > > > > > > > > > - nfsd_file_put(dst);
> > > > > > > > > > > > -}
> > > > > > > > > > > > -
> > > > > > > > > > > > static void nfsd4_cb_offload_release(struct nfsd4_callback *cb)
> > > > > > > > > > > > {
> > > > > > > > > > > > struct nfsd4_cb_offload *cbo =
> > > > > > > > > > > > @@ -1683,12 +1675,18 @@ static void dup_copy_fields(struct
> > > > > > > > > > > > nfsd4_copy *src, struct nfsd4_copy *dst)
> > > > > > > > > > > > dst->ss_nsui = src->ss_nsui;
> > > > > > > > > > > > }
> > > > > > > > > > > > +static void release_copy_files(struct nfsd4_copy *copy)
> > > > > > > > > > > > +{
> > > > > > > > > > > > + if (copy->nf_src)
> > > > > > > > > > > > + nfsd_file_put(copy->nf_src);
> > > > > > > > > > > > + if (copy->nf_dst)
> > > > > > > > > > > > + nfsd_file_put(copy->nf_dst);
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > static void cleanup_async_copy(struct nfsd4_copy *copy)
> > > > > > > > > > > > {
> > > > > > > > > > > > nfs4_free_copy_state(copy);
> > > > > > > > > > > > - nfsd_file_put(copy->nf_dst);
> > > > > > > > > > > > - if (!nfsd4_ssc_is_inter(copy))
> > > > > > > > > > > > - nfsd_file_put(copy->nf_src);
> > > > > > > > > > > > + release_copy_files(copy);
> > > > > > > > > > > > spin_lock(&copy->cp_clp->async_lock);
> > > > > > > > > > > > list_del(&copy->copies);
> > > > > > > > > > > > spin_unlock(&copy->cp_clp->async_lock);
> > > > > > > > > > > > @@ -1748,7 +1746,6 @@ static int nfsd4_do_async_copy(void *data)
> > > > > > > > > > > > } else {
> > > > > > > > > > > > nfserr = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > > > > > > copy->nf_dst->nf_file, false);
> > > > > > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > > > > > > }
> > > > > > > > > > > > do_callback:
> > > > > > > > > > > > @@ -1811,9 +1808,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > > > > > > > > > > > nfsd4_compound_state *cstate,
> > > > > > > > > > > > } else {
> > > > > > > > > > > > status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> > > > > > > > > > > > copy->nf_dst->nf_file, true);
> > > > > > > > > > > > - nfsd4_cleanup_intra_ssc(copy->nf_src, copy->nf_dst);
> > > > > > > > > > > > }
> > > > > > > > > > > > out:
> > > > > > > > > > > > + release_copy_files(copy);
> > > > > > > > > > > > return status;
> > > > > > > > > > > > out_err:
> > > > > > > > > > > This is unrelated to the reference count issue.
> > > > > > > > > > >
> > > > > > > > > > > Here if this is an inter-copy then we need to decrement the reference
> > > > > > > > > > > count of the nfsd4_ssc_umount_item so that the vfsmount can be
> > > > > > > > > > > unmounted
> > > > > > > > > > > later.
> > > > > > > > > > >
> > > > > > > > > > Oh, I think I see what you mean. Maybe something like the (untested)
> > > > > > > > > > patch below on top of the original patch would fix that?
> > > > > > > > > >
> > > > > > > > > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > > > > > > > > index c9057462b973..7475c593553c 100644
> > > > > > > > > > --- a/fs/nfsd/nfs4proc.c
> > > > > > > > > > +++ b/fs/nfsd/nfs4proc.c
> > > > > > > > > > @@ -1511,8 +1511,10 @@ nfsd4_cleanup_inter_ssc(struct
> > > > > > > > > > nfsd4_ssc_umount_item *nsui, struct file *filp,
> > > > > > > > > > struct nfsd_net *nn = net_generic(dst->nf_net, nfsd_net_id);
> > > > > > > > > > long timeout = msecs_to_jiffies(nfsd4_ssc_umount_timeout);
> > > > > > > > > > - nfs42_ssc_close(filp);
> > > > > > > > > > - fput(filp);
> > > > > > > > > > + if (filp) {
> > > > > > > > > > + nfs42_ssc_close(filp);
> > > > > > > > > > + fput(filp);
> > > > > > > > > > + }
> > > > > > > > > > spin_lock(&nn->nfsd_ssc_lo
> > > > > > > > > > list_del(&nsui->nsui_list);
> > > > > > > > > > @@ -1813,8 +1815,13 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > > > > > > > > > nfsd4_compound_state *cstate,
> > > > > > > > > > release_copy_files(copy);
> > > > > > > > > > return status;
> > > > > > > > > > out_err:
> > > > > > > > > > - if (async_copy)
> > > > > > > > > > + if (async_copy) {
> > > > > > > > > > cleanup_async_copy(async_copy);
> > > > > > > > > > + if (nfsd4_ssc_is_inter(async_copy))
> > > > > > > > > We don't need to call nfsd4_cleanup_inter_ssc since the thread
> > > > > > > > > nfsd4_do_async_copy has not started yet so the file is not opened.
> > > > > > > > > We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
> > > > > > > > > you want to change nfsd4_cleanup_inter_ssc to detect this error
> > > > > > > > > condition and only decrement the reference count.
> > > > > > > > >
> > > > > > > > Oh yeah, and this would break anyway since the nsui_list head is not
> > > > > > > > being initialized. Dai, would you mind spinning up a patch for this
> > > > > > > > since you're more familiar with the cleanup here?
> > > > > > > Will do. My patch will only fix the unmount issue. Your patch does
> > > > > > > the clean up potential nfsd_file refcount leaks in COPY codepath.
> > > > > > Or do you want me to merge your patch and mine into one?
> > > > > >
> > > > > It probably is best to merge them, since backporters will probably want
> > > > > both patches anyway.
> > > > Unless these two changes are somehow interdependent, I'd like to keep
> > > > them separate. They address two separate issues, yes?
> > >
> > > Yes.
> > >
> > > >
> > > > And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
> > > > for nfsd-next. I'd rather not mix the two types of change.
> > >
> > > Ok. Can we do this:
> > >
> > > 1. Jeff's patch goes to nfsd-fixes since it has the fix for missing
> > > reference count.
> >
> > To make sure I haven't lost track of anything:
> >
> > The patch you refer to here is this one:
> >
> > https://lore.kernel.org/linux-nfs/[email protected]/
> >
> > Correct?
> >
> > (I was waiting for Jeff and Olga to come to consensus, and I think
> > they have, so I can apply it to nfsd-fixes now).
>
> Sorry folks but I got a bit lost in the thread. I thought Dai pointed
> out that we can't remove the put from the nfsd4_cleanup_inter_ssc()
> because that's the put for the copied structure and not the original
> file references which is what Jeff's patch is trying to address.
>

...and then I replied:

With this patch, the references held by the original copy structure are
put by the call to release_copy_files at the end of nfsd4_copy. That
means that the kthread task is only responsible for putting the
references held by the (kmalloc'ed) async_copy structure. So, I think
this gets the nfsd_file refcounting right.

>
> > > 2. My fix for the cleanup of allocated memory goes to nfsd-fixes.
> >
> > And this one hasn't been posted yet, right? Or did I miss it?
> >
> >
> > > 3. I will do the optimization Jeff proposed about list_head and
> > > nfsd4_compound in a separate patch that goes into nfsd-next.
> >
> > That should be fine.
> >
> >
> > > -Dai
> > >
> > > > > Just make yourself the patch author and keep my S-o-b line.
> > > > >
> > > > > > I think we need a bit more cleanup in addition to your patch. When
> > > > > > kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
> > > > > > fails, the async_copy is not initialized yet so calling cleanup_async_copy
> > > > > > can be a problem.
> > > > > >
> > > > > Yeah.
> > > > >
> > > > > It may even be best to ensure that the list_head and such are fully
> > > > > initialized for both allocated and embedded struct nfsd4_copy's. You
> > > > > might shave off a few cpu cycles by not doing that, but it makes things
> > > > > more fragile.
> > > > >
> > > > > Even better, we really ought to split a lot of the fields in nfsd4_copy
> > > > > into a different structure (maybe nfsd4_async_copy). Trimming down
> > > > > struct nfsd4_copy would cut down the size of nfsd4_compound as well
> > > > > since it has a union that contains it. I was planning on doing that
> > > > > eventually, but if you want to take that on, then that would be fine
> > > > > too.
> > > > >
> > > > > --
> > > > > Jeff Layton <[email protected]>
> > > > --
> > > > Chuck Lever
> >
> > --
> > Chuck Lever
> >
> >
> >

--
Jeff Layton <[email protected]>

2023-01-23 20:32:56