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

[sashiko-bot@kernel.org]

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