Re: [PATCH v2] ext4: fix the logical block counter overflow in indirect migration

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds check on extent length during contiguous block aggregation leads to extent corruption and silent data loss.
- [Critical] Erroneous freeing of active extent metadata blocks if `free_ind_block()` fails after the inode data is permanently swapped.
- [High] `ext4_ext_migrate()` silently overwrites error codes, including the newly added `-EOPNOTSUPP`, falsely reporting success to userspace.
--

commit 551790cf82563190f29580d447430a3723ff4943
Author: Yichong Chen <chenyichong@uniontech.com>
Subject: ext4: fix the logical block counter overflow in indirect migration

This commit prevents a logical block counter overflow in the ext4 indirect
migration path by keeping the counter in 64-bit and refusing migration if a
block reaches EXT_MAX_BLOCKS.

> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index c5e3fc998211a..f492935d5e3de 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c

[ ... ]

> @@ -66,6 +66,10 @@ static int update_extent_range(handle_t *handle, struct inode *inode,
>  			       ext4_fsblk_t pblock, struct migrate_struct *lb)
>  {
>  	int retval;
> +
> +	if (lb->curr_block >= EXT_MAX_BLOCKS)
> +		return -EOPNOTSUPP;
> +
>  	/*
>  	 * See if we can add on to the existing range (if it exists)
>  	 */
>  	if (lb->first_pblock &&
>  		(lb->last_pblock+1 == pblock) &&
>  		(lb->last_block+1 == lb->curr_block)) {
>  		lb->last_pblock = pblock;
>  		lb->last_block = lb->curr_block;
>  		lb->curr_block++;
>  		return 0;
>  	}

[Severity: Critical]
This is a pre-existing issue, but it appears this contiguous block check
blindly accumulates blocks without verifying that the maximum extent length
(EXT_INIT_MAX_LEN) is respected.

If a file has more than 32768 physically contiguous blocks, the resulting
length is later cast to 16 bits in finish_range():

	newext.ee_len   = cpu_to_le16(lb->last_block - lb->first_block + 1);

Could a length greater than 32768 set the 0x8000 (UNWRITTEN) bit or overflow
entirely, causing valid data to read as zeroes and corrupting the file?


[Severity: High]
This isn't a bug introduced by this patch, but the error handling in
ext4_ext_migrate() seems to unconditionally overwrite error codes like the
newly added -EOPNOTSUPP.

Looking at the error path in ext4_ext_migrate():

err_out:
	if (retval)
		/*
		 * Failure case delete the extent information with the
		 * tmp_inode
		 */
		free_ext_block(handle, tmp_inode);
	else {
		retval = ext4_ext_swap_inode_data(handle, inode, tmp_inode);
		if (retval)
			/*
			 * if we fail to swap inode data free the extent
			 * details of the tmp inode
			 */
			free_ext_block(handle, tmp_inode);
	}

	/* 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;

If any function within the migration loop (like the new EXT_MAX_BLOCKS check)
fails, it jumps to err_out with a negative error code in retval. However,
retval is then overwritten by ext4_journal_ensure_credits().

Since this function typically returns 0 or 1 on success, might this falsely
report success to userspace and leave the file unmodified or incomplete?


[Severity: Critical]
This is a pre-existing issue, but I am wondering if the rollback logic in
ext4_ext_migrate() erroneously frees active extent metadata when
ext4_ext_swap_inode_data() encounters an error.

In ext4_ext_swap_inode_data(), the original inode is permanently modified to
use extents:

	ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
	memcpy(ei->i_data, tmp_ei->i_data, sizeof(ei->i_data));

	/* ... snip ... */

	retval = free_ind_block(handle, inode, i_data);
	retval2 = ext4_mark_inode_dirty(handle, inode);
	if (unlikely(retval2 && !retval))
		retval = retval2;

err_out:
	return retval;

If free_ind_block() fails (for example, due to a lack of journal credits), it
returns an error. This error is caught in ext4_ext_migrate(), which then
attempts to clean up:

	else {
		retval = ext4_ext_swap_inode_data(handle, inode, tmp_inode);
		if (retval)
			/*
			 * if we fail to swap inode data free the extent
			 * details of the tmp inode
			 */
			free_ext_block(handle, tmp_inode);
	}

Since the extent tree root was already copied to the active inode, is it
possible that calling free_ext_block(handle, tmp_inode) blindly frees the
exact metadata blocks that the active inode now relies upon, leading to severe
on-disk data corruption?

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