Re: [PATCH] Fuse: Add backing file support for uring_cmd

[Amir Goldstein]

On Fri, Feb 21, 2025 at 6:51 PM Bernd Schubert <bernd@xxxxxxxxxxx> wrote:
>
>
>
> On 2/21/25 18:25, Amir Goldstein wrote:
> > 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.
>
> Probably, but I especially added fuse_req_is_uring() to the API
> to be able to do that. For example to avoid another memcpy when passing
> buffers to another thread.
>

I understand sometimes the server will need to have slightly different logic
depending on the channel, but in this case I think that should be avoided.
If there is an option to link the CMD_BACKING_OPEN with the commit of
OPEN result and back the backing_id for the server, that would be best.

BTW, I am now trying to work out the API for setting up a backing file
for an inode at LOOKUP time for passthrough of inode operations.
For this mode of operation, I was considering to support OPEN
response with FOPEN_PASSTHROUGH and zero backing_id to mean
"the backing file that is associated with the inode".
I've actually reserved backing_id 0 for this purpose.
In this mode of operations the problem at hand will become moot.

One way to deal with the API of FOPEN_PASSTHROUGH in
io_uring is to only use this mode of operation.
IOW, LOOKUP response could have a backing fd and not
a backing id and then the backing ids are not even exposed to
server because the server does not care - for all practical purposes
the nodeid is the backing id.

I personally don't mind if inode operations passthrough
that are setup via LOOKUP response, will require io_uring.
Both features are about metadata operations performance,
so it kind of makes sense to bundle them together, does it not?

Thanks,
Amir.