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

[Chuck Lever III <chuck.lever@oracle.com>]

> 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?


--
Chuck Lever