Re: [PATCH v3] ext4: fix ABBA deadlock in ext4_xattr_inode_cache_find()

[Jan Kara]

On Thu 25-06-26 04:09:35, Aditya Srivastava wrote:
> From: Aditya Prakash Srivastava <aditya.ansh182@xxxxxxxxx>
> 
> Syzbot/stress-ng reported an ABBA deadlock in ext4 when exercising
> concurrent xattr workloads (using the ea_inode mount/format option).
> 
> The deadlock occurs between the running transaction and the eviction
> thread:
> - Task 1 (stress-ng): Holds a reference to a shared mbcache_entry (ce)
>   and calls ext4_xattr_inode_cache_find() -> ext4_iget() to retrieve
>   the corresponding EA inode. Since the EA inode is currently being
>   evicted, ext4_iget() blocks in __wait_on_freeing_inode() waiting for
>   eviction to complete.
> - Task 2 (eviction thread): Currently evicting the same EA inode in
>   ext4_evict_ea_inode(). It calls mb_cache_entry_wait_unused(oe) which
>   blocks waiting for Task 1 to release the reference to the mbcache_entry.
> 
> To break this deadlock, implement a new ext4_iget() configuration flag
> named EXT4_IGET_NOWAIT. When set, perform a non-blocking lookup of the
> inode via VFS's find_inode_nowait() API.
> 
> If the inode is currently being evicted (marked with I_FREEING or
> I_WILL_FREE) or created (I_CREATING), simply skip it (returning -ESTALE)
> rather than waiting for eviction/creation to complete, breaking the ABBA
> cycle. If the returned inode is I_NEW, wait for its initialization to
> clear via wait_on_new_inode(). Finally, standard validation checks
> (including is_bad_inode, EXT4_EA_INODE_FL, file_acl, and xattr flags) are
> executed as normal inside check_igot_inode() to fully guarantee VFS-layer
> safety.
> 
> In ext4_xattr_inode_cache_find(), invoke ext4_iget() with the new
> EXT4_IGET_NOWAIT flag to perform the non-blocking cache search.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Reported-by: Colin Ian King <colin.i.king@xxxxxxxxx>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219283
> Fixes: 0a46ef234756 ("ext4: do not create EA inode under buffer lock")
> Signed-off-by: Aditya Prakash Srivastava <aditya.ansh182@xxxxxxxxx>

...

> +static int ext4_iget_match(struct inode *inode, u64 ino, void *data)
> +{
> +	bool *is_freeing = data;
> +
> +	if (inode->i_ino != ino)
> +		return 0;
> +	spin_lock(&inode->i_lock);
> +	if (inode_state_read(inode) & (I_FREEING | I_WILL_FREE | I_CREATING)) {
> +		if (is_freeing)
> +			*is_freeing = true;
> +		spin_unlock(&inode->i_lock);
> +		return -1;
> +	}
> +	__iget(inode);
> +	spin_unlock(&inode->i_lock);
> +	return 1;
> +}
> +
>  struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
>  			  ext4_iget_flags flags, const char *function,
>  			  unsigned int line)
> @@ -5298,9 +5316,26 @@ struct inode *__ext4_iget(struct super_block *sb, unsigned long ino,
>  		return ERR_PTR(-EFSCORRUPTED);
>  	}
>  
> -	inode = iget_locked(sb, ino);
> -	if (!inode)
> -		return ERR_PTR(-ENOMEM);
> +	if (flags & EXT4_IGET_NOWAIT) {
> +		bool is_freeing = false;
> +
> +		inode = find_inode_nowait(sb, ino, ext4_iget_match, &is_freeing);
> +		if (is_freeing)
> +			return ERR_PTR(-ESTALE);

In case of EXT4_IGET_NOWAIT I just would not bother with returning errors
like this. Just if the inode was not found or cannot be grabbed, I'd return
say -ENOENT from __ext4_iget() and be done with it. Due to the nature of
EXT4_IGET_NOWAIT it is unreliable and the caller thus has to decide what to
do in case the inode cannot be grabbed.

> +		if (!inode) {
> +			inode = iget_locked(sb, ino);
> +			if (!inode)
> +				return ERR_PTR(-ENOMEM);

Hu, what's the point of this? It defeats the purpose of EXT4_IGET_NOWAIT as
it can block...

								Honza

> +		} else {
> +			if (inode_state_read_once(inode) & I_NEW)
> +				wait_on_new_inode(inode);
> +		}
> +	} else {
> +		inode = iget_locked(sb, ino);
> +		if (!inode)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +
>  	if (!(inode_state_read_once(inode) & I_NEW)) {
>  		ret = check_igot_inode(inode, flags, function, line);
>  		if (ret) {
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 982a1f831e22..21b5670d8503 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1550,7 +1550,7 @@ ext4_xattr_inode_cache_find(struct inode *inode, const void *value,
>  
>  	while (ce) {
>  		ea_inode = ext4_iget(inode->i_sb, ce->e_value,
> -				     EXT4_IGET_EA_INODE);
> +				     EXT4_IGET_EA_INODE | EXT4_IGET_NOWAIT);
>  		if (IS_ERR(ea_inode))
>  			goto next_entry;
>  		ext4_xattr_inode_set_class(ea_inode);
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR