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.
Thanks,
Bernd
