Re: [PATCH][RFC] Fix breakage in ffs_fs_mount()

[Michal Nazarewicz <mina86@mina86.com>]

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

On Fri, Sep 20 2013, Al Viro wrote:
> 	There's a bunch of failure exits in ffs_fs_mount() with
> seriously broken recovery logics.  Most of that appears to stem
> from misunderstanding of the ->kill_sb() semantics;

That sounds likely.

[…]

> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Acked-by: Michal Nazarewicz <mina86@mina86.com>

> -- 
> diff --git a/drivers/usb/gadget/f_fs.c b/drivers/usb/gadget/f_fs.c
> index 1a66c5b..0658908 100644
> --- a/drivers/usb/gadget/f_fs.c
> +++ b/drivers/usb/gadget/f_fs.c
> @@ -1034,37 +1034,19 @@ struct ffs_sb_fill_data {
>  	struct ffs_file_perms perms;
>  	umode_t root_mode;
>  	const char *dev_name;
> -	union {
> -		/* set by ffs_fs_mount(), read by ffs_sb_fill() */
> -		void *private_data;
> -		/* set by ffs_sb_fill(), read by ffs_fs_mount */
> -		struct ffs_data *ffs_data;
> -	};
> +	struct ffs_data *ffs_data;
>  };
>  
>  static int ffs_sb_fill(struct super_block *sb, void *_data, int silent)
>  {
>  	struct ffs_sb_fill_data *data = _data;
>  	struct inode	*inode;
> -	struct ffs_data	*ffs;
> +	struct ffs_data	*ffs = data->ffs_data;
>  
>  	ENTER();
>  
> -	/* Initialise data */
> -	ffs = ffs_data_new();
> -	if (unlikely(!ffs))
> -		goto Enomem;
> -
>  	ffs->sb              = sb;
> -	ffs->dev_name        = kstrdup(data->dev_name, GFP_KERNEL);
> -	if (unlikely(!ffs->dev_name))
> -		goto Enomem;
> -	ffs->file_perms      = data->perms;
> -	ffs->private_data    = data->private_data;
> -
> -	/* used by the caller of this function */
> -	data->ffs_data       = ffs;
> -
> +	data->ffs_data       = NULL;
>  	sb->s_fs_info        = ffs;
>  	sb->s_blocksize      = PAGE_CACHE_SIZE;
>  	sb->s_blocksize_bits = PAGE_CACHE_SHIFT;
> @@ -1080,17 +1062,14 @@ static int ffs_sb_fill(struct super_block *sb, void *_data, int silent)
>  				  &data->perms);
>  	sb->s_root = d_make_root(inode);
>  	if (unlikely(!sb->s_root))
> -		goto Enomem;
> +		return -ENOMEM;
>  
>  	/* EP0 file */
>  	if (unlikely(!ffs_sb_create_file(sb, "ep0", ffs,
>  					 &ffs_ep0_operations, NULL)))
> -		goto Enomem;
> +		return -ENOMEM;
>  
>  	return 0;
> -
> -Enomem:
> -	return -ENOMEM;
>  }
>  
>  static int ffs_fs_parse_opts(struct ffs_sb_fill_data *data, char *opts)
> @@ -1193,6 +1172,7 @@ ffs_fs_mount(struct file_system_type *t, int flags,
>  	struct dentry *rv;
>  	int ret;
>  	void *ffs_dev;
> +	struct ffs_data	*ffs;
>  
>  	ENTER();
>  
> @@ -1200,18 +1180,30 @@ ffs_fs_mount(struct file_system_type *t, int flags,
>  	if (unlikely(ret < 0))
>  		return ERR_PTR(ret);
>  
> +	ffs = ffs_data_new();
> +	if (unlikely(!ffs))
> +		return ERR_PTR(-ENOMEM);
> +	ffs->file_perms = data.perms;
> +
> +	ffs->dev_name = kstrdup(dev_name, GFP_KERNEL);
> +	if (unlikely(!ffs->dev_name)) {
> +		ffs_data_put(ffs);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
>  	ffs_dev = functionfs_acquire_dev_callback(dev_name);
> -	if (IS_ERR(ffs_dev))
> -		return ffs_dev;
> +	if (IS_ERR(ffs_dev)) {
> +		ffs_data_put(ffs);
> +		return ERR_CAST(ffs_dev);
> +	}
> +	ffs->private_data = ffs_dev;
> +	data.ffs_data = ffs;
>  
> -	data.dev_name = dev_name;
> -	data.private_data = ffs_dev;
>  	rv = mount_nodev(t, flags, &data, ffs_sb_fill);
> -
> -	/* data.ffs_data is set by ffs_sb_fill */
> -	if (IS_ERR(rv))
> +	if (IS_ERR(rv) && data.ffs_data) {
>  		functionfs_release_dev_callback(data.ffs_data);
> -
> +		ffs_data_put(data.ffs_data);
> +	}
>  	return rv;
>  }
>  

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--

[-- Attachment #2.1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 835 bytes --]