Re: [PATCH] ext4: don't clobber the migration error in ext4_ext_migrate()

[Jan Kara]

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@xxxxxxxxxxxxx>

Indeed. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								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@xxxxxxxx>
SUSE Labs, CR