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

[Chuck Lever III]

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


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