Re: [PATCH] ext4: stop retrying saturated xattr cache entries

On Sun 02-08-26 14:59:41, Matthias Goergens wrote:
> ext4_xattr_block_set() retries when a cache entry selected for reuse
> has a saturated reference count after taking the buffer lock. The retry
> returns to the mbcache lookup without making that entry ineligible, so
> it can select the same unusable entry indefinitely. A task spinning
> there can hold the parent directory's i_rwsem and leave concurrent
> rmdir callers blocked.
> 
> Normally a reusable entry has a reference count below
> EXT4_XATTR_REFCOUNT_MAX because the count and MBE_REUSABLE_B are
> updated under the same buffer lock. A corrupted filesystem can violate
> that invariant. The syzbot reproducer reports allocator and xattr
> corruption before triggering this retry loop.
> 
> Check the untrusted on-disk count before incrementing it, avoiding
> overflow, and clear MBE_REUSABLE_B when it is already saturated. The
> next lookup then skips the entry that was just proven unusable. This
> mirrors the normal transition at EXT4_XATTR_REFCOUNT_MAX; the release
> path marks the entry reusable again on the exact 1024-to-1023
> transition.
> 
> Using the same QEMU harness and guest parameters, current unpatched
> Linux hung in 6 of 8 420-second trials with the do_rmdir signature;
> representative NMI backtraces caught the owner spinning in
> ext4_xattr_block_set(). The patched kernel completed 28 of 28 trials
> without a hung-task report; the final twelve trials exercised the
> reviewed overflow-safe form of the change. syzbot's patch testing also
> completed without reproducing the hang.
> 
> Reported-and-tested-by: [email protected]
> Closes: https://syzkaller.appspot.com/bug?extid=e68dbebd9617a9250e8d
> Fixes: 65f8b80053a1 ("ext4: fix race when reusing xattr blocks")
> Cc: [email protected]
> Signed-off-by: Matthias Goergens <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/ext4/xattr.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
> index 982a1f831e228..3c1bb547b2f54 100644
> --- a/fs/ext4/xattr.c
> +++ b/fs/ext4/xattr.c
> @@ -2075,12 +2075,13 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  				 * stable so we can check the additional
>  				 * reference fits.
>  				 */
> -				ref = le32_to_cpu(BHDR(new_bh)->h_refcount) + 1;
> -				if (ref > EXT4_XATTR_REFCOUNT_MAX) {
> +				ref = le32_to_cpu(BHDR(new_bh)->h_refcount);
> +				if (ref >= EXT4_XATTR_REFCOUNT_MAX) {
>  					/*
>  					 * Undo everything and check mbcache
>  					 * again.
>  					 */
> +					clear_bit(MBE_REUSABLE_B, &ce->e_flags);
>  					unlock_buffer(new_bh);
>  					dquot_free_block(inode,
>  							 EXT4_C2B(EXT4_SB(sb),
> @@ -2091,6 +2092,7 @@ ext4_xattr_block_set(handle_t *handle, struct inode *inode,
>  					new_bh = NULL;
>  					goto inserted;
>  				}
> +				ref++;
>  				BHDR(new_bh)->h_refcount = cpu_to_le32(ref);
>  				if (ref == EXT4_XATTR_REFCOUNT_MAX)
>  					clear_bit(MBE_REUSABLE_B, &ce->e_flags);
> -- 
> 2.55.0
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR