Message-ID: <CAC1kPDP4oO29B_TM-2wvzt1+Gc6hWTDGMfHSJhOax4_Cg2dEkg@mail.gmail.com>
Date: Wed, 7 May 2025 20:33:45 +0800
From: Chen Linxuan <chenlinxuan@...ontech.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: Chen Linxuan <chenlinxuan@...ontech.com>, Miklos Szeredi <miklos@...redi.hu>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] fs: fuse: add backing_files control file

On Wed, May 7, 2025 at 7:03 PM Amir Goldstein <amir73il@...il.com> wrote:
>
> On Wed, May 7, 2025 at 5:29 AM Chen Linxuan <chenlinxuan@...ontech.com> wrote:
> >
> > Add a new FUSE control file "/sys/fs/fuse/connections/*/backing_files"
> > that exposes the paths of all backing files currently being used in
> > FUSE mount points. This is particularly valuable for tracking and
> > debugging files used in FUSE passthrough mode.
> >
> > This approach is similar to how fixed files in io_uring expose their
> > status through fdinfo, providing administrators with visibility into
> > backing file usage. By making backing files visible through the FUSE
> > control filesystem, administrators can monitor which files are being
> > used for passthrough operations and can force-close them if needed by
> > aborting the connection.
> >
> > This exposure of backing files information is an important step towards
> > potentially relaxing CAP_SYS_ADMIN requirements for certain passthrough
> > operations in the future, allowing for better security analysis of
> > passthrough usage patterns.
> >
> > The control file is implemented using the seq_file interface for
> > efficient handling of potentially large numbers of backing files.
> > Access permissions are set to read-only (0400) as this is an
> > informational interface.
> >
> > FUSE_CTL_NUM_DENTRIES has been increased from 5 to 6 to accommodate the
> > additional control file.
> >
> > Some related discussions can be found at:
> >
> > Link: https://lore.kernel.org/all/4b64a41c-6167-4c02-8bae-3021270ca519@fastmail.fm/T/#mc73e04df56b8830b1d7b06b5d9f22e594fba423e
> > Link: https://lore.kernel.org/linux-fsdevel/CAOQ4uxhAY1m7ubJ3p-A3rSufw_53WuDRMT1Zqe_OC0bP_Fb3Zw@mail.gmail.com/
> >
>
> remove newline
>
> > Cc: Amir Goldstein <amir73il@...il.com>
> > Signed-off-by: Chen Linxuan <chenlinxuan@...ontech.com>
> >
> > ---
> > Please review this patch carefully. I am new to kernel development and
> > I am not quite sure if I have followed the best practices, especially
> > in terms of seq_file, error handling and locking. I would appreciate
> > any feedback.
>
> Very nice work!
>
> >
> > I have do some simply testing using libfuse example [1]. It seems to
> > work well.
>
> It would be great if you could add basic sanity tests to libfuse
> maybe in test_passthrough_hp(), but I do not see any tests for
> /sys/fs/fuse/connections.
>
> I also see that there is one kernel selftest that mounts a fuse fs
> tools/testing/selftests/memfd
> maybe that is an easier way to write a simple test to verify the
> /sys/fs/fuse/connections functionally.
>
> Anyway, I do not require that you do that as a condition for merging this patch,
> but I may require that for removing CAP_SYS_ADMIN ;)
>
> >
> > [1]: https://github.com/libfuse/libfuse/blob/master/example/passthrough_hp.cc
> > ---
> >  fs/fuse/control.c | 129 +++++++++++++++++++++++++++++++++++++++++++++-
> >  fs/fuse/fuse_i.h  |   2 +-
> >  2 files changed, 129 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/fuse/control.c b/fs/fuse/control.c
> > index 2a730d88cc3bd..4d1e0acc5030f 100644
> > --- a/fs/fuse/control.c
> > +++ b/fs/fuse/control.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/init.h>
> >  #include <linux/module.h>
> >  #include <linux/fs_context.h>
> > +#include <linux/seq_file.h>
> >
> >  #define FUSE_CTL_SUPER_MAGIC 0x65735543
> >
> > @@ -180,6 +181,129 @@ static ssize_t fuse_conn_congestion_threshold_write(struct file *file,
> >         return ret;
> >  }
> >
> > +struct fuse_backing_files_seq_state {
> > +       struct fuse_conn *fc;
> > +       int pos;
>
> It will be more clear to call this 'backing_id'.
> It is more than an abstract pos in this context.
>
> > +};
> > +
> > +static void *fuse_backing_files_seq_start(struct seq_file *seq, loff_t *pos)
> > +{
> > +       struct fuse_backing_files_seq_state *state = seq->private;
> > +       struct fuse_conn *fc = state->fc;
> > +
> > +       if (!fc)
> > +               return NULL;
> > +
> > +       spin_lock(&fc->lock);
> > +
> > +       if (*pos > idr_get_cursor(&fc->backing_files_map)) {
>
> This won't do after the ida allocator has wrapped up back to 1,
> it will not iterate the high ids.
>
> Please look at using idr_get_next() iteration, like bpf_prog_seq_ops.
>
> With that change, I don't think that you need to take the spin lock
> for iteration.
> I think that you can use rcu_read_lock() for the scope of each
> start(). next(), show()
> because we do not need to promise a "snapshot" of the backing_file at a specific
> time. If backing files are added/removed while iterating it is undefined if they
> are listed or not, just like readdir.
>
> > +               spin_unlock(&fc->lock);
> > +               return NULL;
>
> Not critical, but if you end up needing a "scoped" unlock for the
> entire iteration, you can use
> the unlock in stop() if you return ERR_PTR(ENOENT) instead of NULL in
> those error conditions.
>
> > +       }
> > +
> > +       state->pos = *pos;
> > +       return state;
> > +}
> > +
> > +static void *fuse_backing_files_seq_next(struct seq_file *seq, void *v,
> > +                                        loff_t *pos)
> > +{
> > +       struct fuse_backing_files_seq_state *state = seq->private;
> > +
> > +       (*pos)++;
> > +       state->pos = *pos;
> > +
> > +       if (state->pos > idr_get_cursor(&state->fc->backing_files_map)) {
> > +               spin_unlock(&state->fc->lock);
> > +               return NULL;
> > +       }
> > +
> > +       return state;
> > +}
> > +
> > +static int fuse_backing_files_seq_show(struct seq_file *seq, void *v)
> > +{
> > +       struct fuse_backing_files_seq_state *state = seq->private;
> > +       struct fuse_conn *fc = state->fc;
> > +       struct fuse_backing *fb;
> > +
> > +       fb = idr_find(&fc->backing_files_map, state->pos);
>
> You must fuse_backing_get/put(fb) around dereferencing fb->file
> if not holding the fc->lock.
> See fuse_passthrough_open().
>
> > +       if (!fb || !fb->file)
> > +               return 0;
> > +
> > +       seq_file_path(seq, fb->file, " \t\n\\");
>
> Pls print the backing id that is associated with the open file.

Does the backing id means anything in user space?
I think maybe we shouldn't expose kernel details to userspace.

>
> I wonder out loud if we should also augment the backing fd
> information in fdinfo of specific open fuse FOPEN_PASSTHROUGH files?

Or do you mean that we should display backing id and fuse connection id here?

>
> I am not requiring that you do that, but if you want to take a look
> I think that it could be cool to display this info, along with open_flags
> and other useful fuse_file information.
>
> Thanks,
> Amir.
>
> > +       seq_puts(seq, "\n");
> > +
> > +       return 0;
> > +}
> > +
> > +static void fuse_backing_files_seq_stop(struct seq_file *seq, void *v)
> > +{
> > +       struct fuse_backing_files_seq_state *state = seq->private;
> > +
> > +       if (v)
> > +               spin_unlock(&state->fc->lock);
> > +}
> > +
> > +static const struct seq_operations fuse_backing_files_seq_ops = {
> > +       .start = fuse_backing_files_seq_start,
> > +       .next = fuse_backing_files_seq_next,
> > +       .stop = fuse_backing_files_seq_stop,
> > +       .show = fuse_backing_files_seq_show,
> > +};
> > +
> > +static int fuse_backing_files_seq_open(struct inode *inode, struct file *file)
> > +{
> > +       struct fuse_conn *fc;
> > +       struct fuse_backing_files_seq_state *state;
> > +       int err;
> > +
> > +       fc = fuse_ctl_file_conn_get(file);
> > +       if (!fc)
> > +               return -ENOTCONN;
> > +
> > +       err = seq_open(file, &fuse_backing_files_seq_ops);
> > +       if (err) {
> > +               fuse_conn_put(fc);
> > +               return err;
> > +       }
> > +
> > +       state = kmalloc(sizeof(*state), GFP_KERNEL);
> > +       if (!state) {
> > +               seq_release(file->f_inode, file);
> > +               fuse_conn_put(fc);
> > +               return -ENOMEM;
> > +       }
> > +
> > +       state->fc = fc;
> > +       state->pos = 0;
> > +       ((struct seq_file *)file->private_data)->private = state;
> > +
> > +       return 0;
> > +}
> > +
> > +static int fuse_backing_files_seq_release(struct inode *inode,
> > +                                         struct file *file)
> > +{
> > +       struct seq_file *seq = file->private_data;
> > +       struct fuse_backing_files_seq_state *state = seq->private;
> > +
> > +       if (state) {
> > +               fuse_conn_put(state->fc);
> > +               kfree(state);
> > +               seq->private = NULL;
> > +       }
> > +
> > +       return seq_release(inode, file);
> > +}
> > +
> > +static const struct file_operations fuse_conn_passthrough_backing_files_ops = {
> > +       .open = fuse_backing_files_seq_open,
> > +       .read = seq_read,
> > +       .llseek = seq_lseek,
> > +       .release = fuse_backing_files_seq_release,
> > +};
> > +
> >  static const struct file_operations fuse_ctl_abort_ops = {
> >         .open = nonseekable_open,
> >         .write = fuse_conn_abort_write,
> > @@ -270,7 +394,10 @@ int fuse_ctl_add_conn(struct fuse_conn *fc)
> >                                  1, NULL, &fuse_conn_max_background_ops) ||
> >             !fuse_ctl_add_dentry(parent, fc, "congestion_threshold",
> >                                  S_IFREG | 0600, 1, NULL,
> > -                                &fuse_conn_congestion_threshold_ops))
> > +                                &fuse_conn_congestion_threshold_ops) ||
> > +           !fuse_ctl_add_dentry(parent, fc, "backing_files", S_IFREG | 0400, 1,
> > +                                NULL,
> > +                                &fuse_conn_passthrough_backing_files_ops))
> >                 goto err;
> >
> >         return 0;
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index d56d4fd956db9..2830b05bb0928 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -46,7 +46,7 @@
> >  #define FUSE_NAME_MAX (PATH_MAX - 1)
> >
> >  /** Number of dentries for each connection in the control filesystem */
> > -#define FUSE_CTL_NUM_DENTRIES 5
> > +#define FUSE_CTL_NUM_DENTRIES 6
> >
> >  /* Frequency (in seconds) of request timeout checks, if opted into */
> >  #define FUSE_TIMEOUT_TIMER_FREQ 15
> > --
> > 2.43.0
> >
>
>

Powered by blists - more mailing lists