Re: [PATCH -next v5 19/32] ext4: tolerate unexpected holes in ext4_convert_unwritten_extents()

On 8/14/2026 6:09 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] The patch masks a severe physical block Use-After-Free that causes silent data corruption by suppressing the warning when IO completion observes a hole.
> --
> 
> --- Patch [19]: [PATCH -next v5 19/32] ext4: tolerate unexpected holes in ext4_convert_unwritten_extents() ---
> commit 024b041d5cfa7f6f30a260ff0a2a3cd17336e881
> Author: Zhang Yi <[email protected]>
> Subject: ext4: tolerate unexpected holes in ext4_convert_unwritten_extents()
> 
> This patch suppresses the warning in ext4_convert_unwritten_extents() when
> ext4_map_blocks() returns 0 (indicating a hole). It aims to handle the case
> where a punch hole operation races with writeback when folio sizes are larger
> than the block size.
> 
> Link: https://lore.kernel.org/linux-ext4/[email protected]/
> 
>> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
>> --- a/fs/ext4/extents.c
>> +++ b/fs/ext4/extents.c
> 
> [ ... ]
> 
>> @@ -5166,19 +5166,21 @@ int ext4_convert_unwritten_extents(handle_t *handle, struct inode *inode,
>>   		ret = ext4_map_blocks(handle, inode, &map,
>>   				      EXT4_GET_BLOCKS_IO_CONVERT_EXT |
>>   				      EXT4_EX_NOCACHE);
>> -		if (ret <= 0) {
>> -			/*
>> -			 * If the ret is zero, an unexpected hole may cause
>> -			 * conversion to fail.  To avoid data loss during I/O
>> -			 * end conversion, skip the hole and continue
>> -			 * converting subsequent blocks.
>> -			 */
>> +		/*
>> +		 * A return value of zero means an unexpected hole was found.
>> +		 * This can happen when writeback races with a concurrent
>> +		 * punch hole in the iomap path. Because iomap may not create
>> +		 * ifs for folios larger than block size, the dirty bit can
>> +		 * be set again after punching. If writeback happens between
>> +		 * partial folio invalidation and extent removal, a hole is
>> +		 * observed at I/O completion.
>> +		 */
>> +		if (ret < 0)
>>   			ext4_warning(inode->i_sb,
>>   				     "inode #%llu: block %u: len %u: ext4_map_blocks returned %d",
>>   				     inode->i_ino, map.m_lblk, map.m_len, ret);
>> -		} else {
>> +		else if (ret > 0)
>>   			conv_blocks += map.m_len;
> 
> [Severity: Critical]
> Does masking the ret == 0 case here hide a physical block use-after-free?
> 
> If background writeback via ext4_iomap_writepages() submits a write bio for
> the dirty folio, and a concurrent ext4_punch_hole() removes the extent, the
> physical blocks are freed.
> 
> Since JBD2 does not wait for this in-flight bio during transaction commit
> (due to data=ordered mode being disabled for iomap inodes), those blocks
> return to the free pool and can be reallocated to another file.
> 
> When the in-flight write bio finally completes, will it silently overwrite
> the newly reallocated blocks and cause data corruption?
> 
> Instead of silencing the warning during IO completion in
> ext4_convert_unwritten_extents(), could the underlying race be prevented by
> ensuring sub-folio dirty state is properly tracked so that punched blocks
> are not submitted for writeback in the first place?
> 

Yes, I think you're right. So it seems the best solution is for the
iomap infrastructure to always support ifs when block size is smaller
than folio size.

Otherwise, ext4 would have to fall back to sync writeback to avoid
this issue, e.g. calling filemap_write_and_wait_range() after
ext4_truncate_page_cache_block_range(). The performance impact should
be acceptable because ext4_truncate_page_cache_block_range() already
ensures the folio is clean in most cases, only the partial zeroed
blocks at the boundary would need to be written back. So we wouldn't be
triggering a large amount of writeback. Any thoughts?

Thanks,
Yi.