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

[sashiko-bot@kernel.org]

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 <yi.zhang@huawei.com>
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/a638a8fb-c184-4069-ae33-379ec12cd514@huaweicloud.com/

> 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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814093331.1703882-1-yi.zhang@huaweicloud.com?part=19