[PATCH v2] ext4: drop tmp inode ownership before migration cleanup

[PATCH v2] ext4: drop tmp inode ownership before migration cleanup

[Yichong Chen]

After the extent tree is transferred to the original inode,
ext4_journal_ensure_credits() can fail before tmp_inode is cleared. The
subsequent iput(tmp_inode) may then free the transferred tree.

Move the existing resets of tmp_inode's size, block count and checksum
seed before ext4_journal_ensure_credits(). This leaves the temporary inode
ready for eviction even if obtaining journal credits fails. Keep
ext4_ext_tree_init(), which marks the inode dirty, after the credits check.

Fixes: a413036791d0 ("ext4: Provide function to handle transaction restarts")
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
---

Notes:
    Changes in v2:
    - Move the existing i_size, i_blocks and i_csum_seed resets before
      ext4_journal_ensure_credits() instead of adding a conditional duplicate,
      as suggested by Jan Kara.
    
    v1: https://lore.kernel.org/all/20260930080241.414973-1-chenyichong@uniontech.com/

 fs/ext4/migrate.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 09931d3ba2c6..cf5239c42693 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -566,10 +566,6 @@ int ext4_ext_migrate(struct inode *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;
 	/*
 	 * Mark the tmp_inode as of size zero
 	 */
@@ -586,6 +582,11 @@ int ext4_ext_migrate(struct inode *inode)
 	tmp_inode->i_blocks = 0;
 	EXT4_I(tmp_inode)->i_csum_seed = tmp_csum_seed;
 
+	/* 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;
+
 	/* Reset the extent details */
 	ext4_ext_tree_init(handle, tmp_inode);
 out_stop:

base-commit: 9a482184d910775cf648863148944945b74e2815
-- 
2.51.0

Re: [PATCH v2] ext4: drop tmp inode ownership before migration cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: ext4_ext_migrate() overwrites retval with ext4_journal_ensure_credits()
--

commit 61a58c38b235cc8fa8d6d2591545809a44de0a40
Author: Yichong Chen <chenyichong@uniontech.com>

ext4: drop tmp inode ownership before migration cleanup

This commit moves the resets of tmp_inode's i_size, i_blocks, and
i_csum_seed in ext4_ext_migrate() ahead of ext4_journal_ensure_credits()
so that iput(tmp_inode) will not free the migrated extent tree if
obtaining journal credits fails.

> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 09931d3ba2c6a..cf5239c426935 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -566,10 +566,6 @@ int ext4_ext_migrate(struct inode *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;
>  	/*
>  	 * Mark the tmp_inode as of size zero
>  	 */
> @@ -586,6 +582,11 @@ int ext4_ext_migrate(struct inode *inode)
>  	tmp_inode->i_blocks = 0;
>  	EXT4_I(tmp_inode)->i_csum_seed = tmp_csum_seed;
>  
> +	/* 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;

[Severity: High]
While moving this block from above, assigning the return value of
ext4_journal_ensure_credits() directly to retval in
fs/ext4/migrate.c:ext4_ext_migrate() still overwrites any prior error
stored in retval and propagates positive return values to the caller:

fs/ext4/migrate.c:ext4_ext_migrate() {
	...
err_out:
	if (retval)
		free_ext_block(handle, tmp_inode);
	else {
		retval = ext4_ext_swap_inode_data(handle, inode, tmp_inode);
		if (retval)
			free_ext_block(handle, tmp_inode);
	}
	...
	retval = ext4_journal_ensure_credits(handle, 1, 0);
	if (retval < 0)
		goto out_stop;

	/* Reset the extent details */
	ext4_ext_tree_init(handle, tmp_inode);
out_stop:
	ext4_journal_stop(handle);
out_tmp_inode:
	unlock_new_inode(tmp_inode);
out_unlock:
	ext4_writepages_up_write(inode->i_sb, alloc_ctx);
	iput(tmp_inode);
	return retval;
}

In fs/ext4/ext4_jbd2.h, ext4_journal_ensure_credits_fn() returns 0 when
credits are sufficient or extended, and 1 when the transaction had to be
restarted:

	err = ext4_journal_restart((handle), (extend_cred), (revoke_cred)); \
	if (err == 0)							\
		err = 1;						\

If building the extent tree (update_extent_range(),
update_ind_extent_range(), update_dind_extent_range(),
update_tind_extent_range(), or finish_range()) or swapping the inode data
in ext4_ext_swap_inode_data() fails with a negative error in retval, and
ext4_journal_ensure_credits() then succeeds, retval is overwritten with 0
or 1, losing the migration error.

Conversely, if migration succeeds (retval == 0) and
ext4_journal_ensure_credits() restarts the transaction, ext4_ext_migrate()
returns 1 to callers such as ext4_fileattr_set() -> ext4_ioctl_setflags()
in fs/ext4/ioctl.c:

	err = ext4_ioctl_setflags(inode, flags);
	if (err)
		goto out;
	err = ext4_ioctl_setproject(inode, fa->fsx_projid);

which treats the non-zero return value as an error and aborts before
ext4_ioctl_setproject().

Should ext4_journal_ensure_credits() use a separate error variable (or
only update retval when it returns a negative error and retval is not
already set) so that prior migration errors are preserved and a
transaction restart does not cause ext4_ext_migrate() to return 1?

> +
>  	/* Reset the extent details */
>  	ext4_ext_tree_init(handle, tmp_inode);
>  out_stop:

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

Re: [PATCH v2] ext4: drop tmp inode ownership before migration cleanup

[Jan Kara]

On Thu 22-10-26 15:02:35, Yichong Chen wrote:
> After the extent tree is transferred to the original inode,
> ext4_journal_ensure_credits() can fail before tmp_inode is cleared. The
> subsequent iput(tmp_inode) may then free the transferred tree.
> 
> Move the existing resets of tmp_inode's size, block count and checksum
> seed before ext4_journal_ensure_credits(). This leaves the temporary inode
> ready for eviction even if obtaining journal credits fails. Keep
> ext4_ext_tree_init(), which marks the inode dirty, after the credits check.
> 
> Fixes: a413036791d0 ("ext4: Provide function to handle transaction restarts")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>

Looks good. Feel free to add:

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

								Honza

> ---
> 
> Notes:
>     Changes in v2:
>     - Move the existing i_size, i_blocks and i_csum_seed resets before
>       ext4_journal_ensure_credits() instead of adding a conditional duplicate,
>       as suggested by Jan Kara.
>     
>     v1: https://lore.kernel.org/all/20260930080241.414973-1-chenyichong@uniontech.com/
> 
>  fs/ext4/migrate.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 09931d3ba2c6..cf5239c42693 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -566,10 +566,6 @@ int ext4_ext_migrate(struct inode *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;
>  	/*
>  	 * Mark the tmp_inode as of size zero
>  	 */
> @@ -586,6 +582,11 @@ int ext4_ext_migrate(struct inode *inode)
>  	tmp_inode->i_blocks = 0;
>  	EXT4_I(tmp_inode)->i_csum_seed = tmp_csum_seed;
>  
> +	/* 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;
> +
>  	/* Reset the extent details */
>  	ext4_ext_tree_init(handle, tmp_inode);
>  out_stop:
> 
> base-commit: 9a482184d910775cf648863148944945b74e2815
> -- 
> 2.51.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR