linux-kernel - Re: [PATCH v3 2/3] fs: fuse: add backing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxhQu+Gr8Sn5HbZRyOhDd3MBqN5xKFxZru39LGgk5K-ACw@mail.gmail.com>
Date: Fri, 9 May 2025 11:14:00 +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: [PATCH v3 2/3] fs: fuse: add backing_files control file

On Fri, May 9, 2025 at 9:39 AM Chen Linxuan <chenlinxuan@...ontech.com> wrote:
>
> On Fri, May 9, 2025 at 2:34 PM Chen Linxuan via B4 Relay
> <devnull+chenlinxuan.uniontech.com@...nel.org> wrote:
> >
> > From: Chen Linxuan <chenlinxuan@...ontech.com>
> >
> > 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@fastmail.fm/T/#mc73e04df56b8830b1d7b06b5d9f22e594fba423e
> > Link: https://lore.kernel.org/linux-fsdevel/CAOQ4uxhAY1m7ubJ3p-A3rSufw_53WuDRMT1Zqe_OC0bP_Fb3Zw@mail.gmail.com/
> > Cc: Amir Goldstein <amir73il@...il.com>
> > Signed-off-by: Chen Linxuan <chenlinxuan@...ontech.com>
> > ---
> >  fs/fuse/control.c | 155 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  fs/fuse/fuse_i.h  |   2 +-
> >  2 files changed, 144 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/fuse/control.c b/fs/fuse/control.c
> > index f0874403b1f7c91571f38e4ae9f8cebe259f7dd1..6333fffec85bd562dc9e86ba7cbf88b8bc2d68ce 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,135 @@ 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;
> > +};
>
> As mentioned in the previous v2 related discussion
> backing_id is maintained in state because
> show() of seq_file doesn't accept the pos parameter.
> But actually we can get the pos in the show() function
> via the index field of struct seq_file.
> The softnet_seq_show() function in net-procfs.c are doing this.
> I'm not really sure if it would be better to implement it this way.
>

For better clarity of the code and maintainability,
I think that storing backing_id in the state as you did is better.

> > +
> > +static void fuse_backing_files_seq_state_free(struct fuse_backing_files_seq_state *state)
> > +{
> > +       fuse_conn_put(state->fc);
> > +       kvfree(state);
> > +}
> > +
> > +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;
> > +
> > +       fc = fuse_ctl_file_conn_get(seq->file);
>
> I'm not sure if I should get fc in fuse_backing_files_seq_start here
> and handle fc as (part of) the seq_file iterator.
> Or should I get the fc in fuse_backing_files_seq_open
> and store the fc in the private field of the seq_file more appropriately.
> I guess the difference isn't that big?

The simpler the better.
I think what you did is fine. I see no reason to change it.

Thanks,
Amir.