Re: [PATCH] fuse: allow synchronous FUSE_INIT

[Joanne Koong <joannelkoong@gmail.com>]

On Fri, Aug 22, 2025 at 3:46 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>
> On Fri, Aug 22, 2025 at 4:44 AM Miklos Szeredi <mszeredi@redhat.com> wrote:
> >
> > FUSE_INIT has always been asynchronous with mount.  That means that the
> > server processed this request after the mount syscall returned.
> >
> > This means that FUSE_INIT can't supply the root inode's ID, hence it
> > currently has a hardcoded value.  There are other limitations such as not
> > being able to perform getxattr during mount, which is needed by selinux.
> >
> > To remove these limitations allow server to process FUSE_INIT while
> > initializing the in-core super block for the fuse filesystem.  This can
> > only be done if the server is prepared to handle this, so add
> > FUSE_DEV_IOC_SYNC_INIT ioctl, which
> >
> >  a) lets the server know whether this feature is supported, returning
> >  ENOTTY othewrwise.
> >
> >  b) lets the kernel know to perform a synchronous initialization
> >
> > The implementation is slightly tricky, since fuse_dev/fuse_conn are set up
> > only during super block creation.  This is solved by setting the private
> > data of the fuse device file to a special value ((struct fuse_dev *) 1) and
> > waiting for this to be turned into a proper fuse_dev before commecing with
> > operations on the device file.
> >
> > Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> > ---
> > I tested this with my raw-interface tester, so no libfuse update yet.  Will
> > work on that next.
> >
> >  fs/fuse/cuse.c            |  3 +-
> >  fs/fuse/dev.c             | 74 +++++++++++++++++++++++++++++----------
> >  fs/fuse/dev_uring.c       |  4 +--
> >  fs/fuse/fuse_dev_i.h      | 13 +++++--
> >  fs/fuse/fuse_i.h          |  3 ++
> >  fs/fuse/inode.c           | 46 +++++++++++++++++++-----
> >  include/uapi/linux/fuse.h |  1 +
> >  7 files changed, 112 insertions(+), 32 deletions(-)
> >
>
> Will read this more thoroughly next week but left some comments below for now.
>
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 8ac074414897..948f45c6e0ef 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -1530,14 +1530,34 @@ static int fuse_dev_open(struct inode *inode, struct file *file)
> >         return 0;
> >  }
> >
> > +struct fuse_dev *fuse_get_dev(struct file *file)
> > +{
> > +       struct fuse_dev *fud = __fuse_get_dev(file);
> > +       int err;
> > +
> > +       if (likely(fud))
> > +               return fud;
> > +
> > +       err = wait_event_interruptible(fuse_dev_waitq,
> > +                                      READ_ONCE(file->private_data) != FUSE_DEV_SYNC_INIT);
> > +       if (err)
> > +               return ERR_PTR(err);
> > +
> > +       fud = __fuse_get_dev(file);
> > +       if (!fud)
> > +               return ERR_PTR(-EPERM);
> > +
> > +       return fud;
> > +}
> > +
> >
> > +static long fuse_dev_ioctl_sync_init(struct file *file)
> > +{
> > +       int err = -EINVAL;
> > +
> > +       mutex_lock(&fuse_mutex);
> > +       if (!__fuse_get_dev(file)) {
> > +               WRITE_ONCE(file->private_data, FUSE_DEV_SYNC_INIT);
> > +               err = 0;
> > +       }
> > +       mutex_unlock(&fuse_mutex);
> > +       return err;
>
> Does this let an untrusted server deadlock fuse if they call
> FUSE_DEV_IOC_SYNC_INIT twice? afaict, fuse_mutex is a global lock and
> the 2nd FUSE_DEV_IOC_SYNC_INIT call can forever hold fuse_mutex
> because of the __fuse_get_dev() -> wait_event_interruptible().

Never mind this comment, I got __fuse_get_dev() and fuse_get_dev()
mixed up. fuse_get_dev() is the one that calls
wait_event_interruptible(), not __fuse_get_dev().

>
> > +}
> > +
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 9d26a5bc394d..d5f9f2abc569 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1898,6 +1913,7 @@ EXPORT_SYMBOL_GPL(fuse_fill_super_common);
> >  static int fuse_fill_super(struct super_block *sb, struct fs_context *fsc)
> >  {
> >         struct fuse_fs_context *ctx = fsc->fs_private;
> > +       struct fuse_mount *fm;
> >         int err;
> >
> >         if (!ctx->file || !ctx->rootmode_present ||
> > @@ -1918,8 +1934,22 @@ static int fuse_fill_super(struct super_block *sb, struct fs_context *fsc)
> >                 return err;
> >         /* file->private_data shall be visible on all CPUs after this */
> >         smp_mb();
> > -       fuse_send_init(get_fuse_mount_super(sb));
> > -       return 0;
> > +
> > +       fm = get_fuse_mount_super(sb);
> > +
> > +       if (fm->fc->sync_init) {
> > +               struct fuse_init_args *ia = fuse_new_init(fm);
> > +
> > +               err = fuse_simple_request(fm, &ia->args);
> > +               if (err > 0)
> > +                       err = 0;
> > +               process_init_reply(fm, &ia->args, err);
>
> Do we need a fuse_dev_free() here if err < 0? If err < 0 then the
> mount fails, but fuse_fill_super_common() -> fuse_dev_alloc_install()
> will have already been called which if i'm understanding it correctly
> means otherwise the fc will get leaked in this case. Or I guess
> another option is to retain original behavior with having the mount
> succeed even if the init server reply returns back an error code?
>
> > +       } else {
> > +               fuse_send_init(fm);
> > +               err = 0;
> > +       }
>
> imo this logic looks cleaner if fuse_send_init() takes in a 'bool
> async' arg and the bulk of this logic is handled there. Especially if
> virtio is also meant to support synchronous init requests (which I'm
> not seeing why it wouldn't?)
>
> Thanks,
> Joanne
>
> > +
> > +       return err;
> >  }
> >
> >  /*
> > @@ -1980,7 +2010,7 @@ static int fuse_get_tree(struct fs_context *fsc)
> >          * Allow creating a fuse mount with an already initialized fuse
> >          * connection
> >          */
> > -       fud = READ_ONCE(ctx->file->private_data);
> > +       fud = __fuse_get_dev(ctx->file);
> >         if (ctx->file->f_op == &fuse_dev_operations && fud) {
> >                 fsc->sget_key = fud->fc;
> >                 sb = sget_fc(fsc, fuse_test_super, fuse_set_no_super);