On Wed, Aug 27, 2025 at 03:56:49PM -0700, Joanne Koong wrote:
> On Wed, Aug 27, 2025 at 4:00 AM Miklos Szeredi <mszeredi@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > v2:
> >
> > - make fuse_send_init() perform sync/async sequence based on fc->sync_init
> > (Joanne)
> >
> > 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 | 5 ++-
> > fs/fuse/inode.c | 50 ++++++++++++++++++++------
> > include/uapi/linux/fuse.h | 1 +
> > 7 files changed, 115 insertions(+), 35 deletions(-)
> >
> > 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);
>
> I wonder if we should make the semantics the same for synchronous and
> non-synchronous inits here, i.e. doing a wait for
> "(READ_ONCE(file->private_data) != FUSE_DEV_SYNC_INIT) &&
> READ_ONCE(file->private_data) != NULL", so that from the libfuse point
> of view, the flow can be unified between the two, eg
> i) send sync_init ioctl call if doing a synchronous init
> ii) kick off thread to read requests
> iii) do mount call
> otherwise for async inits, the mount call needs to happen first.
I don't think you can compare it against NULL directly, because
FUSE_DEV_SYNC_INIT != NULL evaluates to true.
How about
err = wait_event_interruptible(fuse_dev_waitq,
__fuse_get_dev(file) != NULL);
?
> > + if (err)
> > + return ERR_PTR(err);
> > +
> > + fud = __fuse_get_dev(file);
> > + if (!fud)
> > + return ERR_PTR(-EPERM);
> > +
> > + return fud;
> > +}
> > +
> > static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
> > {
> > struct fuse_copy_state cs;
> > struct file *file = iocb->ki_filp;
> > struct fuse_dev *fud = fuse_get_dev(file);
> >
> > - if (!fud)
> > - return -EPERM;
> > + if (IS_ERR(fud))
> > + return PTR_ERR(fud);
> >
> > if (!user_backed_iter(to))
> > return -EINVAL;
> > @@ -1557,8 +1577,8 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > struct fuse_copy_state cs;
> > struct fuse_dev *fud = fuse_get_dev(in);
> >
> > - if (!fud)
> > - return -EPERM;
> > + if (IS_ERR(fud))
> > + return PTR_ERR(fud);
> >
> > bufs = kvmalloc_array(pipe->max_usage, sizeof(struct pipe_buffer),
> > GFP_KERNEL);
> > @@ -2233,8 +2253,8 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
> > struct fuse_copy_state cs;
> > struct fuse_dev *fud = fuse_get_dev(iocb->ki_filp);
>
> Does this (and below in fuse_dev_splice_write()) need to be
> fuse_get_dev()? afaict, fuse_dev_write() only starts getting used
> after fud has already been initialized. i see why it's needed for
> fuse_dev_read() since otherwise the server doesn't know when it can
> start calling fuse_dev_read(), but for fuse_dev_write(), it seems like
> that only gets used after fud is already initialized.
I think most of these functions could just do:
struct fuse_dev *fud = __fuse_get_dev(iocb->ki_filp);
if (!fud)
return -EPERM;
Just like the old days, but it's one churn (if test) vs. another
(callsite) type of churn. Either way, we want to error out of all of
these functions if you haven't actually mounted the fuse server to
create the fud, right?
> >
> > - if (!fud)
> > - return -EPERM;
> > + if (IS_ERR(fud))
> > + return PTR_ERR(fud);
> >
> > if (!user_backed_iter(from))
> > return -EINVAL;
> > @@ -2258,8 +2278,8 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> > ssize_t ret;
> >
> > fud = fuse_get_dev(out);
> > - if (!fud)
> > - return -EPERM;
> > + if (IS_ERR(fud))
> > + return PTR_ERR(fud);
> >
> > pipe_lock(pipe);
> >
> > @@ -2581,8 +2601,8 @@ static long fuse_dev_ioctl_backing_open(struct file *file,
> > struct fuse_dev *fud = fuse_get_dev(file);
>
> Should this be __fuse_get_dev()?
>
> > struct fuse_backing_map map;
> >
> > - if (!fud)
> > - return -EPERM;
> > + if (IS_ERR(fud))
> > + return PTR_ERR(fud);
> >
> > if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> > return -EOPNOTSUPP;
> > @@ -2598,8 +2618,8 @@ static long fuse_dev_ioctl_backing_close(struct file *file, __u32 __user *argp)
> > struct fuse_dev *fud = fuse_get_dev(file);
>
> Same question here.
>
> > int backing_id;
> >
> > - if (!fud)
> > - return -EPERM;
> > + if (IS_ERR(fud))
> > + return PTR_ERR(fud);
> >
> > if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> > return -EOPNOTSUPP;
> > @@ -2610,6 +2630,19 @@ static long fuse_dev_ioctl_backing_close(struct file *file, __u32 __user *argp)
> > return fuse_backing_close(fud->fc, backing_id);
> > }
> >
> > +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);
>
> Does this still need a WRITE_ONCE if it's accessed within the scope of
> the mutex? My understanding (maybe wrong) is that a mutex implicitly
> serves as also a memory barrier. If not, then we probably also need a
> WRITE_ONCE() around the *ctx->fudptr assignment in
> fuse_fill_super_common()?
I agree with this, the (re)ordering before the mutex unlock doesn't
matter because the unlock is a write barrier. But I don't think it
hurts to have redundant WRITE_ONCE.
--D
>
> Thanks,
> Joanne
>
> > + err = 0;
> > + }
> > + mutex_unlock(&fuse_mutex);
> > + return err;
> > +}
> > +
> > @@ -1876,8 +1901,10 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
> >
> > list_add_tail(&fc->entry, &fuse_conn_list);
> > sb->s_root = root_dentry;
> > - if (ctx->fudptr)
> > + if (ctx->fudptr) {
> > *ctx->fudptr = fud;
> > + wake_up_all(&fuse_dev_waitq);
> > + }
> > mutex_unlock(&fuse_mutex);
> > return 0;
> >
