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] [day] [month] [year] [list]
Message-ID: <CAOQ4uxjOvsfV7o5Mnn_VBKYCR15FkmQBDASvwq0UQKPwxh1H2g@mail.gmail.com>
Date: Thu, 14 Aug 2025 15:47:53 +0200
From: Amir Goldstein <amir73il@...il.com>
To: NeilBrown <neil@...wn.name>
Cc: Al Viro <viro@...iv.linux.org.uk>, 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>, 
	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 Thu, Aug 14, 2025 at 4:08 AM NeilBrown <neil@...wn.name> wrote:
>
> 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.

We decoded a connected real directory path (from file handle) and we
are trying to lookup in overlay a directory that is referencing the
underlying real dir that we decoded.

This is the context. Not sure what problem exactly this code gives you.

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

It's kind of a hack I guess.
ovl has those rules (see xino) to compose a consistent inode number
from real inode number and layer number.
lookup of children during readdir composes the child stack to realize
the consistent xino.

We could do this internally in ovl by doing lookups on the real layers
and composing the xino, but calling lookup on ovl during readdir was
so much easier :/

Thanks,
Amir.