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 17:11 +0000, Chuck Lever III wrote:
> 
> > On Jan 18, 2023, at 12:06 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Wed, 2023-01-18 at 16:39 +0000, Chuck Lever III wrote:
> > > 
> > > > On Jan 18, 2023, at 11:29 AM, Olga Kornievskaia <aglo@umich.edu> wrote:
> > > > 
> > > > On Wed, Jan 18, 2023 at 10:27 AM Jeff Layton <jlayton@kernel.org> wrote:
> > > > > 
> > > > > On Wed, 2023-01-18 at 09:42 -0500, Olga Kornievskaia wrote:
> > > > > > On Tue, Jan 17, 2023 at 2:38 PM Jeff Layton <jlayton@kernel.org> 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 <jlayton@kernel.org>