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

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

[Yichong Chen]

update_tind_extent_range() advances lb->curr_block, an ext4_lblk_t, by
max_entries * max_entries for every empty triple-indirect slot.  One
triple-indirect block spans max_entries^3 logical blocks, which exceeds
2^32 as soon as the block size is 8K or larger (16384^3 = 2^42 with 64K
blocks), so the counter wraps while that block is walked.

A wrapped counter makes the migration store a block number that is 2^32
blocks away from the one the pointer block describes.  Two ranges can then
end up with the same ee_block, which trips
BUG_ON(newext->ee_block == nearex->ee_block) in ext4_ext_insert_extent(),
and without that collision the data is still moved to the wrong logical
block while the migration reports success.

Keep the counter in 64 bit so that it cannot wrap, and refuse the
migration with -EOPNOTSUPP once a data block would land on EXT_MAX_BLOCKS
or beyond: an extent may not cover that block, because
ext4_valid_extent() rejects a wrapping ee_block + ee_len, and only a
corrupt block map has data there.

Fixes: c14c6fd5c56a ("ext4: Add EXT4_IOC_MIGRATE ioctl")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---

Notes:
    v2: refuse EXT_MAX_BLOCKS (0xffffffff) as well, so the bound is >= instead
        of >.  An extent may not cover that block: ext4_valid_extent() rejects a
        wrapping ee_block + ee_len, so storing it would leave the migrated inode
        corrupt.

 fs/ext4/migrate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index e06d847033a1..2ce587043945 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -14,7 +14,7 @@
  * represented by a single extent
  */
 struct migrate_struct {
-	ext4_lblk_t first_block, last_block, curr_block;
+	u64 first_block, last_block, curr_block;
 	ext4_fsblk_t first_pblock, last_pblock;
 };
 
@@ -65,6 +65,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)
 	 */
-- 
2.51.0

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

[sashiko-bot]

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

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

[Jan Kara]

On Mon 14-09-26 16:25:16, Yichong Chen wrote:
> update_tind_extent_range() advances lb->curr_block, an ext4_lblk_t, by
> max_entries * max_entries for every empty triple-indirect slot.  One
> triple-indirect block spans max_entries^3 logical blocks, which exceeds
> 2^32 as soon as the block size is 8K or larger (16384^3 = 2^42 with 64K
> blocks), so the counter wraps while that block is walked.
> 
> A wrapped counter makes the migration store a block number that is 2^32
> blocks away from the one the pointer block describes.  Two ranges can then
> end up with the same ee_block, which trips
> BUG_ON(newext->ee_block == nearex->ee_block) in ext4_ext_insert_extent(),
> and without that collision the data is still moved to the wrong logical
> block while the migration reports success.
> 
> Keep the counter in 64 bit so that it cannot wrap, and refuse the
> migration with -EOPNOTSUPP once a data block would land on EXT_MAX_BLOCKS
> or beyond: an extent may not cover that block, because
> ext4_valid_extent() rejects a wrapping ee_block + ee_len, and only a
> corrupt block map has data there.
> 
> Fixes: c14c6fd5c56a ("ext4: Add EXT4_IOC_MIGRATE ioctl")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>

This is even better. Feel free to add:

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

								Honza

> ---
> 
> Notes:
>     v2: refuse EXT_MAX_BLOCKS (0xffffffff) as well, so the bound is >= instead
>         of >.  An extent may not cover that block: ext4_valid_extent() rejects a
>         wrapping ee_block + ee_len, so storing it would leave the migrated inode
>         corrupt.
> 
>  fs/ext4/migrate.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index e06d847033a1..2ce587043945 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -14,7 +14,7 @@
>   * represented by a single extent
>   */
>  struct migrate_struct {
> -	ext4_lblk_t first_block, last_block, curr_block;
> +	u64 first_block, last_block, curr_block;
>  	ext4_fsblk_t first_pblock, last_pblock;
>  };
>  
> @@ -65,6 +65,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)
>  	 */
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR