linux-kernel - Re: [PATCH v2 3/3] fs: fuse: add more information to fdinfo

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] [day] [month] [year] [list]

Message-ID: <CAOQ4uxgkg0uOuAWO2wOPNkMmD9wqd5wMX+gTfCT-zVHBC8CkZg@mail.gmail.com>
Date: Thu, 8 May 2025 13:16:06 +0200
From: Amir Goldstein <amir73il@...il.com>
To: chenlinxuan@...ontech.com
Cc: Miklos Szeredi <miklos@...redi.hu>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/3] fs: fuse: add more information to fdinfo

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>
>
> This commit add fuse connection device id and backing_id, if any, to
> fdinfo of opened fuse files.
>
> Related discussions can be found at links below.
>
> Link: https://lore.kernel.org/all/CAOQ4uxgS3OUy9tpphAJKCQFRAn2zTERXXa0QN_KvP6ZOe2KVBw@mail.gmail.com/
> Cc: Amir Goldstein <amir73il@...il.com>
> Signed-off-by: Chen Linxuan <chenlinxuan@...ontech.com>
> ---
> 1. I wonder if we should display fuse connection device id here.
>
> 2. I don't think using idr_for_each_entry is a good idea. But I failed
>    to find another way to get backing_id of fuse_backing effectively.

Indeed.
The thing is that a fuse file could have passthough to backing file
without that backing file having a backing id at all.

1. server maps backing file to backing id N
2. server associates opened file F to backing file N
3. server unmaps backing id N, so now the backing file is "anonymous"

The backing file remains referenced by the kernel until file F is released.

> ---
>  fs/fuse/file.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 754378dd9f7159f20fde6376962d45c4c706b868..5cfb806aa5cd22c57814168eb33de77c6f213da0 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -8,6 +8,8 @@
>
>  #include "fuse_i.h"
>
> +#include "linux/idr.h"
> +#include "linux/rcupdate.h"
>  #include <linux/pagemap.h>
>  #include <linux/slab.h>
>  #include <linux/kernel.h>
> @@ -3392,6 +3394,35 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off,
>         return ret;
>  }
>
> +static void fuse_file_show_fdinfo(struct seq_file *m, struct file *f)
> +{
> +       struct fuse_file *ff = f->private_data;
> +       struct fuse_conn *fc = ff->fm->fc;
> +       struct fuse_inode *fi = get_fuse_inode(file_inode(f));
> +
> +       seq_printf(m, "fuse_conn:\t%u\n", fc->dev);
> +

Let's follow pattern of fanotify_fdinfo() and add some useful information:

seq_printf(m, "fuse conn:%u open_flags:%u\n", fc->dev, ff->open_flags);

> +#ifdef CONFIG_FUSE_PASSTHROUGH
> +       struct fuse_backing *fb;
> +       struct fuse_backing *backing;
> +       int backing_id;

I did not know that it is allowed to define local vars in the middle
of a function in kernel code.
anyway, you shouldn't have to use ifdef here.
compiler will optimize away code inside:
if (fuse_file_passthrough(ff)) {

When CONFIG_FUSE_PASSTHROUGH is not defined.

> +
> +       if (ff->open_flags & FOPEN_PASSTHROUGH) {
> +               fb = fuse_inode_backing(fi);
> +               if (fb) {
> +                       rcu_read_lock();
> +                       idr_for_each_entry(&fc->backing_files_map, backing, backing_id) {
> +                               if (backing == fb) {
> +                                       seq_printf(m, "fuse_backing_id:\t%d\n", backing_id);
> +                                       break;
> +                               }
> +                       }
> +                       rcu_read_unlock();

We cannot display backing_id here, but we can print the fuse_file_passthrough()
file path and we already have a reference to that file, so no need for
any locks/RCU,
so this should work:

   struct fuse_file *ff = f->private_data;
   struct fuse_conn *fc = ff->fm->fc;
   struct file *backing_file = fuse_file_passthrough(ff);

   seq_printf(m, "fuse conn:%u open_flags:%u\n", fc->dev, ff->open_flags);
   if (backing_file) {
       seq_puts(seq, "fuse backing_file: ");
       seq_file_path(seq, fb->file, " \t\n\\");
       seq_puts(seq, "\n");
    }

We want a separate line for backing_file because:
1. There may be multiple backing files per fuse file is the future
(see famfs patches)
2. In that case, there will likely be file range mapping information
printed in this line

Thanks,
Amir.