[PATCH] ext4: Fix possible corruption when moving a directory

[PATCH] ext4: Fix possible corruption when moving a directory

[Jan Kara]

When we are renaming a directory to a different directory, we need to
update '..' entry in the moved directory. However nothing prevents moved
directory from being modified and even converted from the inline format
to the normal format. When such race happens the rename code gets
confused and we crash. Fix the problem by locking the moved directory.

CC: stable@vger.kernel.org
Fixes: 32f7f22c0b52 ("ext4: let ext4_rename handle inline dir")
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/namei.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index dd28453d6ea3..270fbcba75b6 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3872,9 +3872,16 @@ static int ext4_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
 			if (new.dir != old.dir && EXT4_DIR_LINK_MAX(new.dir))
 				goto end_rename;
 		}
+		/*
+		 * We need to protect against old.inode directory getting
+		 * converted from inline directory format into a normal one.
+		 */
+		inode_lock_nested(old.inode, I_MUTEX_NONDIR2);
 		retval = ext4_rename_dir_prepare(handle, &old);
-		if (retval)
+		if (retval) {
+			inode_unlock(old.inode);
 			goto end_rename;
+		}
 	}
 	/*
 	 * If we're renaming a file within an inline_data dir and adding or
@@ -4006,6 +4013,8 @@ static int ext4_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
 	} else {
 		ext4_journal_stop(handle);
 	}
+	if (old.dir_bh)
+		inode_unlock(old.inode);
 release_bh:
 	brelse(old.dir_bh);
 	brelse(old.bh);
-- 
2.35.3

Re: [PATCH] ext4: Fix possible corruption when moving a directory

[Theodore Ts'o]

On Thu, 26 Jan 2023 12:22:21 +0100, Jan Kara wrote:
> When we are renaming a directory to a different directory, we need to
> update '..' entry in the moved directory. However nothing prevents moved
> directory from being modified and even converted from the inline format
> to the normal format. When such race happens the rename code gets
> confused and we crash. Fix the problem by locking the moved directory.
> 
> 
> [...]

Applied, thanks!

[1/1] ext4: Fix possible corruption when moving a directory
      commit: 98c8afa3038a32bcd062efd0b4b7009436049b7d

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>

Re: [PATCH] ext4: Fix possible corruption when moving a directory

[Darrick J. Wong]

On Thu, Jan 26, 2023 at 12:22:21PM +0100, Jan Kara wrote:
> When we are renaming a directory to a different directory, we need to
> update '..' entry in the moved directory. However nothing prevents moved
> directory from being modified and even converted from the inline format
> to the normal format. When such race happens the rename code gets
> confused and we crash. Fix the problem by locking the moved directory.

Four months later, I have a question --

Is it necessary for ext4_cross_rename to inode_lock_nested on both
old.inode and new.inode?  We're resetting the dotdot entries on both
children in that case, which means that we also need to lock out inline
data conversions, right?

--D

> CC: stable@vger.kernel.org
> Fixes: 32f7f22c0b52 ("ext4: let ext4_rename handle inline dir")
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/ext4/namei.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index dd28453d6ea3..270fbcba75b6 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -3872,9 +3872,16 @@ static int ext4_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
>  			if (new.dir != old.dir && EXT4_DIR_LINK_MAX(new.dir))
>  				goto end_rename;
>  		}
> +		/*
> +		 * We need to protect against old.inode directory getting
> +		 * converted from inline directory format into a normal one.
> +		 */
> +		inode_lock_nested(old.inode, I_MUTEX_NONDIR2);
>  		retval = ext4_rename_dir_prepare(handle, &old);
> -		if (retval)
> +		if (retval) {
> +			inode_unlock(old.inode);
>  			goto end_rename;
> +		}
>  	}
>  	/*
>  	 * If we're renaming a file within an inline_data dir and adding or
> @@ -4006,6 +4013,8 @@ static int ext4_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
>  	} else {
>  		ext4_journal_stop(handle);
>  	}
> +	if (old.dir_bh)
> +		inode_unlock(old.inode);
>  release_bh:
>  	brelse(old.dir_bh);
>  	brelse(old.bh);
> -- 
> 2.35.3
> 

Re: [PATCH] ext4: Fix possible corruption when moving a directory

[Jan Kara]

On Tue 16-05-23 21:58:36, Darrick J. Wong wrote:
> On Thu, Jan 26, 2023 at 12:22:21PM +0100, Jan Kara wrote:
> > When we are renaming a directory to a different directory, we need to
> > update '..' entry in the moved directory. However nothing prevents moved
> > directory from being modified and even converted from the inline format
> > to the normal format. When such race happens the rename code gets
> > confused and we crash. Fix the problem by locking the moved directory.
> 
> Four months later, I have a question --
> 
> Is it necessary for ext4_cross_rename to inode_lock_nested on both
> old.inode and new.inode?  We're resetting the dotdot entries on both
> children in that case, which means that we also need to lock out inline
> data conversions, right?

Ouch, you're right. In that path we need to lock both source & target
directories since lock_two_nondirectories() call in vfs_rename() will not
lock them... I'll send a patch. Thanks for spotting this!

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR