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

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2023-01-18 at 21:05 -0800, dai.ngo@oracle.com 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 <jlayton@kernel.org>
> > ---
> >   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 <jlayton@kernel.org>