Re: [PATCH] ext4: fix deadlock in ext4_evict_ea_inode() vs cache_find()

[Jan Kara]

On Mon 29-06-26 13:27:32, Yun Zhou wrote:
> ext4_evict_ea_inode() calls mb_cache_entry_wait_unused() to wait for
> the EA inode's mb_cache entry refcount to drop before deleting it.
> This can deadlock against ext4_xattr_inode_cache_find() which holds
> an mb_cache entry reference while calling ext4_iget():
> 
>   Thread A (evicting EA inode X):
>     ext4_evict_ea_inode -> mb_cache_entry_wait_unused  [waits for refcnt]
> 
>   Thread B (setxattr looking for dedup):
>     ext4_xattr_inode_cache_find -> mb_cache_entry_find  [holds refcnt]
>       -> ext4_iget(X) -> __wait_on_freeing_inode       [waits on I_FREEING]
> 
> Thread A's eviction sets I_FREEING and then waits for the mb_cache
> refcount to drop.  Thread B holds that refcount and waits for
> I_FREEING to clear -- circular dependency.
> 
> Fix by making ext4_evict_ea_inode() non-blocking: try to delete the
> entry once, and if it's currently in use, just leave it.  A stale
> entry is harmless -- subsequent cache_find() calls will iget the
> evicted inode, fail validation or data comparison, and skip it.
> 
> Fixes: 458aee4a6e5b ("ext4: remove EA inode entry from mbcache on inode eviction")
> Reported-by: syzbot+fd5533bcd0f7343bb8ca@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=fd5533bcd0f7343bb8ca
> Signed-off-by: Yun Zhou <yun.zhou@xxxxxxxxxxxxx>

We already have another fix for this [1] and I think it is better. Frankly,
I'd be wary of leaving stale mbcache entries behind. If nothing else, it slows
down things and effectively leaks memory and it could be actually triggered
by user more or less at their will...

								Honza

[1] https://lore.kernel.org/all/20260626054821.1729-1-aditya.ansh182@xxxxxxxxx/

> ---
>  fs/ext4/xattr.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 85ad2561474b..8ca2c87b1db3 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -471,12 +471,16 @@ void ext4_evict_ea_inode(struct inode *inode)
>  
>  	if (!EA_INODE_CACHE(inode))
>  		return;
> -	/* Wait for entry to get unused so that we can remove it */
> -	while ((oe = mb_cache_entry_delete_or_get(EA_INODE_CACHE(inode),
> -			ext4_xattr_inode_get_hash(inode), inode->i_ino))) {
> -		mb_cache_entry_wait_unused(oe);
> +	/*
> +	 * Try to delete the cache entry.  If it's currently in use by
> +	 * another thread (e.g. ext4_xattr_inode_cache_find), just leave
> +	 * it -- the stale entry is harmless.  Waiting here would deadlock
> +	 * if the other thread's iget is blocked on this inode's I_FREEING.
> +	 */
> +	oe = mb_cache_entry_delete_or_get(EA_INODE_CACHE(inode),
> +			ext4_xattr_inode_get_hash(inode), inode->i_ino);
> +	if (oe)
>  		mb_cache_entry_put(EA_INODE_CACHE(inode), oe);
> -	}
>  }
>  
>  static int
> -- 
> 2.43.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR