| 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
| ||
|
Message-ID: <CAOQ4uxh_Ugyy9=Vx_XOzWMTdhqVx6kAu43q+F+afhNF_Zv_9TA@mail.gmail.com>
Date: Thu, 5 Feb 2026 14:04:42 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Jeff Layton <jlayton@...nel.org>
Cc: NeilBrown <neil@...wn.name>, 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 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.
> > + * 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.
> > - 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,
Amir.
Powered by blists - more mailing lists