linux-kernel - Re: [PATCH] fs/namei: Remove redundant DCACHE_MANAGED_DENTRY check in __follow_mount

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <yhleevo3p4d7tlvmc4b27di3mndhnv7dmnlrupgrtjy23ehqok@whlvpgy4kqrv>
Date: Wed, 7 Jan 2026 08:07:50 -0800
From: Breno Leitao <leitao@...ian.org>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: 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, jlayton@...nel.org, rostedt@...dmis.org, kernel-team@...a.com
Subject: Re: [PATCH] fs/namei: Remove redundant DCACHE_MANAGED_DENTRY check
 in __follow_mount_rcu

Hello Mateusz,

On Wed, Jan 07, 2026 at 03:44:27PM +0100, Mateusz Guzik wrote:
> On Mon, Jan 05, 2026 at 07:10:27AM -0800, Breno Leitao wrote:
> > The check for DCACHE_MANAGED_DENTRY at the start of __follow_mount_rcu()
> > is redundant because the only caller (handle_mounts) already verifies
> > d_managed(dentry) before calling this function, so, dentry in
> > __follow_mount_rcu() has always DCACHE_MANAGED_DENTRY set.
> > 
> > This early-out optimization never fires in practice - but it is marking
> > as likely().
> > 
> > This was detected with branch profiling, which shows 100% misprediction
> > in this likely.
> > 
> > Remove the whole if clause instead of removing the likely, given we
> > know for sure that dentry is not DCACHE_MANAGED_DENTRY.
> > 
> > Signed-off-by: Breno Leitao <leitao@...ian.org>
> > ---
> >  fs/namei.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index bf0f66f0e9b9..774a2f5b0a10 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -1623,9 +1623,6 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path)
> >  	struct dentry *dentry = path->dentry;
> >  	unsigned int flags = dentry->d_flags;
> >  
> > -	if (likely(!(flags & DCACHE_MANAGED_DENTRY)))
> > -		return true;
> > -
> 
> This makes me very uneasy.
> 
> You are seeing 100% misses on this one because you are never racing
> against someone mounting and umounting on the dentry as you are doing
> the lookup.

I'm still learning VFS internals, so please bear with me.

If I understand correctly, the current code checks the same condition
twice in succession:

handle_mounts() {
	if (!d_managed(dentry))                       /* dentry->d_flags & DCACHE_MANAGED_DENTRY */
		return 0;
	__follow_mount_rcu() {                       /* Something changes here */
		unsigned int flags = dentry->d_flags;
		if (!(flags & DCACHE_MANAGED_DENTRY))
			return

Is your concern that DCACHE_MANAGED_DENTRY could be cleared between
these two checks?

> As in it is possible that by the time __follow_mount_rcu is invoked,
> DCACHE_MANAGED_DENTRY is no longer set and with the check removed the
> rest of the routine keeps executing.

I see, but couldn't the same race occur after the second check as
well?

In other words, whether we have one check or two, DCACHE_MANAGED_DENTRY
could still be unset immediately after the final check, leading to the
same situation.

> AFAICS this turns harmless as is anyway, but I don't think that's safe
> to rely on future-wise and more imporantly it is trivially avoidable.
> 
> I did not do it at the time because there are no d_ macros which operate
> on already read flags and I could not be bothered to add them. In
> retrospect a bad call, should have went with it and kept the open-coded
> DCACHE_MANAGED_DENTRY check.
> 
> something like this:
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index bf0f66f0e9b9..c6279f8023cf 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1618,13 +1618,11 @@ EXPORT_SYMBOL(follow_down);
>   * Try to skip to top of mountpoint pile in rcuwalk mode.  Fail if
>   * we meet a managed dentry that would need blocking.
>   */
> -static bool __follow_mount_rcu(struct nameidata *nd, struct path *path)
> +static bool __follow_mount_rcu(struct nameidata *nd, struct path *path, int flags)
>  {
>  	struct dentry *dentry = path->dentry;
> -	unsigned int flags = dentry->d_flags;
>  
> -	if (likely(!(flags & DCACHE_MANAGED_DENTRY)))
> -		return true;
> +	VFS_BUG_ON(!(flags & DCACHE_MANAGED_DENTRY));
>  
>  	if (unlikely(nd->flags & LOOKUP_NO_XDEV))
>  		return false;
> @@ -1672,9 +1670,10 @@ static inline int handle_mounts(struct nameidata *nd, struct dentry *dentry,
>  	path->dentry = dentry;
>  	if (nd->flags & LOOKUP_RCU) {
>  		unsigned int seq = nd->next_seq;
> -		if (likely(!d_managed(dentry)))
> +		unsigned int flags = READ_ONCE(dentry->d_flags);
> +		if (likely(!(dentry->d_flags & DCACHE_MANAGED_DENTRY)))

Minor nit: should this be "if (likely(!(flags & DCACHE_MANAGED_DENTRY)))?"
Otherwise you're reading d_flags twice but passing the stale value to
__follow_mount_rcu().

If I understand your intent correctly, you want to read the flags once
and consistently use that snapshot throughout. Is that right?

Thanks for your review,
--breno