Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash nfsd_file

[Chuck Lever III]

> On Jul 11, 2024, at 2:20 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> 
> On Thu, 2024-07-11 at 18:16 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jul 11, 2024, at 1:53 PM, Jeff Layton <jlayton@xxxxxxxxxx>
>>> wrote:
>>> 
>>> On Thu, 2024-07-11 at 13:05 -0400, Youzhong Yang wrote:
>>>> Shouldn't we have fh_put(fhp) before 'retry'?
>>>> 
>>> 
>>> A subtle question, actually...
>>> 
>>> It probably wouldn't hurt to do that, but I don't think it's
>>> required.
>>> 
>>> The main reason we call fh_put is to force a second call to
>>> nfsd_set_fh_dentry. IOW, we want to redo the lookup by filehandle
>>> and
>>> find the inode.
>>> 
>>> In the EEXIST case, presumably we have found the inode but we raced
>>> with another task in setting an nfsd_file for it in the hash.
>>> That's
>>> different from the case where the thing was unhashed or we got an
>>> EOPENSTALE.  So, I think we probably don't require refinding the
>>> inode
>>> in that case.
>>> 
>>> More pointedly, I'm not sure this particular case is actually
>>> possible.
>>> The entries are hashed on the inode pointer value, and we're
>>> searching
>>> and inserting into the hash under the i_lock.
>>> 
>>> Chuck, thoughts?
>> 
>> Is the question whether we want to dput() the dentry that
>> is attached to the fhp ? fh_verify's API contract says:
>> 
>> 310  * Regardless of success or failure of fh_verify(), fh_put()
>> should be
>> 311  * called on @fhp when the caller is finished with the
>> filehandle. 
>> 
>> It looks like none of nfsd_file_acquire's callers do an
>> fh_put() in their error flows.
>> 
>> But maybe I've misunderstood the issue.
>> 
> 
> Note that this API is weird and doesn't conform to typical get/put
> semantics.
> 
> The fhp is instantiated before nfsd_file_do_acquire is called, and all
> of the callers that I can see do eventually call fh_put on it. fh_put
> is idempotent, so there should be no harm in calling it multiple times.
> 
> My question here though was more about this EEXIST case. Should we even
> bother checking for that? I don't see how it's possible.

If memory serves, at one point nfsd_file_acquire() used
rhtable_insert_yada(), which returns -EEXIST for certain
table overflow cases. Possibly with the list version of
rhtable, consumers can't get -EEXIST at all.


>>>> On Wed, Jul 10, 2024 at 9:06 AM Jeff Layton <jlayton@xxxxxxxxxx>
>>>> wrote:
>>>>> 
>>>>> At this point, we have a new nf that we couldn't properly
>>>>> insert
>>>>> into
>>>>> the hashtable. Just free it before retrying, since it was never
>>>>> hashed.
>>>>> 
>>>>> Fixes: c6593366c0bf ("nfsd: don't kill nfsd_files because of
>>>>> lease
>>>>> break error")
>>>>> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
>>>>> ---
>>>>>  fs/nfsd/filecache.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>> 
>>>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>>>> index f84913691b78..4fb5e8546831 100644
>>>>> --- a/fs/nfsd/filecache.c
>>>>> +++ b/fs/nfsd/filecache.c
>>>>> @@ -1038,8 +1038,10 @@ nfsd_file_do_acquire(struct svc_rqst
>>>>> *rqstp,
>>>>> struct svc_fh *fhp,
>>>>>         if (likely(ret == 0))
>>>>>                 goto open_file;
>>>>> 
>>>>> -       if (ret == -EEXIST)
>>>>> +       if (ret == -EEXIST) {
>>>>> +               nfsd_file_free(nf);
>>>>>                 goto retry;
>>>>> +       }
>>>>>         trace_nfsd_file_insert_err(rqstp, inode, may_flags,
>>>>> ret);
>>>>>         status = nfserr_jukebox;
>>>>>         goto construction_err;
>>>>> 
>>>>> --
>>>>> 2.45.2
>>>>> 
>>> 
>>> -- 
>>> Jeff Layton <jlayton@xxxxxxxxxx>
>> 
>> 
>> --
>> Chuck Lever
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@xxxxxxxxxx>

--
Chuck Lever