On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <jlayton@xxxxxxxxxx> 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 <jlayton@xxxxxxxxxx>
> ---
> 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,
> ©->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(©->cp_clp->async_lock);
> list_del(©->copies);
> spin_unlock(©->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
>
