On Thu, May 8, 2025 at 6:45 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Thu, May 8, 2025 at 10:54 AM Chen Linxuan via B4 Relay > <devnull+chenlinxuan.uniontech.com@xxxxxxxxxx> wrote: > > > > From: Chen Linxuan <chenlinxuan@xxxxxxxxxxxxx> > > > > 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 links below. > > > > 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/ > > Cc: Amir Goldstein <amir73il@xxxxxxxxx> > > Signed-off-by: Chen Linxuan <chenlinxuan@xxxxxxxxxxxxx> > > --- > > fs/fuse/control.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++++++++- > > fs/fuse/fuse_i.h | 2 +- > > 2 files changed, 136 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fuse/control.c b/fs/fuse/control.c > > index f0874403b1f7c91571f38e4ae9f8cebe259f7dd1..d1ac934d7b8949577545ffd20535c68a9c4ef90f 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,136 @@ static ssize_t fuse_conn_congestion_threshold_write(struct file *file, > > return ret; > > } > > > > +struct fuse_backing_files_seq_state { > > + struct fuse_conn *fc; > > + int backing_id; > > +}; > > + > > +static void *fuse_backing_files_seq_start(struct seq_file *seq, loff_t *pos) > > +{ > > + struct fuse_backing *fb; > > + struct fuse_backing_files_seq_state *state; > > + struct fuse_conn *fc; > > + int backing_id; > > + void *ret; > > + > > + if (*pos + 1 > INT_MAX) > > + return ERR_PTR(-EINVAL); > > + > > + backing_id = *pos + 1; > > I am not sure if this +1 is correct. > Please make sure that you test reading in several chunks > not only from pos 0 to make sure this is right. > Assuming that is how the code gets to call start() with non zero pos? I am not very certain. In seq_file.c, we can see some processes like stop, allocating a new buffer, and restarting at the original pos when the buffer is full. Without adding this +1, this part of the code actually doesn't work correctly. It outputs the backing_file with id=1 twice. I think the reason for this issue is that I didn't update the value of pos with backing_id after calling idr_get_next(). > > I think that backing_id should always be in sync with *pos for that to > work correctly. > "The next() function should set ``*pos`` to a value that start() can use > to find the new location in the sequence." > > That means that we do not really need the backing_id in the state. > We can just have a local backing_id var that reads from *pos and > *pos is set to it at the end of start/next. I think we need to maintain pos in the state because show() does not accept pos as a parameter. > > > + > > + fc = fuse_ctl_file_conn_get(seq->file); > > + if (!fc) > > + return ERR_PTR(-ENOTCONN); > > + > > + rcu_read_lock(); > > + > > + fb = idr_get_next(&fc->backing_files_map, &backing_id); > > + > > + rcu_read_unlock(); > > + > > + if (!fb) { > > + ret = NULL; > > + goto err; > > + } > > + > > + state = kmalloc(sizeof(*state), GFP_KERNEL); > > + if (!state) { > > + ret = ERR_PTR(-ENOMEM); > > + goto err; > > + } > > + > > + state->fc = fc; > > + state->backing_id = backing_id; > > + > > + ret = state; > > + return ret; > > + > > +err: > > + fuse_conn_put(fc); > > + return ret; > > +} > > + > > +static void *fuse_backing_files_seq_next(struct seq_file *seq, void *v, > > + loff_t *pos) > > +{ > > + struct fuse_backing_files_seq_state *state = v; > > + struct fuse_backing *fb; > > + > > + (*pos)++; > > + state->backing_id++; > > Need to check for INT_MAX overflow? It shouldn't be necessary. Before overflow occurs, idr_get_next() will already return NULL. > > > + > > + rcu_read_lock(); > > + > > + fb = idr_get_next(&state->fc->backing_files_map, &state->backing_id); > > + > > + rcu_read_unlock(); > > + > > + if (!fb) > > + return NULL; > > I think that I gave you the wrong advice on v1 review. > IIUC, if you return NULL from next(), stop() will get a NULL v arg, > so I think you need to put fc and free state here as well. > Please verify that. > > Perhaps a small helper fuse_backing_files_seq_state_free() > can help the code look cleaner, because you may also need it > in the int overflow case above? > > > + > > + > > + return state; > > +} > > + > > +static int fuse_backing_files_seq_show(struct seq_file *seq, void *v) > > +{ > > + struct fuse_backing_files_seq_state *state = v; > > + struct fuse_conn *fc = state->fc; > > + struct fuse_backing *fb; > > + > > + rcu_read_lock(); > > + > > + fb = idr_find(&fc->backing_files_map, state->backing_id); > > + fb = fuse_backing_get(fb); > > rcu_read_unlock(); > > should be here. > After you get a ref on fb, it is safe to deref fb without RCU > so no need for the goto cleanup labels. > > > + if (!fb) > > + goto out; > > + > > + if (!fb->file) > > + goto out_put_fb; > > + > > This would be nicer as > if (!fb->file) { > > to avoid the goto. I guess what you meant to say was if (fb->file)? > > > + seq_printf(seq, "%5u: ", state->backing_id); > > + seq_file_path(seq, fb->file, " \t\n\\"); > > + seq_puts(seq, "\n"); > > + > > +out_put_fb: > > + fuse_backing_put(fb); > > +out: > > + rcu_read_unlock(); > > + return 0; > > +} > > + > > +static void fuse_backing_files_seq_stop(struct seq_file *seq, void *v) > > +{ > > + struct fuse_backing_files_seq_state *state; > > + > > + if (!v) > > + return; > > + > > + state = v; > > + fuse_conn_put(state->fc); > > + kvfree(state); > > That could become a helper > static void fuse_backing_files_seq_state_free(struct > fuse_backing_files_seq_state *state) > > and seq_stop() could become: > > if (v) > fuse_backing_files_seq_state_free(v); > > > Thanks, > Amir. > > I will send the next version of the patch later.
