* Re: [PATCH] ext4: don't clobber the migration error in ext4_ext_migrate()
2026-09-10 5:29 [PATCH] ext4: don't clobber the migration error in ext4_ext_migrate() Yichong Chen
@ 2026-09-10 5:43 ` sashiko-bot
2026-09-10 10:17 ` Jan Kara
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-09-10 5:43 UTC (permalink / raw)
To: Yichong Chen; +Cc: linux-ext4, tytso
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but skipping tmp_inode cleanup on journal credit failure leads to severe data loss via ext4_evict_inode().
- [High] This problem wasn't introduced by this patch, but finish_range() and ext4_ext_migrate() misinterpret the positive return value (1) from transaction restarts as an error, causing valid migrations to incorrectly abort.
- [High] This problem wasn't introduced by this patch, but restarting the journal transaction after ext4_ext_swap_inode_data() splits the migration, risking catastrophic filesystem corruption on crash.
--
commit 1e2365dd0dc0240e7c7b7887168a101dc865de11
Author: Yichong Chen <chenyichong@uniontech.com>
ext4: don't clobber the migration error in ext4_ext_migrate()
This commit ensures that ext4_ext_migrate() does not overwrite the
migration error code when ext4_journal_ensure_credits() returns a
positive value indicating a transaction restart. It introduces a
separate err variable to hold the credit check result.
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 5d60ef10fe110..02bb7c20a008a 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -410,7 +410,7 @@ static int free_ext_block(handle_t *handle, struct inode *inode)
> int ext4_ext_migrate(struct inode *inode)
> {
> handle_t *handle;
> - int retval = 0, i;
> + int retval = 0, err, i;
[Severity: High]
This isn't a bug introduced by this patch, but finish_range() and
ext4_ext_migrate() misinterpret the positive return value (1) from
transaction restarts as an error, causing valid migrations to
incorrectly abort.
During migration, if ext4_datasem_ensure_credits() successfully
restarts the journal, it returns 1. This 1 is propagated through
finish_range() and update_extent_range() back here to
ext4_ext_migrate().
When ext4_ext_migrate() subsequently checks the return value:
retval = update_extent_range(handle, tmp_inode,
le32_to_cpu(i_data[i]), &lb);
if (retval)
goto err_out;
Could this incorrectly treat the 1 as a fatal error, aborting the
migration? This would cause the migration of any file large enough
to require a transaction restart to fail and return 1 to userspace,
breaking the migration feature for large files.
[ ... ]
> @@ -567,9 +567,12 @@ int ext4_ext_migrate(struct inode *inode)
> }
>
> /* We mark the tmp_inode dirty via ext4_ext_tree_init. */
> - retval = ext4_journal_ensure_credits(handle, 1, 0);
> - if (retval < 0)
> + err = ext4_journal_ensure_credits(handle, 1, 0);
[Severity: High]
This is a pre-existing issue, but restarting the journal transaction
after ext4_ext_swap_inode_data() splits the migration, risking
catastrophic filesystem corruption on crash.
ext4_ext_swap_inode_data() commits the extent tree to the actual
inode and dirties it. If this subsequent call to
ext4_journal_ensure_credits() requires restarting the transaction
(returning 1), the current state is committed to disk.
If a crash occurs before the next transaction completes the cleanup
of tmp_inode, wouldn't the disk contain both the migrated inode and
the unlinked tmp_inode pointing to the same data blocks?
Because tmp_inode is unlinked via clear_nlink(tmp_inode) but was never
added to the on-disk orphan list via ext4_orphan_add(), kernel orphan
recovery ignores it, potentially leaving e2fsck to encounter
multiply-claimed blocks.
> + if (err < 0) {
> + if (!retval)
> + retval = err;
> goto out_stop;
> + }
[Severity: Critical]
This problem wasn't introduced by this patch, but skipping tmp_inode
cleanup on journal credit failure leads to severe data loss via
ext4_evict_inode().
If this final credit check fails (e.g., due to ENOMEM or journal
abort), the code jumps to out_stop, bypassing the essential
tmp_inode cleanup:
if (err < 0) {
goto out_stop;
}
...
tmp_inode->i_blocks = 0;
EXT4_I(tmp_inode)->i_csum_seed = tmp_csum_seed;
ext4_ext_tree_init(handle, tmp_inode);
out_stop:
When tmp_inode is then dropped via iput(), its i_nlink is 0, so
ext4_evict_inode() truncates it. Does bypassing the i_blocks = 0
and ext4_ext_tree_init() reset result in freeing all the extent data
blocks that now belong to the user's migrated file?
> /*
> * Mark the tmp_inode as of size zero
> */
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260910052927.3161092-1-chenyichong@uniontech.com?part=1
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] ext4: don't clobber the migration error in ext4_ext_migrate()
2026-09-10 5:29 [PATCH] ext4: don't clobber the migration error in ext4_ext_migrate() Yichong Chen
2026-09-10 5:43 ` sashiko-bot
@ 2026-09-10 10:17 ` Jan Kara
1 sibling, 0 replies; 3+ messages in thread
From: Jan Kara @ 2026-09-10 10:17 UTC (permalink / raw)
To: Yichong Chen
Cc: Theodore Ts'o, linux-ext4, linux-kernel, Andreas Dilger,
Baokun Li, Jan Kara, Ojaswin Mujoo, Ritesh Harjani, Zhang Yi
On Thu 10-09-26 13:29:27, Yichong Chen wrote:
> ext4_ext_migrate() records the failure of the migration in retval, but
> ext4_journal_ensure_credits() overwrites it: it returns 0 when the handle
> has enough credits or the transaction could be extended, and 1 when the
> transaction had to be restarted. A failed migration is therefore
> reported as success, and a restart leaks the internal value 1 to
> userspace through EXT4_IOC_MIGRATE and FS_IOC_SETFLAGS.
>
> Fixes: a413036791d0 ("ext4: Provide function to handle transaction restarts")
> Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
Indeed. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/ext4/migrate.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
> index 2aa6572088cf..e06d847033a1 100644
> --- a/fs/ext4/migrate.c
> +++ b/fs/ext4/migrate.c
> @@ -410,7 +410,7 @@ static int free_ext_block(handle_t *handle, struct inode *inode)
> int ext4_ext_migrate(struct inode *inode)
> {
> handle_t *handle;
> - int retval = 0, i;
> + int retval = 0, err, i;
> __le32 *i_data;
> struct ext4_inode_info *ei;
> struct inode *tmp_inode = NULL;
> @@ -567,9 +567,12 @@ int ext4_ext_migrate(struct inode *inode)
> }
>
> /* We mark the tmp_inode dirty via ext4_ext_tree_init. */
> - retval = ext4_journal_ensure_credits(handle, 1, 0);
> - if (retval < 0)
> + err = ext4_journal_ensure_credits(handle, 1, 0);
> + if (err < 0) {
> + if (!retval)
> + retval = err;
> goto out_stop;
> + }
> /*
> * Mark the tmp_inode as of size zero
> */
> --
> 2.51.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread