linux-kernel - Re: [PATCH] fs: inline step_into() and walk

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGudoHHroVs1pNe575f7wNDKm_+ZVvK3tJhOhk_+5ZgYjFkxCA@mail.gmail.com>
Date: Wed, 19 Nov 2025 20:49:36 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: brauner@...nel.org, jack@...e.cz, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH] fs: inline step_into() and walk_component()

On Wed, Nov 19, 2025 at 8:42 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Wed, Nov 19, 2025 at 07:40:01PM +0100, Mateusz Guzik wrote:
>
> > -static const char *pick_link(struct nameidata *nd, struct path *link,
> > +static noinline const char *pick_link(struct nameidata *nd, struct path *link,
> >                    struct inode *inode, int flags)
> >  {
> >       struct saved *last;
> >       const char *res;
> > -     int error = reserve_stack(nd, link);
> > +     int error;
> > +
> > +     if (nd->flags & LOOKUP_RCU) {
> > +             /* make sure that d_is_symlink above matches inode */
>
> Where would that "above" be?  Have some pity on the readers...
>

this is just copy pasted from the original, i'll patch it

> > +static __always_inline const char *step_into(struct nameidata *nd, int flags,
> > +                  struct dentry *dentry)
> > +{
> > +     struct path path;
> > +     struct inode *inode;
> > +
> > +     path.mnt = nd->path.mnt;
> > +     path.dentry = dentry;
> > +     if (!(nd->flags & LOOKUP_RCU))
> > +             goto slowpath;
> > +     if (unlikely(dentry->d_flags & DCACHE_MANAGED_DENTRY))
> > +             goto slowpath;
> > +     inode = path.dentry->d_inode;
> > +     if (unlikely(d_is_symlink(path.dentry)))
> > +             goto slowpath;
> > +     if (read_seqcount_retry(&path.dentry->d_seq, nd->next_seq))
> > +             return ERR_PTR(-ECHILD);
> > +     if (unlikely(!inode))
> > +             return ERR_PTR(-ENOENT);
> > +     nd->path = path;
> > +     nd->inode = inode;
> > +     nd->seq = nd->next_seq;
> > +     return NULL;
> > +slowpath:
> > +     return step_into_slowpath(nd, flags, dentry);
> > +}
>
> Is there any point keeping that struct path here?  Hell, what's wrong
> with
>
> static __always_inline const char *step_into(struct nameidata *nd, int flags,
>                      struct dentry *dentry)
> {
>         if (likely((nd->flags & LOOKUP_RCU) &&
>             !(dentry->d_flags & DCACHE_MANAGED_DENTRY) &&
>             !d_is_symlink(dentry))) {
>                 // Simple case: no messing with refcounts and
>                 // neither a symlink nor mountpoint.
>                 struct inode *inode = dentry->d_inode;
>
>                 if (read_seqcount_retry(&dentry->d_seq, nd->next_seq))
>                         return ERR_PTR(-ECHILD);
>                 if (unlikely(!inode))
>                         return ERR_PTR(-ENOENT);
>                 nd->path.dentry = dentry;
>                 nd->inode = inode;
>                 nd->seq = nd->next_seq;
>                 return NULL;
>         }
>         return step_into_slowpath(nd, flags, dentry);
> }
>
> for that matter?

this keeps the style of the current step_into, which I can stick with

The gotos, while uglier, imo provide easier handling should one try to
mess with the checks, but I'm not going to die on that hill.

btw, is there a way to combine DCACHE_MANAGED_DENTRY + symlink check
into one branch? The compiler refuses at the moment due to type being
a bitfield. Given how few free flags are remaining this is quite a
bummer.