Re: [patch] ramfs: add support for "mode=" mount option, fix

Ingo Molnar
2009-04-07T06:03:53Z

* 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