linux-kernel - Re: [PATCH 4/7] VFS: introduce dentry

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: <CAOQ4uxgS_wnHQFBmNcVgcCvfxWeEi3Oi_3kEFtSmxNzvZAjUMQ@mail.gmail.com>
Date: Wed, 23 Jul 2025 17:17:38 +0200
From: Amir Goldstein <amir73il@...il.com>
To: NeilBrown <neil@...wn.name>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] VFS: introduce dentry_lookup() and friends

On Tue, Jul 22, 2025 at 1:27 AM NeilBrown <neil@...wn.name> wrote:
>
> On Mon, 21 Jul 2025, Amir Goldstein wrote:
> > On Mon, Jul 21, 2025 at 10:55 AM NeilBrown <neil@...wn.name> wrote:
> > >
> > > dentry_lookup() combines locking the directory and performing a lookup
> > > prior to a change to the directory.
> > > Abstracting this prepares for changing the locking requirements.
> > >
> > > dentry_lookup_noperm() does the same without needing a mnt_idmap and
> > > without checking permissions.  This is useful for internal filesystem
> > > management (e.g.  creating virtual files in response to events) and in
> > > other cases similar to lookup_noperm().
> > >
> > > dentry_lookup_hashed() also does no permissions checking and assumes
> > > that the hash of the name has already been stored in the qstr.
> >
> > That's a very confusing choice of name because _hashed() (to me) sounds
> > like the opposite of d_unhashed() which is not at all the case.
>
> True.  But maybe the confusion what already there.
> You can "d_add()" a dentry and later "d_drop()" the dentry and if the
> dentry isn't between those two operations, then it is "d_unhashed()"
> which leaks out the implementation detail (hash table) for dentry
> lookup. Maybe d_unhashed() should be d_added() with inverted meaning?
>
> There is only one user of this interface outside of namei.c so I could
> unexported to keep the confusion local.  That would mean
> ksmbd_vfs_path_lookup() would hav to use dentry_lookup_noperm() which
> would recalculate the hash which vfs_path_parent_lookup() already
> calculated (and we cannot simply tell it not to bother calculating).
> Actually it already uses lookup_noperm_unlocked() in the
> don't-need-a-lock-branch which recalculates the hash.....
>
> Would making that name static ease your concern?
>
> >
> > > This is useful following filename_parentat().
> > >
> > > These are intended to be paired with done_dentry_lookup() which provides
> > > the inverse of putting the dentry and unlocking.
> > >
> > > Like lookup_one_qstr_excl(), dentry_lookup() returns -ENOENT if
> > > LOOKUP_CREATE was NOT given and the name cannot be found,, and returns
> > > -EEXIST if LOOKUP_EXCL WAS given and the name CAN be found.
> > >
> > > These functions replace all uses of lookup_one_qstr_excl() in namei.c
> > > except for those used for rename.
> > >
> > > Some of the variants should possibly be inlines in a header.
> > >
> > > Signed-off-by: NeilBrown <neil@...wn.name>
> > > ---
> > >  fs/namei.c            | 158 ++++++++++++++++++++++++++++++------------
> > >  include/linux/namei.h |   8 ++-
> > >  2 files changed, 119 insertions(+), 47 deletions(-)
> > >
> > > diff --git a/fs/namei.c b/fs/namei.c
> > > index 950a0d0d54da..f292df61565a 100644
> > > --- a/fs/namei.c
> > > +++ b/fs/namei.c
> > > @@ -1714,17 +1714,98 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > >  }
> > >  EXPORT_SYMBOL(lookup_one_qstr_excl);
> > >
> > > +/**
> > > + * dentry_lookup_hashed - lookup and lock a name prior to dir ops
> > > + * @last: the name in the given directory
> > > + * @base: the directory in which the name is to be found
> > > + * @lookup_flags: %LOOKUP_xxx flags
> > > + *
> > > + * The name is looked up and necessary locks are taken so that
> > > + * the name can be created or removed.
> > > + * The "necessary locks" are currently the inode node lock on @base.
> > > + * The name @last is expected to already have the hash calculated.
> > > + * No permission checks are performed.
> > > + * Returns: the dentry, suitably locked, or an ERR_PTR().
> > > + */
> > > +struct dentry *dentry_lookup_hashed(struct qstr *last,
> > > +                                   struct dentry *base,
> > > +                                   unsigned int lookup_flags)
> > > +{
> > > +       struct dentry *dentry;
> > > +
> > > +       inode_lock_nested(base->d_inode, I_MUTEX_PARENT);
> > > +
> > > +       dentry = lookup_one_qstr_excl(last, base, lookup_flags);
> > > +       if (IS_ERR(dentry))
> > > +               inode_unlock(base->d_inode);
> > > +       return dentry;
> > > +}
> > > +EXPORT_SYMBOL(dentry_lookup_hashed);
> >
> > Observation:
> >
> > This part could be factored out of
> > __kern_path_locked()/kern_path_locked_negative()
>
> This patch does exactly that....
>
> >
> > If you do that in patch 2 while introducing done_dentry_lookup() then
> > it also makes
> > a lot of sense to balance the introduced done_dentry_lookup() with the
> > factored out
> > helper __dentry_lookup_locked() or whatever its name is.
>
> I don't think I want a __dentry_lookup_locked().  The lock and the
> lookup need to be tightly connected.
> But maybe I cold introduce dentry_lookup_hashed() in patch 2 ...
> Or maybe call it __dentry_lookup() ??

__dentry_lookup() sounds good to me.

Thanks,
Amir.