[PATCH] Fuse: Add backing file support for uring_cmd

[PATCH] Fuse: Add backing file support for uring_cmd

[Moinak Bhattacharyya]

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)

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

[Bernd Schubert]

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

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

[Moinak Bhattacharyya]

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

  /**
-- 
2.39.5 (Apple Git-154)

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

[Bernd Schubert]

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

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

[Bernd Schubert]

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

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

[Amir Goldstein]

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.

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

[Amir Goldstein]

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.

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

[Bernd Schubert]

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

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

[Bernd Schubert]

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.

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

[Amir Goldstein]

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.

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

[Bernd Schubert]

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

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

[Moinak Bhattacharyya]

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.

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

[Moinak Bhattacharyya]

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.

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

[Amir Goldstein]

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.

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

[Bernd Schubert]

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

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

[Amir Goldstein]

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.

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

[Moinak Bhattacharyya]

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

That's essentially what registered FD's are, they fget() the FD and 
stash them in an internal data structure. It's not necessarily for I/O 
per se, its more a mechanism to ensure fast Uring access to a given FD.

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

 From my understanding of the previous problem with passthrough, we want 
to make sure that one can't issue arbitrary write() calls to the open 
FUSE FD to sneak in an arbitrary file passthrough (I admit, I don't 
fully understand the concern). In any case, this is obviated by using 
URING-only mechanisms, right?

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

[Moinak Bhattacharyya]

> 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).
Sent out a review for this. IMO even without multiple use sites, the 
static inline helper method seems cleaner to me. I'm ok if we don't want 
it, but I really do think it would make our lives easier.

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

[Miklos Szeredi]

On Fri, 21 Feb 2025 at 19:31, Amir Goldstein <amir73il@gmail.com> wrote:

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

Yeah, the backing-id thing should not be needed for io-uring.

One complaint about the current passthrough API is that it adds extra
syscalls, which is expensive nowadays.

> 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?

Right, this would be the least complex solution.   We could also add
an ioctl(FUSE_DEV_IOC_LOOKUP_REPLY), which would work with the
non-uring API.

Thanks,
Miklos

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

[Pavel Begunkov]

On 2/21/25 17:13, Bernd Schubert wrote:
> On 2/21/25 17:24, Amir Goldstein wrote:
...
>>> +/*
>>> + * 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

That's not necessarily true ...

> a difference to an ioctl, AFAIK. Someone from @io-uring please
> correct me if I'm wrong.

... but it's executed in a context that inherits creds from the
task that submitted the request. It might be trickier if the app
changes creds at runtime, but IIRC the request tries to grab
creds at submission time.

-- 
Pavel Begunkov

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

[Moinak Bhattacharyya]

On 2/24/25 6:08 AM, Miklos Szeredi wrote:

> Right, this would be the least complex solution.   We could also add
> an ioctl(FUSE_DEV_IOC_LOOKUP_REPLY), which would work with the
> non-uring API.

Thia would require a major version tick as we can't mantain back compat, 
unless I'm mistaken? If this is the track we want to take, I'm happy to 
send out another patchset with such a change. Would also need more 
extensive hooking into core FUSE request code.

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

[Miklos Szeredi]

On Mon, 24 Feb 2025 at 17:06, Moinak Bhattacharyya <moinakb001@gmail.com> wrote:

> Thia would require a major version tick as we can't mantain back compat,
> unless I'm mistaken? If this is the track we want to take, I'm happy to
> send out another patchset with such a change. Would also need more
> extensive hooking into core FUSE request code.

I thought we were discussing backing file for FUSE_LOOKUP?  It doesn't
exist yet, so no backward compat issues.

Thanks,
Miklos