Message-ID: <2d9f56ae-7344-4f82-b5da-61522543ef4f@bsbernd.com>
Date: Fri, 21 Feb 2025 18:44:59 +0100
From: Bernd Schubert <bernd@...ernd.com>
To: Amir Goldstein <amir73il@...il.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 2/21/25 18:25, Amir Goldstein wrote:
> 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.

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

Powered by blists - more mailing lists