Date:   Tue, 19 Jan 2021 11:51:49 +0000
From:   Alessio Balsini <balsini@...roid.com>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Alessio Balsini <balsini@...roid.com>,
        Miklos Szeredi <miklos@...redi.hu>,
        Akilesh Kailash <akailash@...gle.com>,
        Antonio SJ Musumeci <trapexit@...wn.link>,
        David Anderson <dvander@...gle.com>,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        Jann Horn <jannh@...gle.com>, Jens Axboe <axboe@...nel.dk>,
        Martijn Coenen <maco@...roid.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Lawrence <paullawrence@...gle.com>,
        Peng Tao <bergwolf@...il.com>,
        Stefano Duo <duostefano93@...il.com>,
        Zimuzo Ezeozue <zezeozue@...gle.com>, wuyan <wu-yan@....com>,
        fuse-devel <fuse-devel@...ts.sourceforge.net>,
        kernel-team <kernel-team@...roid.com>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RESEND V11 3/7] fuse: Definitions and ioctl for
 passthrough

On Tue, Jan 19, 2021 at 08:33:16AM +0200, Amir Goldstein wrote:
> On Mon, Jan 18, 2021 at 9:28 PM Alessio Balsini <balsini@...roid.com> wrote:
> >
> > Expose the FUSE_PASSTHROUGH interface to user space and declare all the
> > basic data structures and functions as the skeleton on top of which the
> > FUSE passthrough functionality will be built.
> >
> > As part of this, introduce the new FUSE passthrough ioctl(), which
> > allows the FUSE daemon to specify a direct connection between a FUSE
> > file and a lower file system file. Such ioctl() requires users pace to
> > pass the file descriptor of one of its opened files through the
> > fuse_passthrough_out data structure introduced in this patch. This
> > structure includes extra fields for possible future extensions.
> > Also, add the passthrough functions for the set-up and tear-down of the
> > data structures and locks that will be used both when fuse_conns and
> > fuse_files are created/deleted.
> >
> > Signed-off-by: Alessio Balsini <balsini@...roid.com>
> > ---
> [...]
> 
> > @@ -699,6 +700,7 @@ void fuse_conn_init(struct fuse_conn *fc, struct fuse_mount *fm,
> >         INIT_LIST_HEAD(&fc->bg_queue);
> >         INIT_LIST_HEAD(&fc->entry);
> >         INIT_LIST_HEAD(&fc->devices);
> > +       idr_init(&fc->passthrough_req);
> >         atomic_set(&fc->num_waiting, 0);
> >         fc->max_background = FUSE_DEFAULT_MAX_BACKGROUND;
> >         fc->congestion_threshold = FUSE_DEFAULT_CONGESTION_THRESHOLD;
> > @@ -1052,6 +1054,12 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> >                                 fc->handle_killpriv_v2 = 1;
> >                                 fm->sb->s_flags |= SB_NOSEC;
> >                         }
> > +                       if (arg->flags & FUSE_PASSTHROUGH) {
> > +                               fc->passthrough = 1;
> > +                               /* Prevent further stacking */
> > +                               fm->sb->s_stack_depth =
> > +                                       FILESYSTEM_MAX_STACK_DEPTH + 1;
> > +                       }
> 
> Hi Allesio,
> 
> I'm sorry I missed the discussion on v10 patch, but this looks wrong.
> First of all, assigning a value above a declared MAX_ is misleading
> and setting a trap for someone else to trip in the future.
> 
> While this may be just a semantic mistake, the code that checks for
> (passthrough_sb->s_stack_depth > FILESYSTEM_MAX_STACK_DEPTH)
> is just cheating.
> 
> fuse_file_{read,write}_iter are stacked operations, no different in any way
> than overlayfs and ecryptfs stacked file operations.
> 
> Peng Tao mentioned a case of passthrough to overlayfs over ecryptfs [1].
> If anyone really thinks this use case is interesting enough (I doubt it), then
> they may propose to bump up FILESYSTEM_MAX_STACK_DEPTH to 3,
> but not to cheat around the currently defined maximum.
> 
> So please set s_max_depth to FILESYSTEM_MAX_STACK_DEPTH and
> restore your v10 check of
> passthrough_sb->s_stack_depth >= FILESYSTEM_MAX_STACK_DEPTH
> 
> Your commit message sounds as if the only purpose of this check is to
> prevent stacking of FUSE passthrough on top of each other, but that
> is not enough.
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/CA+a=Yy6S9spMLr9BqyO1qvU52iAAXU3i9eVtb81SnrzjkCwO5Q@mail.gmail.com/


Hi Amir,

The stacking solution in V10 works for me and, as we agreed last time,
we would still be able to update the stacking policy with FUSE
passthrough as soon as some use cases find it beneficial.

Our use case in Android is somewhat simple and it's difficult for me to
think of how this stacking can be useful or limiting to the different
use cases. It's anyway worth highlighting that if FUSE passthrough is
disabled, as it is by default, the FUSE behavior remains exactly the
same as it was before this series.
For my limited use cases experience here, I have no personal preferences
on the stacking policy I'm just trying to do the right thing based on
the feedback from the community :)

I can change this policy back as this was in V10, but at the same time I
don't want to put extra effort/confusion in the mailing list and to
Miklos with the next patch version. So I'm going to post the diff to
bring back the stacking policy as it was in V10 in reply to "[PATCH
RESEND V11 4/7] fuse: Passthrough initialization and release" and wait
for the community consensus before sending out the V12.

Thanks again for helpful feedback!

Cheers,
Alessio

Powered by blists - more mailing lists