linux-kernel - Re: [PATCH v2] fs/namei.c: Remove unlikely of status being -ECHILD in lookup

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]

Date:   Wed, 13 Jan 2021 18:13:47 -0500
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     LKML <linux-kernel@...r.kernel.org>, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v2] fs/namei.c: Remove unlikely of status being -ECHILD
 in lookup_fast()

Ping?

-- Steve


On Wed, 9 Dec 2020 17:09:28 -0500
Steven Rostedt <rostedt@...dmis.org> wrote:

> From:  Steven Rostedt (VMware) <rostedt@...dmis.org>
> 
> Running my yearly branch profiling code, it detected a 100% wrong branch
> condition in name.c for lookup_fast(). The code in question has:
> 
> 		status = d_revalidate(dentry, nd->flags);
> 		if (likely(status > 0))
> 			return dentry;
> 		if (unlazy_child(nd, dentry, seq))
> 			return ERR_PTR(-ECHILD);
> 		if (unlikely(status == -ECHILD))
> 			/* we'd been told to redo it in non-rcu mode */
> 			status = d_revalidate(dentry, nd->flags);
> 
> If the status of the d_revalidate() is greater than zero, then the function
> finishes. Otherwise, if it is an "unlazy_child" it returns with -ECHILD.
> After the above two checks, the status is compared to -ECHILD, as that is
> what is returned if the original d_revalidate() needed to be done in a
> non-rcu mode.
> 
> Especially this path is called in a condition of:
> 
> 	if (nd->flags & LOOKUP_RCU) {
> 
> And most of the d_revalidate() functions have:
> 
> 	if (flags & LOOKUP_RCU)
> 		return -ECHILD;
> 
> It appears that that is the only case that this if statement is triggered
> on two of my machines, running in production.
> 
> As it is dependent on what filesystem mix is configured in the running
> kernel, simply remove the unlikely() from the if statement.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> ---
> Changes since v1:
> 
>  - Remove unlikely() instead of making it a likely()
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index d4a6dd772303..c7b7e83853f3 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1495,7 +1495,7 @@ static struct dentry *lookup_fast(struct nameidata *nd,
>  			return dentry;
>  		if (unlazy_child(nd, dentry, seq))
>  			return ERR_PTR(-ECHILD);
> -		if (unlikely(status == -ECHILD))
> +		if (status == -ECHILD)
>  			/* we'd been told to redo it in non-rcu mode */
>  			status = d_revalidate(dentry, nd->flags);
>  	} else {