Message-ID: <CAOQ4uxiSkLwPL3YLqmYHMqBStGFm7xxVLjD2+NwyyyzFpj3hFQ@mail.gmail.com>
Date: Fri, 21 Feb 2025 18:25:24 +0100
From: Amir Goldstein <amir73il@...il.com>
To: Bernd Schubert <bernd@...ernd.com>
Cc: Moinak Bhattacharyya <moinakb001@...il.com>, Miklos Szeredi <miklos@...redi.hu>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	io-uring@...r.kernel.org
Subject: Re: [PATCH] Fuse: Add backing file support for uring_cmd

On Fri, Feb 21, 2025 at 6:13 PM Bernd Schubert <bernd@...ernd.com> wrote:
>
>
>
> On 2/21/25 17:24, Amir Goldstein wrote:
> > On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
> > <moinakb001@...il.com> wrote:
> >>
> >> Sorry about that. Correctly-formatted patch follows. Should I send out a
> >> V2 instead?
> >>
> >> Add support for opening and closing backing files in the fuse_uring_cmd
> >> callback. Store backing_map (for open) and backing_id (for close) in the
> >> uring_cmd data.
> >> ---
> >>   fs/fuse/dev_uring.c       | 50 +++++++++++++++++++++++++++++++++++++++
> >>   include/uapi/linux/fuse.h |  6 +++++
> >>   2 files changed, 56 insertions(+)
> >>
> >> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> >> index ebd2931b4f2a..df73d9d7e686 100644
> >> --- a/fs/fuse/dev_uring.c
> >> +++ b/fs/fuse/dev_uring.c
> >> @@ -1033,6 +1033,40 @@ fuse_uring_create_ring_ent(struct io_uring_cmd *cmd,
> >>       return ent;
> >>   }
> >>
> >> +/*
> >> + * Register new backing file for passthrough, getting backing map from
> >> URING_CMD data
> >> + */
> >> +static int fuse_uring_backing_open(struct io_uring_cmd *cmd,
> >> +    unsigned int issue_flags, struct fuse_conn *fc)
> >> +{
> >> +    const struct fuse_backing_map *map = io_uring_sqe_cmd(cmd->sqe);
> >> +    int ret = fuse_backing_open(fc, map);
> >> +
> >
> > I am not that familiar with io_uring, so I need to ask -
> > fuse_backing_open() does
> > fb->cred = prepare_creds();
> > to record server credentials
> > what are the credentials that will be recorded in the context of this
> > io_uring command?
>
> This is run from the io_uring_enter() syscall - it should not make
> a difference to an ioctl, AFAIK. Someone from @io-uring please
> correct me if I'm wrong.
>
> >
> >
> >> +    if (ret < 0) {
> >> +        return ret;
> >> +    }
> >> +
> >> +    io_uring_cmd_done(cmd, ret, 0, issue_flags);
> >> +    return 0;
> >> +}
> >> +
> >> +/*
> >> + * Remove file from passthrough tracking, getting backing_id from
> >> URING_CMD data
> >> + */
> >> +static int fuse_uring_backing_close(struct io_uring_cmd *cmd,
> >> +    unsigned int issue_flags, struct fuse_conn *fc)
> >> +{
> >> +    const int *backing_id = io_uring_sqe_cmd(cmd->sqe);
> >> +    int ret = fuse_backing_close(fc, *backing_id);
> >> +
> >> +    if (ret < 0) {
> >> +        return ret;
> >> +    }
> >> +
> >> +    io_uring_cmd_done(cmd, ret, 0, issue_flags);
> >> +    return 0;
> >> +}
> >> +
> >>   /*
> >>    * Register header and payload buffer with the kernel and puts the
> >>    * entry as "ready to get fuse requests" on the queue
> >> @@ -1144,6 +1178,22 @@ int fuse_uring_cmd(struct io_uring_cmd *cmd,
> >> unsigned int issue_flags)
> >>               return err;
> >>           }
> >>           break;
> >> +    case FUSE_IO_URING_CMD_BACKING_OPEN:
> >> +        err = fuse_uring_backing_open(cmd, issue_flags, fc);
> >> +        if (err) {
> >> +            pr_info_once("FUSE_IO_URING_CMD_BACKING_OPEN failed err=%d\n",
> >> +                    err);
> >> +            return err;
> >> +        }
> >> +        break;
> >> +    case FUSE_IO_URING_CMD_BACKING_CLOSE:
> >> +        err = fuse_uring_backing_close(cmd, issue_flags, fc);
> >> +        if (err) {
> >> +            pr_info_once("FUSE_IO_URING_CMD_BACKING_CLOSE failed err=%d\n",
> >> +                    err);
> >> +            return err;
> >> +        }
> >> +        break;
> >>       default:
> >>           return -EINVAL;
> >>       }
> >> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> >> index 5e0eb41d967e..634265da1328 100644
> >> --- a/include/uapi/linux/fuse.h
> >> +++ b/include/uapi/linux/fuse.h
> >> @@ -1264,6 +1264,12 @@ enum fuse_uring_cmd {
> >>
> >>       /* commit fuse request result and fetch next request */
> >>       FUSE_IO_URING_CMD_COMMIT_AND_FETCH = 2,
> >> +
> >> +    /* add new backing file for passthrough */
> >> +    FUSE_IO_URING_CMD_BACKING_OPEN = 3,
> >> +
> >> +    /* remove passthrough file by backing_id */
> >> +    FUSE_IO_URING_CMD_BACKING_CLOSE = 4,
> >>   };
> >>
> >
> > An anecdote:
> > Why are we using FUSE_DEV_IOC_BACKING_OPEN
> > and not passing the backing fd directly in OPEN response?
> >
> > The reason for that was security related - there was a concern that
> > an adversary would be able to trick some process into writing some fd
> > to /dev/fuse, whereas tricking some proces into doing an ioctl is not
> > so realistic.
> >
> > AFAICT this concern does not exist when OPEN response is via
> > io_uring(?), so the backing_id indirection is not strictly needed,
> > but for the sake of uniformity with standard fuse protocol,
> > I guess we should maintain those commands in io_uring as well.
>
> Yeah, the way it is done is not ideal
>
> fi->backing_id = do_passthrough_open(); /* blocking */
> fuse_reply_create()
>     fill_open()
>       arg->backing_id = f->backing_id; /* f is fi */
>
>
> I.e. there are still two operations that depend on each other.
> Maybe we could find a way to link the SQEs.

If we can utilize io_uring infrastructure to link the two
commands it would be best IMO, to keep protocol uniform.

> Or maybe easier, if the security concern is gone with IO-URING,
> just set FOPEN_PASSTHROUGH for requests over io-uring and then
> let the client/kernel side do the passthrough open internally?

It is possible, for example set FOPEN_PASSTHROUGH_FD to
interpret backing_id as backing_fd, but note that in the current
implementation of passthrough_hp, not every open does
fuse_passthrough_open().
The non-first open of an inode uses a backing_id stashed in inode,
from the first open so we'd need different server logic depending on
the commands channel, which is not nice.

Thanks,
Amir.

Powered by blists - more mailing lists