Messages in this thread | | | Date | Tue, 7 Apr 2009 08:03:09 +0200 | From | Ingo Molnar <> | Subject | Re: [patch] ramfs: add support for "mode=" mount option, fix |
| |
* Wu Fengguang <fengguang.wu@intel.com> wrote:
> On Tue, Apr 07, 2009 at 01:28:01PM +0800, Ingo Molnar wrote: > > > > * Linus Torvalds <torvalds@linux-foundation.org> wrote: > > > > > On Mon, 6 Apr 2009, Linus Torvalds wrote: > > > > > > > > It bisected past them. I'm getting worried that it's timing-related, > > > > because nothing that remains looks even remotely interesting for that Mac > > > > mini, but right now: > > > > > > > > - bad: 56fcef75117a153f298b3fe54af31053f53997dd > > > > - good: bb233fdfc7b7cefe45bfa2e8d1b24e79c60a48e5 > > > > > > > > and there's not a whole lot of commits in between. > > > > > > It's c3b1b1cbf002e65a3cabd479e68b5f35886a26db: 'ramfs: add support > > > for "mode=" mount option'. > > > > > > And I checked. Reverting it at the tip fixes it. So no random > > > timing fluctuations. > > > > > > So that commit causes some random SLAB corruption, that then > > > (depending apparently on luck) may or may not crash in some odd > > > random places later. > > > > ah - forget my previous mail then. > > > > This commit does have a couple of genuinely odd looking lines. > > > > For example: > > > > + sb->s_fs_info = fsi; > > + > > + err = ramfs_parse_options(data, &fsi->mount_opts); > > + if (err) > > + goto fail; > > > > Say we fail in ramfs_parse_options() and do the 'fail' pattern: > > > > +fail: > > + kfree(fsi); > > + iput(inode); > > + return err; > > > > so we have 'fsi' kfree()'d but dont clear sb->s_fs_info! That's > > almost always a bad practice. And indeed, in the kill_super > > Sorry - yes, the double kfree() shall be the root cause! > > get_sb_nodev() calls kill_sb() after a failed fill_super(): > > error = fill_super(s, data, flags & MS_SILENT ? 1 : 0); > if (error) { > up_write(&s->s_umount); > deactivate_super(s); > return error; > } > > > callback: > > > > +static void ramfs_kill_sb(struct super_block *sb) > > +{ > > + kfree(sb->s_fs_info); > > > > What ensures that this cannot be a double kfree() memory corruption? > > That pointer should have been cleared with something like the patch > > below. (totally untested) > > > > And there's also another, probably just theoretical worry about > > another failure path: > > > > + fsi = kzalloc(sizeof(struct ramfs_fs_info), GFP_KERNEL); > > + if (!fsi) { > > + err = -ENOMEM; > > + goto fail; > > + } > > + sb->s_fs_info = fsi; > > > > leaves sb->s_fs_info uninitialized in the failure case, and might > > hit this code unconditionally: > > > > +static void ramfs_kill_sb(struct super_block *sb) > > +{ > > + kfree(sb->s_fs_info); > > + kill_litter_super(sb); > > +} > > > > Leaving this code at the mercy of the external call environment > > initializing sb->s_fs_info. Which if it does not do (or stops > > doing in the future), can trigger a kfree of a random pointer. > > > > (I think ->kill_super() gets called even if ->fill_super() fails, > > but i have not checked closely.) > > You are right, see above. > > > These kinds of assymetric failure paths are really a red flag during > > review. > > > > VFS infrastructure nit: we have 20 other similar looking but > > slightly differently implemented filesystem options parsers, in each > > filesystem. Might make sense to factor that out a bit and > > standardize it across all filesystems and make it all a bit safer. > > Duplicating code like that is never good IMHO. > > > > Ingo > > > > Acked-by: Wu Fengguang <fengguang.wu@intel.com> > > The patch looks pretty good and runs OK here.
ok, good - i didnt even build it - you can add my signoff too:
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Thanks,
Ingo
| |