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

[Ingo Molnar <mingo@elte.hu>]

* 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