diff mbox series

Fuse: Add backing file support for uring_cmd

Message ID CAKXrOwbkMUo9KJd7wHjcFzJieTFj6NPWPp0vD_SgdS3h33Wdsg@mail.gmail.com (mailing list archive)
State New
Headers show
Series Fuse: Add backing file support for uring_cmd | expand

Commit Message

Moinak Bhattacharyya Feb. 21, 2025, 3:19 p.m. UTC 
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(+)

--
2.39.5 (Apple Git-154)

Comments

Bernd Schubert Feb. 21, 2025, 3:24 p.m. UTC | #1 
On 2/21/25 16:19, Moinak Bhattacharyya wrote:
> 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);
> +
> + 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,
>  };
> 
>  /**
> --
> 2.39.5 (Apple Git-154)
> 

This is hard to read - all formatting got lost? Other than that looks
reasonable.


Thanks,
Bernd
Moinak Bhattacharyya Feb. 21, 2025, 3:36 p.m. UTC | #2 
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);
+
+    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,
  };

  /**
Bernd Schubert Feb. 21, 2025, 4:14 p.m. UTC | #3 
On 2/21/25 16:36, Moinak Bhattacharyya 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);

Do you have the libfuse part somewhere? I need to hurry up to split and
clean up my uring branch. Not promised, but maybe this weekend. 
What we need to be careful here about is that in my current 'uring'
libfuse always expects to get a CQE - here you introduce a 2nd user
for CQEs - it needs credit management.


> +
> +    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;
> +    }


Both functions don't have the check for 

	if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
		return -EOPNOTSUPP;

but their ioctl counter parts have that.


Thanks,
Bernd
Bernd Schubert Feb. 21, 2025, 4:17 p.m. UTC | #4 
On 2/21/25 17:14, Bernd Schubert wrote:
> 
> 
> On 2/21/25 16:36, Moinak Bhattacharyya 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);
> 
> Do you have the libfuse part somewhere? I need to hurry up to split and
> clean up my uring branch. Not promised, but maybe this weekend. 
> What we need to be careful here about is that in my current 'uring'
> libfuse always expects to get a CQE - here you introduce a 2nd user
> for CQEs - it needs credit management.
> 
> 
>> +
>> +    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;
>> +    }
> 
> 
> Both functions don't have the check for 
> 
> 	if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> 		return -EOPNOTSUPP;
> 
> but their ioctl counter parts have that.
> 

In order to avoid code dup, maybe that check could be moved
into fuse_backing_open() / fuse_backing_close() as preparation
patch? Amir?

Thanks,
Bernd
Amir Goldstein Feb. 21, 2025, 4:24 p.m. UTC | #5 
On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
<moinakb001@gmail.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?


> +    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.

Thanks,
Amir.
Amir Goldstein Feb. 21, 2025, 4:35 p.m. UTC | #6 
On Fri, Feb 21, 2025 at 5:17 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>
>
>
> On 2/21/25 17:14, Bernd Schubert wrote:
> >
> >
> > On 2/21/25 16:36, Moinak Bhattacharyya 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);
> >
> > Do you have the libfuse part somewhere? I need to hurry up to split and
> > clean up my uring branch. Not promised, but maybe this weekend.
> > What we need to be careful here about is that in my current 'uring'
> > libfuse always expects to get a CQE - here you introduce a 2nd user
> > for CQEs - it needs credit management.
> >
> >
> >> +
> >> +    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;
> >> +    }
> >
> >
> > Both functions don't have the check for
> >
> >       if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
> >               return -EOPNOTSUPP;
> >
> > but their ioctl counter parts have that.
> >
>
> In order to avoid code dup, maybe that check could be moved
> into fuse_backing_open() / fuse_backing_close() as preparation
> patch? Amir?

Without CONFIG_FUSE_PASSTHROUGH, fuse/passthrough.c
is compiled out, so the check cannot be moved into fuse_backing_*
we'd need inline helpers that return -EOPNOTSUPP when
CONFIG_FUSE_PASSTHROUGH is not defined.
I don't mind, but I am not sure this is justified (yet).

Thanks,
Amir.
Bernd Schubert Feb. 21, 2025, 5:13 p.m. UTC | #7 
On 2/21/25 17:24, Amir Goldstein wrote:
> On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
> <moinakb001@gmail.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.
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?


Thanks,
Bernd
Bernd Schubert Feb. 21, 2025, 5:24 p.m. UTC | #8 
On 2/21/25 17:35, Amir Goldstein wrote:
> On Fri, Feb 21, 2025 at 5:17 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>>
>>
>>
>> On 2/21/25 17:14, Bernd Schubert wrote:
>>>
>>>
>>> On 2/21/25 16:36, Moinak Bhattacharyya 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);
>>>
>>> Do you have the libfuse part somewhere? I need to hurry up to split and
>>> clean up my uring branch. Not promised, but maybe this weekend.
>>> What we need to be careful here about is that in my current 'uring'
>>> libfuse always expects to get a CQE - here you introduce a 2nd user
>>> for CQEs - it needs credit management.
>>>
>>>
>>>> +
>>>> +    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;
>>>> +    }
>>>
>>>
>>> Both functions don't have the check for
>>>
>>>       if (!IS_ENABLED(CONFIG_FUSE_PASSTHROUGH))
>>>               return -EOPNOTSUPP;
>>>
>>> but their ioctl counter parts have that.
>>>
>>
>> In order to avoid code dup, maybe that check could be moved
>> into fuse_backing_open() / fuse_backing_close() as preparation
>> patch? Amir?
> 
> Without CONFIG_FUSE_PASSTHROUGH, fuse/passthrough.c
> is compiled out, so the check cannot be moved into fuse_backing_*
> we'd need inline helpers that return -EOPNOTSUPP when
> CONFIG_FUSE_PASSTHROUGH is not defined.
> I don't mind, but I am not sure this is justified (yet).
> 

Ah right, then let's duplicate the check.
Amir Goldstein Feb. 21, 2025, 5:25 p.m. UTC | #9 
On Fri, Feb 21, 2025 at 6:13 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>
>
>
> On 2/21/25 17:24, Amir Goldstein wrote:
> > On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
> > <moinakb001@gmail.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.
Bernd Schubert Feb. 21, 2025, 5:44 p.m. UTC | #10 
On 2/21/25 18:25, Amir Goldstein wrote:
> On Fri, Feb 21, 2025 at 6:13 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>>
>>
>>
>> On 2/21/25 17:24, Amir Goldstein wrote:
>>> On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
>>> <moinakb001@gmail.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
Moinak Bhattacharyya Feb. 21, 2025, 6:13 p.m. UTC | #11 
I don't have the modifications to libfuse. What tree are you using for 
the uring modifications? I dont see any uring patches on the latest 
master liburing.
>> 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.
I wonder if we can just require URING registered FDs (using 
IORING_REGISTER_FILES). I think io_uring does checks on the file 
permissions when the FD is registered.
Moinak Bhattacharyya Feb. 21, 2025, 6:14 p.m. UTC | #12 
s/liburing/libfuse/

On 2/21/25 12:13 PM, Moinak Bhattacharyya wrote:
> I don't have the modifications to libfuse. What tree are you using for 
> the uring modifications? I dont see any uring patches on the latest 
> master liburing.
>>> 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.
> I wonder if we can just require URING registered FDs (using 
> IORING_REGISTER_FILES). I think io_uring does checks on the file 
> permissions when the FD is registered.
Amir Goldstein Feb. 21, 2025, 6:21 p.m. UTC | #13 
On Fri, Feb 21, 2025 at 7:13 PM Moinak Bhattacharyya
<moinakb001@gmail.com> wrote:
>
> I don't have the modifications to libfuse. What tree are you using for
> the uring modifications? I dont see any uring patches on the latest
> master liburing.
> >> 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.
> I wonder if we can just require URING registered FDs (using
> IORING_REGISTER_FILES). I think io_uring does checks on the file
> permissions when the FD is registered.

That's an interesting idea.
There are definitely similarities between IORING_REGISTER_FILES
and registering backing ids.

There is however one difference, which is going to be even more
emphasised when backing files are setup during LOOKUP -
The backing fd setup during BACKING_OPEN does not need to
be open for write - it could even be an O_PATH fd.

So fc->backing_files_map are not really fds registered for IO,
they are essential references to backing inodes.

Thanks,
Amir.
Bernd Schubert Feb. 21, 2025, 6:23 p.m. UTC | #14 
On 2/21/25 19:13, Moinak Bhattacharyya wrote:
> I don't have the modifications to libfuse. What tree are you using for
> the uring modifications? I dont see any uring patches on the latest
> master liburing.

https://github.com/bsbernd/libfuse/tree/uring

This is a development branch, goint to create a new branch out
of that during the next days (now that I'm eventually almost through
with libfuse-3.17).

>>> 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.
> I wonder if we can just require URING registered FDs (using
> IORING_REGISTER_FILES). I think io_uring does checks on the file
> permissions when the FD is registered.

Could you explain how fd registration into the ring would help here?


Thanks,
Bernd
Amir Goldstein Feb. 21, 2025, 6:31 p.m. UTC | #15 
On Fri, Feb 21, 2025 at 6:51 PM Bernd Schubert <bernd@bsbernd.com> wrote:
>
>
>
> On 2/21/25 18:25, Amir Goldstein wrote:
> > On Fri, Feb 21, 2025 at 6:13 PM Bernd Schubert <bernd@bsbernd.com> wrote:
> >>
> >>
> >>
> >> On 2/21/25 17:24, Amir Goldstein wrote:
> >>> On Fri, Feb 21, 2025 at 4:36 PM Moinak Bhattacharyya
> >>> <moinakb001@gmail.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.
>

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.
diff mbox series

Patch

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);
+
+ 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,
 };

 /**