[PATCH 0/2] ext4: fix error handling of failed migration

[PATCH 0/2] ext4: fix error handling of failed migration

[Lukas Czerner]

When converting an inode from indirect blocks to extents (the
EXT4_IOC_MIGRATE ioctl) fails, two problems can show up.

ext4_ext_swap_inode_data() can fail after the temporary inode's extent
tree has already been copied into the original inode. The error path
then frees the extent tree blocks via the temporary inode, leaving the
filesystem with multiply claimed blocks. Fix it by resetting the
temporary inode's extent tree and i_blocks right after the copy, so the
cleanup can neither free the original inode's extent tree blocks nor
truncate its data.

Also, ext4_journal_ensure_credits() overwrites whatever error the
migration ran into previously, so the ioctl reports success even though
the migration failed. Use a separate variable for its return value.

Tested with xfstests quick with defaults, home-brewed EXT4_IOC_MIGRATE
stress test and a reproducer written by AI.

Lukas Czerner (2):
  ext4: fix failed migrate freeing extent tree blocks
  ext4: don't lose the error of a failed migration

 fs/ext4/ext4.h    |  1 +
 fs/ext4/extents.c | 11 ++++++++++-
 fs/ext4/migrate.c | 22 ++++++++++++++++++----
 3 files changed, 29 insertions(+), 5 deletions(-)


base-commit: 5b0764a53235d3c77e1ed674e836e1b3fe1a4b1f
-- 
2.56.0

[PATCH 1/2] ext4: fix failed migrate freeing extent tree blocks

[Lukas Czerner]

In case ext4_ext_swap_inode_data() fails it will clean up a failed
migration by freeing the extent tree in the tmp_inode. However, in
case we already copied the temporary inode's extent tree root into
the original inode, the process has been functionally finished.
The error case clean up calls free_ext_block() which will free those
extent tree blocks while the original inode already points at them and
the filesystem ends up with multiply claimed blocks.

Fix it by resetting the temporary inode's root to an empty tree right
after copying i_data to the original inode, so free_ext_block() won't
free the original inode extent tree blocks. Move temporary inode's
i_blocks as well so eviction won't try to truncate metadata blocks the
tmp_inode no longer owns.

Signed-off-by: Lukas Czerner <lukas@czerner.cz>
---
 fs/ext4/ext4.h    |  1 +
 fs/ext4/extents.c | 11 ++++++++++-
 fs/ext4/migrate.c | 13 ++++++++++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 1ee5733e2b31..915476d4d089 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3903,6 +3903,7 @@ struct ext4_extent;
  */
 #define EXT_MAX_BLOCKS	0xffffffff
 
+extern void ext4_ext_init_root(struct inode *inode);
 extern void ext4_ext_tree_init(handle_t *handle, struct inode *inode);
 extern int ext4_ext_index_trans_blocks(struct inode *inode, int extents);
 extern int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 3bf0a6cdbec2..ada059f83295 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -869,7 +869,11 @@ ext4_ext_binsearch(struct inode *inode,
 
 }
 
-void ext4_ext_tree_init(handle_t *handle, struct inode *inode)
+/*
+ * Initialize the extent tree of inode. This only changes the in-memory
+ * inode, the caller has to mark the inode dirty.
+ */
+void ext4_ext_init_root(struct inode *inode)
 {
 	struct ext4_extent_header *eh;
 
@@ -879,6 +883,11 @@ void ext4_ext_tree_init(handle_t *handle, struct inode *inode)
 	eh->eh_magic = EXT4_EXT_MAGIC;
 	eh->eh_max = cpu_to_le16(ext4_ext_space_root(inode, 0));
 	eh->eh_generation = 0;
+}
+
+void ext4_ext_tree_init(handle_t *handle, struct inode *inode)
+{
+	ext4_ext_init_root(inode);
 	ext4_mark_inode_dirty(handle, inode);
 }
 
diff --git a/fs/ext4/migrate.c b/fs/ext4/migrate.c
index 5d60ef10fe11..0d32cfa55785 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -322,6 +322,12 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
 	 */
 	ext4_set_inode_flag(inode, EXT4_INODE_EXTENTS);
 	memcpy(ei->i_data, tmp_ei->i_data, sizeof(ei->i_data));
+	/*
+	 * The extent tree belongs to the inode now. Make tmp_inode's extent
+	 * tree empty, so that cleaning up tmp_inode can't free the tree
+	 * blocks, if we fail to finish the migration.
+	 */
+	ext4_ext_init_root(tmp_inode);
 
 	/*
 	 * Update i_blocks with the new blocks that got
@@ -331,10 +337,13 @@ static int ext4_ext_swap_inode_data(handle_t *handle, struct inode *inode,
 	 * While converting to extents we need not
 	 * update the original inode i_blocks for extent blocks
 	 * via quota APIs. The quota update happened via tmp_inode already.
+	 * Clear tmp_inode i_blocks to avoid truncating original data blocks
+	 * in case we fail to finish the migration.
 	 */
 	spin_lock(&inode->i_lock);
 	inode->i_blocks += tmp_inode->i_blocks;
 	spin_unlock(&inode->i_lock);
+	tmp_inode->i_blocks = 0;
 	up_write(&EXT4_I(inode)->i_data_sem);
 
 	/*
@@ -561,7 +570,9 @@ int ext4_ext_migrate(struct inode *inode)
 		if (retval)
 			/*
 			 * if we fail to swap inode data free the extent
-			 * details of the tmp inode
+			 * details of the tmp inode.  If the tree was already
+			 * moved to the original inode, the tmp_inode has an
+			 * empty tree and there is nothing to free.
 			 */
 			free_ext_block(handle, tmp_inode);
 	}
-- 
2.56.0

[PATCH 2/2] ext4: don't lose the error of a failed migration

[Lukas Czerner]

ext4_journal_ensure_credits() overwrites whatever error the migration
might have run into previously. As a result EXT4_IOC_MIGRATE ioctl will
report success, while the actual migration failed.

Fix it by using a separate variable to store return value from
ext4_journal_ensure_credits().

Signed-off-by: Lukas Czerner <lukas@czerner.cz>
---
 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 0d32cfa55785..d2f8f2b6206b 100644
--- a/fs/ext4/migrate.c
+++ b/fs/ext4/migrate.c
@@ -419,7 +419,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, retval2, i;
 	__le32 *i_data;
 	struct ext4_inode_info *ei;
 	struct inode *tmp_inode = NULL;
@@ -578,9 +578,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)
+	retval2 = ext4_journal_ensure_credits(handle, 1, 0);
+	if (retval2 < 0) {
+		if (retval == 0)
+			retval = retval2;
 		goto out_stop;
+	}
 	/*
 	 * Mark the tmp_inode as of size zero
 	 */
-- 
2.56.0

Re: [PATCH 2/2] ext4: don't lose the error of a failed migration

[sashiko-bot]

> ext4_journal_ensure_credits() overwrites whatever error the migration
> might have run into previously. As a result EXT4_IOC_MIGRATE ioctl will
> report success, while the actual migration failed.
> 
> Fix it by using a separate variable to store return value from
> ext4_journal_ensure_credits().
> 
> Signed-off-by: Lukas Czerner <lukas@czerner.cz>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025201803.3043712-1-lukas@czerner.cz?part=2

Re: [PATCH 1/2] ext4: fix failed migrate freeing extent tree blocks

[sashiko-bot]

> In case ext4_ext_swap_inode_data() fails it will clean up a failed
> migration by freeing the extent tree in the tmp_inode. However, in
> case we already copied the temporary inode's extent tree root into
> the original inode, the process has been functionally finished.
> The error case clean up calls free_ext_block() which will free those
> extent tree blocks while the original inode already points at them and
> the filesystem ends up with multiply claimed blocks.
> 
> Fix it by resetting the temporary inode's root to an empty tree right
> after copying i_data to the original inode, so free_ext_block() won't
> free the original inode extent tree blocks. Move temporary inode's
> i_blocks as well so eviction won't try to truncate metadata blocks the
> tmp_inode no longer owns.
> 
> Signed-off-by: Lukas Czerner <lukas@czerner.cz>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025201803.3043712-1-lukas@czerner.cz?part=1