Message-ID: <CAOQ4uxjKFXOKQxPpxtS6G_nR0tpw95w0GiO68UcWg_OBhmSY=Q@mail.gmail.com>
Date: Wed, 7 May 2025 13:02:45 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Chen Linxuan <chenlinxuan@...ontech.com>
Cc: 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 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.

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

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