linux-kernel - Re: [PATCH 4/4] configfs: Correct condition for returning -EEXIST in configfs

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: <Z_Wn978o-kwscN29@google.com>
Date: Tue, 8 Apr 2025 15:49:27 -0700
From: Joel Becker <jlbec@...lplan.org>
To: Zijun Hu <zijun_hu@...oud.com>
Cc: Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
	Al Viro <viro@...iv.linux.org.uk>, linux-kernel@...r.kernel.org,
	Zijun Hu <quic_zijuhu@...cinc.com>, stable@...r.kernel.org
Subject: Re: [PATCH 4/4] configfs: Correct condition for returning -EEXIST in
 configfs_symlink()

On Tue, Apr 08, 2025 at 09:26:10PM +0800, Zijun Hu wrote:
> From: Zijun Hu <quic_zijuhu@...cinc.com>
> 
> configfs_symlink() returns -EEXIST under condition d_unhashed(), but the
> condition often means the dentry does not exist.
> 
> Fix by changing the condition to !d_unhashed().

I don't think this is quite right.

viro put this together in 351e5d869e5ac, which was a while ago.  Read
his comment on 351e5d869e5ac.  Because I unlock the parent directory to
look up the target, we can't trust our symlink dentry hasn't been
changed underneath us.

* If there is now dentry->d_inode, some other inode has been put here.
  -EEXIST.
* If the dentry was unhashed, somehow the dentry we are creating was
  removed from the dcache, and adding things to our dentry will at best
  go nowhere, and at worst dangle in space.  I'm pretty sure viro
  returns -EEXIST because if this dentry is unhashed, some *other*
  dentry has entered the dcache in its place (another file type,
  perhaps).

If you instead check for !d_unhashed(), you're discovering our candidate
dentry is still live in the dcache, which is what we expect and want.

How did you identify this as a problem?  Perhaps we need a more nuanced
check than d_unhashed() these days (for example, d_is_positive/negative
didn't exist back then).

Thanks,
Joel

PS: I enjoyed the trip down memory lane to Al reaming me quite
    thoroughly for this API.

> 
> Fixes: 351e5d869e5a ("configfs: fix a deadlock in configfs_symlink()")
> Cc: stable@...r.kernel.org
> Signed-off-by: Zijun Hu <quic_zijuhu@...cinc.com>
> ---
>  fs/configfs/symlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/configfs/symlink.c b/fs/configfs/symlink.c
> index 69133ec1fac2a854241c2a08a3b48c4c2e8d5c24..cccf61fb8317d739643834e1810b7f136058f56c 100644
> --- a/fs/configfs/symlink.c
> +++ b/fs/configfs/symlink.c
> @@ -193,7 +193,7 @@ int configfs_symlink(struct mnt_idmap *idmap, struct inode *dir,
>  	if (ret)
>  		goto out_put;
>  
> -	if (dentry->d_inode || d_unhashed(dentry))
> +	if (dentry->d_inode || !d_unhashed(dentry))
>  		ret = -EEXIST;
>  	else
>  		ret = inode_permission(&nop_mnt_idmap, dir,
> 
> -- 
> 2.34.1
> 

-- 

"We will have to repent in this generation not merely for the
 vitriolic words and actions of the bad people, but for the 
 appalling silence of the good people."
	- Rev. Dr. Martin Luther King, Jr.

			http://www.jlbec.org/
			jlbec@...lplan.org