Message-id: <177033862352.16766.1800645278281300265@noble.neil.brown.name>
Date: Fri, 06 Feb 2026 11:43:43 +1100
From: NeilBrown <neilb@...mail.net>
To: "Amir Goldstein" <amir73il@...il.com>
Cc: "Jeff Layton" <jlayton@...nel.org>,
 "Christian Brauner" <brauner@...nel.org>,
 "Alexander Viro" <viro@...iv.linux.org.uk>,
 "David Howells" <dhowells@...hat.com>, "Jan Kara" <jack@...e.cz>,
 "Chuck Lever" <chuck.lever@...cle.com>, "Miklos Szeredi" <miklos@...redi.hu>,
 "John Johansen" <john.johansen@...onical.com>,
 "Paul Moore" <paul@...l-moore.com>, "James Morris" <jmorris@...ei.org>,
 "Serge E. Hallyn" <serge@...lyn.com>,
 "Stephen Smalley" <stephen.smalley.work@...il.com>,
 linux-kernel@...r.kernel.org, netfs@...ts.linux.dev,
 linux-fsdevel@...r.kernel.org, linux-nfs@...r.kernel.org,
 linux-unionfs@...r.kernel.org, apparmor@...ts.ubuntu.com,
 linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Subject: Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()

On Fri, 06 Feb 2026, Amir Goldstein wrote:
> On Thu, Feb 5, 2026 at 1:38 PM Jeff Layton <jlayton@...nel.org> wrote:
> >
> > On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > > From: NeilBrown <neil@...wn.name>
> > >
> > > The primary purpose of this patch is to remove the locking from
> > > ovl_lookup_real_one() as part of centralising all locking of directories
> > > for name operations.
> > >
> > > The locking here isn't needed.  By performing consistency tests after
> > > the lookup we can be sure that the result of the lookup was valid at
> > > least for a moment, which is all the original code promised.
> > >
> > > lookup_noperm_unlocked() is used for the lookup and it will take the
> > > lock if needed only where it is needed.
> > >
> > > Also:
> > >  - don't take a reference to real->d_parent.  The parent is
> > >    only use for a pointer comparison, and no reference is needed for
> > >    that.
> > >  - Several "if" statements have a "goto" followed by "else" - the
> > >    else isn't needed: the following statement can directly follow
> > >    the "if" as a new statement
> > >  - Use a consistent pattern of setting "err" before performing a test
> > >    and possibly going to "fail".
> > >  - remove the "out" label (now that we don't need to dput(parent) or
> > >    unlock) and simply return from fail:.
> > >
> > > Signed-off-by: NeilBrown <neil@...wn.name>
> > > ---
> > >  fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> > >  1 file changed, 26 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > > index 83f80fdb1567..dcd28ffc4705 100644
> > > --- a/fs/overlayfs/export.c
> > > +++ b/fs/overlayfs/export.c
> > > @@ -359,59 +359,50 @@ static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> > >                                         struct dentry *real,
> > >                                         const struct ovl_layer *layer)
> > >  {
> > > -     struct inode *dir = d_inode(connected);
> > > -     struct dentry *this, *parent = NULL;
> > > +     struct dentry *this;
> > >       struct name_snapshot name;
> > >       int err;
> > >
> > >       /*
> > > -      * Lookup child overlay dentry by real name. The dir mutex protects us
> > > -      * from racing with overlay rename. If the overlay dentry that is above
> > > -      * real has already been moved to a parent that is not under the
> > > -      * connected overlay dir, we return -ECHILD and restart the lookup of
> > > -      * connected real path from the top.
> > > +      * @connected is a directory in the overlay and @real is an object
> > > +      * on @layer which is expected to be a child of @connected.
> > > +      * The goal is to return a dentry from the overlay which corresponds
> 
> As the header comment already says:
> "...return a connected overlay dentry whose real dentry is @real"
> 
> The wording "corresponds to @real" reduces clarity IMO.

Ok, I'll rephrase.


> 
> > > +      * to @real.  This is done by looking up the name from @real in
> > > +      * @connected and checking that the result meets expectations.
> > > +      *
> > > +      * Return %-ECHILD if the parent of @real no-longer corresponds to
> > > +      * @connected, and %-ESTALE if the dentry found by lookup doesn't
> > > +      * correspond to @real.
> > >        */
> 
> I dislike kernel-doc inside code comments.
> I think this is actively discouraged and I haven't found a single example
> of this style in fs code.
> 
> If you want to keep this format, please lift the comment to function
> header comment - it is anyway a very generic comment that explains the
> function in general.

OK, I'll remove the formatting or move it - not sure which.
I find that with parameter names like "connected" and "real", some sort
of syntax helps.


> 
> > > -     inode_lock_nested(dir, I_MUTEX_PARENT);
> > > -     err = -ECHILD;
> > > -     parent = dget_parent(real);
> > > -     if (ovl_dentry_real_at(connected, layer->idx) != parent)
> > > -             goto fail;
> > >
> > > -     /*
> > > -      * We also need to take a snapshot of real dentry name to protect us
> > > -      * from racing with underlying layer rename. In this case, we don't
> > > -      * care about returning ESTALE, only from dereferencing a free name
> > > -      * pointer because we hold no lock on the real dentry.
> > > -      */
> > >       take_dentry_name_snapshot(&name, real);
> > > -     /*
> > > -      * No idmap handling here: it's an internal lookup.
> > > -      */
> > > -     this = lookup_noperm(&name.name, connected);
> > > +     this = lookup_noperm_unlocked(&name.name, connected);
> > >       release_dentry_name_snapshot(&name);
> > > +
> > > +     err = -ECHILD;
> > > +     if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> > > +             goto fail;
> > > +
> > >       err = PTR_ERR(this);
> > > -     if (IS_ERR(this)) {
> > > +     if (IS_ERR(this))
> > >               goto fail;
> > > -     } else if (!this || !this->d_inode) {
> > > -             dput(this);
> > > -             err = -ENOENT;
> > > +
> > > +     err = -ENOENT;
> > > +     if (!this || !this->d_inode)
> > >               goto fail;
> > > -     } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> > > -             dput(this);
> > > -             err = -ESTALE;
> > > +
> > > +     err = -ESTALE;
> > > +     if (ovl_dentry_real_at(this, layer->idx) != real)
> > >               goto fail;
> > > -     }
> > >
> > > -out:
> > > -     dput(parent);
> > > -     inode_unlock(dir);
> > >       return this;
> > >
> > >  fail:
> > >       pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> > >                           real, layer->idx, connected, err);
> > > -     this = ERR_PTR(err);
> > > -     goto out;
> > > +     if (!IS_ERR(this))
> > > +             dput(this);
> > > +     return ERR_PTR(err);
> > >  }
> > >
> > >  static struct dentry *ovl_lookup_real(struct super_block *sb,
> >
> > Reviewed-by: Jeff Layton <jlayton@...nel.org>
> 
> Otherwise, it looks fine.

Thanks,
NeilBrown


> 
> Thanks,
> Amir.
> 

Powered by blists - more mailing lists