From: NeilBrown <neilb@ownmail.net>
To: "Amir Goldstein" <amir73il@gmail.com>
Cc: "Linus Torvalds" <torvalds@linux-foundation.org>,
"Alexander Viro" <viro@zeniv.linux.org.uk>,
"Christian Brauner" <brauner@kernel.org>,
"Jan Kara" <jack@suse.cz>, "Jeff Layton" <jlayton@kernel.org>,
"Trond Myklebust" <trondmy@kernel.org>,
"Anna Schumaker" <anna@kernel.org>,
"Miklos Szeredi" <miklos@szeredi.hu>,
"Jeremy Kerr" <jk@ozlabs.org>, "Ard Biesheuvel" <ardb@kernel.org>,
linux-efi@vger.kernel.org, linux-fsdevel@vger.kernel.org,
linux-nfs@vger.kernel.org, linux-unionfs@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 09/19] ovl: stop using lookup_one() in iterate_shared() handling.
Date: Tue, 28 Apr 2026 10:24:32 +1000 [thread overview]
Message-ID: <177733587210.1474915.5770075726707338974@noble.neil.brown.name> (raw)
In-Reply-To: <CAOQ4uxidCcGpABkCtkn94i3U_u3OWK16zeAVDG2xuhh5e+ws9g@mail.gmail.com>
On Mon, 27 Apr 2026, Amir Goldstein wrote:
> On Mon, Apr 27, 2026 at 6:07 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > lookup_one() is expected to be removed as it does not fit well with
> > proposed changes to directory locking.
> > Specifically d_alloc_parallel() will be ordered outside of i_rwsem
> > and as iterate_shared() is called with i_rwsem held it is not safe
> > to call d_alloc_parallel().
> >
> > We can instead call d_alloc_noblock() and then call the ->lookup, but
> > that can fail if there is a lookup attempt concurrent with the
> > readdir().
> >
> > ovl cannot afford for the lookup to fail as that could produce incorrect
> > results, and it cannot safely drop i_rwsem temporarily and that could
> > introduce races with handling of the directory cache.
> >
> > Instead we rely on the fact that ovl_iterate() has an exclusive lock on
> > the directory, so any concurrent lookup will wait for the ovl_iterate()
> > call to complete. We allocate a separate dentry and if the lookup is
> > successful, it is hashed with the result.
> >
> > When the concurrent lookup gets i_rwsem it mustn't do its own lookup -
> > it must use the existing dentry. This is found, if it exists, using
> > try_lookup_noperm().
>
> Hi Niel,
>
> One nit about the subject -
> Please change it to "ovl: stop using lookup_one() in ovl_iterate()"
That's reasonable. And that for providing Claude's reflections. All
fixed.
Thanks,
NeilBrown
>
> Apart from that, I let Claude do the review, because it has surpassed
> my abilities in reviewing such subtle api changes.
>
> I will copy its output verbatim, if for nothing else, to encourage you
> to start applying Claude review before sending out your patches...
>
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/overlayfs/namei.c | 12 ++++++++++++
> > fs/overlayfs/readdir.c | 24 ++++++++++++++++++++++--
> > 2 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index ca899fdfaafd..69032eb2b1e2 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -1385,6 +1385,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> > struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > struct ovl_entry *poe = OVL_E(dentry->d_parent);
> > bool check_redirect = (ovl_redirect_follow(ofs) || ofs->numdatalayer);
> > + struct dentry *alias;
> > int err;
> > struct ovl_lookup_ctx ctx = {
> > .dentry = dentry,
> > @@ -1399,6 +1400,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> > if (dentry->d_name.len > ofs->namelen)
> > return ERR_PTR(-ENAMETOOLONG);
> >
> > + /*
> > + * The existance of this in-lookup dentry might have forced
>
> "existance" should be "existence" in the namei.c comment
>
> > + * readdir to do the lookup with a new dentry. If so we must
> > + * return that one.
> > + */
> > + alias = try_lookup_noperm(&QSTR_LEN(dentry->d_name.name,
> > + dentry->d_name.len),
> > + dentry->d_parent);
> > + if (alias && !IS_ERR(alias))
> > + return alias;
> > +
> > with_ovl_creds(dentry->d_sb)
> > err = ovl_lookup_layers(&ctx, &d);
> >
> > diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> > index 1dcc75b3a90f..e03b32491941 100644
> > --- a/fs/overlayfs/readdir.c
> > +++ b/fs/overlayfs/readdir.c
> > @@ -574,8 +574,28 @@ static int ovl_cache_update(const struct path *path, struct ovl_cache_entry *p,
> > }
> > }
> > /* This checks also for xwhiteouts */
> > - this = lookup_one(mnt_idmap(path->mnt), &QSTR_LEN(p->name, p->len), dir);
> > - if (IS_ERR_OR_NULL(this) || !this->d_inode) {
> > + this = d_alloc_noblock(dir, &QSTR_LEN(p->name, p->len));
> > + if (this == ERR_PTR(-EWOULDBLOCK)) {
> > + /*
> > + * Some other thead is looking up this name and will
>
> "thead" should be "thread" in the readdir.c comment
>
> > + * block on i_rwsem before it can complete the lookup.
> > + * We will do the lookup in a new dentry and when that
> > + * lookup gets a turn it will find and return this
> > + * dentry.
> > + */
> > + this = d_alloc_name(dir, p->name);
>
> Claude suggests this as the simplest fix to NULL deref bug below:
> if (!this)
> this = ERR_PTR(-ENOMEM);
>
> > + }
> > + if (!IS_ERR(this) && !d_unhashed(this)) {
> > + /* Either we got an in-lookup or we made our own unhashed */
>
> The condition should be d_unhashed(this) (without !), matching the comment
> which says "Either we got an in-lookup or we made our own unhashed".
>
> d_alloc_name() calls d_alloc() which returns NULL on allocation failure.
> The original code checked IS_ERR_OR_NULL(this), but the new code only
> checks IS_ERR(this). A NULL this would pass the !IS_ERR(this) check and
> then crash on !d_unhashed(this) (or d_unhashed(this) with the fix).
>
> > + struct dentry *alias = ovl_lookup(dir->d_inode, this, 0);
> > +
> > + if (alias) {
> > + d_lookup_done(this);
> > + dput(this);
> > + this = alias;
> > + }
> > + }
> > + if (IS_ERR(this) || !this->d_inode) {
> > /* Mark a stale entry */
> > p->is_whiteout = true;
> > if (IS_ERR(this)) {
> > --
>
> Claude (Opus 4.6) review summary:
>
> The patch has a critical condition inversion (!d_unhashed vs d_unhashed)
> that would break impure readdir entirely and cause use-after-free corruption
> in the dcache. It also has a missing NULL check for d_alloc_name() failure.
>
> Thanks,
> Amir.
>
next prev parent reply other threads:[~2026-04-28 0:24 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-27 4:01 [PATCH v3 00/19] Prepare to lift lookup out of exclusive lock for directory ops NeilBrown
2026-04-27 4:01 ` [PATCH v3 01/19] VFS: fix various typos in documentation for start_creating start_removing etc NeilBrown
2026-04-27 4:01 ` [PATCH v3 02/19] VFS: enhance d_splice_alias() to handle in-lookup dentries NeilBrown
2026-04-27 4:01 ` [PATCH v3 03/19] VFS: allow d_alloc_name() to be used with ->d_hash NeilBrown
2026-04-28 2:10 ` Al Viro
2026-04-29 2:44 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 04/19] VFS: use wait_var_event for waiting in d_alloc_parallel() NeilBrown
2026-04-28 3:37 ` Al Viro
2026-04-28 11:18 ` NeilBrown
2026-04-28 14:22 ` Al Viro
2026-04-28 23:26 ` NeilBrown
2026-04-29 5:26 ` Al Viro
2026-04-29 17:07 ` Al Viro
2026-04-29 21:03 ` Linus Torvalds
2026-04-30 23:51 ` NeilBrown
2026-05-01 1:11 ` Al Viro
2026-05-01 1:39 ` NeilBrown
2026-05-01 1:45 ` NeilBrown
2026-05-01 3:37 ` Al Viro
2026-05-01 10:46 ` NeilBrown
2026-05-01 1:20 ` NeilBrown
2026-04-28 16:32 ` Linus Torvalds
2026-04-27 4:01 ` [PATCH v3 05/19] VFS: introduce d_alloc_noblock() NeilBrown
2026-04-28 2:22 ` Al Viro
2026-04-28 11:24 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 06/19] VFS: add d_duplicate() NeilBrown
2026-04-27 4:01 ` [PATCH v3 07/19] VFS: Add LOOKUP_SHARED flag NeilBrown
2026-04-27 7:43 ` Amir Goldstein
2026-04-27 8:47 ` NeilBrown
2026-04-27 9:05 ` Amir Goldstein
2026-04-27 23:51 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 08/19] VFS/xfs/ntfs: drop parent lock across d_alloc_parallel() in d_add_ci() NeilBrown
2026-04-27 7:49 ` Amir Goldstein
2026-04-27 8:48 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 09/19] ovl: stop using lookup_one() in iterate_shared() handling NeilBrown
2026-04-27 10:10 ` Amir Goldstein
2026-04-28 0:24 ` NeilBrown [this message]
2026-04-27 4:01 ` [PATCH v3 10/19] VFS/ovl: add d_alloc_noblock_return() NeilBrown
2026-04-27 9:40 ` Amir Goldstein
2026-04-28 0:34 ` NeilBrown
2026-04-28 4:35 ` Al Viro
2026-04-28 11:44 ` NeilBrown
2026-04-27 4:01 ` [PATCH v3 11/19] efivarfs: use d_alloc_name() NeilBrown
2026-04-27 4:01 ` [PATCH v3 12/19] shmem: use d_duplicate() NeilBrown
2026-04-27 4:01 ` [PATCH v3 13/19] nfs: remove d_drop()/d_alloc_parallel() from nfs_atomic_open() NeilBrown
2026-04-27 4:01 ` [PATCH v3 14/19] nfs: use d_splice_alias() in nfs_link() NeilBrown
2026-04-27 4:01 ` [PATCH v3 15/19] nfs: don't d_drop() before d_splice_alias() NeilBrown
2026-04-27 4:01 ` [PATCH v3 16/19] nfs: don't d_drop() before d_splice_alias() in atomic_create NeilBrown
2026-04-27 4:01 ` [PATCH v3 17/19] nfs: Use d_alloc_noblock() in nfs_prime_dcache() NeilBrown
2026-04-27 4:01 ` [PATCH v3 18/19] nfs: use d_alloc_noblock() in silly-rename NeilBrown
2026-04-27 4:01 ` [PATCH v3 19/19] nfs: use d_duplicate() NeilBrown
2026-04-27 8:42 ` [syzbot ci] Re: Prepare to lift lookup out of exclusive lock for directory ops syzbot ci
2026-04-28 23:16 ` NeilBrown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=177733587210.1474915.5770075726707338974@noble.neil.brown.name \
--to=neilb@ownmail.net \
--cc=amir73il@gmail.com \
--cc=anna@kernel.org \
--cc=ardb@kernel.org \
--cc=brauner@kernel.org \
--cc=jack@suse.cz \
--cc=jk@ozlabs.org \
--cc=jlayton@kernel.org \
--cc=linux-efi@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=miklos@szeredi.hu \
--cc=neil@brown.name \
--cc=torvalds@linux-foundation.org \
--cc=trondmy@kernel.org \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).