Re: [PATCH -next v5 04/32] ext4: skip ordered I/O wait when zeroing beyond i_disksize block

On 8/14/2026 6:14 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Torn read of the 64-bit `i_disksize` on 32-bit architectures can cause the kernel to skip ordered I/O, leading to stale data exposure.
> --
> 
> --- Patch [4]: [PATCH -next v5 04/32] ext4: skip ordered I/O wait when zeroing beyond i_disksize block ---
> commit bb84de10d990ded9aab4107d18dd1616342efccd
> Author: Zhang Yi <[email protected]>
> 
> ext4: skip ordered I/O wait when zeroing beyond i_disksize block
> 
> This commit optimizes ext4_block_zero_eof() by skipping the ordered I/O
> wait when the zeroed range is entirely beyond the block containing
> i_disksize. Since data beyond i_disksize hasn't been written to disk yet,
> the ordered I/O wait is unnecessary and can be bypassed.
> 
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
> [ ... ]
>> @@ -4241,9 +4241,16 @@ int ext4_block_zero_eof(struct inode *inode, loff_t from, loff_t end)
>>   	 * truncating up or performing an append write, because there might be
>>   	 * exposing stale on-disk data which may caused by concurrent post-EOF
>>   	 * mmap write during folio writeback.
>> +	 *
>> +	 * Ordered I/O is required only when zeroing the tail of a block that
>> +	 * overlaps with i_disksize. If the zeroed range falls outside that
>> +	 * block, the zeroed data lies beyond the existing on-disk data. It
>> +	 * will be written out before i_disksize is later extended past
>> +	 * i_size, so no stale data can be exposed.
>>   	 */
>>   	if (ext4_should_order_data(inode) &&
>> -	    did_zero && zero_written && !IS_DAX(inode)) {
>> +	    did_zero && zero_written && !IS_DAX(inode) &&
>> +	    from < round_up(READ_ONCE(EXT4_I(inode)->i_disksize), blocksize)) {
> 
> [Severity: High]
> Could this READ_ONCE() on the 64-bit i_disksize result in a torn read on
> 32-bit architectures?
> 
> If a background writeback thread concurrently updates i_disksize under
> i_data_sem but without holding i_rwsem, and the file size is extending
> across a 4GB boundary (for example, transitioning from 0x00000000FFFFFFFF
> to 0x0000000100000000), a torn read might fetch 0 if the lower 32 bits are
> zeroed first.
> 
> If 0 is returned here, round_up(0, blocksize) evaluates to 0, which makes
> the condition 'from < 0' false. Would this incorrectly bypass the critical
> ordered I/O guarantee and potentially expose stale data in the event of a
> crash?

I agree this concern is valid. The torn 64-bit read on 32-bit SMP is
mechanically possible, and the proposed condition could skip the
ordered I/O path when it shouldn't.

However, this pattern is not introduced by this commit. The existing
ext4 code already has multiple READ_ONCE(i_disksize) sites without
holding i_data_sem, such as the fast-path check in
mpage_map_and_submit_extent() and the size check in
ext4_dio_write_end_io(). i_disksize has no seqcount, unlike
inode->i_size which has i_size_seqcount.

I'm not sure whether all the unlocked sites are harmless, at least
the mpage_map_and_submit_extent() fast-path check looks a bit
problematic to me. If a torn read returns a value larger than the
writer's actual target (e.g. when another ioend worker is concurrently
advancing i_disksize from 0x00000000FFFFFFFF to 0x0000000100000000 —
high 4 bytes already updated to 0x00000001, low 4 bytes still
0xFFFFFFFF, so the reader observes 0x00000001FFFFFFFF), the fast-path
comparison skips the lock-protected recheck, and the i_disksize update
for this writeback batch is missed.

I think this deserves a separate patch series to properly fix it,
perhaps by introducing an i_size_read()-style helper for i_disksize
(seqcount or similar), and reviewing all access sites. What do people
think?

Thanks,
Yi.

> 
>>   		handle_t *handle;
>>   
>>   		handle = ext4_journal_start(inode, EXT4_HT_MISC, 1);
>