[PATCH] ext4: cap migration extent length at EXT_INIT_MAX_LEN

[PATCH] ext4: cap migration extent length at EXT_INIT_MAX_LEN

[Yichong Chen]

update_extent_range() keeps merging physically and logically contiguous
blocks without limiting the accumulated range.  The maximum length of an
initialized extent is EXT_INIT_MAX_LEN, because the MSB of the 16-bit
ee_len field is used to mark unwritten extents.

finish_range() can therefore store a range longer than EXT_INIT_MAX_LEN
in ee_len.  A range of 32769 blocks is stored as 0x8001, which
ext4_ext_is_unwritten() treats as an unwritten extent and
ext4_ext_get_actual_len() reports as 1 block, so the migrated file
silently loses its data.

Stop merging once the accumulated range reaches EXT_INIT_MAX_LEN, so
migration creates several valid extents instead.

Fixes: c14c6fd5c56a ("ext4: Add EXT4_IOC_MIGRATE ioctl")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 fs/ext4/migrate.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index d467b5a13c62..2aa6572088cf 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -70,7 +70,8 @@ static int update_extent_range(handle_t *handle, struct inode *inode,
 	 */
 	if (lb->first_pblock &&
 		(lb->last_pblock+1 == pblock) &&
-		(lb->last_block+1 == lb->curr_block)) {
+		(lb->last_block+1 == lb->curr_block) &&
+		(lb->last_block - lb->first_block + 1 < EXT_INIT_MAX_LEN)) {
 		lb->last_pblock = pblock;
 		lb->last_block = lb->curr_block;
 		lb->curr_block++;
-- 
2.51.0

Re: [PATCH] ext4: cap migration extent length at EXT_INIT_MAX_LEN

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Error handling around ext4_journal_ensure_credits clobbers error codes and causes silent data loss on failure.
- [Critical] Integer overflow of logical block counter causes a kernel panic on >= 8KB block size filesystems.
- [High] Permanent disk block leak when freeing metadata blocks fails on the error path.
--

commit e8f71f9a6f0e209c95682ad7839bfdf0af78c8da
Author: Yichong Chen <chenyichong@uniontech.com>

ext4: cap migration extent length at EXT_INIT_MAX_LEN

update_extent_range() keeps merging contiguous blocks without limiting the
accumulated range, which can exceed EXT_INIT_MAX_LEN. This causes
finish_range() to store lengths that overlap with the unwritten extent bit,
leading to silent data loss. This patch stops merging once the range
reaches EXT_INIT_MAX_LEN to create valid extents.

> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index c5e3fc998211a..5316287ab823a 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -71,7 +71,8 @@ static int update_extent_range(handle_t *handle, struct inode *inode,
>  	 */
>  	if (lb->first_pblock &&
>  		(lb->last_pblock+1 == pblock) &&
> -		(lb->last_block+1 == lb->curr_block)) {
> +		(lb->last_block+1 == lb->curr_block) &&
> +		(lb->last_block - lb->first_block + 1 < EXT_INIT_MAX_LEN)) {
>  		lb->last_pblock = pblock;
>  		lb->last_block = lb->curr_block;
>  		lb->curr_block++;

While reviewing this, I noticed a few pre-existing regressions in the same
file.

fs/ext4/migrate.c:ext4_ext_migrate() {
...
	/* We mark the tmp_inode dirty via ext4_ext_tree_init. */
	retval = ext4_journal_ensure_credits(handle, 1, 0);
	if (retval < 0)
		goto out_stop;
	/*
	 * Mark the tmp_inode as of size zero
	 */
	i_size_write(tmp_inode, 0);
...
	tmp_inode->i_blocks = 0;
...
out_stop:
	ext4_journal_stop(handle);
...
}

[Severity: Critical]
This isn't a bug introduced by this patch, but does the error handling around
ext4_journal_ensure_credits() clobber error codes and cause silent data loss
on failure?

If ext4_journal_ensure_credits() fails after a successful
ext4_ext_swap_inode_data(), the function jumps to out_stop, skipping the
zeroing of tmp_inode->i_blocks and its extent tree. When tmp_inode is
subsequently evicted, ext4_evict_inode() frees all of its blocks. Since these
are the exact physical data blocks now used by the migrated file, the user's
data is entirely deleted.

Alternatively, if migration failed earlier, does this same call overwrite the
negative retval with 0, falsely reporting success while the file is
unmigrated?


fs/ext4/migrate.c:update_tind_extent_range() {
...
	for (i = 0; i < max_entries; i++) {
		if (i_data[i]) {
			retval = update_dind_extent_range(handle, inode,
						le32_to_cpu(i_data[i]), lb);
			if (retval)
				break;
		} else {
			/* Only update the file block number */
			lb->curr_block += max_entries * max_entries;
		}
	}
...
}

[Severity: Critical]
This isn't a bug introduced by this patch, but does the integer overflow of
the logical block counter cause a kernel panic on filesystems with block
sizes of 8KB or larger?

In update_tind_extent_range(), lb->curr_block (a 32-bit ext4_lblk_t) is
incremented by max_entries * max_entries for each empty tind entry. On
filesystems with a block size of 8KB or greater, max_entries is at least
2048. Iterating through all entries adds over 8.5 billion, which overflows
the 32-bit counter.

When ext4_ext_insert_extent() attempts to insert the overlapping extent, can
it hit the BUG_ON(newext->ee_block == nearex->ee_block) check, causing a
kernel panic?


fs/ext4/migrate.c:free_ext_block() {
...
	ix = EXT_FIRST_INDEX(eh);
	for (i = 0; i < le16_to_cpu(eh->eh_entries); i++, ix++) {
		retval = free_ext_idx(handle, inode, ix);
		if (retval)
			return retval;
	}
	return retval;
}

[Severity: High]
This isn't a bug introduced by this patch, but is there a permanent disk block
leak when freeing metadata blocks fails on the error path?

When ext4_ext_migrate() fails, it calls free_ext_block() to release the
allocated extent metadata blocks from tmp_inode. If free_ext_idx()
encounters an error (like failing to ensure journal credits), it aborts the
loop immediately.

ext4_ext_migrate() ignores the return value of free_ext_block() and proceeds
to reset tmp_inode->i_blocks to 0 and clear its extent tree. When tmp_inode is
evicted, the filesystem cannot track or free the remaining metadata blocks,
permanently leaking them on disk.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910034000.3155420-1-chenyichong@uniontech.com?part=1

Re: [PATCH] ext4: cap migration extent length at EXT_INIT_MAX_LEN

[Jan Kara]

On Thu 10-09-26 11:40:00, Yichong Chen wrote:
> update_extent_range() keeps merging physically and logically contiguous
> blocks without limiting the accumulated range.  The maximum length of an
> initialized extent is EXT_INIT_MAX_LEN, because the MSB of the 16-bit
> ee_len field is used to mark unwritten extents.
> 
> finish_range() can therefore store a range longer than EXT_INIT_MAX_LEN
> in ee_len.  A range of 32769 blocks is stored as 0x8001, which
> ext4_ext_is_unwritten() treats as an unwritten extent and
> ext4_ext_get_actual_len() reports as 1 block, so the migrated file
> silently loses its data.
> 
> Stop merging once the accumulated range reaches EXT_INIT_MAX_LEN, so
> migration creates several valid extents instead.

Yeah, or the extent length could overflow the u16 type. The fix looks good.
Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> 
> Fixes: c14c6fd5c56a ("ext4: Add EXT4_IOC_MIGRATE ioctl")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
> ---
>  fs/ext4/migrate.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index d467b5a13c62..2aa6572088cf 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -70,7 +70,8 @@ static int update_extent_range(handle_t *handle, struct inode *inode,
>  	 */
>  	if (lb->first_pblock &&
>  		(lb->last_pblock+1 == pblock) &&
> -		(lb->last_block+1 == lb->curr_block)) {
> +		(lb->last_block+1 == lb->curr_block) &&
> +		(lb->last_block - lb->first_block + 1 < EXT_INIT_MAX_LEN)) {
>  		lb->last_pblock = pblock;
>  		lb->last_block = lb->curr_block;
>  		lb->curr_block++;
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR