[PATCH v1] fuse: use splice for reading user pages on servers that enable it

[PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Joanne Koong]

For direct io writes, splice is disabled when forwarding pages from the
client to the server. This is because the pages in the pipe buffer are
user pages, which is controlled by the client. Thus if a server replies
to the request and then keeps accessing the pages afterwards, there is
the possibility that the client may have modified the contents of the
pages in the meantime. More context on this can be found in commit
0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").

For servers that do not need to access pages after answering the
request, splice gives a non-trivial improvement in performance.
Benchmarks show roughly a 40% speedup.

Allow servers to enable zero-copy splice for servicing client direct io
writes. By enabling this, the server understands that they should not
continue accessing the pipe buffer after completing the request or may
face incorrect data if they do so.

Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 fs/fuse/dev.c             | 18 ++++++++++--------
 fs/fuse/dev_uring.c       |  4 ++--
 fs/fuse/fuse_dev_i.h      |  5 +++--
 fs/fuse/fuse_i.h          |  3 +++
 fs/fuse/inode.c           |  5 ++++-
 include/uapi/linux/fuse.h |  8 ++++++++
 6 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 67d07b4c778a..1b0ea8593f74 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
 	return err;
 }
 
-void fuse_copy_init(struct fuse_copy_state *cs, bool write,
-		    struct iov_iter *iter)
+void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
+		    bool write, struct iov_iter *iter)
 {
 	memset(cs, 0, sizeof(*cs));
 	cs->write = write;
 	cs->iter = iter;
+	cs->splice_read_user_pages = fc->splice_read_user_pages;
 }
 
 /* Unmap and put previous page of userspace buffer */
@@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
 		if (cs->write && cs->pipebufs && page) {
 			/*
 			 * Can't control lifetime of pipe buffers, so always
-			 * copy user pages.
+			 * copy user pages if server does not support splice
+			 * for reading user pages.
 			 */
-			if (cs->req->args->user_pages) {
+			if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
 				err = fuse_copy_fill(cs);
 				if (err)
 					return err;
@@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
 	if (!user_backed_iter(to))
 		return -EINVAL;
 
-	fuse_copy_init(&cs, true, to);
+	fuse_copy_init(&cs, fud->fc, true, to);
 
 	return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
 }
@@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
 	if (!bufs)
 		return -ENOMEM;
 
-	fuse_copy_init(&cs, true, NULL);
+	fuse_copy_init(&cs, fud->fc, true, NULL);
 	cs.pipebufs = bufs;
 	cs.pipe = pipe;
 	ret = fuse_dev_do_read(fud, in, &cs, len);
@@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
 	if (!user_backed_iter(from))
 		return -EINVAL;
 
-	fuse_copy_init(&cs, false, from);
+	fuse_copy_init(&cs, fud->fc, false, from);
 
 	return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
 }
@@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
 	}
 	pipe_unlock(pipe);
 
-	fuse_copy_init(&cs, false, NULL);
+	fuse_copy_init(&cs, fud->fc, false, NULL);
 	cs.pipebufs = bufs;
 	cs.nr_segs = nbuf;
 	cs.pipe = pipe;
diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
index ef470c4a9261..52b883a6a79d 100644
--- a/fs/fuse/dev_uring.c
+++ b/fs/fuse/dev_uring.c
@@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
 	if (err)
 		return err;
 
-	fuse_copy_init(&cs, false, &iter);
+	fuse_copy_init(&cs, ring->fc, false, &iter);
 	cs.is_uring = true;
 	cs.req = req;
 
@@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
 		return err;
 	}
 
-	fuse_copy_init(&cs, true, &iter);
+	fuse_copy_init(&cs, ring->fc, true, &iter);
 	cs.is_uring = true;
 	cs.req = req;
 
diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
index db136e045925..25e593e64c67 100644
--- a/fs/fuse/fuse_dev_i.h
+++ b/fs/fuse/fuse_dev_i.h
@@ -32,6 +32,7 @@ struct fuse_copy_state {
 	bool write:1;
 	bool move_pages:1;
 	bool is_uring:1;
+	bool splice_read_user_pages:1;
 	struct {
 		unsigned int copied_sz; /* copied size into the user buffer */
 	} ring;
@@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
 
 void fuse_dev_end_requests(struct list_head *head);
 
-void fuse_copy_init(struct fuse_copy_state *cs, bool write,
-			   struct iov_iter *iter);
+void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
+		    bool write, struct iov_iter *iter);
 int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
 		   unsigned int argpages, struct fuse_arg *args,
 		   int zeroing);
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 3d5289cb82a5..e21875f16220 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -898,6 +898,9 @@ struct fuse_conn {
 	/* Use io_uring for communication */
 	bool io_uring:1;
 
+	/* Allow splice for reading user pages */
+	bool splice_read_user_pages:1;
+
 	/** Maximum stack depth for passthrough backing files */
 	int max_stack_depth;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 43b6643635ee..e82e96800fde 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 
 			if (flags & FUSE_REQUEST_TIMEOUT)
 				timeout = arg->request_timeout;
+
+			if (flags & FUSE_SPLICE_READ_USER_PAGES)
+				fc->splice_read_user_pages = true;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = true;
@@ -1489,7 +1492,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
 		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
 		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND | FUSE_ALLOW_IDMAP |
-		FUSE_REQUEST_TIMEOUT;
+		FUSE_REQUEST_TIMEOUT | FUSE_SPLICE_READ_USER_PAGES;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index 122d6586e8d4..b35f6bbcb322 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -235,6 +235,9 @@
  *
  *  7.44
  *  - add FUSE_NOTIFY_INC_EPOCH
+ *
+ *  7.45
+ *  - add FUSE_SPLICE_READ_USER_PAGES
  */
 
 #ifndef _LINUX_FUSE_H
@@ -443,6 +446,10 @@ struct fuse_file_lock {
  * FUSE_OVER_IO_URING: Indicate that client supports io-uring
  * FUSE_REQUEST_TIMEOUT: kernel supports timing out requests.
  *			 init_out.request_timeout contains the timeout (in secs)
+ * FUSE_SPLICE_READ_USER_PAGES: kernel supports splice on the device for reading
+ *				user pages. If the server enables this, then the
+ *				server should not access the pipe buffer after
+ *				completing the request.
  */
 #define FUSE_ASYNC_READ		(1 << 0)
 #define FUSE_POSIX_LOCKS	(1 << 1)
@@ -490,6 +497,7 @@ struct fuse_file_lock {
 #define FUSE_ALLOW_IDMAP	(1ULL << 40)
 #define FUSE_OVER_IO_URING	(1ULL << 41)
 #define FUSE_REQUEST_TIMEOUT	(1ULL << 42)
+#define FUSE_SPLICE_READ_USER_PAGES (1ULL << 43)
 
 /**
  * CUSE INIT request/reply flags
-- 
2.47.1

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Bernd Schubert]

On 4/19/25 02:06, Joanne Koong wrote:
> For direct io writes, splice is disabled when forwarding pages from the
> client to the server. This is because the pages in the pipe buffer are
> user pages, which is controlled by the client. Thus if a server replies
> to the request and then keeps accessing the pages afterwards, there is
> the possibility that the client may have modified the contents of the
> pages in the meantime. More context on this can be found in commit
> 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
> 
> For servers that do not need to access pages after answering the
> request, splice gives a non-trivial improvement in performance.
> Benchmarks show roughly a 40% speedup.
> 
> Allow servers to enable zero-copy splice for servicing client direct io
> writes. By enabling this, the server understands that they should not
> continue accessing the pipe buffer after completing the request or may
> face incorrect data if they do so.
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> ---
>  fs/fuse/dev.c             | 18 ++++++++++--------
>  fs/fuse/dev_uring.c       |  4 ++--
>  fs/fuse/fuse_dev_i.h      |  5 +++--
>  fs/fuse/fuse_i.h          |  3 +++
>  fs/fuse/inode.c           |  5 ++++-
>  include/uapi/linux/fuse.h |  8 ++++++++
>  6 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 67d07b4c778a..1b0ea8593f74 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
>  	return err;
>  }
>  
> -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> -		    struct iov_iter *iter)
> +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
> +		    bool write, struct iov_iter *iter)
>  {
>  	memset(cs, 0, sizeof(*cs));
>  	cs->write = write;
>  	cs->iter = iter;
> +	cs->splice_read_user_pages = fc->splice_read_user_pages;
>  }
>  
>  /* Unmap and put previous page of userspace buffer */
> @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
>  		if (cs->write && cs->pipebufs && page) {
>  			/*
>  			 * Can't control lifetime of pipe buffers, so always
> -			 * copy user pages.
> +			 * copy user pages if server does not support splice
> +			 * for reading user pages.
>  			 */
> -			if (cs->req->args->user_pages) {
> +			if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
>  				err = fuse_copy_fill(cs);
>  				if (err)
>  					return err;
> @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
>  	if (!user_backed_iter(to))
>  		return -EINVAL;
>  
> -	fuse_copy_init(&cs, true, to);
> +	fuse_copy_init(&cs, fud->fc, true, to);
>  
>  	return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
>  }
> @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
>  	if (!bufs)
>  		return -ENOMEM;
>  
> -	fuse_copy_init(&cs, true, NULL);
> +	fuse_copy_init(&cs, fud->fc, true, NULL);
>  	cs.pipebufs = bufs;
>  	cs.pipe = pipe;
>  	ret = fuse_dev_do_read(fud, in, &cs, len);
> @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
>  	if (!user_backed_iter(from))
>  		return -EINVAL;
>  
> -	fuse_copy_init(&cs, false, from);
> +	fuse_copy_init(&cs, fud->fc, false, from);
>  
>  	return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
>  }
> @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
>  	}
>  	pipe_unlock(pipe);
>  
> -	fuse_copy_init(&cs, false, NULL);
> +	fuse_copy_init(&cs, fud->fc, false, NULL);
>  	cs.pipebufs = bufs;
>  	cs.nr_segs = nbuf;
>  	cs.pipe = pipe;
> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> index ef470c4a9261..52b883a6a79d 100644
> --- a/fs/fuse/dev_uring.c
> +++ b/fs/fuse/dev_uring.c
> @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
>  	if (err)
>  		return err;
>  
> -	fuse_copy_init(&cs, false, &iter);
> +	fuse_copy_init(&cs, ring->fc, false, &iter);
>  	cs.is_uring = true;
>  	cs.req = req;
>  
> @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
>  		return err;
>  	}
>  
> -	fuse_copy_init(&cs, true, &iter);
> +	fuse_copy_init(&cs, ring->fc, true, &iter);
>  	cs.is_uring = true;
>  	cs.req = req;
>  
> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> index db136e045925..25e593e64c67 100644
> --- a/fs/fuse/fuse_dev_i.h
> +++ b/fs/fuse/fuse_dev_i.h
> @@ -32,6 +32,7 @@ struct fuse_copy_state {
>  	bool write:1;
>  	bool move_pages:1;
>  	bool is_uring:1;
> +	bool splice_read_user_pages:1;
>  	struct {
>  		unsigned int copied_sz; /* copied size into the user buffer */
>  	} ring;
> @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
>  
>  void fuse_dev_end_requests(struct list_head *head);
>  
> -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> -			   struct iov_iter *iter);
> +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
> +		    bool write, struct iov_iter *iter);
>  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
>  		   unsigned int argpages, struct fuse_arg *args,
>  		   int zeroing);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 3d5289cb82a5..e21875f16220 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -898,6 +898,9 @@ struct fuse_conn {
>  	/* Use io_uring for communication */
>  	bool io_uring:1;
>  
> +	/* Allow splice for reading user pages */
> +	bool splice_read_user_pages:1;
> +
>  	/** Maximum stack depth for passthrough backing files */
>  	int max_stack_depth;
>  
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index 43b6643635ee..e82e96800fde 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>  
>  			if (flags & FUSE_REQUEST_TIMEOUT)
>  				timeout = arg->request_timeout;
> +
> +			if (flags & FUSE_SPLICE_READ_USER_PAGES)
> +				fc->splice_read_user_pages = true;


Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
that one can access file content although the write is already
marked as completed? I.e. a fuse file system might get data
it was never exposed to and possibly secret data?
A more complex version version could check for CAP_SYS_ADMIN, but
also allow later on read/write to files that have the same uid as
the fuser-server process?


Thanks,
Bernd


>  		} else {
>  			ra_pages = fc->max_read / PAGE_SIZE;
>  			fc->no_lock = true;
> @@ -1489,7 +1492,7 @@ void fuse_send_init(struct fuse_mount *fm)
>  		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
>  		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
>  		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND | FUSE_ALLOW_IDMAP |
> -		FUSE_REQUEST_TIMEOUT;
> +		FUSE_REQUEST_TIMEOUT | FUSE_SPLICE_READ_USER_PAGES;
>  #ifdef CONFIG_FUSE_DAX
>  	if (fm->fc->dax)
>  		flags |= FUSE_MAP_ALIGNMENT;
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index 122d6586e8d4..b35f6bbcb322 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -235,6 +235,9 @@
>   *
>   *  7.44
>   *  - add FUSE_NOTIFY_INC_EPOCH
> + *
> + *  7.45
> + *  - add FUSE_SPLICE_READ_USER_PAGES
>   */
>  
>  #ifndef _LINUX_FUSE_H
> @@ -443,6 +446,10 @@ struct fuse_file_lock {
>   * FUSE_OVER_IO_URING: Indicate that client supports io-uring
>   * FUSE_REQUEST_TIMEOUT: kernel supports timing out requests.
>   *			 init_out.request_timeout contains the timeout (in secs)
> + * FUSE_SPLICE_READ_USER_PAGES: kernel supports splice on the device for reading
> + *				user pages. If the server enables this, then the
> + *				server should not access the pipe buffer after
> + *				completing the request.
>   */
>  #define FUSE_ASYNC_READ		(1 << 0)
>  #define FUSE_POSIX_LOCKS	(1 << 1)
> @@ -490,6 +497,7 @@ struct fuse_file_lock {
>  #define FUSE_ALLOW_IDMAP	(1ULL << 40)
>  #define FUSE_OVER_IO_URING	(1ULL << 41)
>  #define FUSE_REQUEST_TIMEOUT	(1ULL << 42)
> +#define FUSE_SPLICE_READ_USER_PAGES (1ULL << 43)
>  
>  /**
>   * CUSE INIT request/reply flags

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Jeff Layton]

On Mon, 2025-04-21 at 13:49 +0200, Bernd Schubert wrote:
> 
> On 4/19/25 02:06, Joanne Koong wrote:
> > For direct io writes, splice is disabled when forwarding pages from the
> > client to the server. This is because the pages in the pipe buffer are
> > user pages, which is controlled by the client. Thus if a server replies
> > to the request and then keeps accessing the pages afterwards, there is
> > the possibility that the client may have modified the contents of the
> > pages in the meantime. More context on this can be found in commit
> > 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
> > 
> > For servers that do not need to access pages after answering the
> > request, splice gives a non-trivial improvement in performance.
> > Benchmarks show roughly a 40% speedup.
> > 
> > Allow servers to enable zero-copy splice for servicing client direct io
> > writes. By enabling this, the server understands that they should not
> > continue accessing the pipe buffer after completing the request or may
> > face incorrect data if they do so.
> > 
> > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > ---
> >  fs/fuse/dev.c             | 18 ++++++++++--------
> >  fs/fuse/dev_uring.c       |  4 ++--
> >  fs/fuse/fuse_dev_i.h      |  5 +++--
> >  fs/fuse/fuse_i.h          |  3 +++
> >  fs/fuse/inode.c           |  5 ++++-
> >  include/uapi/linux/fuse.h |  8 ++++++++
> >  6 files changed, 30 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > index 67d07b4c778a..1b0ea8593f74 100644
> > --- a/fs/fuse/dev.c
> > +++ b/fs/fuse/dev.c
> > @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
> >  	return err;
> >  }
> >  
> > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > -		    struct iov_iter *iter)
> > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
> > +		    bool write, struct iov_iter *iter)
> >  {
> >  	memset(cs, 0, sizeof(*cs));
> >  	cs->write = write;
> >  	cs->iter = iter;
> > +	cs->splice_read_user_pages = fc->splice_read_user_pages;
> >  }
> >  
> >  /* Unmap and put previous page of userspace buffer */
> > @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
> >  		if (cs->write && cs->pipebufs && page) {
> >  			/*
> >  			 * Can't control lifetime of pipe buffers, so always
> > -			 * copy user pages.
> > +			 * copy user pages if server does not support splice
> > +			 * for reading user pages.
> >  			 */
> > -			if (cs->req->args->user_pages) {
> > +			if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
> >  				err = fuse_copy_fill(cs);
> >  				if (err)
> >  					return err;
> > @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
> >  	if (!user_backed_iter(to))
> >  		return -EINVAL;
> >  
> > -	fuse_copy_init(&cs, true, to);
> > +	fuse_copy_init(&cs, fud->fc, true, to);
> >  
> >  	return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
> >  }
> > @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> >  	if (!bufs)
> >  		return -ENOMEM;
> >  
> > -	fuse_copy_init(&cs, true, NULL);
> > +	fuse_copy_init(&cs, fud->fc, true, NULL);
> >  	cs.pipebufs = bufs;
> >  	cs.pipe = pipe;
> >  	ret = fuse_dev_do_read(fud, in, &cs, len);
> > @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
> >  	if (!user_backed_iter(from))
> >  		return -EINVAL;
> >  
> > -	fuse_copy_init(&cs, false, from);
> > +	fuse_copy_init(&cs, fud->fc, false, from);
> >  
> >  	return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
> >  }
> > @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> >  	}
> >  	pipe_unlock(pipe);
> >  
> > -	fuse_copy_init(&cs, false, NULL);
> > +	fuse_copy_init(&cs, fud->fc, false, NULL);
> >  	cs.pipebufs = bufs;
> >  	cs.nr_segs = nbuf;
> >  	cs.pipe = pipe;
> > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > index ef470c4a9261..52b883a6a79d 100644
> > --- a/fs/fuse/dev_uring.c
> > +++ b/fs/fuse/dev_uring.c
> > @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
> >  	if (err)
> >  		return err;
> >  
> > -	fuse_copy_init(&cs, false, &iter);
> > +	fuse_copy_init(&cs, ring->fc, false, &iter);
> >  	cs.is_uring = true;
> >  	cs.req = req;
> >  
> > @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
> >  		return err;
> >  	}
> >  
> > -	fuse_copy_init(&cs, true, &iter);
> > +	fuse_copy_init(&cs, ring->fc, true, &iter);
> >  	cs.is_uring = true;
> >  	cs.req = req;
> >  
> > diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> > index db136e045925..25e593e64c67 100644
> > --- a/fs/fuse/fuse_dev_i.h
> > +++ b/fs/fuse/fuse_dev_i.h
> > @@ -32,6 +32,7 @@ struct fuse_copy_state {
> >  	bool write:1;
> >  	bool move_pages:1;
> >  	bool is_uring:1;
> > +	bool splice_read_user_pages:1;
> >  	struct {
> >  		unsigned int copied_sz; /* copied size into the user buffer */
> >  	} ring;
> > @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
> >  
> >  void fuse_dev_end_requests(struct list_head *head);
> >  
> > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > -			   struct iov_iter *iter);
> > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
> > +		    bool write, struct iov_iter *iter);
> >  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
> >  		   unsigned int argpages, struct fuse_arg *args,
> >  		   int zeroing);
> > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > index 3d5289cb82a5..e21875f16220 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -898,6 +898,9 @@ struct fuse_conn {
> >  	/* Use io_uring for communication */
> >  	bool io_uring:1;
> >  
> > +	/* Allow splice for reading user pages */
> > +	bool splice_read_user_pages:1;
> > +
> >  	/** Maximum stack depth for passthrough backing files */
> >  	int max_stack_depth;
> >  
> > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > index 43b6643635ee..e82e96800fde 100644
> > --- a/fs/fuse/inode.c
> > +++ b/fs/fuse/inode.c
> > @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> >  
> >  			if (flags & FUSE_REQUEST_TIMEOUT)
> >  				timeout = arg->request_timeout;
> > +
> > +			if (flags & FUSE_SPLICE_READ_USER_PAGES)
> > +				fc->splice_read_user_pages = true;
> 
> 
> Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
> that one can access file content although the write is already
> marked as completed? I.e. a fuse file system might get data
> it was never exposed to and possibly secret data?
> A more complex version version could check for CAP_SYS_ADMIN, but
> also allow later on read/write to files that have the same uid as
> the fuser-server process?
> 

IDGI. I don't see how this allows the server access to something it
didn't have access to before.

This patchset seems to be about a "contract" between the kernel and the
userland server. The server is agreeing to be very careful about not
touching pages after a write request completes, and the kernel allows
splicing the pages if that's the case.

Can you explain the potential attack vector?


> 
> >  		} else {
> >  			ra_pages = fc->max_read / PAGE_SIZE;
> >  			fc->no_lock = true;
> > @@ -1489,7 +1492,7 @@ void fuse_send_init(struct fuse_mount *fm)
> >  		FUSE_SECURITY_CTX | FUSE_CREATE_SUPP_GROUP |
> >  		FUSE_HAS_EXPIRE_ONLY | FUSE_DIRECT_IO_ALLOW_MMAP |
> >  		FUSE_NO_EXPORT_SUPPORT | FUSE_HAS_RESEND | FUSE_ALLOW_IDMAP |
> > -		FUSE_REQUEST_TIMEOUT;
> > +		FUSE_REQUEST_TIMEOUT | FUSE_SPLICE_READ_USER_PAGES;
> >  #ifdef CONFIG_FUSE_DAX
> >  	if (fm->fc->dax)
> >  		flags |= FUSE_MAP_ALIGNMENT;
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index 122d6586e8d4..b35f6bbcb322 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -235,6 +235,9 @@
> >   *
> >   *  7.44
> >   *  - add FUSE_NOTIFY_INC_EPOCH
> > + *
> > + *  7.45
> > + *  - add FUSE_SPLICE_READ_USER_PAGES
> >   */
> >  
> >  #ifndef _LINUX_FUSE_H
> > @@ -443,6 +446,10 @@ struct fuse_file_lock {
> >   * FUSE_OVER_IO_URING: Indicate that client supports io-uring
> >   * FUSE_REQUEST_TIMEOUT: kernel supports timing out requests.
> >   *			 init_out.request_timeout contains the timeout (in secs)
> > + * FUSE_SPLICE_READ_USER_PAGES: kernel supports splice on the device for reading
> > + *				user pages. If the server enables this, then the
> > + *				server should not access the pipe buffer after
> > + *				completing the request.
> >   */
> >  #define FUSE_ASYNC_READ		(1 << 0)
> >  #define FUSE_POSIX_LOCKS	(1 << 1)
> > @@ -490,6 +497,7 @@ struct fuse_file_lock {
> >  #define FUSE_ALLOW_IDMAP	(1ULL << 40)
> >  #define FUSE_OVER_IO_URING	(1ULL << 41)
> >  #define FUSE_REQUEST_TIMEOUT	(1ULL << 42)
> > +#define FUSE_SPLICE_READ_USER_PAGES (1ULL << 43)
> >  
> >  /**
> >   * CUSE INIT request/reply flags
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Bernd Schubert]

On 4/21/25 14:35, Jeff Layton wrote:
> On Mon, 2025-04-21 at 13:49 +0200, Bernd Schubert wrote:
>>
>> On 4/19/25 02:06, Joanne Koong wrote:
>>> For direct io writes, splice is disabled when forwarding pages from the
>>> client to the server. This is because the pages in the pipe buffer are
>>> user pages, which is controlled by the client. Thus if a server replies
>>> to the request and then keeps accessing the pages afterwards, there is
>>> the possibility that the client may have modified the contents of the
>>> pages in the meantime. More context on this can be found in commit
>>> 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
>>>
>>> For servers that do not need to access pages after answering the
>>> request, splice gives a non-trivial improvement in performance.
>>> Benchmarks show roughly a 40% speedup.
>>>
>>> Allow servers to enable zero-copy splice for servicing client direct io
>>> writes. By enabling this, the server understands that they should not
>>> continue accessing the pipe buffer after completing the request or may
>>> face incorrect data if they do so.
>>>
>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>> ---
>>>  fs/fuse/dev.c             | 18 ++++++++++--------
>>>  fs/fuse/dev_uring.c       |  4 ++--
>>>  fs/fuse/fuse_dev_i.h      |  5 +++--
>>>  fs/fuse/fuse_i.h          |  3 +++
>>>  fs/fuse/inode.c           |  5 ++++-
>>>  include/uapi/linux/fuse.h |  8 ++++++++
>>>  6 files changed, 30 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
>>> index 67d07b4c778a..1b0ea8593f74 100644
>>> --- a/fs/fuse/dev.c
>>> +++ b/fs/fuse/dev.c
>>> @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
>>>  	return err;
>>>  }
>>>  
>>> -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
>>> -		    struct iov_iter *iter)
>>> +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
>>> +		    bool write, struct iov_iter *iter)
>>>  {
>>>  	memset(cs, 0, sizeof(*cs));
>>>  	cs->write = write;
>>>  	cs->iter = iter;
>>> +	cs->splice_read_user_pages = fc->splice_read_user_pages;
>>>  }
>>>  
>>>  /* Unmap and put previous page of userspace buffer */
>>> @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
>>>  		if (cs->write && cs->pipebufs && page) {
>>>  			/*
>>>  			 * Can't control lifetime of pipe buffers, so always
>>> -			 * copy user pages.
>>> +			 * copy user pages if server does not support splice
>>> +			 * for reading user pages.
>>>  			 */
>>> -			if (cs->req->args->user_pages) {
>>> +			if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
>>>  				err = fuse_copy_fill(cs);
>>>  				if (err)
>>>  					return err;
>>> @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
>>>  	if (!user_backed_iter(to))
>>>  		return -EINVAL;
>>>  
>>> -	fuse_copy_init(&cs, true, to);
>>> +	fuse_copy_init(&cs, fud->fc, true, to);
>>>  
>>>  	return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
>>>  }
>>> @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
>>>  	if (!bufs)
>>>  		return -ENOMEM;
>>>  
>>> -	fuse_copy_init(&cs, true, NULL);
>>> +	fuse_copy_init(&cs, fud->fc, true, NULL);
>>>  	cs.pipebufs = bufs;
>>>  	cs.pipe = pipe;
>>>  	ret = fuse_dev_do_read(fud, in, &cs, len);
>>> @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
>>>  	if (!user_backed_iter(from))
>>>  		return -EINVAL;
>>>  
>>> -	fuse_copy_init(&cs, false, from);
>>> +	fuse_copy_init(&cs, fud->fc, false, from);
>>>  
>>>  	return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
>>>  }
>>> @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
>>>  	}
>>>  	pipe_unlock(pipe);
>>>  
>>> -	fuse_copy_init(&cs, false, NULL);
>>> +	fuse_copy_init(&cs, fud->fc, false, NULL);
>>>  	cs.pipebufs = bufs;
>>>  	cs.nr_segs = nbuf;
>>>  	cs.pipe = pipe;
>>> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
>>> index ef470c4a9261..52b883a6a79d 100644
>>> --- a/fs/fuse/dev_uring.c
>>> +++ b/fs/fuse/dev_uring.c
>>> @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
>>>  	if (err)
>>>  		return err;
>>>  
>>> -	fuse_copy_init(&cs, false, &iter);
>>> +	fuse_copy_init(&cs, ring->fc, false, &iter);
>>>  	cs.is_uring = true;
>>>  	cs.req = req;
>>>  
>>> @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
>>>  		return err;
>>>  	}
>>>  
>>> -	fuse_copy_init(&cs, true, &iter);
>>> +	fuse_copy_init(&cs, ring->fc, true, &iter);
>>>  	cs.is_uring = true;
>>>  	cs.req = req;
>>>  
>>> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
>>> index db136e045925..25e593e64c67 100644
>>> --- a/fs/fuse/fuse_dev_i.h
>>> +++ b/fs/fuse/fuse_dev_i.h
>>> @@ -32,6 +32,7 @@ struct fuse_copy_state {
>>>  	bool write:1;
>>>  	bool move_pages:1;
>>>  	bool is_uring:1;
>>> +	bool splice_read_user_pages:1;
>>>  	struct {
>>>  		unsigned int copied_sz; /* copied size into the user buffer */
>>>  	} ring;
>>> @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
>>>  
>>>  void fuse_dev_end_requests(struct list_head *head);
>>>  
>>> -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
>>> -			   struct iov_iter *iter);
>>> +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
>>> +		    bool write, struct iov_iter *iter);
>>>  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
>>>  		   unsigned int argpages, struct fuse_arg *args,
>>>  		   int zeroing);
>>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
>>> index 3d5289cb82a5..e21875f16220 100644
>>> --- a/fs/fuse/fuse_i.h
>>> +++ b/fs/fuse/fuse_i.h
>>> @@ -898,6 +898,9 @@ struct fuse_conn {
>>>  	/* Use io_uring for communication */
>>>  	bool io_uring:1;
>>>  
>>> +	/* Allow splice for reading user pages */
>>> +	bool splice_read_user_pages:1;
>>> +
>>>  	/** Maximum stack depth for passthrough backing files */
>>>  	int max_stack_depth;
>>>  
>>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
>>> index 43b6643635ee..e82e96800fde 100644
>>> --- a/fs/fuse/inode.c
>>> +++ b/fs/fuse/inode.c
>>> @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
>>>  
>>>  			if (flags & FUSE_REQUEST_TIMEOUT)
>>>  				timeout = arg->request_timeout;
>>> +
>>> +			if (flags & FUSE_SPLICE_READ_USER_PAGES)
>>> +				fc->splice_read_user_pages = true;
>>
>>
>> Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
>> that one can access file content although the write is already
>> marked as completed? I.e. a fuse file system might get data
>> it was never exposed to and possibly secret data?
>> A more complex version version could check for CAP_SYS_ADMIN, but
>> also allow later on read/write to files that have the same uid as
>> the fuser-server process?
>>
> 
> IDGI. I don't see how this allows the server access to something it
> didn't have access to before.
> 
> This patchset seems to be about a "contract" between the kernel and the
> userland server. The server is agreeing to be very careful about not
> touching pages after a write request completes, and the kernel allows
> splicing the pages if that's the case.
> 
> Can you explain the potential attack vector?

Let's the server claim it does FUSE_SPLICE_READ_USER_PAGES, i.e. claims
it stops using splice buffers before completing write requests. But then
it actually first replies to the write and after an arbitrary amount
of time writes out the splice buffer. User application might be using
the buffer it send for write for other things and might not want to
expose that. I.e. application expects that after write(, buf,)
it can use 'buf' for other purposes and that the file system does not
access it anymore once write() is complete. I.e. it can put sensitive
data into the buffer, which it might not want to expose.
From my point of the issue is mainly with allow_other in combination
with "user_allow_other" in libfuse, as root has better ways to access data.


Thanks,
Bernd

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Joanne Koong]

On Mon, Apr 21, 2025 at 2:38 PM Bernd Schubert
<bernd.schubert@fastmail.fm> wrote:
>
> On 4/21/25 14:35, Jeff Layton wrote:
> > On Mon, 2025-04-21 at 13:49 +0200, Bernd Schubert wrote:
> >>
> >> On 4/19/25 02:06, Joanne Koong wrote:
> >>> For direct io writes, splice is disabled when forwarding pages from the
> >>> client to the server. This is because the pages in the pipe buffer are
> >>> user pages, which is controlled by the client. Thus if a server replies
> >>> to the request and then keeps accessing the pages afterwards, there is
> >>> the possibility that the client may have modified the contents of the
> >>> pages in the meantime. More context on this can be found in commit
> >>> 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
> >>>
> >>> For servers that do not need to access pages after answering the
> >>> request, splice gives a non-trivial improvement in performance.
> >>> Benchmarks show roughly a 40% speedup.
> >>>
> >>> Allow servers to enable zero-copy splice for servicing client direct io
> >>> writes. By enabling this, the server understands that they should not
> >>> continue accessing the pipe buffer after completing the request or may
> >>> face incorrect data if they do so.
> >>>
> >>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> >>> ---
> >>>  fs/fuse/dev.c             | 18 ++++++++++--------
> >>>  fs/fuse/dev_uring.c       |  4 ++--
> >>>  fs/fuse/fuse_dev_i.h      |  5 +++--
> >>>  fs/fuse/fuse_i.h          |  3 +++
> >>>  fs/fuse/inode.c           |  5 ++++-
> >>>  include/uapi/linux/fuse.h |  8 ++++++++
> >>>  6 files changed, 30 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> >>> index 67d07b4c778a..1b0ea8593f74 100644
> >>> --- a/fs/fuse/dev.c
> >>> +++ b/fs/fuse/dev.c
> >>> @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
> >>>     return err;
> >>>  }
> >>>
> >>> -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> >>> -               struct iov_iter *iter)
> >>> +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
> >>> +               bool write, struct iov_iter *iter)
> >>>  {
> >>>     memset(cs, 0, sizeof(*cs));
> >>>     cs->write = write;
> >>>     cs->iter = iter;
> >>> +   cs->splice_read_user_pages = fc->splice_read_user_pages;
> >>>  }
> >>>
> >>>  /* Unmap and put previous page of userspace buffer */
> >>> @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
> >>>             if (cs->write && cs->pipebufs && page) {
> >>>                     /*
> >>>                      * Can't control lifetime of pipe buffers, so always
> >>> -                    * copy user pages.
> >>> +                    * copy user pages if server does not support splice
> >>> +                    * for reading user pages.
> >>>                      */
> >>> -                   if (cs->req->args->user_pages) {
> >>> +                   if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
> >>>                             err = fuse_copy_fill(cs);
> >>>                             if (err)
> >>>                                     return err;
> >>> @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
> >>>     if (!user_backed_iter(to))
> >>>             return -EINVAL;
> >>>
> >>> -   fuse_copy_init(&cs, true, to);
> >>> +   fuse_copy_init(&cs, fud->fc, true, to);
> >>>
> >>>     return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
> >>>  }
> >>> @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> >>>     if (!bufs)
> >>>             return -ENOMEM;
> >>>
> >>> -   fuse_copy_init(&cs, true, NULL);
> >>> +   fuse_copy_init(&cs, fud->fc, true, NULL);
> >>>     cs.pipebufs = bufs;
> >>>     cs.pipe = pipe;
> >>>     ret = fuse_dev_do_read(fud, in, &cs, len);
> >>> @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
> >>>     if (!user_backed_iter(from))
> >>>             return -EINVAL;
> >>>
> >>> -   fuse_copy_init(&cs, false, from);
> >>> +   fuse_copy_init(&cs, fud->fc, false, from);
> >>>
> >>>     return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
> >>>  }
> >>> @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> >>>     }
> >>>     pipe_unlock(pipe);
> >>>
> >>> -   fuse_copy_init(&cs, false, NULL);
> >>> +   fuse_copy_init(&cs, fud->fc, false, NULL);
> >>>     cs.pipebufs = bufs;
> >>>     cs.nr_segs = nbuf;
> >>>     cs.pipe = pipe;
> >>> diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> >>> index ef470c4a9261..52b883a6a79d 100644
> >>> --- a/fs/fuse/dev_uring.c
> >>> +++ b/fs/fuse/dev_uring.c
> >>> @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
> >>>     if (err)
> >>>             return err;
> >>>
> >>> -   fuse_copy_init(&cs, false, &iter);
> >>> +   fuse_copy_init(&cs, ring->fc, false, &iter);
> >>>     cs.is_uring = true;
> >>>     cs.req = req;
> >>>
> >>> @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
> >>>             return err;
> >>>     }
> >>>
> >>> -   fuse_copy_init(&cs, true, &iter);
> >>> +   fuse_copy_init(&cs, ring->fc, true, &iter);
> >>>     cs.is_uring = true;
> >>>     cs.req = req;
> >>>
> >>> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> >>> index db136e045925..25e593e64c67 100644
> >>> --- a/fs/fuse/fuse_dev_i.h
> >>> +++ b/fs/fuse/fuse_dev_i.h
> >>> @@ -32,6 +32,7 @@ struct fuse_copy_state {
> >>>     bool write:1;
> >>>     bool move_pages:1;
> >>>     bool is_uring:1;
> >>> +   bool splice_read_user_pages:1;
> >>>     struct {
> >>>             unsigned int copied_sz; /* copied size into the user buffer */
> >>>     } ring;
> >>> @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
> >>>
> >>>  void fuse_dev_end_requests(struct list_head *head);
> >>>
> >>> -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> >>> -                      struct iov_iter *iter);
> >>> +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
> >>> +               bool write, struct iov_iter *iter);
> >>>  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
> >>>                unsigned int argpages, struct fuse_arg *args,
> >>>                int zeroing);
> >>> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> >>> index 3d5289cb82a5..e21875f16220 100644
> >>> --- a/fs/fuse/fuse_i.h
> >>> +++ b/fs/fuse/fuse_i.h
> >>> @@ -898,6 +898,9 @@ struct fuse_conn {
> >>>     /* Use io_uring for communication */
> >>>     bool io_uring:1;
> >>>
> >>> +   /* Allow splice for reading user pages */
> >>> +   bool splice_read_user_pages:1;
> >>> +
> >>>     /** Maximum stack depth for passthrough backing files */
> >>>     int max_stack_depth;
> >>>
> >>> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> >>> index 43b6643635ee..e82e96800fde 100644
> >>> --- a/fs/fuse/inode.c
> >>> +++ b/fs/fuse/inode.c
> >>> @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> >>>
> >>>                     if (flags & FUSE_REQUEST_TIMEOUT)
> >>>                             timeout = arg->request_timeout;
> >>> +
> >>> +                   if (flags & FUSE_SPLICE_READ_USER_PAGES)
> >>> +                           fc->splice_read_user_pages = true;
> >>
> >>
> >> Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
> >> that one can access file content although the write is already
> >> marked as completed? I.e. a fuse file system might get data
> >> it was never exposed to and possibly secret data?
> >> A more complex version version could check for CAP_SYS_ADMIN, but
> >> also allow later on read/write to files that have the same uid as
> >> the fuser-server process?
> >>
> >
> > IDGI. I don't see how this allows the server access to something it
> > didn't have access to before.
> >
> > This patchset seems to be about a "contract" between the kernel and the
> > userland server. The server is agreeing to be very careful about not
> > touching pages after a write request completes, and the kernel allows
> > splicing the pages if that's the case.
> >
> > Can you explain the potential attack vector?
>
> Let's the server claim it does FUSE_SPLICE_READ_USER_PAGES, i.e. claims
> it stops using splice buffers before completing write requests. But then
> it actually first replies to the write and after an arbitrary amount
> of time writes out the splice buffer. User application might be using
> the buffer it send for write for other things and might not want to
> expose that. I.e. application expects that after write(, buf,)
> it can use 'buf' for other purposes and that the file system does not
> access it anymore once write() is complete. I.e. it can put sensitive
> data into the buffer, which it might not want to expose.
> From my point of the issue is mainly with allow_other in combination
> with "user_allow_other" in libfuse, as root has better ways to access data.
>

As I understand it, user_allow_other is disabled by default and is
only enabled if explicitly opted into by root.

It seems to me, philosophically, that if a client chooses to interact
with / use a specific fuse mount then it chooses to place its trust in
that fuse server and accepts the possible repercussions from any
malicious actions the server may take. For example, currently any fuse
server right now could choose to deadlock or hang a request which
would stall the client indefinitely.

Curious to hear if you and Jeff agree or disagree with this.

Thanks,
Joanne
>
> Thanks,
> Bernd
>

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Jeff Layton]

On Mon, 2025-04-21 at 17:49 -0700, Joanne Koong wrote:
> On Mon, Apr 21, 2025 at 2:38 PM Bernd Schubert
> <bernd.schubert@fastmail.fm> wrote:
> > 
> > On 4/21/25 14:35, Jeff Layton wrote:
> > > On Mon, 2025-04-21 at 13:49 +0200, Bernd Schubert wrote:
> > > > 
> > > > On 4/19/25 02:06, Joanne Koong wrote:
> > > > > For direct io writes, splice is disabled when forwarding pages from the
> > > > > client to the server. This is because the pages in the pipe buffer are
> > > > > user pages, which is controlled by the client. Thus if a server replies
> > > > > to the request and then keeps accessing the pages afterwards, there is
> > > > > the possibility that the client may have modified the contents of the
> > > > > pages in the meantime. More context on this can be found in commit
> > > > > 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
> > > > > 
> > > > > For servers that do not need to access pages after answering the
> > > > > request, splice gives a non-trivial improvement in performance.
> > > > > Benchmarks show roughly a 40% speedup.
> > > > > 
> > > > > Allow servers to enable zero-copy splice for servicing client direct io
> > > > > writes. By enabling this, the server understands that they should not
> > > > > continue accessing the pipe buffer after completing the request or may
> > > > > face incorrect data if they do so.
> > > > > 
> > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > ---
> > > > >  fs/fuse/dev.c             | 18 ++++++++++--------
> > > > >  fs/fuse/dev_uring.c       |  4 ++--
> > > > >  fs/fuse/fuse_dev_i.h      |  5 +++--
> > > > >  fs/fuse/fuse_i.h          |  3 +++
> > > > >  fs/fuse/inode.c           |  5 ++++-
> > > > >  include/uapi/linux/fuse.h |  8 ++++++++
> > > > >  6 files changed, 30 insertions(+), 13 deletions(-)
> > > > > 
> > > > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > > > index 67d07b4c778a..1b0ea8593f74 100644
> > > > > --- a/fs/fuse/dev.c
> > > > > +++ b/fs/fuse/dev.c
> > > > > @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
> > > > >     return err;
> > > > >  }
> > > > > 
> > > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > > -               struct iov_iter *iter)
> > > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
> > > > > +               bool write, struct iov_iter *iter)
> > > > >  {
> > > > >     memset(cs, 0, sizeof(*cs));
> > > > >     cs->write = write;
> > > > >     cs->iter = iter;
> > > > > +   cs->splice_read_user_pages = fc->splice_read_user_pages;
> > > > >  }
> > > > > 
> > > > >  /* Unmap and put previous page of userspace buffer */
> > > > > @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
> > > > >             if (cs->write && cs->pipebufs && page) {
> > > > >                     /*
> > > > >                      * Can't control lifetime of pipe buffers, so always
> > > > > -                    * copy user pages.
> > > > > +                    * copy user pages if server does not support splice
> > > > > +                    * for reading user pages.
> > > > >                      */
> > > > > -                   if (cs->req->args->user_pages) {
> > > > > +                   if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
> > > > >                             err = fuse_copy_fill(cs);
> > > > >                             if (err)
> > > > >                                     return err;
> > > > > @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
> > > > >     if (!user_backed_iter(to))
> > > > >             return -EINVAL;
> > > > > 
> > > > > -   fuse_copy_init(&cs, true, to);
> > > > > +   fuse_copy_init(&cs, fud->fc, true, to);
> > > > > 
> > > > >     return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
> > > > >  }
> > > > > @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > > >     if (!bufs)
> > > > >             return -ENOMEM;
> > > > > 
> > > > > -   fuse_copy_init(&cs, true, NULL);
> > > > > +   fuse_copy_init(&cs, fud->fc, true, NULL);
> > > > >     cs.pipebufs = bufs;
> > > > >     cs.pipe = pipe;
> > > > >     ret = fuse_dev_do_read(fud, in, &cs, len);
> > > > > @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
> > > > >     if (!user_backed_iter(from))
> > > > >             return -EINVAL;
> > > > > 
> > > > > -   fuse_copy_init(&cs, false, from);
> > > > > +   fuse_copy_init(&cs, fud->fc, false, from);
> > > > > 
> > > > >     return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
> > > > >  }
> > > > > @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> > > > >     }
> > > > >     pipe_unlock(pipe);
> > > > > 
> > > > > -   fuse_copy_init(&cs, false, NULL);
> > > > > +   fuse_copy_init(&cs, fud->fc, false, NULL);
> > > > >     cs.pipebufs = bufs;
> > > > >     cs.nr_segs = nbuf;
> > > > >     cs.pipe = pipe;
> > > > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > > > > index ef470c4a9261..52b883a6a79d 100644
> > > > > --- a/fs/fuse/dev_uring.c
> > > > > +++ b/fs/fuse/dev_uring.c
> > > > > @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
> > > > >     if (err)
> > > > >             return err;
> > > > > 
> > > > > -   fuse_copy_init(&cs, false, &iter);
> > > > > +   fuse_copy_init(&cs, ring->fc, false, &iter);
> > > > >     cs.is_uring = true;
> > > > >     cs.req = req;
> > > > > 
> > > > > @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
> > > > >             return err;
> > > > >     }
> > > > > 
> > > > > -   fuse_copy_init(&cs, true, &iter);
> > > > > +   fuse_copy_init(&cs, ring->fc, true, &iter);
> > > > >     cs.is_uring = true;
> > > > >     cs.req = req;
> > > > > 
> > > > > diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> > > > > index db136e045925..25e593e64c67 100644
> > > > > --- a/fs/fuse/fuse_dev_i.h
> > > > > +++ b/fs/fuse/fuse_dev_i.h
> > > > > @@ -32,6 +32,7 @@ struct fuse_copy_state {
> > > > >     bool write:1;
> > > > >     bool move_pages:1;
> > > > >     bool is_uring:1;
> > > > > +   bool splice_read_user_pages:1;
> > > > >     struct {
> > > > >             unsigned int copied_sz; /* copied size into the user buffer */
> > > > >     } ring;
> > > > > @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
> > > > > 
> > > > >  void fuse_dev_end_requests(struct list_head *head);
> > > > > 
> > > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > > -                      struct iov_iter *iter);
> > > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
> > > > > +               bool write, struct iov_iter *iter);
> > > > >  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
> > > > >                unsigned int argpages, struct fuse_arg *args,
> > > > >                int zeroing);
> > > > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > > > index 3d5289cb82a5..e21875f16220 100644
> > > > > --- a/fs/fuse/fuse_i.h
> > > > > +++ b/fs/fuse/fuse_i.h
> > > > > @@ -898,6 +898,9 @@ struct fuse_conn {
> > > > >     /* Use io_uring for communication */
> > > > >     bool io_uring:1;
> > > > > 
> > > > > +   /* Allow splice for reading user pages */
> > > > > +   bool splice_read_user_pages:1;
> > > > > +
> > > > >     /** Maximum stack depth for passthrough backing files */
> > > > >     int max_stack_depth;
> > > > > 
> > > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > > > index 43b6643635ee..e82e96800fde 100644
> > > > > --- a/fs/fuse/inode.c
> > > > > +++ b/fs/fuse/inode.c
> > > > > @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > > > > 
> > > > >                     if (flags & FUSE_REQUEST_TIMEOUT)
> > > > >                             timeout = arg->request_timeout;
> > > > > +
> > > > > +                   if (flags & FUSE_SPLICE_READ_USER_PAGES)
> > > > > +                           fc->splice_read_user_pages = true;
> > > > 
> > > > 
> > > > Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
> > > > that one can access file content although the write is already
> > > > marked as completed? I.e. a fuse file system might get data
> > > > it was never exposed to and possibly secret data?
> > > > A more complex version version could check for CAP_SYS_ADMIN, but
> > > > also allow later on read/write to files that have the same uid as
> > > > the fuser-server process?
> > > > 
> > > 
> > > IDGI. I don't see how this allows the server access to something it
> > > didn't have access to before.
> > > 
> > > This patchset seems to be about a "contract" between the kernel and the
> > > userland server. The server is agreeing to be very careful about not
> > > touching pages after a write request completes, and the kernel allows
> > > splicing the pages if that's the case.
> > > 
> > > Can you explain the potential attack vector?
> > 
> > Let's the server claim it does FUSE_SPLICE_READ_USER_PAGES, i.e. claims
> > it stops using splice buffers before completing write requests. But then
> > it actually first replies to the write and after an arbitrary amount
> > of time writes out the splice buffer. User application might be using
> > the buffer it send for write for other things and might not want to
> > expose that. I.e. application expects that after write(, buf,)
> > it can use 'buf' for other purposes and that the file system does not
> > access it anymore once write() is complete. I.e. it can put sensitive
> > data into the buffer, which it might not want to expose.
> > From my point of the issue is mainly with allow_other in combination
> > with "user_allow_other" in libfuse, as root has better ways to access data.
> > 
> 
> As I understand it, user_allow_other is disabled by default and is
> only enabled if explicitly opted into by root.
> 
> It seems to me, philosophically, that if a client chooses to interact
> with / use a specific fuse mount then it chooses to place its trust in
> that fuse server and accepts the possible repercussions from any
> malicious actions the server may take. For example, currently any fuse
> server right now could choose to deadlock or hang a request which
> would stall the client indefinitely.
> 
> Curious to hear if you and Jeff agree or disagree with this.
> 
> 

I'm not sure here -- again FUSE isn't my area of expertise, but
disclosing potentially private info is generally considered worse than
a denial of service attack.

I wonder whether we could check if there are extra folio refs
outstanding after the I/O is done?

IOW, get the refcount on the folios you're splicing before you send
them to userland. After the I/O is done, get their refcounts again and
see if they have been elevated? If so, then something is probably
misusing those buffers?
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Joanne Koong]

On Tue, Apr 22, 2025 at 4:37 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2025-04-21 at 17:49 -0700, Joanne Koong wrote:
> > On Mon, Apr 21, 2025 at 2:38 PM Bernd Schubert
> > <bernd.schubert@fastmail.fm> wrote:
> > >
> > > On 4/21/25 14:35, Jeff Layton wrote:
> > > > On Mon, 2025-04-21 at 13:49 +0200, Bernd Schubert wrote:
> > > > >
> > > > > On 4/19/25 02:06, Joanne Koong wrote:
> > > > > > For direct io writes, splice is disabled when forwarding pages from the
> > > > > > client to the server. This is because the pages in the pipe buffer are
> > > > > > user pages, which is controlled by the client. Thus if a server replies
> > > > > > to the request and then keeps accessing the pages afterwards, there is
> > > > > > the possibility that the client may have modified the contents of the
> > > > > > pages in the meantime. More context on this can be found in commit
> > > > > > 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
> > > > > >
> > > > > > For servers that do not need to access pages after answering the
> > > > > > request, splice gives a non-trivial improvement in performance.
> > > > > > Benchmarks show roughly a 40% speedup.
> > > > > >
> > > > > > Allow servers to enable zero-copy splice for servicing client direct io
> > > > > > writes. By enabling this, the server understands that they should not
> > > > > > continue accessing the pipe buffer after completing the request or may
> > > > > > face incorrect data if they do so.
> > > > > >
> > > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > > ---
> > > > > >  fs/fuse/dev.c             | 18 ++++++++++--------
> > > > > >  fs/fuse/dev_uring.c       |  4 ++--
> > > > > >  fs/fuse/fuse_dev_i.h      |  5 +++--
> > > > > >  fs/fuse/fuse_i.h          |  3 +++
> > > > > >  fs/fuse/inode.c           |  5 ++++-
> > > > > >  include/uapi/linux/fuse.h |  8 ++++++++
> > > > > >  6 files changed, 30 insertions(+), 13 deletions(-)
> > > > > >
> > > > > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > > > > index 67d07b4c778a..1b0ea8593f74 100644
> > > > > > --- a/fs/fuse/dev.c
> > > > > > +++ b/fs/fuse/dev.c
> > > > > > @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
> > > > > >     return err;
> > > > > >  }
> > > > > >
> > > > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > > > -               struct iov_iter *iter)
> > > > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
> > > > > > +               bool write, struct iov_iter *iter)
> > > > > >  {
> > > > > >     memset(cs, 0, sizeof(*cs));
> > > > > >     cs->write = write;
> > > > > >     cs->iter = iter;
> > > > > > +   cs->splice_read_user_pages = fc->splice_read_user_pages;
> > > > > >  }
> > > > > >
> > > > > >  /* Unmap and put previous page of userspace buffer */
> > > > > > @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
> > > > > >             if (cs->write && cs->pipebufs && page) {
> > > > > >                     /*
> > > > > >                      * Can't control lifetime of pipe buffers, so always
> > > > > > -                    * copy user pages.
> > > > > > +                    * copy user pages if server does not support splice
> > > > > > +                    * for reading user pages.
> > > > > >                      */
> > > > > > -                   if (cs->req->args->user_pages) {
> > > > > > +                   if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
> > > > > >                             err = fuse_copy_fill(cs);
> > > > > >                             if (err)
> > > > > >                                     return err;
> > > > > > @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
> > > > > >     if (!user_backed_iter(to))
> > > > > >             return -EINVAL;
> > > > > >
> > > > > > -   fuse_copy_init(&cs, true, to);
> > > > > > +   fuse_copy_init(&cs, fud->fc, true, to);
> > > > > >
> > > > > >     return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
> > > > > >  }
> > > > > > @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > > > >     if (!bufs)
> > > > > >             return -ENOMEM;
> > > > > >
> > > > > > -   fuse_copy_init(&cs, true, NULL);
> > > > > > +   fuse_copy_init(&cs, fud->fc, true, NULL);
> > > > > >     cs.pipebufs = bufs;
> > > > > >     cs.pipe = pipe;
> > > > > >     ret = fuse_dev_do_read(fud, in, &cs, len);
> > > > > > @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
> > > > > >     if (!user_backed_iter(from))
> > > > > >             return -EINVAL;
> > > > > >
> > > > > > -   fuse_copy_init(&cs, false, from);
> > > > > > +   fuse_copy_init(&cs, fud->fc, false, from);
> > > > > >
> > > > > >     return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
> > > > > >  }
> > > > > > @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> > > > > >     }
> > > > > >     pipe_unlock(pipe);
> > > > > >
> > > > > > -   fuse_copy_init(&cs, false, NULL);
> > > > > > +   fuse_copy_init(&cs, fud->fc, false, NULL);
> > > > > >     cs.pipebufs = bufs;
> > > > > >     cs.nr_segs = nbuf;
> > > > > >     cs.pipe = pipe;
> > > > > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > > > > > index ef470c4a9261..52b883a6a79d 100644
> > > > > > --- a/fs/fuse/dev_uring.c
> > > > > > +++ b/fs/fuse/dev_uring.c
> > > > > > @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
> > > > > >     if (err)
> > > > > >             return err;
> > > > > >
> > > > > > -   fuse_copy_init(&cs, false, &iter);
> > > > > > +   fuse_copy_init(&cs, ring->fc, false, &iter);
> > > > > >     cs.is_uring = true;
> > > > > >     cs.req = req;
> > > > > >
> > > > > > @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
> > > > > >             return err;
> > > > > >     }
> > > > > >
> > > > > > -   fuse_copy_init(&cs, true, &iter);
> > > > > > +   fuse_copy_init(&cs, ring->fc, true, &iter);
> > > > > >     cs.is_uring = true;
> > > > > >     cs.req = req;
> > > > > >
> > > > > > diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> > > > > > index db136e045925..25e593e64c67 100644
> > > > > > --- a/fs/fuse/fuse_dev_i.h
> > > > > > +++ b/fs/fuse/fuse_dev_i.h
> > > > > > @@ -32,6 +32,7 @@ struct fuse_copy_state {
> > > > > >     bool write:1;
> > > > > >     bool move_pages:1;
> > > > > >     bool is_uring:1;
> > > > > > +   bool splice_read_user_pages:1;
> > > > > >     struct {
> > > > > >             unsigned int copied_sz; /* copied size into the user buffer */
> > > > > >     } ring;
> > > > > > @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
> > > > > >
> > > > > >  void fuse_dev_end_requests(struct list_head *head);
> > > > > >
> > > > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > > > -                      struct iov_iter *iter);
> > > > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
> > > > > > +               bool write, struct iov_iter *iter);
> > > > > >  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
> > > > > >                unsigned int argpages, struct fuse_arg *args,
> > > > > >                int zeroing);
> > > > > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > > > > index 3d5289cb82a5..e21875f16220 100644
> > > > > > --- a/fs/fuse/fuse_i.h
> > > > > > +++ b/fs/fuse/fuse_i.h
> > > > > > @@ -898,6 +898,9 @@ struct fuse_conn {
> > > > > >     /* Use io_uring for communication */
> > > > > >     bool io_uring:1;
> > > > > >
> > > > > > +   /* Allow splice for reading user pages */
> > > > > > +   bool splice_read_user_pages:1;
> > > > > > +
> > > > > >     /** Maximum stack depth for passthrough backing files */
> > > > > >     int max_stack_depth;
> > > > > >
> > > > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > > > > index 43b6643635ee..e82e96800fde 100644
> > > > > > --- a/fs/fuse/inode.c
> > > > > > +++ b/fs/fuse/inode.c
> > > > > > @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > > > > >
> > > > > >                     if (flags & FUSE_REQUEST_TIMEOUT)
> > > > > >                             timeout = arg->request_timeout;
> > > > > > +
> > > > > > +                   if (flags & FUSE_SPLICE_READ_USER_PAGES)
> > > > > > +                           fc->splice_read_user_pages = true;
> > > > >
> > > > >
> > > > > Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
> > > > > that one can access file content although the write is already
> > > > > marked as completed? I.e. a fuse file system might get data
> > > > > it was never exposed to and possibly secret data?
> > > > > A more complex version version could check for CAP_SYS_ADMIN, but
> > > > > also allow later on read/write to files that have the same uid as
> > > > > the fuser-server process?
> > > > >
> > > >
> > > > IDGI. I don't see how this allows the server access to something it
> > > > didn't have access to before.
> > > >
> > > > This patchset seems to be about a "contract" between the kernel and the
> > > > userland server. The server is agreeing to be very careful about not
> > > > touching pages after a write request completes, and the kernel allows
> > > > splicing the pages if that's the case.
> > > >
> > > > Can you explain the potential attack vector?
> > >
> > > Let's the server claim it does FUSE_SPLICE_READ_USER_PAGES, i.e. claims
> > > it stops using splice buffers before completing write requests. But then
> > > it actually first replies to the write and after an arbitrary amount
> > > of time writes out the splice buffer. User application might be using
> > > the buffer it send for write for other things and might not want to
> > > expose that. I.e. application expects that after write(, buf,)
> > > it can use 'buf' for other purposes and that the file system does not
> > > access it anymore once write() is complete. I.e. it can put sensitive
> > > data into the buffer, which it might not want to expose.
> > > From my point of the issue is mainly with allow_other in combination
> > > with "user_allow_other" in libfuse, as root has better ways to access data.
> > >
> >
> > As I understand it, user_allow_other is disabled by default and is
> > only enabled if explicitly opted into by root.
> >
> > It seems to me, philosophically, that if a client chooses to interact
> > with / use a specific fuse mount then it chooses to place its trust in
> > that fuse server and accepts the possible repercussions from any
> > malicious actions the server may take. For example, currently any fuse
> > server right now could choose to deadlock or hang a request which
> > would stall the client indefinitely.
> >
> > Curious to hear if you and Jeff agree or disagree with this.
> >
> >
>
> I'm not sure here -- again FUSE isn't my area of expertise, but
> disclosing potentially private info is generally considered worse than
> a denial of service attack.

In that case then, we should probably just add the CAP_SYS_ADMIN gating.

>
> I wonder whether we could check if there are extra folio refs
> outstanding after the I/O is done?
>
> IOW, get the refcount on the folios you're splicing before you send
> them to userland. After the I/O is done, get their refcounts again and
> see if they have been elevated? If so, then something is probably
> misusing those buffers?

The pages are routed to the server through the pipebuffer, so I don't
think looking at the refcount on those folios gets us anything here.
The only solution here I see is invalidating the page in the pipe
buffer after the server completes the request, so that any subsequent
read attempts on that fd by the server will fail, but I think that
gets ugly with then having to associate pipe buffers with fuse
requests.

For now I'll gate this behind CAP_SYS_ADMIN and in the future if this
needs to be relaxed, we could add that in.

Thanks,
Joanne
> --
> Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Jeff Layton]

On Mon, 2025-04-21 at 23:38 +0200, Bernd Schubert wrote:
> 
> On 4/21/25 14:35, Jeff Layton wrote:
> > On Mon, 2025-04-21 at 13:49 +0200, Bernd Schubert wrote:
> > > 
> > > On 4/19/25 02:06, Joanne Koong wrote:
> > > > For direct io writes, splice is disabled when forwarding pages from the
> > > > client to the server. This is because the pages in the pipe buffer are
> > > > user pages, which is controlled by the client. Thus if a server replies
> > > > to the request and then keeps accessing the pages afterwards, there is
> > > > the possibility that the client may have modified the contents of the
> > > > pages in the meantime. More context on this can be found in commit
> > > > 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
> > > > 
> > > > For servers that do not need to access pages after answering the
> > > > request, splice gives a non-trivial improvement in performance.
> > > > Benchmarks show roughly a 40% speedup.
> > > > 
> > > > Allow servers to enable zero-copy splice for servicing client direct io
> > > > writes. By enabling this, the server understands that they should not
> > > > continue accessing the pipe buffer after completing the request or may
> > > > face incorrect data if they do so.
> > > > 
> > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > ---
> > > >  fs/fuse/dev.c             | 18 ++++++++++--------
> > > >  fs/fuse/dev_uring.c       |  4 ++--
> > > >  fs/fuse/fuse_dev_i.h      |  5 +++--
> > > >  fs/fuse/fuse_i.h          |  3 +++
> > > >  fs/fuse/inode.c           |  5 ++++-
> > > >  include/uapi/linux/fuse.h |  8 ++++++++
> > > >  6 files changed, 30 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > > index 67d07b4c778a..1b0ea8593f74 100644
> > > > --- a/fs/fuse/dev.c
> > > > +++ b/fs/fuse/dev.c
> > > > @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
> > > >  	return err;
> > > >  }
> > > >  
> > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > -		    struct iov_iter *iter)
> > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
> > > > +		    bool write, struct iov_iter *iter)
> > > >  {
> > > >  	memset(cs, 0, sizeof(*cs));
> > > >  	cs->write = write;
> > > >  	cs->iter = iter;
> > > > +	cs->splice_read_user_pages = fc->splice_read_user_pages;
> > > >  }
> > > >  
> > > >  /* Unmap and put previous page of userspace buffer */
> > > > @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
> > > >  		if (cs->write && cs->pipebufs && page) {
> > > >  			/*
> > > >  			 * Can't control lifetime of pipe buffers, so always
> > > > -			 * copy user pages.
> > > > +			 * copy user pages if server does not support splice
> > > > +			 * for reading user pages.
> > > >  			 */
> > > > -			if (cs->req->args->user_pages) {
> > > > +			if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
> > > >  				err = fuse_copy_fill(cs);
> > > >  				if (err)
> > > >  					return err;
> > > > @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
> > > >  	if (!user_backed_iter(to))
> > > >  		return -EINVAL;
> > > >  
> > > > -	fuse_copy_init(&cs, true, to);
> > > > +	fuse_copy_init(&cs, fud->fc, true, to);
> > > >  
> > > >  	return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
> > > >  }
> > > > @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > >  	if (!bufs)
> > > >  		return -ENOMEM;
> > > >  
> > > > -	fuse_copy_init(&cs, true, NULL);
> > > > +	fuse_copy_init(&cs, fud->fc, true, NULL);
> > > >  	cs.pipebufs = bufs;
> > > >  	cs.pipe = pipe;
> > > >  	ret = fuse_dev_do_read(fud, in, &cs, len);
> > > > @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
> > > >  	if (!user_backed_iter(from))
> > > >  		return -EINVAL;
> > > >  
> > > > -	fuse_copy_init(&cs, false, from);
> > > > +	fuse_copy_init(&cs, fud->fc, false, from);
> > > >  
> > > >  	return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
> > > >  }
> > > > @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> > > >  	}
> > > >  	pipe_unlock(pipe);
> > > >  
> > > > -	fuse_copy_init(&cs, false, NULL);
> > > > +	fuse_copy_init(&cs, fud->fc, false, NULL);
> > > >  	cs.pipebufs = bufs;
> > > >  	cs.nr_segs = nbuf;
> > > >  	cs.pipe = pipe;
> > > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > > > index ef470c4a9261..52b883a6a79d 100644
> > > > --- a/fs/fuse/dev_uring.c
> > > > +++ b/fs/fuse/dev_uring.c
> > > > @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
> > > >  	if (err)
> > > >  		return err;
> > > >  
> > > > -	fuse_copy_init(&cs, false, &iter);
> > > > +	fuse_copy_init(&cs, ring->fc, false, &iter);
> > > >  	cs.is_uring = true;
> > > >  	cs.req = req;
> > > >  
> > > > @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
> > > >  		return err;
> > > >  	}
> > > >  
> > > > -	fuse_copy_init(&cs, true, &iter);
> > > > +	fuse_copy_init(&cs, ring->fc, true, &iter);
> > > >  	cs.is_uring = true;
> > > >  	cs.req = req;
> > > >  
> > > > diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> > > > index db136e045925..25e593e64c67 100644
> > > > --- a/fs/fuse/fuse_dev_i.h
> > > > +++ b/fs/fuse/fuse_dev_i.h
> > > > @@ -32,6 +32,7 @@ struct fuse_copy_state {
> > > >  	bool write:1;
> > > >  	bool move_pages:1;
> > > >  	bool is_uring:1;
> > > > +	bool splice_read_user_pages:1;
> > > >  	struct {
> > > >  		unsigned int copied_sz; /* copied size into the user buffer */
> > > >  	} ring;
> > > > @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
> > > >  
> > > >  void fuse_dev_end_requests(struct list_head *head);
> > > >  
> > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > -			   struct iov_iter *iter);
> > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
> > > > +		    bool write, struct iov_iter *iter);
> > > >  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
> > > >  		   unsigned int argpages, struct fuse_arg *args,
> > > >  		   int zeroing);
> > > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > > index 3d5289cb82a5..e21875f16220 100644
> > > > --- a/fs/fuse/fuse_i.h
> > > > +++ b/fs/fuse/fuse_i.h
> > > > @@ -898,6 +898,9 @@ struct fuse_conn {
> > > >  	/* Use io_uring for communication */
> > > >  	bool io_uring:1;
> > > >  
> > > > +	/* Allow splice for reading user pages */
> > > > +	bool splice_read_user_pages:1;
> > > > +
> > > >  	/** Maximum stack depth for passthrough backing files */
> > > >  	int max_stack_depth;
> > > >  
> > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > > index 43b6643635ee..e82e96800fde 100644
> > > > --- a/fs/fuse/inode.c
> > > > +++ b/fs/fuse/inode.c
> > > > @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > > >  
> > > >  			if (flags & FUSE_REQUEST_TIMEOUT)
> > > >  				timeout = arg->request_timeout;
> > > > +
> > > > +			if (flags & FUSE_SPLICE_READ_USER_PAGES)
> > > > +				fc->splice_read_user_pages = true;
> > > 
> > > 
> > > Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
> > > that one can access file content although the write is already
> > > marked as completed? I.e. a fuse file system might get data
> > > it was never exposed to and possibly secret data?
> > > A more complex version version could check for CAP_SYS_ADMIN, but
> > > also allow later on read/write to files that have the same uid as
> > > the fuser-server process?
> > > 
> > 
> > IDGI. I don't see how this allows the server access to something it
> > didn't have access to before.
> > 
> > This patchset seems to be about a "contract" between the kernel and the
> > userland server. The server is agreeing to be very careful about not
> > touching pages after a write request completes, and the kernel allows
> > splicing the pages if that's the case.
> > 
> > Can you explain the potential attack vector?
> 
> Let's the server claim it does FUSE_SPLICE_READ_USER_PAGES, i.e. claims
> it stops using splice buffers before completing write requests. But then
> it actually first replies to the write and after an arbitrary amount
> of time writes out the splice buffer. User application might be using
> the buffer it send for write for other things and might not want to
> expose that. I.e. application expects that after write(, buf,)
> it can use 'buf' for other purposes and that the file system does not
> access it anymore once write() is complete. I.e. it can put sensitive
> data into the buffer, which it might not want to expose.
> From my point of the issue is mainly with allow_other in combination
> with "user_allow_other" in libfuse, as root has better ways to access data.
> 

That would definitely break the contract. The question is whether
that's a security issue. I'm not convinced that it is, given all of the
other ways you can do nefarious stuff with an unprivileged server.

That said, if there is any question, gating this behind CAP_SYS_ADMIN
seems like a reasonable thing to do. We could always relax that later
if we decide that it's a non-issue.

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1] fuse: use splice for reading user pages on servers that enable it

[Joanne Koong]

On Tue, Apr 22, 2025 at 4:33 AM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Mon, 2025-04-21 at 23:38 +0200, Bernd Schubert wrote:
> >
> > On 4/21/25 14:35, Jeff Layton wrote:
> > > On Mon, 2025-04-21 at 13:49 +0200, Bernd Schubert wrote:
> > > >
> > > > On 4/19/25 02:06, Joanne Koong wrote:
> > > > > For direct io writes, splice is disabled when forwarding pages from the
> > > > > client to the server. This is because the pages in the pipe buffer are
> > > > > user pages, which is controlled by the client. Thus if a server replies
> > > > > to the request and then keeps accessing the pages afterwards, there is
> > > > > the possibility that the client may have modified the contents of the
> > > > > pages in the meantime. More context on this can be found in commit
> > > > > 0c4bcfdecb1a ("fuse: fix pipe buffer lifetime for direct_io").
> > > > >
> > > > > For servers that do not need to access pages after answering the
> > > > > request, splice gives a non-trivial improvement in performance.
> > > > > Benchmarks show roughly a 40% speedup.
> > > > >
> > > > > Allow servers to enable zero-copy splice for servicing client direct io
> > > > > writes. By enabling this, the server understands that they should not
> > > > > continue accessing the pipe buffer after completing the request or may
> > > > > face incorrect data if they do so.
> > > > >
> > > > > Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
> > > > > ---
> > > > >  fs/fuse/dev.c             | 18 ++++++++++--------
> > > > >  fs/fuse/dev_uring.c       |  4 ++--
> > > > >  fs/fuse/fuse_dev_i.h      |  5 +++--
> > > > >  fs/fuse/fuse_i.h          |  3 +++
> > > > >  fs/fuse/inode.c           |  5 ++++-
> > > > >  include/uapi/linux/fuse.h |  8 ++++++++
> > > > >  6 files changed, 30 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> > > > > index 67d07b4c778a..1b0ea8593f74 100644
> > > > > --- a/fs/fuse/dev.c
> > > > > +++ b/fs/fuse/dev.c
> > > > > @@ -816,12 +816,13 @@ static int unlock_request(struct fuse_req *req)
> > > > >         return err;
> > > > >  }
> > > > >
> > > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > > -                   struct iov_iter *iter)
> > > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *fc,
> > > > > +                   bool write, struct iov_iter *iter)
> > > > >  {
> > > > >         memset(cs, 0, sizeof(*cs));
> > > > >         cs->write = write;
> > > > >         cs->iter = iter;
> > > > > +       cs->splice_read_user_pages = fc->splice_read_user_pages;
> > > > >  }
> > > > >
> > > > >  /* Unmap and put previous page of userspace buffer */
> > > > > @@ -1105,9 +1106,10 @@ static int fuse_copy_page(struct fuse_copy_state *cs, struct page **pagep,
> > > > >                 if (cs->write && cs->pipebufs && page) {
> > > > >                         /*
> > > > >                          * Can't control lifetime of pipe buffers, so always
> > > > > -                        * copy user pages.
> > > > > +                        * copy user pages if server does not support splice
> > > > > +                        * for reading user pages.
> > > > >                          */
> > > > > -                       if (cs->req->args->user_pages) {
> > > > > +                       if (cs->req->args->user_pages && !cs->splice_read_user_pages) {
> > > > >                                 err = fuse_copy_fill(cs);
> > > > >                                 if (err)
> > > > >                                         return err;
> > > > > @@ -1538,7 +1540,7 @@ static ssize_t fuse_dev_read(struct kiocb *iocb, struct iov_iter *to)
> > > > >         if (!user_backed_iter(to))
> > > > >                 return -EINVAL;
> > > > >
> > > > > -       fuse_copy_init(&cs, true, to);
> > > > > +       fuse_copy_init(&cs, fud->fc, true, to);
> > > > >
> > > > >         return fuse_dev_do_read(fud, file, &cs, iov_iter_count(to));
> > > > >  }
> > > > > @@ -1561,7 +1563,7 @@ static ssize_t fuse_dev_splice_read(struct file *in, loff_t *ppos,
> > > > >         if (!bufs)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > -       fuse_copy_init(&cs, true, NULL);
> > > > > +       fuse_copy_init(&cs, fud->fc, true, NULL);
> > > > >         cs.pipebufs = bufs;
> > > > >         cs.pipe = pipe;
> > > > >         ret = fuse_dev_do_read(fud, in, &cs, len);
> > > > > @@ -2227,7 +2229,7 @@ static ssize_t fuse_dev_write(struct kiocb *iocb, struct iov_iter *from)
> > > > >         if (!user_backed_iter(from))
> > > > >                 return -EINVAL;
> > > > >
> > > > > -       fuse_copy_init(&cs, false, from);
> > > > > +       fuse_copy_init(&cs, fud->fc, false, from);
> > > > >
> > > > >         return fuse_dev_do_write(fud, &cs, iov_iter_count(from));
> > > > >  }
> > > > > @@ -2301,7 +2303,7 @@ static ssize_t fuse_dev_splice_write(struct pipe_inode_info *pipe,
> > > > >         }
> > > > >         pipe_unlock(pipe);
> > > > >
> > > > > -       fuse_copy_init(&cs, false, NULL);
> > > > > +       fuse_copy_init(&cs, fud->fc, false, NULL);
> > > > >         cs.pipebufs = bufs;
> > > > >         cs.nr_segs = nbuf;
> > > > >         cs.pipe = pipe;
> > > > > diff --git a/fs/fuse/dev_uring.c b/fs/fuse/dev_uring.c
> > > > > index ef470c4a9261..52b883a6a79d 100644
> > > > > --- a/fs/fuse/dev_uring.c
> > > > > +++ b/fs/fuse/dev_uring.c
> > > > > @@ -593,7 +593,7 @@ static int fuse_uring_copy_from_ring(struct fuse_ring *ring,
> > > > >         if (err)
> > > > >                 return err;
> > > > >
> > > > > -       fuse_copy_init(&cs, false, &iter);
> > > > > +       fuse_copy_init(&cs, ring->fc, false, &iter);
> > > > >         cs.is_uring = true;
> > > > >         cs.req = req;
> > > > >
> > > > > @@ -623,7 +623,7 @@ static int fuse_uring_args_to_ring(struct fuse_ring *ring, struct fuse_req *req,
> > > > >                 return err;
> > > > >         }
> > > > >
> > > > > -       fuse_copy_init(&cs, true, &iter);
> > > > > +       fuse_copy_init(&cs, ring->fc, true, &iter);
> > > > >         cs.is_uring = true;
> > > > >         cs.req = req;
> > > > >
> > > > > diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> > > > > index db136e045925..25e593e64c67 100644
> > > > > --- a/fs/fuse/fuse_dev_i.h
> > > > > +++ b/fs/fuse/fuse_dev_i.h
> > > > > @@ -32,6 +32,7 @@ struct fuse_copy_state {
> > > > >         bool write:1;
> > > > >         bool move_pages:1;
> > > > >         bool is_uring:1;
> > > > > +       bool splice_read_user_pages:1;
> > > > >         struct {
> > > > >                 unsigned int copied_sz; /* copied size into the user buffer */
> > > > >         } ring;
> > > > > @@ -51,8 +52,8 @@ struct fuse_req *fuse_request_find(struct fuse_pqueue *fpq, u64 unique);
> > > > >
> > > > >  void fuse_dev_end_requests(struct list_head *head);
> > > > >
> > > > > -void fuse_copy_init(struct fuse_copy_state *cs, bool write,
> > > > > -                          struct iov_iter *iter);
> > > > > +void fuse_copy_init(struct fuse_copy_state *cs, struct fuse_conn *conn,
> > > > > +                   bool write, struct iov_iter *iter);
> > > > >  int fuse_copy_args(struct fuse_copy_state *cs, unsigned int numargs,
> > > > >                    unsigned int argpages, struct fuse_arg *args,
> > > > >                    int zeroing);
> > > > > diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> > > > > index 3d5289cb82a5..e21875f16220 100644
> > > > > --- a/fs/fuse/fuse_i.h
> > > > > +++ b/fs/fuse/fuse_i.h
> > > > > @@ -898,6 +898,9 @@ struct fuse_conn {
> > > > >         /* Use io_uring for communication */
> > > > >         bool io_uring:1;
> > > > >
> > > > > +       /* Allow splice for reading user pages */
> > > > > +       bool splice_read_user_pages:1;
> > > > > +
> > > > >         /** Maximum stack depth for passthrough backing files */
> > > > >         int max_stack_depth;
> > > > >
> > > > > diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> > > > > index 43b6643635ee..e82e96800fde 100644
> > > > > --- a/fs/fuse/inode.c
> > > > > +++ b/fs/fuse/inode.c
> > > > > @@ -1439,6 +1439,9 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
> > > > >
> > > > >                         if (flags & FUSE_REQUEST_TIMEOUT)
> > > > >                                 timeout = arg->request_timeout;
> > > > > +
> > > > > +                       if (flags & FUSE_SPLICE_READ_USER_PAGES)
> > > > > +                               fc->splice_read_user_pages = true;
> > > >
> > > >
> > > > Shouldn't that check for capable(CAP_SYS_ADMIN)? Isn't the issue
> > > > that one can access file content although the write is already
> > > > marked as completed? I.e. a fuse file system might get data
> > > > it was never exposed to and possibly secret data?
> > > > A more complex version version could check for CAP_SYS_ADMIN, but
> > > > also allow later on read/write to files that have the same uid as
> > > > the fuser-server process?
> > > >
> > >
> > > IDGI. I don't see how this allows the server access to something it
> > > didn't have access to before.
> > >
> > > This patchset seems to be about a "contract" between the kernel and the
> > > userland server. The server is agreeing to be very careful about not
> > > touching pages after a write request completes, and the kernel allows
> > > splicing the pages if that's the case.
> > >
> > > Can you explain the potential attack vector?
> >
> > Let's the server claim it does FUSE_SPLICE_READ_USER_PAGES, i.e. claims
> > it stops using splice buffers before completing write requests. But then
> > it actually first replies to the write and after an arbitrary amount
> > of time writes out the splice buffer. User application might be using
> > the buffer it send for write for other things and might not want to
> > expose that. I.e. application expects that after write(, buf,)
> > it can use 'buf' for other purposes and that the file system does not
> > access it anymore once write() is complete. I.e. it can put sensitive
> > data into the buffer, which it might not want to expose.
> > From my point of the issue is mainly with allow_other in combination
> > with "user_allow_other" in libfuse, as root has better ways to access data.
> >
>
> That would definitely break the contract. The question is whether
> that's a security issue. I'm not convinced that it is, given all of the
> other ways you can do nefarious stuff with an unprivileged server.
>
> That said, if there is any question, gating this behind CAP_SYS_ADMIN
> seems like a reasonable thing to do. We could always relax that later
> if we decide that it's a non-issue.

I'm not convinced it is either, especially with allow_other +
user_allow_other needing root privilege to opt into it,  but I also
don't feel strongly enough about it to insist on it and I think it's
better to be safe than sorry. That's a great point about being able to
relax this in the future.

I'll add CAP_SYS_ADMIN gating in v2.


Thanks,
Joanne

>
> --
> Jeff Layton <jlayton@kernel.org>