linux-kernel - Re: [PATCH v2] fs: try an opportunistic lookup for O

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: <0bcdf69cb17dd542d84c121cb7862dc4867f56af.camel@kernel.org>
Date: Wed, 14 Aug 2024 12:46:32 -0400
From: Jeff Layton <jlayton@...nel.org>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Andrew
 Morton <akpm@...ux-foundation.org>, Mateusz Guzik <mjguzik@...il.com>,
 Josef Bacik <josef@...icpanda.com>, linux-fsdevel@...r.kernel.org, 
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] fs: try an opportunistic lookup for O_CREAT opens too

On Wed, 2024-08-14 at 16:42 +0100, Al Viro wrote:
> On Wed, Aug 14, 2024 at 07:48:17AM -0400, Jeff Layton wrote:
> > On Wed, 2024-08-14 at 03:40 +0100, Al Viro wrote:
> > > On Wed, Aug 14, 2024 at 03:18:17AM +0100, Al Viro wrote:
> > > 
> > > > That's not the only problem; your "is it negative" test is
> > > > inherently
> > > > racy in RCU mode.  IOW, what is positive at the time you get
> > > > here can
> > > > bloody well go negative immediately afterwards.  Hit that with
> > > > O_CREAT and you've got a bogus ENOENT...
> > > 
> > > Hmm...  OTOH, in that case you end up in step_into(), which will
> > > do the
> > > right thing...
> > > 
> > > 	How well does that series survive NFS client regression
> > > tests?
> > > That's where I'd expect potentially subtle shite, what with
> > > short-circuited
> > > ->d_revalidate() on the final pathwalk step in open()...
> > 
> > Christian took in my v3 patch which is a bit different from this
> > one.
> > It seems to be doing fine in testing with NFS and otherwise.
> > 
> > I don't think we short-circuit the d_revalidate though, do we? That
> > version calls lookup_fast on the last component which should
> > d_revalidate the last dentry before returning it.
> 
> It's not about a skipped call of ->d_revalidate(); it's about the NFS
> (especially NFS4) dances inside ->d_revalidate(), where it tries to
> cut down on roundtrips where possible.  The interplay with -
> >atomic_open()
> and ->open() is subtle and I'm not sure that we do not depend upon
> the
> details of ->i_rwsem locking by fs/namei.c in there - proof of
> correctness
> used to be rather convoluted there, especially wrt the unhashing and
> rehashing aliases.
> 
> I'm not saying that your changes break things in there, but that's
> one
> area where I would look for trouble.  NFS has fairly extensive
> regression
> tests, and it would be a good idea to beat that patchset with those.

I've already run a bunch of NFS tests on it and it seems to be OK so
far, but I'll keep testing it. My take:

Opening an extant file with O_CREAT set should behave the same as with
O_CREAT not set.

I did crawl through NFS's d_revalidate functions. There are a couple of
places that check for O_CREAT, but they didn't seem to depend on the
i_rwsem or any particular locking.

Please do let me know if you see anything I missed though.

Thanks,
-- 
Jeff Layton <jlayton@...nel.org>