Fixes for major copy_file_range() issues

From: Dave Chinner <[email protected]>

Right now if vfs_copy_file_range() does not use any offload mechanism, it falls back to calling do_splice_direct(). This fails to do basic sanity checks on the files being copied. Before we start adding this necessarily functionality to the fallback path, separate it out into generic_copy_file_range().

generic_copy_file_range() has the same prototype as ->copy_file_range() so that filesystems can use it in their custom ->copy_file_range() method if they so choose.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---

 fs/read_write.c    | 35 ++++++++++++++++++++++++++++++++---
 include/linux/fs.h |  3 +++
 2 files changed, 35 insertions(+), 3 deletions(-)

fs/read_write.c +32 -3

index c543d965e288..676b02fae589 100644
@@ -1565,6 +1565,36 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int, out_fd, int, in_fd,
 }
 #endif
 
+/**
+ * generic_copy_file_range - copy data between two files
+ * @file_in:	file structure to read from
+ * @pos_in:	file offset to read from
+ * @file_out:	file structure to write data to
+ * @pos_out:	file offset to write data to
+ * @len:	amount of data to copy
+ * @flags:	copy flags
+ *
+ * This is a generic filesystem helper to copy data from one file to another.
+ * It has no constraints on the source or destination file owners - the files
+ * can belong to different superblocks and different filesystem types. Short
+ * copies are allowed.
+ *
+ * This should be called from the @file_out filesystem, as per the
+ * ->copy_file_range() method.
+ *
+ * Returns the number of bytes copied or a negative error indicating the
+ * failure.
+ */
+
+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)
+{
+	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
+				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
+}
+EXPORT_SYMBOL(generic_copy_file_range);
+
 /*
  * copy_file_range() differs from regular file read and write in that it
  * specifically allows return partial success.  When it does so is up to

@@ -1632,9 +1662,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 			goto done;
 	}
 
-	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
-			len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
-
+	ret = generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
+				      flags);
 done:
 	if (ret > 0) {
 		fsnotify_access(file_in);

include/linux/fs.h +3 -0

index f7fdfe93e25d..ea17858310ff 100644
@@ -1889,6 +1889,9 @@ extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
 		unsigned long, loff_t *, rwf_t);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
+extern 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);
 extern int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 					 struct file *file_out, loff_t pos_out,
 					 loff_t *count,

Signature · 1 line

2.17.1

From: Dave Chinner <[email protected]>

Now that we have generic_copy_file_range(), remove it as a fallback case when offloads fail. This puts the responsibility for executing fallbacks on the filesystems that implement ->copy_file_range and allows us to add operational validity checks to generic_copy_file_range().

Rework vfs_copy_file_range() to call a new do_copy_file_range() helper to exceute the copying callout, and move calls to

generic_file_copy_range() into filesystem methods where they currently return failures.

[Amir] overlayfs is not responsible of executing the fallback. It is the responsibility of the underlying filesystem.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/ceph/file.c    | 21 ++++++++++++++++++---
 fs/cifs/cifsfs.c  |  4 ++++
 fs/fuse/file.c    | 21 ++++++++++++++++++---
 fs/nfs/nfs4file.c | 20 +++++++++++++++++---
 fs/read_write.c   | 25 ++++++++++++++++---------
 5 files changed, 73 insertions(+), 18 deletions(-)

fs/ceph/file.c +18 -3

index 305daf043eb0..e87f7b2023af 100644
@@ -1889,9 +1889,9 @@ static int is_file_size_ok(struct inode *src_inode, struct inode *dst_inode,
 	return 0;
 }
 
-static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off,
-				    struct file *dst_file, loff_t dst_off,
-				    size_t len, unsigned int flags)
+static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
+				      struct file *dst_file, loff_t dst_off,
+				      size_t len, unsigned int flags)
 {
 	struct inode *src_inode = file_inode(src_file);
 	struct inode *dst_inode = file_inode(dst_file);

@@ -2100,6 +2100,21 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off,
 	return ret;
 }
 
+static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off,
+				    struct file *dst_file, loff_t dst_off,
+				    size_t len, unsigned int flags)
+{
+	ssize_t ret;
+
+	ret = __ceph_copy_file_range(src_file, src_off, dst_file, dst_off,
+				     len, flags);
+
+	if (ret == -EOPNOTSUPP)
+		ret = generic_copy_file_range(src_file, src_off, dst_file,
+					      dst_off, len, flags);
+	return ret;
+}
+
 const struct file_operations ceph_file_fops = {
 	.open = ceph_open,
 	.release = ceph_release,

fs/cifs/cifsfs.c +4 -0

index f5fcd6360056..c65823270313 100644
@@ -1148,6 +1148,10 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
 	rc = cifs_file_copychunk_range(xid, src_file, off, dst_file, destoff,
 					len, flags);
 	free_xid(xid);
+
+	if (rc == -EOPNOTSUPP)
+		rc = generic_copy_file_range(src_file, off, dst_file,
+					     destoff, len, flags);
 	return rc;
 }
 

fs/fuse/file.c +18 -3

index 3959f08279e6..e03901ae729b 100644
@@ -3097,9 +3097,9 @@ static long fuse_file_fallocate(struct file *file, int mode, loff_t offset,
 	return err;
 }
 
-static ssize_t fuse_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)
+static ssize_t __fuse_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)
 {
 	struct fuse_file *ff_in = file_in->private_data;
 	struct fuse_file *ff_out = file_out->private_data;

@@ -3173,6 +3173,21 @@ static ssize_t fuse_copy_file_range(struct file *file_in, loff_t pos_in,
 	return err;
 }
 
+static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off,
+				    struct file *dst_file, loff_t dst_off,
+				    size_t len, unsigned int flags)
+{
+	ssize_t ret;
+
+	ret = __fuse_copy_file_range(src_file, src_off, dst_file, dst_off,
+				     len, flags);
+
+	if (ret == -EOPNOTSUPP)
+		ret = generic_copy_file_range(src_file, src_off, dst_file,
+					      dst_off, len, flags);
+	return ret;
+}
+
 static const struct file_operations fuse_file_operations = {
 	.llseek		= fuse_file_llseek,
 	.read_iter	= fuse_file_read_iter,

fs/nfs/nfs4file.c +17 -3

index cf42a8b939e3..4842f3ab3161 100644
@@ -129,9 +129,9 @@ nfs4_file_flush(struct file *file, fl_owner_t id)
 }
 
 #ifdef CONFIG_NFS_V4_2
-static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
-				    struct file *file_out, loff_t pos_out,
-				    size_t count, unsigned int flags)
+static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
+				      struct file *file_out, loff_t pos_out,
+				      size_t count, unsigned int flags)
 {
 	if (!nfs_server_capable(file_inode(file_out), NFS_CAP_COPY))
 		return -EOPNOTSUPP;

@@ -140,6 +140,20 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
 	return nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count);
 }
 
+static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
+				    struct file *file_out, loff_t pos_out,
+				    size_t count, unsigned int flags)
+{
+	ssize_t ret;
+
+	ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count,
+				     flags);
+	if (ret == -EOPNOTSUPP)
+		ret = generic_copy_file_range(file_in, pos_in, file_out,
+					      pos_out, count, flags);
+	return ret;
+}
+
 static loff_t nfs4_file_llseek(struct file *filep, loff_t offset, int whence)
 {
 	loff_t ret;

fs/read_write.c +16 -9

index 676b02fae589..b63dcb4e4fe9 100644
@@ -1595,6 +1595,19 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
 }
 EXPORT_SYMBOL(generic_copy_file_range);
 
+static ssize_t do_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)
+{
+	if (file_out->f_op->copy_file_range)
+		return file_out->f_op->copy_file_range(file_in, pos_in,
+						       file_out, pos_out,
+						       len, flags);
+
+	return generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
+				       flags);
+}
+
 /*
  * copy_file_range() differs from regular file read and write in that it
  * specifically allows return partial success.  When it does so is up to

@@ -1655,15 +1668,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 		}
 	}
 
-	if (file_out->f_op->copy_file_range) {
-		ret = file_out->f_op->copy_file_range(file_in, pos_in, file_out,
-						      pos_out, len, flags);
-		if (ret != -EOPNOTSUPP)
-			goto done;
-	}
-
-	ret = generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
-				      flags);
+	ret = do_copy_file_range(file_in, pos_in, file_out, pos_out, len,
+				flags);
+	WARN_ON_ONCE(ret == -EOPNOTSUPP);
 done:
 	if (ret > 0) {
 		fsnotify_access(file_in);

Signature · 1 line

2.17.1

Factor out helper with some checks on in/out file that are common to clone_file_range and copy_file_range.

Suggested-by Darrick J. Wong <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c    | 38 +++++++++++---------------------------
 include/linux/fs.h |  1 +
 mm/filemap.c       | 24 ++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 27 deletions(-)

fs/read_write.c +11 -27

index b63dcb4e4fe9..f1900bdb3127 100644
@@ -1617,17 +1617,18 @@ ssize_t vfs_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)
 {
-	struct inode *inode_in = file_inode(file_in);
-	struct inode *inode_out = file_inode(file_out);
 	ssize_t ret;
 
 	if (flags != 0)
 		return -EINVAL;
 
-	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
-		return -EISDIR;
-	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
-		return -EINVAL;
+	/* this could be relaxed once a method supports cross-fs copies */
+	if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
+		return -EXDEV;
+
+	ret = generic_file_rw_checks(file_in, file_out);
+	if (unlikely(ret))
+		return ret;
 
 	ret = rw_verify_area(READ, file_in, &pos_in, len);
 	if (unlikely(ret))

@@ -1637,15 +1638,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	if (unlikely(ret))
 		return ret;
 
-	if (!(file_in->f_mode & FMODE_READ) ||
-	    !(file_out->f_mode & FMODE_WRITE) ||
-	    (file_out->f_flags & O_APPEND))
-		return -EBADF;
-
-	/* this could be relaxed once a method supports cross-fs copies */
-	if (inode_in->i_sb != inode_out->i_sb)
-		return -EXDEV;
-
 	if (len == 0)
 		return 0;
 

@@ -2013,29 +2005,21 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
 			   struct file *file_out, loff_t pos_out,
 			   loff_t len, unsigned int remap_flags)
 {
-	struct inode *inode_in = file_inode(file_in);
-	struct inode *inode_out = file_inode(file_out);
 	loff_t ret;
 
 	WARN_ON_ONCE(remap_flags & REMAP_FILE_DEDUP);
 
-	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
-		return -EISDIR;
-	if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
-		return -EINVAL;
-
 	/*
 	 * FICLONE/FICLONERANGE ioctls enforce that src and dest files are on
 	 * the same mount. Practically, they only need to be on the same file
 	 * system.
 	 */
-	if (inode_in->i_sb != inode_out->i_sb)
+	if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
 		return -EXDEV;
 
-	if (!(file_in->f_mode & FMODE_READ) ||
-	    !(file_out->f_mode & FMODE_WRITE) ||
-	    (file_out->f_flags & O_APPEND))
-		return -EBADF;
+	ret = generic_file_rw_checks(file_in, file_out);
+	if (ret < 0)
+		return ret;
 
 	if (!file_in->f_op->remap_file_range)
 		return -EOPNOTSUPP;

include/linux/fs.h +1 -0

index ea17858310ff..89b9b73eb581 100644
@@ -3049,6 +3049,7 @@ extern ssize_t generic_write_checks(struct kiocb *, struct iov_iter *);
 extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
 				struct file *file_out, loff_t pos_out,
 				loff_t *count, unsigned int remap_flags);
+extern int generic_file_rw_checks(struct file *file_in, struct file *file_out);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);

mm/filemap.c +13 -0

index c5af80c43d36..798aac92cd76 100644
@@ -3040,6 +3040,30 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	return 0;
 }
 
+
+/*
+ * Performs common checks before doing a file copy/clone
+ * from @file_in to @file_out.
+ */
+int generic_file_rw_checks(struct file *file_in, struct file *file_out)
+{
+	struct inode *inode_in = file_inode(file_in);
+	struct inode *inode_out = file_inode(file_out);
+
+	/* Don't copy dirs, pipes, sockets... */
+	if (S_ISDIR(inode_in->i_mode) || S_ISDIR(inode_out->i_mode))
+		return -EISDIR;

mm/filemap.c +1 -0

+
 int pagecache_write_begin(struct file *file, struct address_space *mapping,
 				loff_t pos, unsigned len, unsigned flags,
 				struct page **pagep, void **fsdata)

Signature · 1 line

2.17.1

Like the clone and dedupe interfaces we've recently fixed, the copy_file_range() implementation is missing basic sanity, limits and boundary condition tests on the parameters that are passed to it from userspace. Create a new "generic_copy_file_checks()" function modelled on the generic_remap_checks() function to provide this missing functionality.

[Amir] Shorten copy length instead of checking pos_in limits because input file size already abides by the limits.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c    |  3 ++-
 include/linux/fs.h |  3 +++
 mm/filemap.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

fs/read_write.c +2 -1

index f1900bdb3127..b0fb1176b628 100644
@@ -1626,7 +1626,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
 		return -EXDEV;
 
-	ret = generic_file_rw_checks(file_in, file_out);
+	ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
+				       flags);
 	if (unlikely(ret))
 		return ret;
 

include/linux/fs.h +3 -0

index 89b9b73eb581..e4d382c4342a 100644
@@ -3050,6 +3050,9 @@ extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
 				struct file *file_out, loff_t pos_out,
 				loff_t *count, unsigned int remap_flags);
 extern int generic_file_rw_checks(struct file *file_in, struct file *file_out);
+extern int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
+				    struct file *file_out, loff_t pos_out,
+				    size_t *count, unsigned int flags);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);

mm/filemap.c +0 -0

index 798aac92cd76..1852fbf08eeb 100644

mm/filemap.c +24 -0

+ * Returns appropriate error code that caller should return or
+ * zero in case the copy should be allowed.
+ */
+int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
+			     struct file *file_out, loff_t pos_out,
+			     size_t *req_count, unsigned int flags)
+{
+	struct inode *inode_in = file_inode(file_in);
+	struct inode *inode_out = file_inode(file_out);
+	uint64_t count = *req_count;
+	loff_t size_in;
+	int ret;
+
+	ret = generic_file_rw_checks(file_in, file_out);
+	if (ret)
+		return ret;
+
+	/* Don't touch certain kinds of inodes */
+	if (IS_IMMUTABLE(inode_out))
+		return -EPERM;
+
+	if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
+		return -ETXTBSY;
+

mm/filemap.c +14 -0

+	ret = generic_write_check_limits(file_out, pos_out, &count);
+	if (ret)
+		return ret;
+
+	/* Don't allow overlapped copying within the same file. */
+	if (inode_in == inode_out &&
+	    pos_out + count > pos_in &&
+	    pos_out < pos_in + count)
+		return -EINVAL;
+
+	*req_count = count;
+	return 0;
+}
+
 int pagecache_write_begin(struct file *file, struct address_space *mapping,
 				loff_t pos, unsigned len, unsigned flags,
 				struct page **pagep, void **fsdata)

Signature · 1 line

2.17.1

Quoted 54 lines · Amir Goldstein

Like the clone and dedupe interfaces we've recently fixed, the copy_file_range() implementation is missing basic sanity, limits and boundary condition tests on the parameters that are passed to it from userspace. Create a new "generic_copy_file_checks()" function modelled on the generic_remap_checks() function to provide this missing functionality.

[Amir] Shorten copy length instead of checking pos_in limits because input file size already abides by the limits.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c    |  3 ++-
 include/linux/fs.h |  3 +++
 mm/filemap.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

fs/read_write.c +2 -1

index f1900bdb3127..b0fb1176b628 100644
@@ -1626,7 +1626,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
      if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
              return -EXDEV;
 
-     ret = generic_file_rw_checks(file_in, file_out);
+     ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
+                                    flags);
      if (unlikely(ret))
              return ret;
 

include/linux/fs.h +3 -0

index 89b9b73eb581..e4d382c4342a 100644
@@ -3050,6 +3050,9 @@ extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
                              struct file *file_out, loff_t pos_out,
                              loff_t *count, unsigned int remap_flags);
 extern int generic_file_rw_checks(struct file *file_in, struct file *file_out);
+extern int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
+                                 struct file *file_out, loff_t pos_out,
+                                 size_t *count, unsigned int flags);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);

mm/filemap.c +0 -0

index 798aac92cd76..1852fbf08eeb 100644
@@ -3064,6 +3064,59 @@ int generic_file_rw_checks(struct file *file_in, struct file *file_out)
      return 0;
 }
 

mm/filemap.c int generic_file_rw_checks(struct file *file_in, struct file *file_out) · line 3070

Quoted 4 lines

mm/filemap.c +4 -0

+/*
+ * Performs necessary checks before doing a file copy
+ *
+ * Can adjust amount of bytes to copy

That's the @req_count parameter, correct?

Quoted 30 lines

mm/filemap.c +29 -0

+ * Returns appropriate error code that caller should return or
+ * zero in case the copy should be allowed.
+ */
+int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
+                          struct file *file_out, loff_t pos_out,
+                          size_t *req_count, unsigned int flags)
+{
+     struct inode *inode_in = file_inode(file_in);
+     struct inode *inode_out = file_inode(file_out);
+     uint64_t count = *req_count;
+     loff_t size_in;
+     int ret;
+
+     ret = generic_file_rw_checks(file_in, file_out);
+     if (ret)
+             return ret;
+
+     /* Don't touch certain kinds of inodes */
+     if (IS_IMMUTABLE(inode_out))
+             return -EPERM;
+
+     if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
+             return -ETXTBSY;
+
+     /* Ensure offsets don't wrap. */
+     if (pos_in + count < pos_in || pos_out + count < pos_out)
+             return -EOVERFLOW;
+
+     /* Shorten the copy to EOF */

mm/filemap.c int generic_file_rw_checks(struct file *file_in, struct file *file_out) · line 3104

Quoted 5 lines

mm/filemap.c +5 -0

+     size_in = i_size_read(inode_in);
+     if (pos_in >= size_in)
+             count = 0;
+     else
+             count = min(count, size_in - (uint64_t)pos_in);

Do we need a call to generic_access_check_limits(file_in...) here to prevent copies from ranges that the page cache doesn't support?

Quoted 56 lines · Darrick J. Wong

Quoted 54 lines · Amir Goldstein

Like the clone and dedupe interfaces we've recently fixed, the copy_file_range() implementation is missing basic sanity, limits and boundary condition tests on the parameters that are passed to it from userspace. Create a new "generic_copy_file_checks()" function modelled on the generic_remap_checks() function to provide this missing functionality.

[Amir] Shorten copy length instead of checking pos_in limits because input file size already abides by the limits.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c    |  3 ++-
 include/linux/fs.h |  3 +++
 mm/filemap.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

fs/read_write.c +2 -1

index f1900bdb3127..b0fb1176b628 100644
@@ -1626,7 +1626,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
      if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
              return -EXDEV;
 
-     ret = generic_file_rw_checks(file_in, file_out);
+     ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
+                                    flags);
      if (unlikely(ret))
              return ret;
 

include/linux/fs.h +3 -0

index 89b9b73eb581..e4d382c4342a 100644
@@ -3050,6 +3050,9 @@ extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
                              struct file *file_out, loff_t pos_out,
                              loff_t *count, unsigned int remap_flags);
 extern int generic_file_rw_checks(struct file *file_in, struct file *file_out);
+extern int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
+                                 struct file *file_out, loff_t pos_out,
+                                 size_t *count, unsigned int flags);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);

mm/filemap.c +0 -0

index 798aac92cd76..1852fbf08eeb 100644
@@ -3064,6 +3064,59 @@ int generic_file_rw_checks(struct file *file_in, struct file *file_out)
      return 0;
 }
 

Quoted 6 lines

mm/filemap.c int generic_file_rw_checks(struct file *file_in, struct file *file_out) · line 3070

Quoted 4 lines

mm/filemap.c +4 -0

+/*
+ * Performs necessary checks before doing a file copy
+ *
+ * Can adjust amount of bytes to copy

That's the @req_count parameter, correct?

Correct. Same as generic_remap_checks()

Quoted 30 lines

Quoted 30 lines

mm/filemap.c +29 -0

+ * Returns appropriate error code that caller should return or
+ * zero in case the copy should be allowed.
+ */
+int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
+                          struct file *file_out, loff_t pos_out,
+                          size_t *req_count, unsigned int flags)
+{
+     struct inode *inode_in = file_inode(file_in);
+     struct inode *inode_out = file_inode(file_out);
+     uint64_t count = *req_count;
+     loff_t size_in;
+     int ret;
+
+     ret = generic_file_rw_checks(file_in, file_out);
+     if (ret)
+             return ret;
+
+     /* Don't touch certain kinds of inodes */
+     if (IS_IMMUTABLE(inode_out))
+             return -EPERM;
+
+     if (IS_SWAPFILE(inode_in) || IS_SWAPFILE(inode_out))
+             return -ETXTBSY;
+
+     /* Ensure offsets don't wrap. */
+     if (pos_in + count < pos_in || pos_out + count < pos_out)
+             return -EOVERFLOW;
+
+     /* Shorten the copy to EOF */

Quoted 8 lines

mm/filemap.c int generic_file_rw_checks(struct file *file_in, struct file *file_out) · line 3104

Quoted 5 lines

mm/filemap.c +5 -0

+     size_in = i_size_read(inode_in);
+     if (pos_in >= size_in)
+             count = 0;
+     else
+             count = min(count, size_in - (uint64_t)pos_in);

Do we need a call to generic_access_check_limits(file_in...) here to prevent copies from ranges that the page cache doesn't support?

No. Because i_size cannot be of an illegal size and we cap the read to i_size.
I also removed generic_access_check_limits() from generic_remap_checks() for a similar reason in patch #8.

Thanks,
Amir.

Quoted 64 lines · Amir Goldstein

Quoted 62 lines · Darrick J. Wong

Quoted 51 lines · Amir Goldstein

Like the clone and dedupe interfaces we've recently fixed, the copy_file_range() implementation is missing basic sanity, limits and boundary condition tests on the parameters that are passed to it from userspace. Create a new "generic_copy_file_checks()" function modelled on the generic_remap_checks() function to provide this missing functionality.

[Amir] Shorten copy length instead of checking pos_in limits because input file size already abides by the limits.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c    |  3 ++-
 include/linux/fs.h |  3 +++
 mm/filemap.c       | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 1 deletion(-)

fs/read_write.c +2 -1

index f1900bdb3127..b0fb1176b628 100644
@@ -1626,7 +1626,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
      if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
              return -EXDEV;
 
-     ret = generic_file_rw_checks(file_in, file_out);
+     ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
+                                    flags);
      if (unlikely(ret))
              return ret;
 

include/linux/fs.h +3 -0

index 89b9b73eb581..e4d382c4342a 100644
@@ -3050,6 +3050,9 @@ extern int generic_remap_checks(struct file *file_in, loff_t pos_in,
                              struct file *file_out, loff_t pos_out,
                              loff_t *count, unsigned int remap_flags);
 extern int generic_file_rw_checks(struct file *file_in, struct file *file_out);
+extern int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
+                                 struct file *file_out, loff_t pos_out,
+                                 size_t *count, unsigned int flags);
 extern ssize_t generic_file_read_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t __generic_file_write_iter(struct kiocb *, struct iov_iter *);
 extern ssize_t generic_file_write_iter(struct kiocb *, struct iov_iter *);

mm/filemap.c +0 -0

index 798aac92cd76..1852fbf08eeb 100644

mm/filemap.c int generic_file_rw_checks(struct file *file_in, struct file *file_out) · line 3070

Quoted 7 lines

mm/filemap.c +4 -0

@@ -3064,6 +3064,59 @@ int generic_file_rw_checks(struct file *file_in, struct file *file_out)
      return 0;
 }
 
+/*
+ * Performs necessary checks before doing a file copy
+ *
+ * Can adjust amount of bytes to copy

That's the @req_count parameter, correct?

Quoted 1 line

Correct. Same as generic_remap_checks()

Ok. Would you mind updating the comment?

The file we are copying data into needs to have its setuid bit stripped before we start the data copy so that unprivileged users can't copy data into executables that are run with root privs.

[Amir] Introduce the helper generic_copy_file_range_prep() modelled after generic_remap_file_range_prep(). Helper is called by filesystem before the copy_file_range operation and with output inode locked.

For ceph and for default generic_copy_file_range() implementation there is no inode lock held throughout the copy operation, so we do best effort and remove setuid bit before copy starts. This does not protect suid file from changing if suid bit is set after copy started.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/ceph/file.c     |  9 +++++++++
 fs/cifs/cifsfs.c   |  9 ++++++---
 fs/fuse/file.c     |  4 ++++
 fs/nfs/nfs42proc.c |  8 +++++---
 fs/read_write.c    | 31 +++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 6 files changed, 57 insertions(+), 6 deletions(-)

fs/ceph/file.c +9 -0

index e87f7b2023af..54cfc877a6ef 100644
@@ -1947,6 +1947,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 		goto out;
 	}
 
+	/* Should inode lock be held throughout the copy operation? */
+	inode_lock(dst_inode);
+	ret = generic_copy_file_range_prep(src_file, dst_file);
+	inode_unlock(dst_inode);
+	if (ret < 0) {
+		dout("failed to copy from src to dst file (%zd)\n", ret);
+		goto out;
+	}
+
 	/*
 	 * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other
 	 * clients may have dirty data in their caches.  And OSDs know nothing

fs/cifs/cifsfs.c +6 -3

index c65823270313..e103b499aaa8 100644
@@ -1096,6 +1096,10 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 		goto out;
 	}
 
+	rc = -EOPNOTSUPP;
+	if (!target_tcon->ses->server->ops->copychunk_range)
+		goto out;
+
 	/*
 	 * Note: cifs case is easier than btrfs since server responsible for
 	 * checks for proper open modes and file type and if it wants

@@ -1107,11 +1111,10 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	/* should we flush first and last page first */
 	truncate_inode_pages(&target_inode->i_data, 0);
 
-	if (target_tcon->ses->server->ops->copychunk_range)
+	rc = generic_copy_file_range_prep(src_file, dst_file);
+	if (!rc)
 		rc = target_tcon->ses->server->ops->copychunk_range(xid,
 			smb_file_src, smb_file_target, off, len, destoff);
-	else
-		rc = -EOPNOTSUPP;
 
 	/* force revalidate of size and timestamps of target file now
 	 * that target is updated on the server

fs/fuse/file.c +4 -0

index e03901ae729b..3531d4a3d9ec 100644
@@ -3128,6 +3128,10 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
 
 	inode_lock(inode_out);
 
+	err = generic_copy_file_range_prep(file_in, file_out);
+	if (err)
+		goto out;
+
 	if (fc->writeback_cache) {
 		err = filemap_write_and_wait_range(inode_out->i_mapping,
 						   pos_out, pos_out + len);

fs/nfs/nfs42proc.c +5 -3

index 5196bfa7894d..b387951e1d86 100644
@@ -345,9 +345,11 @@ ssize_t nfs42_proc_copy(struct file *src, loff_t pos_src,
 
 	do {
 		inode_lock(file_inode(dst));
-		err = _nfs42_proc_copy(src, src_lock,
-				dst, dst_lock,
-				&args, &res);
+		err = generic_copy_file_range_prep(src, dst);
+		if (!err)
+			err = _nfs42_proc_copy(src, src_lock,
+					       dst, dst_lock,
+					       &args, &res);
 		inode_unlock(file_inode(dst));
 
 		if (err >= 0)

fs/read_write.c +31 -0

index b0fb1176b628..e16bcafc0da2 100644
@@ -1565,6 +1565,28 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int, out_fd, int, in_fd,
 }
 #endif
 
+/*
+ * Prepare inodes for copy from @file_in to @file_out.
+ *
+ * Caller must hold output inode lock.
+ */
+int generic_copy_file_range_prep(struct file *file_in, struct file *file_out)
+{
+	int ret;
+
+	WARN_ON_ONCE(!inode_is_locked(file_inode(file_out)));
+
+	/*
+	 * Clear the security bits if the process is not being run by root.
+	 * This keeps people from modifying setuid and setgid binaries.
+	 */
+	ret = file_remove_privs(file_out);
+
+	return ret;
+
+}
+EXPORT_SYMBOL(generic_copy_file_range_prep);
+
 /**
  * generic_copy_file_range - copy data between two files
  * @file_in:	file structure to read from

@@ -1590,6 +1612,15 @@ 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)
 {
+	int ret;
+
+	/* Should inode lock be held throughout the copy operation? */
+	inode_lock(file_inode(file_out));
+	ret = generic_copy_file_range_prep(file_in, file_out);
+	inode_unlock(file_inode(file_out));
+	if (ret)
+		return ret;
+
 	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
 				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
 }

include/linux/fs.h +2 -0

index e4d382c4342a..3e03a96d9ab6 100644
@@ -1889,6 +1889,8 @@ extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
 		unsigned long, loff_t *, rwf_t);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
+extern int generic_copy_file_range_prep(struct file *file_in,
+					struct file *file_out);
 extern 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);

Signature · 1 line

2.17.1

The file we are copying data into needs to have its setuid bit stripped before we start the data copy so that unprivileged users can't copy data into executables that are run with root privs.

[Amir] Introduce the helper generic_copy_file_range_prep() modelled after generic_remap_file_range_prep(). Helper is called by filesystem before the copy_file_range operation and with output inode locked.

For ceph and for default generic_copy_file_range() implementation there is no inode lock held throughout the copy operation, so we do best effort and remove setuid bit before copy starts. This does not protect suid file from changing if suid bit is set after copy started.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/ceph/file.c     |  9 +++++++++
 fs/cifs/cifsfs.c   |  9 ++++++---
 fs/fuse/file.c     |  4 ++++
 fs/nfs/nfs42proc.c |  8 +++++---
 fs/read_write.c    | 31 +++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 6 files changed, 57 insertions(+), 6 deletions(-)

fs/ceph/file.c +9 -0

index e87f7b2023af..54cfc877a6ef 100644
@@ -1947,6 +1947,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 		goto out;
 	}
 
+	/* Should inode lock be held throughout the copy operation? */
+	inode_lock(dst_inode);
+	ret = generic_copy_file_range_prep(src_file, dst_file);
+	inode_unlock(dst_inode);
+	if (ret < 0) {
+		dout("failed to copy from src to dst file (%zd)\n", ret);
+		goto out;
+	}
+
 	/*
 	 * We need FILE_WR caps for dst_ci and FILE_RD for src_ci as other
 	 * clients may have dirty data in their caches.  And OSDs know nothing

fs/cifs/cifsfs.c +6 -3

index c65823270313..e103b499aaa8 100644
@@ -1096,6 +1096,10 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 		goto out;
 	}
 
+	rc = -EOPNOTSUPP;
+	if (!target_tcon->ses->server->ops->copychunk_range)
+		goto out;
+
 	/*
 	 * Note: cifs case is easier than btrfs since server responsible for
 	 * checks for proper open modes and file type and if it wants

@@ -1107,11 +1111,10 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	/* should we flush first and last page first */
 	truncate_inode_pages(&target_inode->i_data, 0);
 
-	if (target_tcon->ses->server->ops->copychunk_range)
+	rc = generic_copy_file_range_prep(src_file, dst_file);
+	if (!rc)
 		rc = target_tcon->ses->server->ops->copychunk_range(xid,
 			smb_file_src, smb_file_target, off, len, destoff);
-	else
-		rc = -EOPNOTSUPP;
 
 	/* force revalidate of size and timestamps of target file now
 	 * that target is updated on the server

fs/fuse/file.c +4 -0

index e03901ae729b..3531d4a3d9ec 100644
@@ -3128,6 +3128,10 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
 
 	inode_lock(inode_out);
 
+	err = generic_copy_file_range_prep(file_in, file_out);
+	if (err)
+		goto out;
+
 	if (fc->writeback_cache) {
 		err = filemap_write_and_wait_range(inode_out->i_mapping,
 						   pos_out, pos_out + len);

fs/nfs/nfs42proc.c +5 -3

index 5196bfa7894d..b387951e1d86 100644
@@ -345,9 +345,11 @@ ssize_t nfs42_proc_copy(struct file *src, loff_t pos_src,
 
 	do {
 		inode_lock(file_inode(dst));
-		err = _nfs42_proc_copy(src, src_lock,
-				dst, dst_lock,
-				&args, &res);
+		err = generic_copy_file_range_prep(src, dst);
+		if (!err)
+			err = _nfs42_proc_copy(src, src_lock,
+					       dst, dst_lock,
+					       &args, &res);
 		inode_unlock(file_inode(dst));
 
 		if (err >= 0)

fs/read_write.c +31 -0

index b0fb1176b628..e16bcafc0da2 100644
@@ -1565,6 +1565,28 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int, out_fd, int, in_fd,
 }
 #endif
 
+/*
+ * Prepare inodes for copy from @file_in to @file_out.
+ *
+ * Caller must hold output inode lock.
+ */
+int generic_copy_file_range_prep(struct file *file_in, struct file *file_out)
+{
+	int ret;
+
+	WARN_ON_ONCE(!inode_is_locked(file_inode(file_out)));
+
+	/*
+	 * Clear the security bits if the process is not being run by root.
+	 * This keeps people from modifying setuid and setgid binaries.
+	 */
+	ret = file_remove_privs(file_out);
+
+	return ret;
+
+}
+EXPORT_SYMBOL(generic_copy_file_range_prep);
+
 /**
  * generic_copy_file_range - copy data between two files
  * @file_in:	file structure to read from

@@ -1590,6 +1612,15 @@ 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)
 {
+	int ret;
+
+	/* Should inode lock be held throughout the copy operation? */
+	inode_lock(file_inode(file_out));
+	ret = generic_copy_file_range_prep(file_in, file_out);
+	inode_unlock(file_inode(file_out));
+	if (ret)
+		return ret;
+
 	return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
 				len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
 }

include/linux/fs.h +2 -0

index e4d382c4342a..3e03a96d9ab6 100644
@@ -1889,6 +1889,8 @@ extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
 		unsigned long, loff_t *, rwf_t);
 extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
 				   loff_t, size_t, unsigned int);
+extern int generic_copy_file_range_prep(struct file *file_in,
+					struct file *file_out);
 extern 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);

Signature · 1 line

2.17.1

From: Dave Chinner <[email protected]>

Timestamps are not updated right now, so programs looking for timestamp updates for file modifications (like rsync) will not detect that files have changed. We are also accessing the source data when doing a copy (but not when cloning) so we need to update atime on the source file as well.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/read_write.c +0 -0

index e16bcafc0da2..4b23a86aacd9 100644
@@ -1576,6 +1576,16 @@ int generic_copy_file_range_prep(struct file *file_in, struct file *file_out)
 
 	WARN_ON_ONCE(!inode_is_locked(file_inode(file_out)));
 

fs/read_write.c +0 -0

 	/*
 	 * Clear the security bits if the process is not being run by root.
 	 * This keeps people from modifying setuid and setgid binaries.

Signature · 1 line

2.17.1

Quoted 22 lines · Amir Goldstein

From: Dave Chinner <[email protected]>

Timestamps are not updated right now, so programs looking for timestamp updates for file modifications (like rsync) will not detect that files have changed. We are also accessing the source data when doing a copy (but not when cloning) so we need to update atime on the source file as well.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/read_write.c +0 -0

index e16bcafc0da2..4b23a86aacd9 100644
@@ -1576,6 +1576,16 @@ int generic_copy_file_range_prep(struct file *file_in, struct file *file_out)
 
 	WARN_ON_ONCE(!inode_is_locked(file_inode(file_out)));
 

fs/read_write.c int generic_copy_file_range_prep(struct file *file_in, struct file *file_out) · line 1588

Quoted 10 lines

fs/read_write.c +10 -0

+	/* Update source timestamps, because we are accessing file data */
+	file_accessed(file_in);
+
+	/* Update destination timestamps, since we can alter file contents. */
+	if (!(file_out->f_mode & FMODE_NOCMTIME)) {
+		ret = file_update_time(file_out);
+		if (ret)
+			return ret;
+	}
+

Is this the right place for updating the timestamps? I see that in same cases we may be updating the timestamp even if there was an error and no copy was performed. For example, if file_remove_privs fails.

Quoted 23 lines · Luis Henriques

Quoted 22 lines · Amir Goldstein

From: Dave Chinner <[email protected]>

Timestamps are not updated right now, so programs looking for timestamp updates for file modifications (like rsync) will not detect that files have changed. We are also accessing the source data when doing a copy (but not when cloning) so we need to update atime on the source file as well.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/read_write.c +0 -0

index e16bcafc0da2..4b23a86aacd9 100644
@@ -1576,6 +1576,16 @@ int generic_copy_file_range_prep(struct file *file_in, struct file *file_out)
 
 	WARN_ON_ONCE(!inode_is_locked(file_inode(file_out)));
 

Quoted 14 lines

fs/read_write.c int generic_copy_file_range_prep(struct file *file_in, struct file *file_out) · line 1588

Quoted 10 lines

fs/read_write.c +10 -0

+	/* Update source timestamps, because we are accessing file data */
+	file_accessed(file_in);
+
+	/* Update destination timestamps, since we can alter file contents. */
+	if (!(file_out->f_mode & FMODE_NOCMTIME)) {
+		ret = file_update_time(file_out);
+		if (ret)
+			return ret;
+	}
+

Is this the right place for updating the timestamps? I see that in same cases we may be updating the timestamp even if there was an error and no copy was performed. For example, if file_remove_privs fails.

It's the same place we do it for read - file_accessed() is called before we do the IO - and the same place for write - file_update_time() is called before we copy data into the pagecache or do direct IO. As such, it really doesn't matter if it is before or after file_remove_privs() - the IO can still fail for many reasons after we've updated the timestamps and in some of the failure cases (e.g. we failed the sync at the end of an O_DSYNC buffered write) we still want the timestamps to be modified because the data and/or user visible metadata /may/ have been changed.

cfr operates under the same constraints as read() and write(), so we need to update the timestamps up front regardless of whether the copy ends up succeeding or not....

From: Dave Chinner <[email protected]>

Timestamps are not updated right now, so programs looking for timestamp updates for file modifications (like rsync) will not detect that files have changed. We are also accessing the source data when doing a copy (but not when cloning) so we need to update atime on the source file as well.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/read_write.c +3 -0

index e16bcafc0da2..4b23a86aacd9 100644
@@ -1576,6 +1576,16 @@ int generic_copy_file_range_prep(struct file *file_in, struct file *file_out)
 
 	WARN_ON_ONCE(!inode_is_locked(file_inode(file_out)));
 
+	/* Update source timestamps, because we are accessing file data */
+	file_accessed(file_in);
+

Signature · 1 line

2.17.1

Quoted 22 lines · Amir Goldstein

From: Dave Chinner <[email protected]>

Timestamps are not updated right now, so programs looking for timestamp updates for file modifications (like rsync) will not detect that files have changed. We are also accessing the source data when doing a copy (but not when cloning) so we need to update atime on the source file as well.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/read_write.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

fs/read_write.c +0 -0

index e16bcafc0da2..4b23a86aacd9 100644
@@ -1576,6 +1576,16 @@ int generic_copy_file_range_prep(struct file *file_in, struct file *file_out)
 
      WARN_ON_ONCE(!inode_is_locked(file_inode(file_out)));
 

fs/read_write.c int generic_copy_file_range_prep(struct file *file_in, struct file *file_out) · line 1591

Quoted 13 lines

fs/read_write.c +10 -0

+     /* Update source timestamps, because we are accessing file data */
+     file_accessed(file_in);
+
+     /* Update destination timestamps, since we can alter file contents. */
+     if (!(file_out->f_mode & FMODE_NOCMTIME)) {
+             ret = file_update_time(file_out);
+             if (ret)
+                     return ret;
+     }
+
      /*
       * Clear the security bits if the process is not being run by root.
       * This keeps people from modifying setuid and setgid binaries.

Should the file_update_time and file_remove_privs calls be factored into a separate small function to be called by generic_{copy,remap}_range_prep?

We want to enable cross-filesystem copy_file_range functionality where possible, so push the "same superblock only" checks down to the individual filesystem callouts so they can make their own decisions about cross-superblock copy offload and fallack to generic_copy_file_range() for cross-superblock copy.

[Amir] We do not call ->remap_file_range() in case the inodes are not on the same sb and do not call ->copy_file_range() in case the inodes are not on the same filesystem type.

This changes behavior of the copy_file_range(2) syscall, which will now allow cross filesystem in-kernel copy. CIFS already supports cross-superblock copy, between two shares to the same server. This functionality will now be available via the copy_file_range(2) syscall.

Cc Steve French <[email protected]>

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 fs/ceph/file.c    |  4 +++-
 fs/cifs/cifsfs.c  |  2 +-
 fs/fuse/file.c    |  5 ++++-
 fs/nfs/nfs4file.c |  5 ++++-
 fs/read_write.c   | 20 ++++++++++++++------
 5 files changed, 26 insertions(+), 10 deletions(-)

fs/ceph/file.c +3 -1

index 54cfc877a6ef..adf99557c46c 100644
@@ -1909,6 +1909,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 
 	if (src_inode == dst_inode)
 		return -EINVAL;
+	if (src_inode->i_sb != dst_inode->i_sb)
+		return -EXDEV;
 	if (ceph_snap(dst_inode) != CEPH_NOSNAP)
 		return -EROFS;
 

@@ -2118,7 +2120,7 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off,
 	ret = __ceph_copy_file_range(src_file, src_off, dst_file, dst_off,
 				     len, flags);
 
-	if (ret == -EOPNOTSUPP)
+	if (ret == -EOPNOTSUPP || ret == -EXDEV)
 		ret = generic_copy_file_range(src_file, src_off, dst_file,
 					      dst_off, len, flags);
 	return ret;

fs/cifs/cifsfs.c +1 -1

index e103b499aaa8..7bde046110ce 100644
@@ -1152,7 +1152,7 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
 					len, flags);
 	free_xid(xid);
 
-	if (rc == -EOPNOTSUPP)
+	if (rc == -EOPNOTSUPP || rc == -EXDEV)
 		rc = generic_copy_file_range(src_file, off, dst_file,
 					     destoff, len, flags);
 	return rc;

fs/fuse/file.c +4 -1

index 3531d4a3d9ec..180161f6f0bd 100644
@@ -3126,6 +3126,9 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
 	if (fc->no_copy_file_range)
 		return -EOPNOTSUPP;
 
+	if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
+		return -EXDEV;
+
 	inode_lock(inode_out);
 
 	err = generic_copy_file_range_prep(file_in, file_out);

@@ -3186,7 +3189,7 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off,
 	ret = __fuse_copy_file_range(src_file, src_off, dst_file, dst_off,
 				     len, flags);
 
-	if (ret == -EOPNOTSUPP)
+	if (ret == -EOPNOTSUPP || ret == -EXDEV)
 		ret = generic_copy_file_range(src_file, src_off, dst_file,
 					      dst_off, len, flags);
 	return ret;

fs/nfs/nfs4file.c +4 -1

index 4842f3ab3161..f4157eb1f69d 100644
@@ -133,6 +133,9 @@ static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
 				      struct file *file_out, loff_t pos_out,
 				      size_t count, unsigned int flags)
 {
+	/* Only offload copy if superblock is the same */
+	if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
+		return -EXDEV;
 	if (!nfs_server_capable(file_inode(file_out), NFS_CAP_COPY))
 		return -EOPNOTSUPP;
 	if (file_inode(file_in) == file_inode(file_out))

@@ -148,7 +151,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
 
 	ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count,
 				     flags);
-	if (ret == -EOPNOTSUPP)
+	if (ret == -EOPNOTSUPP || ret == -EXDEV)
 		ret = generic_copy_file_range(file_in, pos_in, file_out,
 					      pos_out, count, flags);
 	return ret;

fs/read_write.c +12 -5

index 4b23a86aacd9..283ec30d2136 100644
@@ -1640,7 +1640,18 @@ static ssize_t do_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)
 {
-	if (file_out->f_op->copy_file_range)
+	/*
+	 * Although we now allow filesystems to handle cross sb copy, passing
+	 * an inode of the wrong filesystem type to filesystem operation can
+	 * often result in an attempt to dereference the wrong concrete inode
+	 * struct, so avoid doing that until we really have a good reason.
+	 * The incentive for passing inode from different sb to filesystem is
+	 * NFS cross server copy and for that use case, enforcing same
+	 * filesystem type is acceptable.
+	 */
+	if (file_out->f_op->copy_file_range &&
+	    file_inode(file_in)->i_sb->s_type ==
+	    file_inode(file_out)->i_sb->s_type)
 		return file_out->f_op->copy_file_range(file_in, pos_in,
 						       file_out, pos_out,
 						       len, flags);

@@ -1663,10 +1674,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	if (flags != 0)
 		return -EINVAL;
 
-	/* this could be relaxed once a method supports cross-fs copies */
-	if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
-		return -EXDEV;
-
 	ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
 				       flags);
 	if (unlikely(ret))

fs/read_write.c +0 -0

 		loff_t cloned;
 
 		cloned = file_in->f_op->remap_file_range(file_in, pos_in,

Signature · 1 line

2.17.1

The access limit checks on input file range in generic_remap_checks() are redundant because the input file size is guarantied to be within

limits and pos+len are already checked to be within input file size.

Beyond the fact that the check cannot fail, if it would have failed, it could return -EFBIG for input file range error. There is no precedent for that. -EFBIG is returned in syscalls that would change file length.

Signed-off-by Amir Goldstein <[email protected]>

---
 mm/filemap.c | 4 ----
 1 file changed, 4 deletions(-)

mm/filemap.c +0 -0

index 1852fbf08eeb..7e1aa36d57a2 100644

mm/filemap.c +0 -3

-	if (ret)
-		return ret;
-
 	ret = generic_write_check_limits(file_out, pos_out, &count);
 	if (ret)
 		return ret;

Signature · 1 line

2.17.1

Update with all the missing errors the syscall can return, the behaviour the syscall should have w.r.t. to copies within single files, etc.

[Amir] Copying beyond EOF returns zero.

Signed-off-by Dave Chinner <[email protected]>

Signed-off-by Amir Goldstein <[email protected]>

---
 man2/copy_file_range.2 | 93 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 77 insertions(+), 16 deletions(-)

man2/copy_file_range.2 +0 -0

index 2438b63c8..fab11f977 100644

man2/copy_file_range.2 +12 -3

 .PP
 The
 .I flags

@@ -84,6 +89,11 @@ Upon successful completion,
 .BR copy_file_range ()
 will return the number of bytes copied between files.
 This could be less than the length originally requested.
+If the file offset of
+.I fd_in
+is at or past the end of file, no bytes are copied, and
+.BR copy_file_range ()
+returns zero.
 .PP
 On error,
 .BR copy_file_range ()

@@ -93,12 +103,16 @@ is set to indicate the error.
 .SH ERRORS
 .TP
 .B EBADF
-One or more file descriptors are not valid; or
+One or more file descriptors are not valid.
+.TP
+.B EBADF
 .I fd_in
 is not open for reading; or
 .I fd_out
-is not open for writing; or
-the
+is not open for writing.
+.TP
+.B EBADF
+The
 .B O_APPEND
 flag is set for the open file description (see
 .BR open (2))

@@ -106,17 +120,36 @@ referred to by the file descriptor
 .IR fd_out .
 .TP

man2/copy_file_range.2 +2 -3

 .TP
 .B EINVAL
-Requested range extends beyond the end of the source file; or the
+The
 .I flags
 argument is not 0.
 .TP
-.B EIO
-A low-level I/O error occurred while copying.
+.B EINVAL

man2/copy_file_range.2 +7 -1

 .TP
 .B EISDIR
 .I fd_in

@@ -124,22 +157,50 @@ or
 .I fd_out
 refers to a directory.
 .TP
+.B EOVERFLOW
+The requested source or destination range is too large to represent in the
+specified data types.
+.TP
+.B EIO
+A low-level I/O error occurred while copying.
+.TP
 .B ENOMEM
 Out of memory.
 .TP
-.B ENOSPC

man2/copy_file_range.2 +7 -0

+.TP
+.B EPERM
+.I fd_out
+refers to an immutable file.
+.TP
+.B EACCES
+The user does not have write permissions for the destination file.
 .SH VERSIONS

man2/copy_file_range.2 +4 -1

+.PP
+First support for cross-filesystem copies was introduced in Linux 5.3. Older
+kernels will return -EXDEV when cross-filesystem copies are attempted.
 .SH CONFORMING TO
 The
 .BR copy_file_range ()

@@ -224,7 +285,7 @@ main(int argc, char **argv)
         }
 
         len \-= ret;
-    } while (len > 0);
+    } while (len > 0 && ret > 0);
 
     close(fd_in);
     close(fd_out);

Signature · 1 line

2.17.1

man2/copy_file_range.2 or · line 200

Quoted 4 lines

man2/copy_file_range.2 +4 -0

+A major rework of the kernel implementation occurred in 5.3. Areas of the API
+that weren't clearly defined were clarified and the API bounds are much more
+strictly checked than on earlier kernels. Applications should target the
+behaviour and requirements of 5.3 kernels.

Are there any weird cases where a program targetting 5.3 behavior would fail or get stuck in an infinite loop on a 5.2 kernel?

Particularly since glibc spat out a copy_file_range fallback for 2.29 that tries to emulate the kernel behavior 100%. It even refuses cross-filesystem copies (because hey, we documented that :() even though that's perfectly fine for a userspace implementation.

TBH I suspect that we ought to get the glibc developers to remove the "no cross device copies" code from their implementation and then update the manpage to say that cross device copies are supposed to be supported all the time, at least as of glibc 2.(futureversion).

Anyways, thanks for taking on the c_f_r cleanup! :)