Re: [PATCH] ext4: clear journal restart indication after inserting extent

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Preexisting bug: `update_extent_range` does not cap contiguous block accumulation at `EXT_INIT_MAX_LEN`, leading to silent data corruption during migration.
--

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

ext4: clear journal restart indication after inserting extent
    
ext4_datasem_ensure_credits() returns 1 when it successfully restarts
the transaction. finish_range() leaves that value in retval when the
subsequent ext4_ext_insert_extent() succeeds.

> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 5d60ef10fe110..d467b5a13c62e 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -53,8 +53,7 @@ static int finish_range(handle_t *handle, struct inode *inode,
>  	if (retval < 0)
>  		goto err_out;
>  	path = ext4_ext_insert_extent(handle, inode, path, &newext, 0);

[Severity: Critical]
This is a pre-existing issue, but does update_extent_range() fail to cap
contiguous block accumulation at EXT_INIT_MAX_LEN?

If a local user executes chattr +e on a file they own that contains a
contiguous block allocation exceeding 128MB, update_extent_range() appears to
aggressively merge blocks that are physically and logically contiguous without
bounds checking:

fs/ext4/migrate.c:update_extent_range() {
	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;
	}
    ...
}

Could this create an extent larger than 32768 blocks? When finish_range()
creates the extent:

fs/ext4/migrate.c:finish_range() {
    ...
	newext.ee_len   = cpu_to_le16(lb->last_block - lb->first_block + 1);
    ...
}

Will this unbounded length assignment to a 16-bit field cause truncation or
set the unwritten flag for values > 32768, potentially leading to silent data
loss during migration?

> -	if (IS_ERR(path))
> -		retval = PTR_ERR(path);
> +	retval = PTR_ERR_OR_ZERO(path);
>  err_out:
>  	up_write((&EXT4_I(inode)->i_data_sem));
>  	ext4_free_ext_path(path);

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