Re: [PATCH] fs/dcache: annotate the lockless d_unhashed() check in retain_dentry()

[Jan Kara]

On Mon 12-10-26 20:22:18, Nguyen Ngoc Thang wrote:
> KCSAN reports a data race between __d_drop() clearing d_hash.pprev
> and the d_unhashed() test in retain_dentry(), reached from fast_dput()
> without ->d_lock held:
> 
>   BUG: KCSAN: data-race in dentry_kill / fast_dput
> 
>   write to 0xffff888107394e50 of 8 bytes by task 3667 on cpu 0:
>    __d_drop fs/dcache.c:578 [inline]
>    dentry_kill+0x139/0x500 fs/dcache.c:824
>    finish_dput+0x16/0x1d0 fs/dcache.c:1001
>    dput+0x48/0x50 fs/dcache.c:1042
>    lookup_one_qstr_excl+0x156/0x240
>    filename_unlinkat+0x149/0x410
> 
>   read to 0xffff888107394e50 of 8 bytes by task 3668 on cpu 1:
>    hlist_bl_unhashed include/linux/list_bl.h:54 [inline]
>    d_unhashed include/linux/dcache.h:396 [inline]
>    retain_dentry fs/dcache.c:872 [inline]
>    fast_dput+0x64/0x300 fs/dcache.c:971
>    dput+0x1f/0x50 fs/dcache.c:1040
>    handle_mounts fs/namei.c:1753 [inline]
>    step_into_slowpath+0x38c/0x4c0 fs/namei.c:2104
>    path_lookupat+0x2eb/0x4c0
>    do_readlinkat+0x74/0x2f0
> 
>   value changed: 0xffff8882376dc230 -> 0x0000000000000000
> 
> The race is intentional. As the comment above retain_dentry() says,
> without ->d_lock the predicates are not stable, and it is enough that
> the dentry was hashed at some point after the reference was dropped.
> If the lockless reader sees a stale "hashed" value, the dentry is kept
> with a zero refcount, exactly as if it had been dropped just after the
> check. If it sees "unhashed", it takes ->d_lock and rechecks. A single
> aligned pointer load cannot tear, so neither outcome is harmful.
> 
> Mark the read with data_race() to document this and silence KCSAN.
> No functional change.
> 
> Reproduced with the syzbot C reproducer on a KCSAN kernel in QEMU;
> with this patch the dentry_kill race no longer shows up.
> 
> Reported-by: syzbot+1cfd29c242c7cd26da02@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=1cfd29c242c7cd26da02
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/dcache.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 1b1a81f10da6..3eb3d7e7ab0f 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -868,8 +868,9 @@ static inline bool retain_dentry(struct dentry *dentry, bool locked)
>  	smp_rmb();
>  	d_flags = READ_ONCE(dentry->d_flags);
>  
> -	// Unreachable? Nobody would be able to look it up, no point retaining
> -	if (unlikely(d_unhashed(dentry)))
> +	// Unreachable? Nobody would be able to look it up, no point retaining.
> +	// Racy when !locked: a stale answer only costs a recheck under ->d_lock.
> +	if (unlikely(data_race(d_unhashed(dentry))))
>  		return false;
>  
>  	// Same if it's disconnected
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR