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

[sashiko-bot@kernel.org]

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