[PATCH v2 0/3] Avert possible deadlock with splice() and fanotify

[PATCH v2 0/3] Avert possible deadlock with splice() and fanotify

[Amir Goldstein]

Christian,

Josef has helped me see the light and figure out how to avoid the
possible deadlock, which involves:
- splice() from source file in a loop mounted fs to dest file in
  a host fs, where the loop image file is
- fsfreeze on host fs
- write to host fs in context of fanotify permission event handler
  (FAN_ACCESS_PERM) on the splice source file

The first patch should not be changing any logic.
I only build tested the ceph patch, so hoping to get an
Acked-by/Tested-by from Jeff.

The following patches rids us of the deadlock by not holding
file_start_write() while reading from splice source file in the
cases where source and destination can be on different arbitrary
filesystems.

The patches apply and tested on top of vfs.rw branch.

Thanks,
Amir.

Changes since v1:
- Add patch to deal with nfsd/ksmbd server-side-copy
- Shorten helper name to splice_file_range()
- Added assertion for flags value in generic_copy_file_range()
- Added RVB from Jan

Amir Goldstein (3):
  fs: fork splice_file_range() from do_splice_direct()
  fs: move file_start_write() into direct_splice_actor()
  fs: use do_splice_direct() for nfsd/ksmbd server-side-copy

 fs/ceph/file.c         |  9 +++--
 fs/overlayfs/copy_up.c |  2 -
 fs/read_write.c        | 42 +++++++++++---------
 fs/splice.c            | 88 +++++++++++++++++++++++++++++++-----------
 include/linux/fs.h     |  2 -
 include/linux/splice.h | 13 ++++---
 6 files changed, 103 insertions(+), 53 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Amir Goldstein]

In preparation of calling do_splice_direct() without file_start_write()
held, create a new helper splice_file_range(), to be called from context
of ->copy_file_range() methods instead of do_splice_direct().

Currently, the only difference is that splice_file_range() does not take
flags argument and that it asserts that file_start_write() is held, but
we factor out a common helper do_splice_direct_actor() that will be used
later.

Use the new helper from __ceph_copy_file_range(), that was incorrectly
passing to do_splice_direct() the copy flags argument as splice flags.
The value of copy flags in ceph is always 0, so it is a smenatic bug fix.

Move the declaration of both helpers to linux/splice.h.

Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ceph/file.c         |  9 +++---
 fs/read_write.c        |  6 ++--
 fs/splice.c            | 71 ++++++++++++++++++++++++++++++------------
 include/linux/fs.h     |  2 --
 include/linux/splice.h | 13 +++++---
 5 files changed, 66 insertions(+), 35 deletions(-)

diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 3b5aae29e944..f11de6e1f1c1 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -12,6 +12,7 @@
 #include <linux/falloc.h>
 #include <linux/iversion.h>
 #include <linux/ktime.h>
+#include <linux/splice.h>
 
 #include "super.h"
 #include "mds_client.h"
@@ -3010,8 +3011,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 		 * {read,write}_iter, which will get caps again.
 		 */
 		put_rd_wr_caps(src_ci, src_got, dst_ci, dst_got);
-		ret = do_splice_direct(src_file, &src_off, dst_file,
-				       &dst_off, src_objlen, flags);
+		ret = splice_file_range(src_file, &src_off, dst_file, &dst_off,
+					src_objlen);
 		/* Abort on short copies or on error */
 		if (ret < (long)src_objlen) {
 			doutc(cl, "Failed partial copy (%zd)\n", ret);
@@ -3065,8 +3066,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 	 */
 	if (len && (len < src_ci->i_layout.object_size)) {
 		doutc(cl, "Final partial copy of %zu bytes\n", len);
-		bytes = do_splice_direct(src_file, &src_off, dst_file,
-					 &dst_off, len, flags);
+		bytes = splice_file_range(src_file, &src_off, dst_file,
+					  &dst_off, len);
 		if (bytes > 0)
 			ret += bytes;
 		else
diff --git a/fs/read_write.c b/fs/read_write.c
index f791555fa246..642c7ce1ced1 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1423,10 +1423,8 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
 				struct file *file_out, loff_t pos_out,
 				size_t len, unsigned int flags)
 {
-	lockdep_assert(file_write_started(file_out));
-
-	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
-				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
+	return splice_file_range(file_in, &pos_in, file_out, &pos_out,
+				 min_t(size_t, len, MAX_RW_COUNT));
 }
 EXPORT_SYMBOL(generic_copy_file_range);
 
diff --git a/fs/splice.c b/fs/splice.c
index 3fce5f6072dd..9007b2c8baa8 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1170,25 +1170,10 @@ static void direct_file_splice_eof(struct splice_desc *sd)
 		file->f_op->splice_eof(file);
 }
 
-/**
- * do_splice_direct - splices data directly between two files
- * @in:		file to splice from
- * @ppos:	input file offset
- * @out:	file to splice to
- * @opos:	output file offset
- * @len:	number of bytes to splice
- * @flags:	splice modifier flags
- *
- * Description:
- *    For use by do_sendfile(). splice can easily emulate sendfile, but
- *    doing it in the application would incur an extra system call
- *    (splice in + splice out, as compared to just sendfile()). So this helper
- *    can splice directly through a process-private pipe.
- *
- * Callers already called rw_verify_area() on the entire range.
- */
-long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
-		      loff_t *opos, size_t len, unsigned int flags)
+static long do_splice_direct_actor(struct file *in, loff_t *ppos,
+				   struct file *out, loff_t *opos,
+				   size_t len, unsigned int flags,
+				   splice_direct_actor *actor)
 {
 	struct splice_desc sd = {
 		.len		= len,
@@ -1207,14 +1192,60 @@ long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
 	if (unlikely(out->f_flags & O_APPEND))
 		return -EINVAL;
 
-	ret = splice_direct_to_actor(in, &sd, direct_splice_actor);
+	ret = splice_direct_to_actor(in, &sd, actor);
 	if (ret > 0)
 		*ppos = sd.pos;
 
 	return ret;
 }
+/**
+ * do_splice_direct - splices data directly between two files
+ * @in:		file to splice from
+ * @ppos:	input file offset
+ * @out:	file to splice to
+ * @opos:	output file offset
+ * @len:	number of bytes to splice
+ * @flags:	splice modifier flags
+ *
+ * Description:
+ *    For use by do_sendfile(). splice can easily emulate sendfile, but
+ *    doing it in the application would incur an extra system call
+ *    (splice in + splice out, as compared to just sendfile()). So this helper
+ *    can splice directly through a process-private pipe.
+ *
+ * Callers already called rw_verify_area() on the entire range.
+ */
+long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
+		      loff_t *opos, size_t len, unsigned int flags)
+{
+	return do_splice_direct_actor(in, ppos, out, opos, len, flags,
+				      direct_splice_actor);
+}
 EXPORT_SYMBOL(do_splice_direct);
 
+/**
+ * splice_file_range - splices data between two files for copy_file_range()
+ * @in:		file to splice from
+ * @ppos:	input file offset
+ * @out:	file to splice to
+ * @opos:	output file offset
+ * @len:	number of bytes to splice
+ *
+ * Description:
+ *    For use by generic_copy_file_range() and ->copy_file_range() methods.
+ *
+ * Callers already called rw_verify_area() on the entire range.
+ */
+long splice_file_range(struct file *in, loff_t *ppos, struct file *out,
+		       loff_t *opos, size_t len)
+{
+	lockdep_assert(file_write_started(out));
+
+	return do_splice_direct_actor(in, ppos, out, opos, len, 0,
+				      direct_splice_actor);
+}
+EXPORT_SYMBOL(splice_file_range);
+
 static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
 {
 	for (;;) {
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ae0e2fb7bcea..04422a0eccdd 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3052,8 +3052,6 @@ ssize_t copy_splice_read(struct file *in, loff_t *ppos,
 			 size_t len, unsigned int flags);
 extern ssize_t iter_file_splice_write(struct pipe_inode_info *,
 		struct file *, loff_t *, size_t, unsigned int);
-extern long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
-		loff_t *opos, size_t len, unsigned int flags);
 
 
 extern void
diff --git a/include/linux/splice.h b/include/linux/splice.h
index 6c461573434d..49532d5dda52 100644
--- a/include/linux/splice.h
+++ b/include/linux/splice.h
@@ -80,11 +80,14 @@ extern ssize_t add_to_pipe(struct pipe_inode_info *,
 long vfs_splice_read(struct file *in, loff_t *ppos,
 		     struct pipe_inode_info *pipe, size_t len,
 		     unsigned int flags);
-extern ssize_t splice_direct_to_actor(struct file *, struct splice_desc *,
-				      splice_direct_actor *);
-extern long do_splice(struct file *in, loff_t *off_in,
-		      struct file *out, loff_t *off_out,
-		      size_t len, unsigned int flags);
+ssize_t splice_direct_to_actor(struct file *file, struct splice_desc *sd,
+			       splice_direct_actor *actor);
+long do_splice(struct file *in, loff_t *off_in, struct file *out,
+	       loff_t *off_out, size_t len, unsigned int flags);
+long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
+		      loff_t *opos, size_t len, unsigned int flags);
+long splice_file_range(struct file *in, loff_t *ppos, struct file *out,
+		       loff_t *opos, size_t len);
 
 extern long do_tee(struct file *in, struct file *out, size_t len,
 		   unsigned int flags);
-- 
2.34.1

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Jeff Layton]

On Thu, 2023-11-30 at 16:16 +0200, Amir Goldstein wrote:
> In preparation of calling do_splice_direct() without file_start_write()
> held, create a new helper splice_file_range(), to be called from context
> of ->copy_file_range() methods instead of do_splice_direct().
> 
> Currently, the only difference is that splice_file_range() does not take
> flags argument and that it asserts that file_start_write() is held, but
> we factor out a common helper do_splice_direct_actor() that will be used
> later.
> 
> Use the new helper from __ceph_copy_file_range(), that was incorrectly
> passing to do_splice_direct() the copy flags argument as splice flags.
> The value of copy flags in ceph is always 0, so it is a smenatic bug fix.
> 
> Move the declaration of both helpers to linux/splice.h.
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/ceph/file.c         |  9 +++---
>  fs/read_write.c        |  6 ++--
>  fs/splice.c            | 71 ++++++++++++++++++++++++++++++------------
>  include/linux/fs.h     |  2 --
>  include/linux/splice.h | 13 +++++---
>  5 files changed, 66 insertions(+), 35 deletions(-)
> 
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 3b5aae29e944..f11de6e1f1c1 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -12,6 +12,7 @@
>  #include <linux/falloc.h>
>  #include <linux/iversion.h>
>  #include <linux/ktime.h>
> +#include <linux/splice.h>
>  
>  #include "super.h"
>  #include "mds_client.h"
> @@ -3010,8 +3011,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>  		 * {read,write}_iter, which will get caps again.
>  		 */
>  		put_rd_wr_caps(src_ci, src_got, dst_ci, dst_got);
> -		ret = do_splice_direct(src_file, &src_off, dst_file,
> -				       &dst_off, src_objlen, flags);
> +		ret = splice_file_range(src_file, &src_off, dst_file, &dst_off,
> +					src_objlen);
>  		/* Abort on short copies or on error */
>  		if (ret < (long)src_objlen) {
>  			doutc(cl, "Failed partial copy (%zd)\n", ret);
> @@ -3065,8 +3066,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
>  	 */
>  	if (len && (len < src_ci->i_layout.object_size)) {
>  		doutc(cl, "Final partial copy of %zu bytes\n", len);
> -		bytes = do_splice_direct(src_file, &src_off, dst_file,
> -					 &dst_off, len, flags);
> +		bytes = splice_file_range(src_file, &src_off, dst_file,
> +					  &dst_off, len);
>  		if (bytes > 0)
>  			ret += bytes;
>  		else
> diff --git a/fs/read_write.c b/fs/read_write.c
> index f791555fa246..642c7ce1ced1 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1423,10 +1423,8 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
>  				struct file *file_out, loff_t pos_out,
>  				size_t len, unsigned int flags)
>  {
> -	lockdep_assert(file_write_started(file_out));
> -
> -	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> -				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
> +	return splice_file_range(file_in, &pos_in, file_out, &pos_out,
> +				 min_t(size_t, len, MAX_RW_COUNT));
>  }
>  EXPORT_SYMBOL(generic_copy_file_range);
>  
> diff --git a/fs/splice.c b/fs/splice.c
> index 3fce5f6072dd..9007b2c8baa8 100644
> --- a/fs/splice.c
> +++ b/fs/splice.c
> @@ -1170,25 +1170,10 @@ static void direct_file_splice_eof(struct splice_desc *sd)
>  		file->f_op->splice_eof(file);
>  }
>  
> -/**
> - * do_splice_direct - splices data directly between two files
> - * @in:		file to splice from
> - * @ppos:	input file offset
> - * @out:	file to splice to
> - * @opos:	output file offset
> - * @len:	number of bytes to splice
> - * @flags:	splice modifier flags
> - *
> - * Description:
> - *    For use by do_sendfile(). splice can easily emulate sendfile, but
> - *    doing it in the application would incur an extra system call
> - *    (splice in + splice out, as compared to just sendfile()). So this helper
> - *    can splice directly through a process-private pipe.
> - *
> - * Callers already called rw_verify_area() on the entire range.
> - */
> -long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
> -		      loff_t *opos, size_t len, unsigned int flags)
> +static long do_splice_direct_actor(struct file *in, loff_t *ppos,
> +				   struct file *out, loff_t *opos,
> +				   size_t len, unsigned int flags,
> +				   splice_direct_actor *actor)
>  {
>  	struct splice_desc sd = {
>  		.len		= len,
> @@ -1207,14 +1192,60 @@ long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
>  	if (unlikely(out->f_flags & O_APPEND))
>  		return -EINVAL;
>  
> -	ret = splice_direct_to_actor(in, &sd, direct_splice_actor);
> +	ret = splice_direct_to_actor(in, &sd, actor);
>  	if (ret > 0)
>  		*ppos = sd.pos;
>  
>  	return ret;
>  }
> +/**
> + * do_splice_direct - splices data directly between two files
> + * @in:		file to splice from
> + * @ppos:	input file offset
> + * @out:	file to splice to
> + * @opos:	output file offset
> + * @len:	number of bytes to splice
> + * @flags:	splice modifier flags
> + *
> + * Description:
> + *    For use by do_sendfile(). splice can easily emulate sendfile, but
> + *    doing it in the application would incur an extra system call
> + *    (splice in + splice out, as compared to just sendfile()). So this helper
> + *    can splice directly through a process-private pipe.
> + *
> + * Callers already called rw_verify_area() on the entire range.
> + */
> +long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
> +		      loff_t *opos, size_t len, unsigned int flags)
> +{
> +	return do_splice_direct_actor(in, ppos, out, opos, len, flags,
> +				      direct_splice_actor);
> +}
>  EXPORT_SYMBOL(do_splice_direct);
>  
> +/**
> + * splice_file_range - splices data between two files for copy_file_range()
> + * @in:		file to splice from
> + * @ppos:	input file offset
> + * @out:	file to splice to
> + * @opos:	output file offset
> + * @len:	number of bytes to splice
> + *
> + * Description:
> + *    For use by generic_copy_file_range() and ->copy_file_range() methods.
> + *
> + * Callers already called rw_verify_area() on the entire range.
> + */
> +long splice_file_range(struct file *in, loff_t *ppos, struct file *out,
> +		       loff_t *opos, size_t len)
> +{
> +	lockdep_assert(file_write_started(out));
> +
> +	return do_splice_direct_actor(in, ppos, out, opos, len, 0,
> +				      direct_splice_actor);
> +}
> +EXPORT_SYMBOL(splice_file_range);
> +
>  static int wait_for_space(struct pipe_inode_info *pipe, unsigned flags)
>  {
>  	for (;;) {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ae0e2fb7bcea..04422a0eccdd 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3052,8 +3052,6 @@ ssize_t copy_splice_read(struct file *in, loff_t *ppos,
>  			 size_t len, unsigned int flags);
>  extern ssize_t iter_file_splice_write(struct pipe_inode_info *,
>  		struct file *, loff_t *, size_t, unsigned int);
> -extern long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
> -		loff_t *opos, size_t len, unsigned int flags);
>  
>  
>  extern void
> diff --git a/include/linux/splice.h b/include/linux/splice.h
> index 6c461573434d..49532d5dda52 100644
> --- a/include/linux/splice.h
> +++ b/include/linux/splice.h
> @@ -80,11 +80,14 @@ extern ssize_t add_to_pipe(struct pipe_inode_info *,
>  long vfs_splice_read(struct file *in, loff_t *ppos,
>  		     struct pipe_inode_info *pipe, size_t len,
>  		     unsigned int flags);
> -extern ssize_t splice_direct_to_actor(struct file *, struct splice_desc *,
> -				      splice_direct_actor *);
> -extern long do_splice(struct file *in, loff_t *off_in,
> -		      struct file *out, loff_t *off_out,
> -		      size_t len, unsigned int flags);
> +ssize_t splice_direct_to_actor(struct file *file, struct splice_desc *sd,
> +			       splice_direct_actor *actor);
> +long do_splice(struct file *in, loff_t *off_in, struct file *out,
> +	       loff_t *off_out, size_t len, unsigned int flags);
> +long do_splice_direct(struct file *in, loff_t *ppos, struct file *out,
> +		      loff_t *opos, size_t len, unsigned int flags);
> +long splice_file_range(struct file *in, loff_t *ppos, struct file *out,
> +		       loff_t *opos, size_t len);
>  
>  extern long do_tee(struct file *in, struct file *out, size_t len,
>  		   unsigned int flags);

Looks OK to me:

Acked-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Christoph Hellwig]

>  		put_rd_wr_caps(src_ci, src_got, dst_ci, dst_got);
> -		ret = do_splice_direct(src_file, &src_off, dst_file,
> -				       &dst_off, src_objlen, flags);
> +		ret = splice_file_range(src_file, &src_off, dst_file, &dst_off,
> +					src_objlen);

Shouldb't ceph be switched to use generic_copy_file_range?
That does the capping of the size which we want, and doesn't update
the file offsets, which would require recalculation in the ceph code.

But this could avoid another exported API as splice_file_range could
simply be folded into generic_copy_file_range which should reduce
confusion.  And splice really is a mess for so many different layers
of the onion being exposed.  I've been wanting to reduce some of that
for a while but haven't found a really nice way yet.

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 09:37:33AM +0100, Christoph Hellwig wrote:
> >  		put_rd_wr_caps(src_ci, src_got, dst_ci, dst_got);
> > -		ret = do_splice_direct(src_file, &src_off, dst_file,
> > -				       &dst_off, src_objlen, flags);
> > +		ret = splice_file_range(src_file, &src_off, dst_file, &dst_off,
> > +					src_objlen);
> 
> Shouldb't ceph be switched to use generic_copy_file_range?
> That does the capping of the size which we want, and doesn't update
> the file offsets, which would require recalculation in the ceph code.
> 
> But this could avoid another exported API as splice_file_range could
> simply be folded into generic_copy_file_range which should reduce
> confusion.  And splice really is a mess for so many different layers
> of the onion being exposed.  I've been wanting to reduce some of that
> for a while but haven't found a really nice way yet.

(and generic_copy_file_range really should be renamed to
splice_copy_file_range and moved to splice.c)

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Amir Goldstein]

On Mon, Dec 4, 2023 at 10:38 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Dec 04, 2023 at 09:37:33AM +0100, Christoph Hellwig wrote:
> > >             put_rd_wr_caps(src_ci, src_got, dst_ci, dst_got);
> > > -           ret = do_splice_direct(src_file, &src_off, dst_file,
> > > -                                  &dst_off, src_objlen, flags);
> > > +           ret = splice_file_range(src_file, &src_off, dst_file, &dst_off,
> > > +                                   src_objlen);
> >
> > Shouldn't ceph be switched to use generic_copy_file_range?
> > That does the capping of the size which we want, and doesn't update
> > the file offsets, which would require recalculation in the ceph code.
> >

IDK. I did not want to change the logic of the ceph code.
I am not sure that we must impose MAX_RW_COUNT limit on ceph,
although, i_layout.object_size may already be limited? Jeff?

> > But this could avoid another exported API as splice_file_range could
> > simply be folded into generic_copy_file_range which should reduce
> > confusion.  And splice really is a mess for so many different layers
> > of the onion being exposed.  I've been wanting to reduce some of that
> > for a while but haven't found a really nice way yet.
>
> (and generic_copy_file_range really should be renamed to
> splice_copy_file_range and moved to splice.c)

That depends if we are keeping two helpers.
One with a cap of MAX_RW_COUNT and one without.
If we are going to keep two helpers, I'd rather keep things as they are.
If one helper, then I personally prefer splice_file_range() over
splice_copy_file_range() and other reviewers (Jan) liked this
name as well.

Thanks,
Amir.

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 03:29:43PM +0200, Amir Goldstein wrote:
> > > Shouldn't ceph be switched to use generic_copy_file_range?
> > > That does the capping of the size which we want, and doesn't update
> > > the file offsets, which would require recalculation in the ceph code.
> > >
> 
> IDK. I did not want to change the logic of the ceph code.
> I am not sure that we must impose MAX_RW_COUNT limit on ceph,
> although, i_layout.object_size may already be limited? Jeff?

We better don't go beyond it, as it is called from the copy_file_range
implementation which is expected to never return more than MAX_RW_COUNT.
So either it is a noop change, or it fixes a bug.

> 
> > > But this could avoid another exported API as splice_file_range could
> > > simply be folded into generic_copy_file_range which should reduce
> > > confusion.  And splice really is a mess for so many different layers
> > > of the onion being exposed.  I've been wanting to reduce some of that
> > > for a while but haven't found a really nice way yet.
> >
> > (and generic_copy_file_range really should be renamed to
> > splice_copy_file_range and moved to splice.c)
> 
> That depends if we are keeping two helpers.
> One with a cap of MAX_RW_COUNT and one without.
> If we are going to keep two helpers, I'd rather keep things as they are.
> If one helper, then I personally prefer splice_file_range() over
> splice_copy_file_range() and other reviewers (Jan) liked this
> name as well.

Well, splice_file_range makes sense if it is a separate helper.  But when
is the default implementation for ->copy_file_range and matches the
signature, naming it that way is not only sensible but required to keep
sanity.

> 
> Thanks,
> Amir.
---end quoted text---

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Amir Goldstein]

On Mon, Dec 4, 2023 at 4:07 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Dec 04, 2023 at 03:29:43PM +0200, Amir Goldstein wrote:
> > > > Shouldn't ceph be switched to use generic_copy_file_range?
> > > > That does the capping of the size which we want, and doesn't update
> > > > the file offsets, which would require recalculation in the ceph code.
> > > >
> >
> > IDK. I did not want to change the logic of the ceph code.
> > I am not sure that we must impose MAX_RW_COUNT limit on ceph,
> > although, i_layout.object_size may already be limited? Jeff?
>
> We better don't go beyond it, as it is called from the copy_file_range
> implementation which is expected to never return more than MAX_RW_COUNT.
> So either it is a noop change, or it fixes a bug.
>

ok.

> >
> > > > But this could avoid another exported API as splice_file_range could
> > > > simply be folded into generic_copy_file_range which should reduce
> > > > confusion.  And splice really is a mess for so many different layers
> > > > of the onion being exposed.  I've been wanting to reduce some of that
> > > > for a while but haven't found a really nice way yet.
> > >
> > > (and generic_copy_file_range really should be renamed to
> > > splice_copy_file_range and moved to splice.c)
> >
> > That depends if we are keeping two helpers.
> > One with a cap of MAX_RW_COUNT and one without.
> > If we are going to keep two helpers, I'd rather keep things as they are.
> > If one helper, then I personally prefer splice_file_range() over
> > splice_copy_file_range() and other reviewers (Jan) liked this
> > name as well.
>
> Well, splice_file_range makes sense if it is a separate helper.  But when
> is the default implementation for ->copy_file_range and matches the
> signature, naming it that way is not only sensible but required to keep
> sanity.
>

It is NOT a default implementation of ->copy_file_range(), but
a fallback helper.
Specifically, it is never expected to have a filesystem that does
.copy_file_range = generic_copy_file_range,
so getting rid of generic_copy_file_range() would be good.

Note also that generic_copy_file_range() gets a flags argument
that is COPY_FILE_* flags (currently only for the vfs level)
and this flags argument is NOT the splice flags argument, so
I intentionally removed the flags argument from splice_file_range()
to reduce confusion.

I like the idea of moving MAX_RW_COUNT into splice_file_range()
and replacing generic_copy_file_range() calls with splice_file_range().

I do not feel strongly against splice_copy_file_range() name, but
I would like to get feedback from other reviewers that approved the
name splice_file_range() before changing it.

Thanks,
Amir.

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Jan Kara]

On Mon 04-12-23 16:29:02, Amir Goldstein wrote:
> On Mon, Dec 4, 2023 at 4:07 PM Christoph Hellwig <hch@lst.de> wrote:
> > On Mon, Dec 04, 2023 at 03:29:43PM +0200, Amir Goldstein wrote:
> > Well, splice_file_range makes sense if it is a separate helper.  But when
> > is the default implementation for ->copy_file_range and matches the
> > signature, naming it that way is not only sensible but required to keep
> > sanity.
> >
> 
> It is NOT a default implementation of ->copy_file_range(), but
> a fallback helper.
> Specifically, it is never expected to have a filesystem that does
> .copy_file_range = generic_copy_file_range,
> so getting rid of generic_copy_file_range() would be good.
> 
> Note also that generic_copy_file_range() gets a flags argument
> that is COPY_FILE_* flags (currently only for the vfs level)
> and this flags argument is NOT the splice flags argument, so
> I intentionally removed the flags argument from splice_file_range()
> to reduce confusion.
> 
> I like the idea of moving MAX_RW_COUNT into splice_file_range()
> and replacing generic_copy_file_range() calls with splice_file_range().
> 
> I do not feel strongly against splice_copy_file_range() name, but
> I would like to get feedback from other reviewers that approved the
> name splice_file_range() before changing it.

For me the name is not a big deal either way.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 1/3] fs: fork splice_file_range() from do_splice_direct()

[Amir Goldstein]

On Mon, Dec 4, 2023 at 7:16 PM Jan Kara <jack@suse.cz> wrote:
>
> On Mon 04-12-23 16:29:02, Amir Goldstein wrote:
> > On Mon, Dec 4, 2023 at 4:07 PM Christoph Hellwig <hch@lst.de> wrote:
> > > On Mon, Dec 04, 2023 at 03:29:43PM +0200, Amir Goldstein wrote:
> > > Well, splice_file_range makes sense if it is a separate helper.  But when
> > > is the default implementation for ->copy_file_range and matches the
> > > signature, naming it that way is not only sensible but required to keep
> > > sanity.
> > >
> >
> > It is NOT a default implementation of ->copy_file_range(), but
> > a fallback helper.
> > Specifically, it is never expected to have a filesystem that does
> > .copy_file_range = generic_copy_file_range,
> > so getting rid of generic_copy_file_range() would be good.
> >
> > Note also that generic_copy_file_range() gets a flags argument
> > that is COPY_FILE_* flags (currently only for the vfs level)
> > and this flags argument is NOT the splice flags argument, so
> > I intentionally removed the flags argument from splice_file_range()
> > to reduce confusion.
> >
> > I like the idea of moving MAX_RW_COUNT into splice_file_range()
> > and replacing generic_copy_file_range() calls with splice_file_range().
> >
> > I do not feel strongly against splice_copy_file_range() name, but
> > I would like to get feedback from other reviewers that approved the
> > name splice_file_range() before changing it.
>
> For me the name is not a big deal either way.

I would rather add this inline wrapper than uniting the two helpers:

static inline long splice_copy_file_range(struct file *in, loff_t pos_in,
                                          struct file *out, loff_t pos_out,
                                          size_t len)
{
        return splice_file_range(in, &pos_in, out, &pos_out,
                                      min_t(size_t, len, MAX_RW_COUNT));
}

It is keeping the same signature as copy_file_range() minus flags,
to be used for ->copy_file_range() fallback and keeps the current
splice_file_range() helpers for special cases as ceph that want to
update the in/out offset.

Thanks,
Amir.

[PATCH v2 2/3] fs: move file_start_write() into direct_splice_actor()

[Amir Goldstein]

The callers of do_splice_direct() hold file_start_write() on the output
file.

This may cause file permission hooks to be called indirectly on an
overlayfs lower layer, which is on the same filesystem of the output
file and could lead to deadlock with fanotify permission events.

To fix this potential deadlock, move file_start_write() from the callers
into the direct_splice_actor(), so file_start_write() will not be held
while splicing from the input file.

Suggested-by: Josef Bacik <josef@toxicpanda.com>
Link: https://lore.kernel.org/r/20231128214258.GA2398475@perftesting/
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/copy_up.c |  2 --
 fs/read_write.c        |  2 --
 fs/splice.c            | 19 ++++++++++++++++---
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 7a44c8212331..294b330aba9f 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -333,11 +333,9 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry,
 		if (error)
 			break;
 
-		ovl_start_write(dentry);
 		bytes = do_splice_direct(old_file, &old_pos,
 					 new_file, &new_pos,
 					 this_len, SPLICE_F_MOVE);
-		ovl_end_write(dentry);
 		if (bytes <= 0) {
 			error = bytes;
 			break;
diff --git a/fs/read_write.c b/fs/read_write.c
index 642c7ce1ced1..0bc99f38e623 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1286,10 +1286,8 @@ static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
 		retval = rw_verify_area(WRITE, out.file, &out_pos, count);
 		if (retval < 0)
 			goto fput_out;
-		file_start_write(out.file);
 		retval = do_splice_direct(in.file, &pos, out.file, &out_pos,
 					  count, fl);
-		file_end_write(out.file);
 	} else {
 		if (out.file->f_flags & O_NONBLOCK)
 			fl |= SPLICE_F_NONBLOCK;
diff --git a/fs/splice.c b/fs/splice.c
index 9007b2c8baa8..7cda013e5a1e 100644
--- a/fs/splice.c
+++ b/fs/splice.c
@@ -1157,9 +1157,20 @@ static int direct_splice_actor(struct pipe_inode_info *pipe,
 			       struct splice_desc *sd)
 {
 	struct file *file = sd->u.file;
+	long ret;
+
+	file_start_write(file);
+	ret = do_splice_from(pipe, file, sd->opos, sd->total_len, sd->flags);
+	file_end_write(file);
+	return ret;
+}
 
-	return do_splice_from(pipe, file, sd->opos, sd->total_len,
-			      sd->flags);
+static int splice_file_range_actor(struct pipe_inode_info *pipe,
+					struct splice_desc *sd)
+{
+	struct file *file = sd->u.file;
+
+	return do_splice_from(pipe, file, sd->opos, sd->total_len, sd->flags);
 }
 
 static void direct_file_splice_eof(struct splice_desc *sd)
@@ -1233,6 +1244,8 @@ EXPORT_SYMBOL(do_splice_direct);
  *
  * Description:
  *    For use by generic_copy_file_range() and ->copy_file_range() methods.
+ *    Like do_splice_direct(), but vfs_copy_file_range() already holds
+ *    start_file_write() on @out file.
  *
  * Callers already called rw_verify_area() on the entire range.
  */
@@ -1242,7 +1255,7 @@ long splice_file_range(struct file *in, loff_t *ppos, struct file *out,
 	lockdep_assert(file_write_started(out));
 
 	return do_splice_direct_actor(in, ppos, out, opos, len, 0,
-				      direct_splice_actor);
+				      splice_file_range_actor);
 }
 EXPORT_SYMBOL(splice_file_range);
 
-- 
2.34.1

Re: [PATCH v2 2/3] fs: move file_start_write() into direct_splice_actor()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH v2 3/3] fs: use do_splice_direct() for nfsd/ksmbd server-side-copy

[Amir Goldstein]

nfsd/ksmbd call vfs_copy_file_range() with flag COPY_FILE_SPLICE to
perform kernel copy between two files on any two filesystems.

Splicing input file, while holding file_start_write() on the output file
which is on a different sb, posses a risk for fanotify related deadlocks.

We only need to call splice_file_range() from within the context of
->copy_file_range() filesystem methods with file_start_write() held.

To avoid the possible deadlocks, always use do_splice_direct() instead of
splice_file_range() for the kernel copy fallback in vfs_copy_file_range()
without holding file_start_write().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/read_write.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index 0bc99f38e623..e0c2c1b5962b 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1421,6 +1421,10 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
 				struct file *file_out, loff_t pos_out,
 				size_t len, unsigned int flags)
 {
+	/* May only be called from within ->copy_file_range() methods */
+	if (WARN_ON_ONCE(flags))
+		return -EINVAL;
+
 	return splice_file_range(file_in, &pos_in, file_out, &pos_out,
 				 min_t(size_t, len, MAX_RW_COUNT));
 }
@@ -1541,19 +1545,22 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 		ret = file_out->f_op->copy_file_range(file_in, pos_in,
 						      file_out, pos_out,
 						      len, flags);
-		goto done;
-	}
-
-	if (!splice && file_in->f_op->remap_file_range &&
-	    file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
+	} else if (!splice && file_in->f_op->remap_file_range &&
+		   file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
 		ret = file_in->f_op->remap_file_range(file_in, pos_in,
 				file_out, pos_out,
 				min_t(loff_t, MAX_RW_COUNT, len),
 				REMAP_FILE_CAN_SHORTEN);
-		if (ret > 0)
-			goto done;
+		/* fallback to splice */
+		if (ret <= 0)
+			splice = true;
 	}
 
+	file_end_write(file_out);
+
+	if (!splice)
+		goto done;
+
 	/*
 	 * We can get here for same sb copy of filesystems that do not implement
 	 * ->copy_file_range() in case filesystem does not support clone or in
@@ -1565,11 +1572,16 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	 * and which filesystems do not, that will allow userspace tools to
 	 * make consistent desicions w.r.t using copy_file_range().
 	 *
-	 * We also get here if caller (e.g. nfsd) requested COPY_FILE_SPLICE.
+	 * We also get here if caller (e.g. nfsd) requested COPY_FILE_SPLICE
+	 * for server-side-copy between any two sb.
+	 *
+	 * In any case, we call do_splice_direct() and not splice_file_range(),
+	 * without file_start_write() held, to avoid possible deadlocks related
+	 * to splicing from input file, while file_start_write() is held on
+	 * the output file on a different sb.
 	 */
-	ret = generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
-				      flags);
-
+	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
+			       min_t(size_t, len, MAX_RW_COUNT), 0);
 done:
 	if (ret > 0) {
 		fsnotify_access(file_in);
@@ -1581,8 +1593,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	inc_syscr(current);
 	inc_syscw(current);
 
-	file_end_write(file_out);
-
 	return ret;
 }
 EXPORT_SYMBOL(vfs_copy_file_range);
-- 
2.34.1

Re: [PATCH v2 3/3] fs: use do_splice_direct() for nfsd/ksmbd server-side-copy

[Jan Kara]

On Thu 30-11-23 16:16:24, Amir Goldstein wrote:
> nfsd/ksmbd call vfs_copy_file_range() with flag COPY_FILE_SPLICE to
> perform kernel copy between two files on any two filesystems.
> 
> Splicing input file, while holding file_start_write() on the output file
> which is on a different sb, posses a risk for fanotify related deadlocks.
> 
> We only need to call splice_file_range() from within the context of
> ->copy_file_range() filesystem methods with file_start_write() held.
> 
> To avoid the possible deadlocks, always use do_splice_direct() instead of
> splice_file_range() for the kernel copy fallback in vfs_copy_file_range()
> without holding file_start_write().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/read_write.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 0bc99f38e623..e0c2c1b5962b 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1421,6 +1421,10 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
>  				struct file *file_out, loff_t pos_out,
>  				size_t len, unsigned int flags)
>  {
> +	/* May only be called from within ->copy_file_range() methods */
> +	if (WARN_ON_ONCE(flags))
> +		return -EINVAL;
> +
>  	return splice_file_range(file_in, &pos_in, file_out, &pos_out,
>  				 min_t(size_t, len, MAX_RW_COUNT));
>  }
> @@ -1541,19 +1545,22 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  		ret = file_out->f_op->copy_file_range(file_in, pos_in,
>  						      file_out, pos_out,
>  						      len, flags);
> -		goto done;
> -	}
> -
> -	if (!splice && file_in->f_op->remap_file_range &&
> -	    file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
> +	} else if (!splice && file_in->f_op->remap_file_range &&
> +		   file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
>  		ret = file_in->f_op->remap_file_range(file_in, pos_in,
>  				file_out, pos_out,
>  				min_t(loff_t, MAX_RW_COUNT, len),
>  				REMAP_FILE_CAN_SHORTEN);
> -		if (ret > 0)
> -			goto done;
> +		/* fallback to splice */
> +		if (ret <= 0)
> +			splice = true;
>  	}
>  
> +	file_end_write(file_out);
> +
> +	if (!splice)
> +		goto done;
> +
>  	/*
>  	 * We can get here for same sb copy of filesystems that do not implement
>  	 * ->copy_file_range() in case filesystem does not support clone or in
> @@ -1565,11 +1572,16 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	 * and which filesystems do not, that will allow userspace tools to
>  	 * make consistent desicions w.r.t using copy_file_range().
>  	 *
> -	 * We also get here if caller (e.g. nfsd) requested COPY_FILE_SPLICE.
> +	 * We also get here if caller (e.g. nfsd) requested COPY_FILE_SPLICE
> +	 * for server-side-copy between any two sb.
> +	 *
> +	 * In any case, we call do_splice_direct() and not splice_file_range(),
> +	 * without file_start_write() held, to avoid possible deadlocks related
> +	 * to splicing from input file, while file_start_write() is held on
> +	 * the output file on a different sb.
>  	 */
> -	ret = generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
> -				      flags);
> -
> +	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> +			       min_t(size_t, len, MAX_RW_COUNT), 0);
>  done:
>  	if (ret > 0) {
>  		fsnotify_access(file_in);
> @@ -1581,8 +1593,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>  	inc_syscr(current);
>  	inc_syscw(current);
>  
> -	file_end_write(file_out);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL(vfs_copy_file_range);
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 3/3] fs: use do_splice_direct() for nfsd/ksmbd server-side-copy

[Christoph Hellwig]

On Thu, Nov 30, 2023 at 04:16:24PM +0200, Amir Goldstein wrote:
> nfsd/ksmbd call vfs_copy_file_range() with flag COPY_FILE_SPLICE to
> perform kernel copy between two files on any two filesystems.
> 
> Splicing input file, while holding file_start_write() on the output file
> which is on a different sb, posses a risk for fanotify related deadlocks.
> 
> We only need to call splice_file_range() from within the context of
> ->copy_file_range() filesystem methods with file_start_write() held.
> 
> To avoid the possible deadlocks, always use do_splice_direct() instead of
> splice_file_range() for the kernel copy fallback in vfs_copy_file_range()
> without holding file_start_write().

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

(although I wish do_splice_direct had a better name like
vfs_splice_direct, espcially before growing more users)

Re: [PATCH v2 3/3] fs: use do_splice_direct() for nfsd/ksmbd server-side-copy

[Amir Goldstein]

On Mon, Dec 4, 2023 at 10:39 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, Nov 30, 2023 at 04:16:24PM +0200, Amir Goldstein wrote:
> > nfsd/ksmbd call vfs_copy_file_range() with flag COPY_FILE_SPLICE to
> > perform kernel copy between two files on any two filesystems.
> >
> > Splicing input file, while holding file_start_write() on the output file
> > which is on a different sb, posses a risk for fanotify related deadlocks.
> >
> > We only need to call splice_file_range() from within the context of
> > ->copy_file_range() filesystem methods with file_start_write() held.
> >
> > To avoid the possible deadlocks, always use do_splice_direct() instead of
> > splice_file_range() for the kernel copy fallback in vfs_copy_file_range()
> > without holding file_start_write().
>
> Looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> (although I wish do_splice_direct had a better name like
> vfs_splice_direct, espcially before growing more users)

I tried very hard in this series to add a little bit of consistency
for function names and indication of what it may be responsible for.

After this cleanup series, many of the file permission hooks and
moved from do_XXX() helpers to vfs_XXX() helpers, so I cannot in
good conscience rename do_splice_direct(), which does not have
file permission hooks to vfs_splice_direct().

I can rename it to splice_direct() as several other splice_XXX()
exported helpers in this file.

ok?

Thanks,
Amir.

Re: [PATCH v2 3/3] fs: use do_splice_direct() for nfsd/ksmbd server-side-copy

[Christoph Hellwig]

On Mon, Dec 04, 2023 at 03:19:26PM +0200, Amir Goldstein wrote:
> I tried very hard in this series to add a little bit of consistency
> for function names and indication of what it may be responsible for.
> 
> After this cleanup series, many of the file permission hooks and
> moved from do_XXX() helpers to vfs_XXX() helpers, so I cannot in
> good conscience rename do_splice_direct(), which does not have
> file permission hooks to vfs_splice_direct().
> 
> I can rename it to splice_direct() as several other splice_XXX()
> exported helpers in this file.

Let's keep the name for now.  do_ prefixes are not great, especially
for exported functions, but no prefix at all isn't great either.
So let's get your work done and then we can look into introducing
a consistent naming scheme eventually.

[PATCH] fs: read_write: make default in vfs_copy_file_range() reachable

[Bert Karwatzki]

If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
and both file_out->f_op->copy_file_range and file_in->f_op->remap_file_range
are NULL, too, the default call to do_splice_direct() cannot be reached.
This patch adds an else clause to make the default reachable in all
cases.

Signed-off-by: Bert Karwatzki <spasswolf@web.de>
---
 fs/read_write.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/read_write.c b/fs/read_write.c
index e0c2c1b5962b..3599c54bd26d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 		/* fallback to splice */
 		if (ret <= 0)
 			splice = true;
+	} else {
+		splice = true;
 	}

 	file_end_write(file_out);
--
2.39.2

Since linux-next-20231204 I noticed that it was impossible to start the
game Path of Exile (using the steam client). I bisected the error to
commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
in linux-next-20231204 made the game start again and after inserting
printks into vfs_copy_file_range() I found that steam (via python3)
calls this function with (flags & COPY_FILE_SPLICE == 0),
file_out->f_op->copy_file_range == NULL and
file_in->f_op->remap_file_range == NULL so the default is never reached.
This patch adds a catch all else clause so the default is reached in
all cases. This patch fixes the describe issue with steam and Path of
Exile.

Bert Karwatzki

Re: [PATCH] fs: read_write: make default in vfs_copy_file_range() reachable

[Amir Goldstein]

On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <spasswolf@web.de> wrote:
>
> If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
> and both file_out->f_op->copy_file_range and file_in->f_op->remap_file_range
> are NULL, too, the default call to do_splice_direct() cannot be reached.
> This patch adds an else clause to make the default reachable in all
> cases.
>
> Signed-off-by: Bert Karwatzki <spasswolf@web.de>

Hi Bert,

Thank you for testing and reporting this so early!!

I would edit the commit message differently, but anyway, I think that
the fix should be folded into commit 05ee2d85cd4a ("fs: use
do_splice_direct() for nfsd/ksmbd server-side-copy").

Since I end up making a mistake every time I touch this code,
I also added a small edit to your patch below, that should make the logic
more clear to readers. Hopefully, that will help me avoid making a mistake
the next time I touch this code...

Would you mind testing my revised fix, so we can add:
  Tested-by: Bert Karwatzki <spasswolf@web.de>
when folding it into the original patch?

Thanks,
Amir.

> ---
>  fs/read_write.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index e0c2c1b5962b..3599c54bd26d 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
>                 /* fallback to splice */
>                 if (ret <= 0)
>                         splice = true;
> +       } else {

This is logically correct because of the earlier "same sb" check in
generic_copy_file_checks(), but we better spell out the logic here as well:

+            } else if (file_inode(file_in)->i_sb ==
file_inode(file_out)->i_sb) {
+                    /* Fallback to splice for same sb copy for
backward compat */

> +               splice = true;
>         }
>
>         file_end_write(file_out);
> --
> 2.39.2
>
> Since linux-next-20231204 I noticed that it was impossible to start the
> game Path of Exile (using the steam client). I bisected the error to
> commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
> in linux-next-20231204 made the game start again and after inserting
> printks into vfs_copy_file_range() I found that steam (via python3)
> calls this function with (flags & COPY_FILE_SPLICE == 0),
> file_out->f_op->copy_file_range == NULL and
> file_in->f_op->remap_file_range == NULL so the default is never reached.
> This patch adds a catch all else clause so the default is reached in
> all cases. This patch fixes the describe issue with steam and Path of
> Exile.
>
> Bert Karwatzki

Re: [PATCH] fs: read_write: make default in vfs_copy_file_range() reachable

[Amir Goldstein]

[-- Attachment #1: Type: text/plain, Size: 3163 bytes --]

On Tue, Dec 5, 2023 at 5:45 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <spasswolf@web.de> wrote:
> >
> > If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
> > and both file_out->f_op->copy_file_range and file_in->f_op->remap_file_range
> > are NULL, too, the default call to do_splice_direct() cannot be reached.
> > This patch adds an else clause to make the default reachable in all
> > cases.
> >
> > Signed-off-by: Bert Karwatzki <spasswolf@web.de>
>
> Hi Bert,
>
> Thank you for testing and reporting this so early!!
>
> I would edit the commit message differently, but anyway, I think that
> the fix should be folded into commit 05ee2d85cd4a ("fs: use
> do_splice_direct() for nfsd/ksmbd server-side-copy").
>
> Since I end up making a mistake every time I touch this code,
> I also added a small edit to your patch below, that should make the logic
> more clear to readers. Hopefully, that will help me avoid making a mistake
> the next time I touch this code...
>
> Would you mind testing my revised fix, so we can add:
>   Tested-by: Bert Karwatzki <spasswolf@web.de>
> when folding it into the original patch?
>

Attached an even cleaner version of the fix patch for you to test.
I tested fstests check -g copy_range on ext4.
My fault was that I had tested earlier only on xfs and overlayfs
(the two other cases in the if/else if statement).

Thanks,
Amir.

> > ---
> >  fs/read_write.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index e0c2c1b5962b..3599c54bd26d 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> >                 /* fallback to splice */
> >                 if (ret <= 0)
> >                         splice = true;
> > +       } else {
>
> This is logically correct because of the earlier "same sb" check in
> generic_copy_file_checks(), but we better spell out the logic here as well:
>
> +            } else if (file_inode(file_in)->i_sb ==
> file_inode(file_out)->i_sb) {
> +                    /* Fallback to splice for same sb copy for
> backward compat */
>
> > +               splice = true;
> >         }
> >
> >         file_end_write(file_out);
> > --
> > 2.39.2
> >
> > Since linux-next-20231204 I noticed that it was impossible to start the
> > game Path of Exile (using the steam client). I bisected the error to
> > commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
> > in linux-next-20231204 made the game start again and after inserting
> > printks into vfs_copy_file_range() I found that steam (via python3)
> > calls this function with (flags & COPY_FILE_SPLICE == 0),
> > file_out->f_op->copy_file_range == NULL and
> > file_in->f_op->remap_file_range == NULL so the default is never reached.
> > This patch adds a catch all else clause so the default is reached in
> > all cases. This patch fixes the describe issue with steam and Path of
> > Exile.
> >
> > Bert Karwatzki

[-- Attachment #2: 0001-fs-fix-vfs_copy_file_range-for-files-on-same-sb.patch --]
[-- Type: text/x-patch, Size: 1837 bytes --]

From 33eb3caeacb52a46f808984969c808528d10ee01 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Tue, 5 Dec 2023 06:26:57 +0200
Subject: [PATCH] fs: fix vfs_copy_file_range() for files on same sb

copy_file-range() should fallback to splice with two files on same sb
that does not implement ->copy_file_range() nor ->remap_file_range().

Fixes: 042e4d9d17ae ("fs: use splice_copy_file_range() inline helper")
Reported-by: Bert Karwatzki <spasswolf@web.de>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/read_write.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/read_write.c b/fs/read_write.c
index e0c2c1b5962b..01a14570015b 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1514,6 +1514,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 {
 	ssize_t ret;
 	bool splice = flags & COPY_FILE_SPLICE;
+	bool samesb = file_inode(file_in)->i_sb == file_inode(file_out)->i_sb;
 
 	if (flags & ~COPY_FILE_SPLICE)
 		return -EINVAL;
@@ -1545,8 +1546,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 		ret = file_out->f_op->copy_file_range(file_in, pos_in,
 						      file_out, pos_out,
 						      len, flags);
-	} else if (!splice && file_in->f_op->remap_file_range &&
-		   file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
+	} else if (!splice && file_in->f_op->remap_file_range && samesb) {
 		ret = file_in->f_op->remap_file_range(file_in, pos_in,
 				file_out, pos_out,
 				min_t(loff_t, MAX_RW_COUNT, len),
@@ -1554,6 +1554,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 		/* fallback to splice */
 		if (ret <= 0)
 			splice = true;
+	} else if (samesb) {
+		/* Fallback to splice for same sb copy for backward compat */
+		splice = true;
 	}
 
 	file_end_write(file_out);
-- 
2.34.1

Re: [PATCH] fs: read_write: make default in vfs_copy_file_range() reachable

[Bert Karwatzki]

Am Dienstag, dem 05.12.2023 um 07:01 +0200 schrieb Amir Goldstein:
> On Tue, Dec 5, 2023 at 5:45 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <spasswolf@web.de> wrote:
> > >
> > > If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
> > > and both file_out->f_op->copy_file_range and file_in->f_op-
> > > >remap_file_range
> > > are NULL, too, the default call to do_splice_direct() cannot be reached.
> > > This patch adds an else clause to make the default reachable in all
> > > cases.
> > >
> > > Signed-off-by: Bert Karwatzki <spasswolf@web.de>
> >
> > Hi Bert,
> >
> > Thank you for testing and reporting this so early!!
> >
> > I would edit the commit message differently, but anyway, I think that
> > the fix should be folded into commit 05ee2d85cd4a ("fs: use
> > do_splice_direct() for nfsd/ksmbd server-side-copy").
> >
> > Since I end up making a mistake every time I touch this code,
> > I also added a small edit to your patch below, that should make the logic
> > more clear to readers. Hopefully, that will help me avoid making a mistake
> > the next time I touch this code...
> >
> > Would you mind testing my revised fix, so we can add:
> >   Tested-by: Bert Karwatzki <spasswolf@web.de>
> > when folding it into the original patch?
> >
>
> Attached an even cleaner version of the fix patch for you to test.
> I tested fstests check -g copy_range on ext4.
> My fault was that I had tested earlier only on xfs and overlayfs
> (the two other cases in the if/else if statement).
>
> Thanks,
> Amir.
>
> > > ---
> > >  fs/read_write.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > index e0c2c1b5962b..3599c54bd26d 100644
> > > --- a/fs/read_write.c
> > > +++ b/fs/read_write.c
> > > @@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in,
> > > loff_t pos_in,
> > >                 /* fallback to splice */
> > >                 if (ret <= 0)
> > >                         splice = true;
> > > +       } else {
> >
> > This is logically correct because of the earlier "same sb" check in
> > generic_copy_file_checks(), but we better spell out the logic here as well:
> >
> > +            } else if (file_inode(file_in)->i_sb ==
> > file_inode(file_out)->i_sb) {
> > +                    /* Fallback to splice for same sb copy for
> > backward compat */
> >
> > > +               splice = true;
> > >         }
> > >
> > >         file_end_write(file_out);
> > > --
> > > 2.39.2
> > >
> > > Since linux-next-20231204 I noticed that it was impossible to start the
> > > game Path of Exile (using the steam client). I bisected the error to
> > > commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
> > > in linux-next-20231204 made the game start again and after inserting
> > > printks into vfs_copy_file_range() I found that steam (via python3)
> > > calls this function with (flags & COPY_FILE_SPLICE == 0),
> > > file_out->f_op->copy_file_range == NULL and
> > > file_in->f_op->remap_file_range == NULL so the default is never reached.
> > > This patch adds a catch all else clause so the default is reached in
> > > all cases. This patch fixes the describe issue with steam and Path of
> > > Exile.
> > >
> > > Bert Karwatzki

Your new patch works fine (I applied it to linux-next-20231204), again tested by
starting Path of Exile via steam from an ext4 filesystem.

Bert Karwatzki

Re: [PATCH v2 0/3] Avert possible deadlock with splice() and fanotify

[Jeff Layton]

On Thu, 2023-11-30 at 16:16 +0200, Amir Goldstein wrote:
> Christian,
> 
> Josef has helped me see the light and figure out how to avoid the
> possible deadlock, which involves:
> - splice() from source file in a loop mounted fs to dest file in
>   a host fs, where the loop image file is
> - fsfreeze on host fs
> - write to host fs in context of fanotify permission event handler
>   (FAN_ACCESS_PERM) on the splice source file
> 
> The first patch should not be changing any logic.
> I only build tested the ceph patch, so hoping to get an
> Acked-by/Tested-by from Jeff.
> 

Xiubo (cc'ed) is probably in a better position these days to review and
get you test results with this patch. 

> The following patches rids us of the deadlock by not holding
> file_start_write() while reading from splice source file in the
> cases where source and destination can be on different arbitrary
> filesystems.
> 
> The patches apply and tested on top of vfs.rw branch.
> 
> Thanks,
> Amir.
> 
> Changes since v1:
> - Add patch to deal with nfsd/ksmbd server-side-copy
> - Shorten helper name to splice_file_range()
> - Added assertion for flags value in generic_copy_file_range()
> - Added RVB from Jan
> 
> Amir Goldstein (3):
>   fs: fork splice_file_range() from do_splice_direct()
>   fs: move file_start_write() into direct_splice_actor()
>   fs: use do_splice_direct() for nfsd/ksmbd server-side-copy
> 
>  fs/ceph/file.c         |  9 +++--
>  fs/overlayfs/copy_up.c |  2 -
>  fs/read_write.c        | 42 +++++++++++---------
>  fs/splice.c            | 88 +++++++++++++++++++++++++++++++-----------
>  include/linux/fs.h     |  2 -
>  include/linux/splice.h | 13 ++++---
>  6 files changed, 103 insertions(+), 53 deletions(-)
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v2 0/3] Avert possible deadlock with splice() and fanotify

[Christian Brauner]

On Thu, 30 Nov 2023 16:16:21 +0200, Amir Goldstein wrote:
> Christian,
> 
> Josef has helped me see the light and figure out how to avoid the
> possible deadlock, which involves:
> - splice() from source file in a loop mounted fs to dest file in
>   a host fs, where the loop image file is
> - fsfreeze on host fs
> - write to host fs in context of fanotify permission event handler
>   (FAN_ACCESS_PERM) on the splice source file
> 
> [...]

Applied to the vfs.rw branch of the vfs/vfs.git tree.
Patches in the vfs.rw branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.rw

[1/3] fs: fork splice_file_range() from do_splice_direct()
      https://git.kernel.org/vfs/vfs/c/42a29fdc3202
[2/3] fs: move file_start_write() into direct_splice_actor()
      https://git.kernel.org/vfs/vfs/c/8b524ba13575
[3/3] fs: use do_splice_direct() for nfsd/ksmbd server-side-copy
      https://git.kernel.org/vfs/vfs/c/05ee2d85cd4a