Message ID | 1307950258.15392.113.camel@sli10-conroe |
---|---|
State | Rejected, archived |
Headers | show |
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; > > > >
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
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
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 --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;