linux-kernel - Re: [PATCH 11/11] VFS: introduce d_alloc_noblock() and d_alloc

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: <175513726277.2234665.5395852687971371437@noble.neil.brown.name>
Date: Thu, 14 Aug 2025 12:07:42 +1000
From: "NeilBrown" <neil@...wn.name>
To: "Al Viro" <viro@...iv.linux.org.uk>
Cc: "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "David Howells" <dhowells@...hat.com>,
 "Marc Dionne" <marc.dionne@...istor.com>, "Xiubo Li" <xiubli@...hat.com>,
 "Ilya Dryomov" <idryomov@...il.com>, "Tyler Hicks" <code@...icks.com>,
 "Miklos Szeredi" <miklos@...redi.hu>, "Richard Weinberger" <richard@....at>,
 "Anton Ivanov" <anton.ivanov@...bridgegreys.com>,
 "Johannes Berg" <johannes@...solutions.net>,
 "Trond Myklebust" <trondmy@...nel.org>, "Anna Schumaker" <anna@...nel.org>,
 "Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
 "Amir Goldstein" <amir73il@...il.com>, "Steve French" <sfrench@...ba.org>,
 "Namjae Jeon" <linkinjeon@...nel.org>, "Carlos Maiolino" <cem@...nel.org>,
 linux-fsdevel@...r.kernel.org, linux-afs@...ts.infradead.org,
 netfs@...ts.linux.dev, ceph-devel@...r.kernel.org, ecryptfs@...r.kernel.org,
 linux-um@...ts.infradead.org, linux-nfs@...r.kernel.org,
 linux-unionfs@...r.kernel.org, linux-cifs@...r.kernel.org,
 linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject:
 Re: [PATCH 11/11] VFS: introduce d_alloc_noblock() and d_alloc_locked()

On Wed, 13 Aug 2025, Al Viro wrote:
> On Tue, Aug 12, 2025 at 12:25:14PM +1000, NeilBrown wrote:
> > Several filesystems use the results of readdir to prime the dcache.
> > These filesystems use d_alloc_parallel() which can block if there is a
> > concurrent lookup.  Blocking in that case is pointless as the lookup
> > will add info to the dcache and there is no value in the readdir waiting
> > to see if it should add the info too.
> > 
> > Also these calls to d_alloc_parallel() are made while the parent
> > directory is locked.  A proposed change to locking will lock the parent
> > later, after d_alloc_parallel().  This means it won't be safe to wait in
> > d_alloc_parallel() while holding the directory lock.
> > 
> > So this patch introduces d_alloc_noblock() which doesn't block
> > but instead returns ERR_PTR(-EWOULDBLOCK).  Filesystems that prime the
> > dcache now use that and ignore -EWOULDBLOCK errors as harmless.
> > 
> > A few filesystems need more than -EWOULDBLOCK - they need to be able to
> > create the missing dentry within the readdir.  procfs is a good example
> > as the inode number is not known until the lookup completes, so readdir
> > must perform a full lookup.
> > 
> > For these filesystems d_alloc_locked() is provided.  It will return a
> > dentry which is already d_in_lookup() but will also lock it against
> > concurrent lookup.  The filesystem's ->lookup function must co-operate
> > by calling lock_lookup() before proceeding with the lookup.  This way we
> > can ensure exclusion between a lookup performed in ->iterate_shared and
> > a lookup performed in ->lookup.  Currently this exclusion is provided by
> > waiting in d_wait_lookup().  The proposed changed to dir locking will
> > mean that calling d_wait_lookup() (in readdir) while already holding
> > i_rwsem could deadlock.
> 
> The last one is playing fast and loose with one assertion that is used
> in quite a few places in correctness proofs - that the only thing other
> threads do to in-lookup dentries is waiting on them (and that - only
> in d_wait_lookup()).  I can't tell whether it will be a problem without
> seeing what you do in the users of that thing, but that creates an
> unpleasant areas to watch out for in the future ;-/

Yeah, it's not my favourite part of the series.

> 
> Which filesystems are those, aside of procfs?
> 

afs in afs_lookup_atsys().  While looking up a name that ends "@sys" it
need to look up the prefix with various alternate suffixes appended.
So this isn't readdir related, but is a lookup-within-a-lookup.

The use of d_add_ci() in xfs is the same basic pattern.

overlayfs does something in ovl_lookup_real_one() that I don't
understand yet but it seems to need a lookup while the directory is
locked. 

ovl_cache_update is in the ovl iterate_shared code (which in fact holds
an exclusive lock).  I think this is the same pattern as procfs in that
an inode number needs to be allocated at lookup time, but there might be
more too it.

So it is:
  procfs and overlayfs for lookup in readdir
  xfs and afs for nested lookup.

The only other approach I could come up with was to arrange some sort of
proxy-execution. i.e. instead of d_alloc_locked() provide a
  d_alloc_proxy()
which, if it found a d_in_lookup() dentry, would perform the ->lookup
itself with some sort of interlock with lookup_slow etc.
That would prevent the DCACHE_PAR_LOOKUP dentry leaking out, but would
be more intrusive and would affect the lookup path for filesystems which
didn't need it.

NeilBrown