Re: [PATCH] fuse: allow synchronous FUSE_INIT

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Aug 22, 2025 at 03:52:38PM -0700, Joanne Koong wrote:
> 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

Er... are you asking if we should drop the newly created fud via
fuse_dev_release if err != 0?  (AFAICT there is no fuse_dev_free?)

> > 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?

<shrug> I was figuring that it was fine to leave the fud attached to the
device fd until the caller close()s it, but OTOH maybe the fuse server
would like to try to mount again?  Do fuse servers do that?

--D

> > > +       } 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);
>