Re: [PATCH RFC 07/17] ext4: use fast incremental CRC update in ext4_mb_mark_context()

[Baokun Li <libaokun@linux.alibaba.com>]

在 2026/5/9 06:31, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The fast incremental CRC update in ext4_mb_mark_context() corrupts the block bitmap checksum when a double-free or double-allocation occurs.
> --
>
> commit 09892d15c378eafad6a406a3bb75ec38cfead08f
> Author: Baokun Li <libaokun@linux.alibaba.com>
>
> ext4: use fast incremental CRC update in ext4_mb_mark_context()
>
> This commit introduces a fast incremental block bitmap checksum update using
> ext4_block_bitmap_csum_set_range() to avoid re-scanning the entire bitmap.
> It falls back to a full recalculation only when EXT4_MB_BITMAP_MARKED_CHECK
> or BLOCK_UNINIT are present.
>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index ff2023c9f52c6..77f6309916d1e 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
> [ ... ]
>
>> @@ -4127,12 +4128,28 @@ ext4_mb_mark_context(handle_t *handle, struct super_block *sb, bool state,
>>  			goto out_err;
>>  	}
>>  
>> +	/*
>> +	 * fast_crc: Use incremental CRC update via crc32c_flip_range().
>> +	 * This is only valid when all bits in [blkoff, blkoff+len) are
>> +	 * guaranteed to be in the opposite state (i.e., every bit will
>> +	 * actually flip). When EXT4_MB_BITMAP_MARKED_CHECK is set,
>> +	 * mb_set_bits/mb_clear_bits are idempotent, so some bits may not
>> +	 * change and incremental CRC would produce incorrect results.
>> +	 */
>> +	fast_crc = !(flags & EXT4_MB_BITMAP_MARKED_CHECK);
> Since EXT4_MB_BITMAP_MARKED_CHECK is typically not passed during normal
> operations like ext4_free_blocks() (unless AGGRESSIVE_CHECK is defined),
> fast_crc defaults to true.
>
> If a block is double-freed or double-allocated due to an earlier filesystem
> corruption or bug, mb_clear_bits() and mb_set_bits() will leave the
> already-modified bits unchanged.
>
> Because fast_crc evaluates to true in this case, does this apply the
> incremental CRC update assuming the bits flipped, resulting in a bogus
> checksum that no longer matches the block bitmap data?
>
> [ ... ]
>
>>  	if (state) {
>>  		mb_set_bits(bitmap_bh->b_data, blkoff, len);
>>  		ext4_free_group_clusters_set(sb, gdp,
>>  			ext4_free_group_clusters(sb, gdp) - changed);
>>  	} else {
>>  		mb_clear_bits(bitmap_bh->b_data, blkoff, len);
>>  		ext4_free_group_clusters_set(sb, gdp,
>>  			ext4_free_group_clusters(sb, gdp) + changed);
>>  	}
>>  
>> -	ext4_block_bitmap_csum_set(sb, gdp, bitmap_bh);
>> +	if (fast_crc)
>> +		ext4_block_bitmap_csum_set_range(sb, gdp, blkoff, len);
> While the buddy allocator might subsequently detect the double-free (for
> example, in mb_free_blocks() via ext4_grp_locked_error()), it appears the
> block bitmap buffer has already been dirtied with the corrupted CRC here.
>
> Could this cause the entire block group to fail checksum validation on the
> next mount, unnecessarily escalating a localized error into a metadata
> corruption that requires fsck?
>

Yes, I missed the case of errors=continue. In the next version, I will
revert to full calculation when EXT4_ERROR_FS is set.

Thanks,
Baokun