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

[Jan Kara]

On Tue 23-06-26 09:59:11, 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, perform a non-blocking lookup of the EA inode
> using VFS's find_inode_nowait() API. If the EA inode is currently being
> evicted (marked with I_FREEING or I_WILL_FREE), simply skip it (treat
> as a cache miss) rather than waiting for eviction to complete. If the
> returned inode is found to be I_NEW, wait for its initialization to
> clear using wait_on_new_inode().
> 
> This deadlock was made much easier to hit after commit 0a46ef234756
> ("ext4: do not create EA inode under buffer lock") which removed
> synchronization on the buffer lock.
> 
> 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>

I was looking into this for quite some time in the past and then run out of
time when redesigning locking of the xattrs (which is a mess). Your
solution is a bit hacky but as a quick stability fix before we can rework
xattr locking it actually looks as a neat idea!

> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 982a1f831e22..ef13e7a76153 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -1523,6 +1523,20 @@ static struct inode *ext4_xattr_inode_create(handle_t *handle,
>  	return ea_inode;
>  }
>  
> +static int ext4_xattr_inode_match(struct inode *inode, u64 ino, void *data)
> +{
> +	if (inode->i_ino != ino)
> +		return 0;
> +	spin_lock(&inode->i_lock);
> +	if (inode_state_read(inode) & (I_FREEING | I_WILL_FREE)) {
> +		spin_unlock(&inode->i_lock);
> +		return 0;
> +	}

I think you should also skip I_CREATING inodes here... I don't think you
can really spot them here but just that we don't have to worry.

> +	__iget(inode);
> +	spin_unlock(&inode->i_lock);
> +	return 1;
> +}
> +
>  static struct inode *
>  ext4_xattr_inode_cache_find(struct inode *inode, const void *value,
>  			    size_t value_len, u32 hash)
> @@ -1549,10 +1563,19 @@ 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);
> -		if (IS_ERR(ea_inode))
> +		ea_inode = find_inode_nowait(inode->i_sb, ce->e_value,
> +					     ext4_xattr_inode_match, NULL);
> +		if (!ea_inode)
>  			goto next_entry;
> +		if (inode_state_read_once(ea_inode) & I_NEW)
> +			wait_on_new_inode(ea_inode);
> +		if (is_bad_inode(ea_inode) ||
> +		    !(EXT4_I(ea_inode)->i_flags & EXT4_EA_INODE_FL) ||
> +		    ext4_test_inode_state(ea_inode, EXT4_STATE_XATTR) ||
> +		    EXT4_I(ea_inode)->i_file_acl) {
> +			iput(ea_inode);
> +			goto next_entry;
> +		}

So instead of opencoding these checks here, I'd rather implement
EXT4_IGET_NOWAIT flag which will use find_inode_nowait() like above for the
inode lookup and then you don't have to opencode the sanity checks here and
they can stay contained in ext4_iget() code...

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR