Re: [PATCH 4/4] configfs: Correct condition for returning -EEXIST in configfs_symlink()

[Zijun Hu]

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.