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: <79206e66-f683-4733-894d-36e197cdde9a@icloud.com>
Date: Thu, 10 Apr 2025 09:17:57 +0800
From: Zijun Hu <zijun_hu@...oud.com>
To: 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 2025/4/9 06:49, Joel Becker wrote:
>> 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.
> 

agree.

> 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

for current condition to return -EEXIST, if hit d_unhashed(dentry), that
means that "if ((dentry->d_inode == NULL) && d_unhashed(dentry)) return
-EEXIST" which looks weird and not right as well.

> check than d_unhashed() these days (for example, d_is_positive/negative
> didn't exist back then).
> 

any suggestions about how to correct the condition to return -EEXIST ?

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