Re: [PATCH v3 09/19] ovl: stop using lookup_one() in iterate_shared() handling.

Amir Goldstein
2026-04-27T10:10:42Z

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()"

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.