Message-ID: <CAC1kPDNC4ZTTMFJYQB9g1XKW4Gk7grkdZUY6KOBOvadGstT9Ug@mail.gmail.com>
Date: Fri, 9 May 2025 14:33:05 +0800
From: Chen Linxuan <chenlinxuan@...ontech.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: chenlinxuan@...ontech.com, Miklos Szeredi <miklos@...redi.hu>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/3] fs: fuse: add backing_files control file

On Thu, May 8, 2025 at 6:45 PM Amir Goldstein <amir73il@...il.com> wrote:
>
> On Thu, May 8, 2025 at 10:54 AM 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 | 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.

Powered by blists - more mailing lists