On Fri, Feb 21, 2025 at 6:13 PM Bernd Schubert <bernd@xxxxxxxxxxx> wrote:
>
>
>
> On 2/21/25 17:24, Amir Goldstein wrote:
> > On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
> > <moinakb001@xxxxxxxxx> 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.
