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

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

[Yichong Chen]

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. Its callers interpret any
nonzero return as a migration failure, so they stop building the extent
tree and take the cleanup path instead of completing the migration.

Before ext4_ext_insert_extent() was changed to return a path, its integer
return value overwrote the restart indication. Restore that behavior with
PTR_ERR_OR_ZERO(), preserving insertion errors and returning zero on
success.

Fixes: f7d1331f16a8 ("ext4: get rid of ppath in ext4_ext_insert_extent()")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---
 fs/ext4/migrate.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 5d60ef10fe11..d467b5a13c62 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);
-	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);
-- 
2.51.0

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

[sashiko-bot]

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

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

[Jan Kara]

On Wed 09-09-26 11:35:42, Yichong Chen wrote:
> 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. Its callers interpret any
> nonzero return as a migration failure, so they stop building the extent
> tree and take the cleanup path instead of completing the migration.
> 
> Before ext4_ext_insert_extent() was changed to return a path, its integer
> return value overwrote the restart indication. Restore that behavior with
> PTR_ERR_OR_ZERO(), preserving insertion errors and returning zero on
> success.
> 
> Fixes: f7d1331f16a8 ("ext4: get rid of ppath in ext4_ext_insert_extent()")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  fs/ext4/migrate.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 5d60ef10fe11..d467b5a13c62 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);
> -	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);
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR