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: <50afe4a63be26a946257c335188e230c86beb148.camel@kernel.org>
Date: Thu, 11 Jul 2024 13:53:12 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Youzhong Yang <youzhong@...il.com>
Cc: Chuck Lever <chuck.lever@...cle.com>, Neil Brown <neilb@...e.de>, Olga
 Kornievskaia <kolga@...app.com>, Dai Ngo <Dai.Ngo@...cle.com>, Tom Talpey
 <tom@...pey.com>,  linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] nfsd: fix refcount leak when failing to hash
 nfsd_file

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?

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ