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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1874B388-3A15-4D06-A328-C8581F5FE896@oracle.com>
Date: Thu, 11 Jul 2024 18:16:17 +0000
From: Chuck Lever III <chuck.lever@...cle.com>
To: Jeff Layton <jlayton@...nel.org>, Youzhong Yang <youzhong@...il.com>
CC: Neil Brown <neilb@...e.de>, Olga Kornievskaia <kolga@...app.com>,
        Dai Ngo
	<dai.ngo@...cle.com>, Tom Talpey <tom@...pey.com>,
        Linux NFS Mailing List
	<linux-nfs@...r.kernel.org>,
        Linux Kernel Mailing List
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash
 nfsd_file



> On Jul 11, 2024, at 1:53 PM, Jeff Layton <jlayton@...nel.org> 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@...nel.org>
>> 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@...nel.org>
>>> ---
>>>  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@...nel.org>


--
Chuck Lever