Date:   Fri, 18 Sep 2020 17:33:54 +0100
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>,
        David Anderson <dvander@...gle.com>,
        Eric Yan <eric.yan@...plus.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>,
        Stefano Duo <stefanoduo@...gle.com>,
        Zimuzo Ezeozue <zezeozue@...gle.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 V8 1/3] fuse: Definitions and ioctl() for passthrough

Hi Amir,

Thanks again for your feedback.

On Sat, Sep 12, 2020 at 02:06:02PM +0300, Amir Goldstein wrote:
> On Fri, Sep 11, 2020 at 7:34 PM Alessio Balsini <balsini@...roid.com> wrote:
> [...]
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index bba747520e9b..eb223130a917 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -965,6 +965,12 @@ static void process_init_reply(struct fuse_conn *fc, struct fuse_args *args,
> >                                         min_t(unsigned int, FUSE_MAX_MAX_PAGES,
> >                                         max_t(unsigned int, arg->max_pages, 1));
> >                         }
> > +                       if (arg->flags & FUSE_PASSTHROUGH) {
> > +                               fc->passthrough = 1;
> > +                               /* Prevent further stacking */
> > +                               fc->sb->s_stack_depth =
> > +                                       FILESYSTEM_MAX_STACK_DEPTH;
> > +                       }
> 
> That seems a bit limiting.
> I suppose what you really want to avoid is loops into FUSE fd.
> There may be a way to do this with forbidding overlay over FUSE passthrough
> or the other way around.
> 
> You can set fc->sb->s_stack_depth = FILESYSTEM_MAX_STACK_DEPTH - 1
> here and in passthrough ioctl you can check for looping into a fuse fs with
> passthrough enabled on the passed fd (see below) ...
>
> [...]
> > diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
> > new file mode 100644
> > index 000000000000..86ab4eafa7bf
> > --- /dev/null
> > +++ b/fs/fuse/passthrough.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include "fuse_i.h"
> > +
> > +int fuse_passthrough_setup(struct fuse_req *req, unsigned int fd)
> > +{
> > +       int ret;
> > +       int fs_stack_depth;
> > +       struct file *passthrough_filp;
> > +       struct inode *passthrough_inode;
> > +       struct super_block *passthrough_sb;
> > +
> > +       /* Passthrough mode can only be enabled at file open/create time */
> > +       if (req->in.h.opcode != FUSE_OPEN && req->in.h.opcode != FUSE_CREATE) {
> > +               pr_err("FUSE: invalid OPCODE for request.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       passthrough_filp = fget(fd);
> > +       if (!passthrough_filp) {
> > +               pr_err("FUSE: invalid file descriptor for passthrough.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = -EINVAL;
> > +       if (!passthrough_filp->f_op->read_iter ||
> > +           !passthrough_filp->f_op->write_iter) {
> > +               pr_err("FUSE: passthrough file misses file operations.\n");
> > +               goto out;
> > +       }
> > +
> > +       passthrough_inode = file_inode(passthrough_filp);
> > +       passthrough_sb = passthrough_inode->i_sb;
> > +       fs_stack_depth = passthrough_sb->s_stack_depth + 1;
> 
> ... for example:
> 
>        if (fs_stack_depth && passthrough_sb->s_type == fuse_fs_type) {
>                pr_err("FUSE: stacked passthrough file\n");
>                goto out;
>        }
> 
> But maybe we want to ban passthrough to any lower FUSE at least for start.


Yes, what I proposed here is very conservative, and your solution sounds
good to me. Unfortunately I don't have a clear idea of what could go wrong
if we relax this constraint. I need some guidance from you experts here.

What do you think if we keep this overly strict rule for now to avoid
unintended behaviors and come back as we find affected use case?


> 
> > +       ret = -EEXIST;
> 
> Why EEXIST? Why not EINVAL?
> 


Reaching the stacking limit sounded like an error caused by the undesired
existence of something, thus EEXIST sounded like a good fit.
No problem in changing that to EINVAL.



> > +       if (fs_stack_depth > FILESYSTEM_MAX_STACK_DEPTH) {
> > +               pr_err("FUSE: maximum fs stacking depth exceeded for passthrough\n");
> > +               goto out;
> > +       }
> > +
> > +       req->args->passthrough_filp = passthrough_filp;
> > +       return 0;
> > +out:
> > +       fput(passthrough_filp);
> > +       return ret;
> > +}
> > +
> 
> And speaking of overlayfs, I believe you may be able to test your code with
> fuse-overlayfs (passthrough to upper files).
> 
> This is a project with real users running real workloads who may be
> able to provide you with valuable feedback from testing.
> 
> Thanks,
> Amir.
> 
> [1] https://github.com/containers/fuse-overlayfs

This is indeed a project with several common elements to what we are doing
in Android, and that would probably benefit from this change. Will surely
involve them in the discussion.
Thanks for sharing!

Alessio

Powered by blists - more mailing lists