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

[dai.ngo@xxxxxxxxxx]

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 <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);

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)