linux-kernel - Re: [PATCH 0/1] Fuse Passthrough cache issues

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: <CAOQ4uxjHe6m1LUxTzrmOSFnbF=yZRuL0180V0gFH8qJTfcsNeQ@mail.gmail.com>
Date: Wed, 3 Jul 2024 19:24:06 +0300
From: Amir Goldstein <amir73il@...il.com>
To: Bernd Schubert <bernd.schubert@...tmail.fm>
Cc: Daniel Rosenberg <drosen@...gle.com>, Miklos Szeredi <miklos@...redi.hu>, linux-kernel@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH 0/1] Fuse Passthrough cache issues

On Wed, Jul 3, 2024 at 7:09 PM Bernd Schubert
<bernd.schubert@...tmail.fm> wrote:
>
>
>
> On 7/3/24 16:41, Amir Goldstein wrote:
> > On Wed, Jul 3, 2024 at 4:27 PM Bernd Schubert
> > <bernd.schubert@...tmail.fm> wrote:
> >>
> >>
> >>
> >> On 7/3/24 03:02, Daniel Rosenberg wrote:
> >>> I've been attempting to recreate Android's usage of Fuse Passthrough with the
> >>> version now merged in the kernel, and I've run into a couple issues. The first
> >>> one was pretty straightforward, and I've included a patch, although I'm not
> >>> convinced that it should be conditional, and it may need to do more to ensure
> >>> that the cache is up to date.
> >>>
> >>> If your fuse daemon is running with writeback cache enabled, writes with
> >>> passthrough files will cause problems. Fuse will invalidate attributes on
> >>> write, but because it's in writeback cache mode, it will ignore the requested
> >>> attributes when the daemon provides them. The kernel is the source of truth in
> >>> this case, and should update the cached values during the passthrough write.
> >>
> >> Could you explain why you want to have the combination passthrough and
> >> writeback cache?
> >>
> >> I think Amirs intention was to have passthrough and cache writes
> >> conflicting, see fuse_file_passthrough_open() and
> >> fuse_file_cached_io_open().
> >
> > Yes, this was an explicit design requirement from Miklos [1].
> > I also have use cases to handle some read/writes from server
> > and the compromise was that for the first version these cases should
> > use FOPEN_DIRECT_IO, which does not conflict with FOPEN_PASSTHROUGH.
> >
> > I guess this is not good enough for Android applications opening photos
> > that need the FUSE readahead cache for performance?
> >
> > In that case, a future BPF filter can decide whether to send the IO direct
> > to server or to backing inode.
> >
> > Or a future backing inode mapping API could map part of the file to
> > backing inode
> > and the metadata portion will not be mapped to backing inode will fall back to
> > direct IO to server.
> >
> > [1] https://lore.kernel.org/linux-fsdevel/CAJfpegtWdGVm9iHgVyXfY2mnR98XJ=6HtpaA+W83vvQea5PycQ@mail.gmail.com/
> >
> >>
> >> Also in <libfuse>/example/passthrough_hp.cc in sfs_init():
> >>
> >>     /* Passthrough and writeback cache are conflicting modes */
> >>
> >>
> >>
> >> With that I wonder if either fc->writeback_cache should be ignored when
> >> a file is opened in passthrough mode, or if fuse_file_io_open() should
> >> ignore FOPEN_PASSTHROUGH when fc->writeback_cache is set. Either of both
> >> would result in the opposite of what you are trying to achieve - which
> >> is why I think it is important to understand what is your actual goal.
> >>
> >
> > Is there no standard way for FUSE client to tell the server that the
> > INIT response is invalid?
>
> Problem is that at FUSE_INIT time it is already mounted. process_init_reply()
> can set an error state, but fuse_get_req*() will just result in ECONNREFUSED.*
>
> >
> > Anyway, we already ignore  FUSE_PASSTHROUGH in INIT response
> > for several cases, so this could be another case.
> > Then FOPEN_PASSTHROUGH will fail with EIO (not be ignored).
>
> So basically this?
>
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 573550e7bbe1..36c6dcd47a53 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1327,7 +1327,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>                         if (IS_ENABLED(CONFIG_FUSE_PASSTHROUGH) &&
>                             (flags & FUSE_PASSTHROUGH) &&
>                             arg->max_stack_depth > 0 &&
> -                           arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH) {
> +                           arg->max_stack_depth <= FILESYSTEM_MAX_STACK_DEPTH &&
> +                           !(flags & FUSE_WRITEBACK_CACHE)) {
>                                 fc->passthrough = 1;
>                                 fc->max_stack_depth = arg->max_stack_depth;
>                                 fm->sb->s_stack_depth = arg->max_stack_depth;
>
>

Yap.
Maybe add something to the comment, because the comment is
about max_stack_depth and someone may assume that it also refers
to FUSE_WRITEBACK_CACHE somehow.

Thanks,
Amir.