ext4: check orig_data in remount

Message ID 20180606082658.11297-1-cgxu519@gmx.com
State Superseded, archived
Headers show
Series
  • ext4: check orig_data in remount
Related show

Commit Message

Chengguang Xu June 6, 2018, 8:26 a.m.
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(+)

Comments

Chengguang Xu June 20, 2018, 3 a.m. | #1
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;
Andreas Dilger June 20, 2018, 4:31 a.m. | #2
> 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
Chengguang Xu June 20, 2018, 5:40 a.m. | #3
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.

Patch

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;