ext4: check orig_data in remount

Currently duplicate data to orig_data in ext4_remount()
but there is lack of check for duplication result.
This patch adds proper check for it.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 fs/ext4/super.c | 3 +++
 1 file changed, 3 insertions(+)

Hi Ted, Andreas

Could you have a look at this trivial patch?

Thanks.

On 06/06/2018 04:26 PM, Chengguang Xu wrote:
> Currently duplicate data to orig_data in ext4_remount()
> but there is lack of check for duplication result.
> This patch adds proper check for it.
>
> Signed-off-by: Chengguang Xu<cgxu519@gmx.com>
> ---
>   fs/ext4/super.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 00fe75a71c4b..cba245460a9c 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5014,6 +5014,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>   #endif
>   	char *orig_data = kstrdup(data, GFP_KERNEL);
>   
> +	if (data && !orig_data)
> +		return -ENOMEM;
> +
>   	/* Store the original options */
>   	old_sb_flags = sb->s_flags;
>   	old_opts.s_mount_opt = sbi->s_mount_opt;

> On Jun 6, 2018, at 2:26 AM, Chengguang Xu <cgxu519@gmx.com> wrote:
> 
> Currently duplicate data to orig_data in ext4_remount()
> but there is lack of check for duplication result.
> This patch adds proper check for it.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>

Thank you for the patch.  I think it looks correct, but I was
initially confused by the commit message.  I think something
like the following would be more clear:

    There is no check for allocation failure when duplicating
    "orig_data" in ext4_remount().  Check for failure and
    return an error in this case.


While I don't think it is (currently) possible for a one-page
allocation to fail (always true because of copy_mount_options()
used in ksys_mount()), I prefer to always check for allocation
failures in case this ever changes in the future, or if there
is allocation fault injection or other fuzzing turned on.

In any case, you can add:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/super.c | 3 +++
> 1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 00fe75a71c4b..cba245460a9c 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5014,6 +5014,9 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> #endif
> 	char *orig_data = kstrdup(data, GFP_KERNEL);
> 
> +	if (data && !orig_data)
> +		return -ENOMEM;
> +
> 	/* Store the original options */
> 	old_sb_flags = sb->s_flags;
> 	old_opts.s_mount_opt = sbi->s_mount_opt;
> --
> 2.17.1
> 


Cheers, Andreas

On 06/20/2018 12:31 PM, Andreas Dilger wrote:
>> On Jun 6, 2018, at 2:26 AM, Chengguang Xu <cgxu519@gmx.com> wrote:
>>
>> Currently duplicate data to orig_data in ext4_remount()
>> but there is lack of check for duplication result.
>> This patch adds proper check for it.
>>
>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> Thank you for the patch.  I think it looks correct, but I was
> initially confused by the commit message.  I think something
> like the following would be more clear:
>
>      There is no check for allocation failure when duplicating
>      "orig_data" in ext4_remount().  Check for failure and
>      return an error in this case.
>
>
> While I don't think it is (currently) possible for a one-page
> allocation to fail (always true because of copy_mount_options()
> used in ksys_mount()), I prefer to always check for allocation
> failures in case this ever changes in the future, or if there
> is allocation fault injection or other fuzzing turned on.
>
> In any case, you can add:
>
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Thanks for your review, I'll modify changelog in v2.

Thanks,
Chengguang.