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

[dai.ngo@oracle.com]

On 1/22/23 8:45 AM, Chuck Lever III wrote:
>
>> On Jan 21, 2023, at 4:28 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>>
>> On 1/21/23 12:12 PM, Chuck Lever III wrote:
>>>> On Jan 21, 2023, at 3:05 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>>
>>>> On Sat, 2023-01-21 at 11:50 -0800, dai.ngo@oracle.com wrote:
>>>>> On 1/21/23 10:56 AM, dai.ngo@oracle.com wrote:
>>>>>> On 1/20/23 3:43 AM, Jeff Layton wrote:
>>>>>>> On Thu, 2023-01-19 at 10:38 -0800, dai.ngo@oracle.com wrote:
>>>>>>>> On 1/19/23 2:56 AM, Jeff Layton wrote:
>>>>>>>>> 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.
>>>>>>>> Yes, I see. One refcount is decremented by release_copy_files at end
>>>>>>>> of nfsd4_copy and another is decremented by release_copy_files in
>>>>>>>> cleanup_async_copy.
>>>>>>>>
>>>>>>>>>>>          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))
>>>>>>>> We don't need to call nfsd4_cleanup_inter_ssc since the thread
>>>>>>>> nfsd4_do_async_copy has not started yet so the file is not opened.
>>>>>>>> We just need to do refcount_dec(&copy->ss_nsui->nsui_refcnt), unless
>>>>>>>> you want to change nfsd4_cleanup_inter_ssc to detect this error
>>>>>>>> condition and only decrement the reference count.
>>>>>>>>
>>>>>>> Oh yeah, and this would break anyway since the nsui_list head is not
>>>>>>> being initialized. Dai, would you mind spinning up a patch for this
>>>>>>> since you're more familiar with the cleanup here?
>>>>>> Will do. My patch will only fix the unmount issue. Your patch does
>>>>>> the clean up potential nfsd_file refcount leaks in COPY codepath.
>>>>> Or do you want me to merge your patch and mine into one?
>>>>>
>>>> It probably is best to merge them, since backporters will probably want
>>>> both patches anyway.
>>> Unless these two changes are somehow interdependent, I'd like to keep
>>> them separate. They address two separate issues, yes?
>> Yes.
>>
>>> And -- narrow fixes need to go to nfsd-fixes, but clean-ups can wait
>>> for nfsd-next. I'd rather not mix the two types of change.
>> Ok. Can we do this:
>>
>> 1. Jeff's patch goes to nfsd-fixes since it has the fix for missing
>> reference count.
> To make sure I haven't lost track of anything:
>
> The patch you refer to here is this one:
>
> https://lore.kernel.org/linux-nfs/20230117193831.75201-3-jlayton@kernel.org/
>
> Correct?
>
> (I was waiting for Jeff and Olga to come to consensus, and I think
> they have, so I can apply it to nfsd-fixes now).
>
>
>> 2. My fix for the cleanup of allocated memory goes to nfsd-fixes.
> And this one hasn't been posted yet, right? Or did I miss it?

I will post this patch soon.

>
>
>> 3. I will do the optimization Jeff proposed about list_head and
>> nfsd4_compound in a separate patch that goes into nfsd-next.
> That should be fine.

Thanks,
-Dai

>
>
>> -Dai
>>
>>>> Just make yourself the patch author and keep my S-o-b line.
>>>>
>>>>> I think we need a bit more cleanup in addition to your patch. When
>>>>> kmalloc(sizeof(*async_copy->cp_src), ..) or nfs4_init_copy_state
>>>>> fails, the async_copy is not initialized yet so calling cleanup_async_copy
>>>>> can be a problem.
>>>>>
>>>> Yeah.
>>>>
>>>> It may even be best to ensure that the list_head and such are fully
>>>> initialized for both allocated and embedded struct nfsd4_copy's. You
>>>> might shave off a few cpu cycles by not doing that, but it makes things
>>>> more fragile.
>>>>
>>>> Even better, we really ought to split a lot of the fields in nfsd4_copy
>>>> into a different structure (maybe nfsd4_async_copy). Trimming down
>>>> struct nfsd4_copy would cut down the size of nfsd4_compound as well
>>>> since it has a union that contains it. I was planning on doing that
>>>> eventually, but if you want to take that on, then that would be fine
>>>> too.
>>>>
>>>> -- 
>>>> Jeff Layton <jlayton@kernel.org>
>>> --
>>> Chuck Lever
> --
> Chuck Lever
>
>
>