orig_data could be NULL.
Signed-off-by: Shaohua Li <[email protected]>
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cc5c157..45fc255 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3057,6 +3057,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
ext4_group_t first_not_zeroed;
+ if (!orig_data)
+ return ret;
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
if (!sbi)
goto out_free_orig;
@@ -4285,6 +4287,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
#endif
char *orig_data = kstrdup(data, GFP_KERNEL);
+ if (!orig_data)
+ return -ENOMEM;
/* Store the original options */
lock_super(sb);
old_sb_flags = sb->s_flags;
On Thu, 9 Jun 2011, Shaohua Li wrote:
> orig_data could be NULL.
Now, that is the commit description :). Could you please be more
descriptive in the "descritpion" ? Also the subject is not right either,
please see Documentation/SubmittingPatches
Thanks!
-Lukas
>
> Signed-off-by: Shaohua Li <[email protected]>
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cc5c157..45fc255 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3057,6 +3057,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> ext4_group_t first_not_zeroed;
>
> + if (!orig_data)
> + return ret;
Again no data, no reason for backing off.
> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> if (!sbi)
> goto out_free_orig;
> @@ -4285,6 +4287,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> #endif
> char *orig_data = kstrdup(data, GFP_KERNEL);
>
> + if (!orig_data)
> + return -ENOMEM;
This does not seem right, it there is no data we will end with ENOMEM
for no reason.
> /* Store the original options */
> lock_super(sb);
> old_sb_flags = sb->s_flags;
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
On 6/9/11 4:24 AM, Lukas Czerner wrote:
> On Thu, 9 Jun 2011, Shaohua Li wrote:
>
>> orig_data could be NULL.
>
> Now, that is the commit description :). Could you please be more
> descriptive in the "descritpion" ? Also the subject is not right either,
> please see Documentation/SubmittingPatches
Yes; if possible please use the commit message to describe how/why orig_data
can be NULL; a testcase if one exists; the resulting flaw (null pointer deref?)
etc.
something like:
Subject: [PATCH] ext4: check for NULL orig_data pointer in mount paths
The orig_data pointer in ext4_fill_super() and ext4_remount()
can be null if < ??? >, which can lead to < ??? > in the mount
and remount paths. This can be demonstrated by < ??? >.
To avoid this, we can simply test for the null pointer
and return an error in ext4_fill_super() and ext4_remount().
> Thanks!
> -Lukas
>
>>
>> Signed-off-by: Shaohua Li <[email protected]>
>>
>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
>> index cc5c157..45fc255 100644
>> --- a/fs/ext4/super.c
>> +++ b/fs/ext4/super.c
>> @@ -3057,6 +3057,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>> unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
>> ext4_group_t first_not_zeroed;
>>
>> + if (!orig_data)
>> + return ret;
>
> Again no data, no reason for backing off.
orig_data could be NULL if *data is NULL, or if kstrdup got ENOMEM.
Anyway, please describe how the bug can arise, and then we can better evaluate the change.
Thanks!
-Eric
>> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
>> if (!sbi)
>> goto out_free_orig;
>> @@ -4285,6 +4287,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
>> #endif
>> char *orig_data = kstrdup(data, GFP_KERNEL);
>>
>> + if (!orig_data)
>> + return -ENOMEM;
>
>
> This does not seem right, it there is no data we will end with ENOMEM
> for no reason.
>
>> /* Store the original options */
>> lock_super(sb);
>> old_sb_flags = sb->s_flags;
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
On Thu, 2011-06-09 at 22:51 +0800, Eric Sandeen wrote:
> On 6/9/11 4:24 AM, Lukas Czerner wrote:
> > On Thu, 9 Jun 2011, Shaohua Li wrote:
> >
> >> orig_data could be NULL.
> >
> > Now, that is the commit description :). Could you please be more
> > descriptive in the "descritpion" ? Also the subject is not right either,
> > please see Documentation/SubmittingPatches
>
> Yes; if possible please use the commit message to describe how/why orig_data
> can be NULL; a testcase if one exists; the resulting flaw (null pointer deref?)
> etc.
>
> something like:
>
> Subject: [PATCH] ext4: check for NULL orig_data pointer in mount paths
>
> The orig_data pointer in ext4_fill_super() and ext4_remount()
> can be null if < ??? >, which can lead to < ??? > in the mount
> and remount paths. This can be demonstrated by < ??? >.
> To avoid this, we can simply test for the null pointer
> and return an error in ext4_fill_super() and ext4_remount().
I thought the reason is pretty straightforward, anyway here is the
updated patch.
Subject: [patch]ext4: check NULL pointer for mount and remount
orig_data could be NULL, because the memory allocation of kstrdup() could fail.
Add the NULL check.
Signed-off-by: Shaohua Li <[email protected]>
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cc5c157..45fc255 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3057,6 +3057,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
ext4_group_t first_not_zeroed;
+ if (!orig_data)
+ return ret;
sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
if (!sbi)
goto out_free_orig;
@@ -4285,6 +4287,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
#endif
char *orig_data = kstrdup(data, GFP_KERNEL);
+ if (!orig_data)
+ return -ENOMEM;
/* Store the original options */
lock_super(sb);
old_sb_flags = sb->s_flags;
On Fri, 10 Jun 2011, Shaohua Li wrote:
> On Thu, 2011-06-09 at 22:51 +0800, Eric Sandeen wrote:
> > On 6/9/11 4:24 AM, Lukas Czerner wrote:
> > > On Thu, 9 Jun 2011, Shaohua Li wrote:
> > >
> > >> orig_data could be NULL.
> > >
> > > Now, that is the commit description :). Could you please be more
> > > descriptive in the "descritpion" ? Also the subject is not right either,
> > > please see Documentation/SubmittingPatches
> >
> > Yes; if possible please use the commit message to describe how/why orig_data
> > can be NULL; a testcase if one exists; the resulting flaw (null pointer deref?)
> > etc.
> >
> > something like:
> >
> > Subject: [PATCH] ext4: check for NULL orig_data pointer in mount paths
> >
> > The orig_data pointer in ext4_fill_super() and ext4_remount()
> > can be null if < ??? >, which can lead to < ??? > in the mount
> > and remount paths. This can be demonstrated by < ??? >.
> > To avoid this, we can simply test for the null pointer
> > and return an error in ext4_fill_super() and ext4_remount().
> I thought the reason is pretty straightforward, anyway here is the
> updated patch.
>
> Subject: [patch]ext4: check NULL pointer for mount and remount
>
> orig_data could be NULL, because the memory allocation of kstrdup() could fail.
> Add the NULL check.
I am sorry, but as I pointed out in previous mail this is not true.
*orig_data can be also NULL in the case that *data is NULL and hence
there is no reason for exiting with error. Also please use the subject
Eric suggested.
Thanks!
-Lukas
>
> Signed-off-by: Shaohua Li <[email protected]>
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cc5c157..45fc255 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3057,6 +3057,8 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> unsigned int journal_ioprio = DEFAULT_JOURNAL_IOPRIO;
> ext4_group_t first_not_zeroed;
>
> + if (!orig_data)
> + return ret;
> sbi = kzalloc(sizeof(*sbi), GFP_KERNEL);
> if (!sbi)
> goto out_free_orig;
> @@ -4285,6 +4287,8 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> #endif
> char *orig_data = kstrdup(data, GFP_KERNEL);
>
> + if (!orig_data)
> + return -ENOMEM;
> /* Store the original options */
> lock_super(sb);
> old_sb_flags = sb->s_flags;
>
>
>
--
On Fri, 2011-06-10 at 16:32 +0800, Lukas Czerner wrote:
> On Fri, 10 Jun 2011, Shaohua Li wrote:
>
> > On Thu, 2011-06-09 at 22:51 +0800, Eric Sandeen wrote:
> > > On 6/9/11 4:24 AM, Lukas Czerner wrote:
> > > > On Thu, 9 Jun 2011, Shaohua Li wrote:
> > > >
> > > >> orig_data could be NULL.
> > > >
> > > > Now, that is the commit description :). Could you please be more
> > > > descriptive in the "descritpion" ? Also the subject is not right either,
> > > > please see Documentation/SubmittingPatches
> > >
> > > Yes; if possible please use the commit message to describe how/why orig_data
> > > can be NULL; a testcase if one exists; the resulting flaw (null pointer deref?)
> > > etc.
> > >
> > > something like:
> > >
> > > Subject: [PATCH] ext4: check for NULL orig_data pointer in mount paths
> > >
> > > The orig_data pointer in ext4_fill_super() and ext4_remount()
> > > can be null if < ??? >, which can lead to < ??? > in the mount
> > > and remount paths. This can be demonstrated by < ??? >.
> > > To avoid this, we can simply test for the null pointer
> > > and return an error in ext4_fill_super() and ext4_remount().
> > I thought the reason is pretty straightforward, anyway here is the
> > updated patch.
> >
> > Subject: [patch]ext4: check NULL pointer for mount and remount
> >
> > orig_data could be NULL, because the memory allocation of kstrdup() could fail.
> > Add the NULL check.
>
> I am sorry, but as I pointed out in previous mail this is not true.
> *orig_data can be also NULL in the case that *data is NULL and hence
> there is no reason for exiting with error. Also please use the subject
> Eric suggested.
Hmm, maybe we just don't use the pointer if it's NULl. it's just print
info anyway.
Subject: [patch]ext4: check NULL orig_data pointer for mount and remount
orig_data could be NULL, because the memory allocation of kstrdup()
could fail or data is NULL. Add the NULL check.
Signed-off-by: Shaohua Li <[email protected]>
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cc5c157..68eba3b 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3706,7 +3706,7 @@ no_journal:
ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
"Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
- *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
+ *sbi->s_es->s_mount_opts ? "; " : "", orig_data ? : ";");
if (es->s_error_count)
mod_timer(&sbi->s_err_report, jiffies + 300*HZ); /* 5 minutes */
@@ -4443,7 +4443,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
if (enable_quota)
dquot_resume(sb, -1);
- ext4_msg(sb, KERN_INFO, "re-mounted. Opts: %s", orig_data);
+ ext4_msg(sb, KERN_INFO, "re-mounted. Opts: %s", orig_data ? : ";");
kfree(orig_data);
return 0;
On Mon, 13 Jun 2011, Shaohua Li wrote:
> On Fri, 2011-06-10 at 16:32 +0800, Lukas Czerner wrote:
> > On Fri, 10 Jun 2011, Shaohua Li wrote:
> >
> > > On Thu, 2011-06-09 at 22:51 +0800, Eric Sandeen wrote:
> > > > On 6/9/11 4:24 AM, Lukas Czerner wrote:
> > > > > On Thu, 9 Jun 2011, Shaohua Li wrote:
> > > > >
> > > > >> orig_data could be NULL.
> > > > >
> > > > > Now, that is the commit description :). Could you please be more
> > > > > descriptive in the "descritpion" ? Also the subject is not right either,
> > > > > please see Documentation/SubmittingPatches
> > > >
> > > > Yes; if possible please use the commit message to describe how/why orig_data
> > > > can be NULL; a testcase if one exists; the resulting flaw (null pointer deref?)
> > > > etc.
> > > >
> > > > something like:
> > > >
> > > > Subject: [PATCH] ext4: check for NULL orig_data pointer in mount paths
> > > >
> > > > The orig_data pointer in ext4_fill_super() and ext4_remount()
> > > > can be null if < ??? >, which can lead to < ??? > in the mount
> > > > and remount paths. This can be demonstrated by < ??? >.
> > > > To avoid this, we can simply test for the null pointer
> > > > and return an error in ext4_fill_super() and ext4_remount().
> > > I thought the reason is pretty straightforward, anyway here is the
> > > updated patch.
> > >
> > > Subject: [patch]ext4: check NULL pointer for mount and remount
> > >
> > > orig_data could be NULL, because the memory allocation of kstrdup() could fail.
> > > Add the NULL check.
> >
> > I am sorry, but as I pointed out in previous mail this is not true.
> > *orig_data can be also NULL in the case that *data is NULL and hence
> > there is no reason for exiting with error. Also please use the subject
> > Eric suggested.
> Hmm, maybe we just don't use the pointer if it's NULl. it's just print
> info anyway.
>
> Subject: [patch]ext4: check NULL orig_data pointer for mount and remount
>
> orig_data could be NULL, because the memory allocation of kstrdup()
> could fail or data is NULL. Add the NULL check.
>
> Signed-off-by: Shaohua Li <[email protected]>
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index cc5c157..68eba3b 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -3706,7 +3706,7 @@ no_journal:
>
> ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> - *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> + *sbi->s_es->s_mount_opts ? "; " : "", orig_data ? : ";");
Hi,
so you are trying resolve the problem when the allocation fails right ?
But what you do is not solving anything, but rather hiding it and it is
not different than we had before.
So what about this:
if (data && !orig_data)
return ret;
>
> if (es->s_error_count)
> mod_timer(&sbi->s_err_report, jiffies + 300*HZ); /* 5 minutes */
> @@ -4443,7 +4443,7 @@ static int ext4_remount(struct super_block *sb, int *flags, char *data)
> if (enable_quota)
> dquot_resume(sb, -1);
>
> - ext4_msg(sb, KERN_INFO, "re-mounted. Opts: %s", orig_data);
> + ext4_msg(sb, KERN_INFO, "re-mounted. Opts: %s", orig_data ? : ";");
> kfree(orig_data);
> return 0;
>
>
>
>
--
On Mon, 2011-06-13 at 17:20 +0800, Lukas Czerner wrote:
> On Mon, 13 Jun 2011, Shaohua Li wrote:
>
> > On Fri, 2011-06-10 at 16:32 +0800, Lukas Czerner wrote:
> > > On Fri, 10 Jun 2011, Shaohua Li wrote:
> > >
> > > > On Thu, 2011-06-09 at 22:51 +0800, Eric Sandeen wrote:
> > > > > On 6/9/11 4:24 AM, Lukas Czerner wrote:
> > > > > > On Thu, 9 Jun 2011, Shaohua Li wrote:
> > > > > >
> > > > > >> orig_data could be NULL.
> > > > > >
> > > > > > Now, that is the commit description :). Could you please be more
> > > > > > descriptive in the "descritpion" ? Also the subject is not right either,
> > > > > > please see Documentation/SubmittingPatches
> > > > >
> > > > > Yes; if possible please use the commit message to describe how/why orig_data
> > > > > can be NULL; a testcase if one exists; the resulting flaw (null pointer deref?)
> > > > > etc.
> > > > >
> > > > > something like:
> > > > >
> > > > > Subject: [PATCH] ext4: check for NULL orig_data pointer in mount paths
> > > > >
> > > > > The orig_data pointer in ext4_fill_super() and ext4_remount()
> > > > > can be null if < ??? >, which can lead to < ??? > in the mount
> > > > > and remount paths. This can be demonstrated by < ??? >.
> > > > > To avoid this, we can simply test for the null pointer
> > > > > and return an error in ext4_fill_super() and ext4_remount().
> > > > I thought the reason is pretty straightforward, anyway here is the
> > > > updated patch.
> > > >
> > > > Subject: [patch]ext4: check NULL pointer for mount and remount
> > > >
> > > > orig_data could be NULL, because the memory allocation of kstrdup() could fail.
> > > > Add the NULL check.
> > >
> > > I am sorry, but as I pointed out in previous mail this is not true.
> > > *orig_data can be also NULL in the case that *data is NULL and hence
> > > there is no reason for exiting with error. Also please use the subject
> > > Eric suggested.
> > Hmm, maybe we just don't use the pointer if it's NULl. it's just print
> > info anyway.
> >
> > Subject: [patch]ext4: check NULL orig_data pointer for mount and remount
> >
> > orig_data could be NULL, because the memory allocation of kstrdup()
> > could fail or data is NULL. Add the NULL check.
> >
> > Signed-off-by: Shaohua Li <[email protected]>
> >
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index cc5c157..68eba3b 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -3706,7 +3706,7 @@ no_journal:
> >
> > ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> > "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> > - *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> > + *sbi->s_es->s_mount_opts ? "; " : "", orig_data ? : ";");
>
> Hi,
>
> so you are trying resolve the problem when the allocation fails right ?
> But what you do is not solving anything, but rather hiding it and it is
> not different than we had before.
>
> So what about this:
>
> if (data && !orig_data)
> return ret;
how could this work? if data is NULL, orig_data will be NULL, the
ext4_msg will still use a NULL pointer.
On Mon, Jun 13, 2011 at 11:20:17AM +0200, Lukas Czerner wrote:
> > ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> > "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> > - *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> > + *sbi->s_es->s_mount_opts ? "; " : "", orig_data ? : ";");
>
> Hi,
>
> so you are trying resolve the problem when the allocation fails right ?
> But what you do is not solving anything, but rather hiding it and it is
> not different than we had before.
So a couple of observations here. The kernel's sprintf/printk
functions will not OOPS if "%s" is asked to expand a NULL pointer; it
will simply print "(null)". So it's not a disaster if orig_data is
NULL. It's would perhaps be better (for cosmetic reasons) if we
printed something such as "Opts: (none)", but it's not a huge deal
either way.
> So what about this:
>
> if (data && !orig_data)
> return ret;
Sure, that's technically better. I'll note though that if we fail the
kstrdup(), there are so many other memory allocations happening later
in ext4_fill_super() that it's highly likely one of the others will
fail and we will then return ENOMEM.
That's not to say that patches here aren't welcome, but (a) it's
useful to take a look at the big picture, and (b) I'm going to
prioritize this as a "clean up" patch that can wait until the merge
window for v3.1. It will be a great opportunity for Shaohua to
practice submitting a high quality patch that complies with the
Documentation/SubmittingPatches requirements.
Regards,
- Ted
On Tue, 14 Jun 2011, Shaohua Li wrote:
--snip--
> > > Hmm, maybe we just don't use the pointer if it's NULl. it's just print
> > > info anyway.
> > >
> > > Subject: [patch]ext4: check NULL orig_data pointer for mount and remount
> > >
> > > orig_data could be NULL, because the memory allocation of kstrdup()
> > > could fail or data is NULL. Add the NULL check.
> > >
> > > Signed-off-by: Shaohua Li <[email protected]>
> > >
> > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > > index cc5c157..68eba3b 100644
> > > --- a/fs/ext4/super.c
> > > +++ b/fs/ext4/super.c
> > > @@ -3706,7 +3706,7 @@ no_journal:
> > >
> > > ext4_msg(sb, KERN_INFO, "mounted filesystem with%s. "
> > > "Opts: %s%s%s", descr, sbi->s_es->s_mount_opts,
> > > - *sbi->s_es->s_mount_opts ? "; " : "", orig_data);
> > > + *sbi->s_es->s_mount_opts ? "; " : "", orig_data ? : ";");
> >
> > Hi,
> >
> > so you are trying resolve the problem when the allocation fails right ?
> > But what you do is not solving anything, but rather hiding it and it is
> > not different than we had before.
> >
> > So what about this:
> >
> > if (data && !orig_data)
> > return ret;
> how could this work? if data is NULL, orig_data will be NULL, the
> ext4_msg will still use a NULL pointer.
>
Hi,
I am sorry if it was not clear, but as Ted already pointed out printk
can handle NULL pointer, so there is no need to care about it in this
case.
Also it is not a *big* deal not to handle allocation failure in kstrdup,
because there are other allocations in fill_super and remount which will
result in error, however I think that it is better to catch such cases
as early as we can, so it is good to fix this.
Thanks!
-Lukas