On Wed, May 7, 2025 at 7:03 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Wed, May 7, 2025 at 5:29 AM Chen Linxuan <chenlinxuan@xxxxxxxxxxxxx> 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@xxxxxxxxxxx/T/#mc73e04df56b8830b1d7b06b5d9f22e594fba423e > > Link: https://lore.kernel.org/linux-fsdevel/CAOQ4uxhAY1m7ubJ3p-A3rSufw_53WuDRMT1Zqe_OC0bP_Fb3Zw@xxxxxxxxxxxxxx/ > > > > remove newline > > > Cc: Amir Goldstein <amir73il@xxxxxxxxx> > > Signed-off-by: Chen Linxuan <chenlinxuan@xxxxxxxxxxxxx> > > > > --- > > 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 > > > >
