On Thu, Apr 30, 2020 at 7:18 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>
> As of now fuse_fill_super_common() allocates and installs one fuse device.
> Filesystems like virtiofs can have more than one filesystem queues and
> can have one fuse device per queue. Give, fuse_fill_super_common() only
> handles one device, virtiofs allocates and installes fuse devices for
> all queues except one.
>
> This makes logic little twisted and hard to understand. It probably
> is better to not do any device allocation/installation in
> fuse_fill_super_common() and let caller take care of it instead.
Taking a closer look...
>
> v2: Removed fuse_dev_alloc_install() call from fuse_fill_super_common().
>
> Signed-off-by: Vivek Goyal <vgoyal@xxxxxxxxxx>
> ---
> fs/fuse/fuse_i.h | 3 ---
> fs/fuse/inode.c | 30 ++++++++++++++----------------
> fs/fuse/virtio_fs.c | 9 +--------
> 3 files changed, 15 insertions(+), 27 deletions(-)
>
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index ca344bf71404..df0a62f963a8 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -485,9 +485,6 @@ struct fuse_fs_context {
> unsigned int max_read;
> unsigned int blksize;
> const char *subtype;
> -
> - /* fuse_dev pointer to fill in, should contain NULL on entry */
> - void **fudptr;
> };
>
> /**
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 95d712d44ca1..6b38e0391c96 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1113,7 +1113,6 @@ EXPORT_SYMBOL_GPL(fuse_dev_free);
>
> int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> {
> - struct fuse_dev *fud;
> struct fuse_conn *fc = get_fuse_conn_super(sb);
> struct inode *root;
> struct dentry *root_dentry;
> @@ -1155,15 +1154,11 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> if (sb->s_user_ns != &init_user_ns)
> sb->s_xattr = fuse_no_acl_xattr_handlers;
>
> - fud = fuse_dev_alloc_install(fc);
> - if (!fud)
> - goto err;
> -
> fc->dev = sb->s_dev;
> fc->sb = sb;
> err = fuse_bdi_init(fc, sb);
> if (err)
> - goto err_dev_free;
> + goto err;
>
> /* Handle umasking inside the fuse code */
> if (sb->s_flags & SB_POSIXACL)
> @@ -1185,30 +1180,24 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> sb->s_d_op = &fuse_root_dentry_operations;
> root_dentry = d_make_root(root);
> if (!root_dentry)
> - goto err_dev_free;
> + goto err;
> /* Root dentry doesn't have .d_revalidate */
> sb->s_d_op = &fuse_dentry_operations;
>
> mutex_lock(&fuse_mutex);
> err = -EINVAL;
> - if (*ctx->fudptr)
> - goto err_unlock;
> -
> err = fuse_ctl_add_conn(fc);
> if (err)
> goto err_unlock;
>
> list_add_tail(&fc->entry, &fuse_conn_list);
> sb->s_root = root_dentry;
> - *ctx->fudptr = fud;
> mutex_unlock(&fuse_mutex);
> return 0;
>
> err_unlock:
> mutex_unlock(&fuse_mutex);
> dput(root_dentry);
> - err_dev_free:
> - fuse_dev_free(fud);
> err:
> return err;
> }
> @@ -1220,6 +1209,7 @@ static int fuse_fill_super(struct super_block *sb, struct fs_context *fsc)
> struct file *file;
> int err;
> struct fuse_conn *fc;
> + struct fuse_dev *fud;
>
> err = -EINVAL;
> file = fget(ctx->fd);
> @@ -1233,13 +1223,16 @@ static int fuse_fill_super(struct super_block *sb, struct fs_context *fsc)
> if ((file->f_op != &fuse_dev_operations) ||
> (file->f_cred->user_ns != sb->s_user_ns))
> goto err_fput;
> - ctx->fudptr = &file->private_data;
>
> - fc = kmalloc(sizeof(*fc), GFP_KERNEL);
> err = -ENOMEM;
> - if (!fc)
> + fud = fuse_dev_alloc();
> + if (!fud)
> goto err_fput;
>
> + fc = kmalloc(sizeof(*fc), GFP_KERNEL);
> + if (!fc)
> + goto err_free_dev;
> +
> fuse_conn_init(fc, sb->s_user_ns, &fuse_dev_fiq_ops, NULL);
> fc->release = fuse_free_conn;
> sb->s_fs_info = fc;
> @@ -1247,6 +1240,9 @@ static int fuse_fill_super(struct super_block *sb, struct fs_context *fsc)
> err = fuse_fill_super_common(sb, ctx);
> if (err)
> goto err_put_conn;
> +
> + fuse_dev_install(fud, fc);
> + file->private_data = fud;
We've lost the check for non-null file->private_data; i.e. a fuse fd
already bound to a super block. That needs to be restored, together
with protection against two such instances racing with each other.
Maybe we are better off moving the whole fuse_mutex protected block
from the end of fuse_fill_super_common() into the callers.
Thanks,
Miklos
