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: <20240814154250.GS13701@ZenIV>
Date: Wed, 14 Aug 2024 16:42:50 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Jeff Layton <jlayton@...nel.org>
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, 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.