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

[Wu Fengguang <fengguang.wu@intel.com>]

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.

Thanks,
Fengguang

> diff --git a/fs/ramfs/inode.c b/fs/ramfs/inode.c
> index a404fb8..3a6b193 100644
> --- a/fs/ramfs/inode.c
> +++ b/fs/ramfs/inode.c
> @@ -221,22 +221,23 @@ static int ramfs_fill_super(struct super_block * sb, void * data, int silent)
>  	save_mount_options(sb, data);
>  
>  	fsi = kzalloc(sizeof(struct ramfs_fs_info), GFP_KERNEL);
> +	sb->s_fs_info = fsi;
>  	if (!fsi) {
>  		err = -ENOMEM;
>  		goto fail;
>  	}
> -	sb->s_fs_info = fsi;
>  
>  	err = ramfs_parse_options(data, &fsi->mount_opts);
>  	if (err)
>  		goto fail;
>  
> -	sb->s_maxbytes = MAX_LFS_FILESIZE;
> -	sb->s_blocksize = PAGE_CACHE_SIZE;
> -	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
> -	sb->s_magic = RAMFS_MAGIC;
> -	sb->s_op = &ramfs_ops;
> -	sb->s_time_gran = 1;
> +	sb->s_maxbytes		= MAX_LFS_FILESIZE;
> +	sb->s_blocksize		= PAGE_CACHE_SIZE;
> +	sb->s_blocksize_bits	= PAGE_CACHE_SHIFT;
> +	sb->s_magic		= RAMFS_MAGIC;
> +	sb->s_op		= &ramfs_ops;
> +	sb->s_time_gran		= 1;
> +
>  	inode = ramfs_get_inode(sb, S_IFDIR | fsi->mount_opts.mode, 0);
>  	if (!inode) {
>  		err = -ENOMEM;
> @@ -244,14 +245,16 @@ static int ramfs_fill_super(struct super_block * sb, void * data, int silent)
>  	}
>  
>  	root = d_alloc_root(inode);
> +	sb->s_root = root;
>  	if (!root) {
>  		err = -ENOMEM;
>  		goto fail;
>  	}
> -	sb->s_root = root;
> +
>  	return 0;
>  fail:
>  	kfree(fsi);
> +	sb->s_fs_info = NULL;
>  	iput(inode);
>  	return err;
>  }