diff mbox

check NULL pointer

Message ID 1307950258.15392.113.camel@sli10-conroe
State Rejected, archived
Headers show

Commit Message

Shaohua Li June 13, 2011, 7:30 a.m. UTC 
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 <shaohua.li@intel.com>



--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Lukas Czerner June 13, 2011, 9:20 a.m. UTC | #1 
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 <shaohua.li@intel.com>
> 
> 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;
>  
> 
> 
>
Theodore Ts'o June 13, 2011, 9:02 p.m. UTC | #2 
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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shaohua Li June 14, 2011, 12:31 a.m. UTC | #3 
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 <shaohua.li@intel.com>
> > 
> > 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukas Czerner June 14, 2011, 10:07 a.m. UTC | #4 
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 <shaohua.li@intel.com>
> > > 
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;