Fixes for major copy_file_range() issues

The access limit checks on input file range in generic_remap_checks() are redundant because the input file size is guaranteied 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.

With that call removed, we can fold generic_access_check_limits() into generic_write_check_limits().

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

---
 mm/filemap.c | 33 ++++++++++++---------------------
 1 file changed, 12 insertions(+), 21 deletions(-)

mm/filemap.c +12 -21

index a38619a4a6af..44361928bbb0 100644
@@ -2895,24 +2895,11 @@ EXPORT_SYMBOL(read_cache_page_gfp);
  * LFS limits.  If pos is under the limit it becomes a short access.  If it
  * exceeds the limit we return -EFBIG.
  */
-static int generic_access_check_limits(struct file *file, loff_t pos,
-				       loff_t *count)
-{
-	struct inode *inode = file->f_mapping->host;
-	loff_t max_size = inode->i_sb->s_maxbytes;
-
-	if (!(file->f_flags & O_LARGEFILE))
-		max_size = MAX_NON_LFS;
-
-	if (unlikely(pos >= max_size))
-		return -EFBIG;
-	*count = min(*count, max_size - pos);
-	return 0;
-}
-
 static int generic_write_check_limits(struct file *file, loff_t pos,
 				      loff_t *count)
 {
+	struct inode *inode = file->f_mapping->host;
+	loff_t max_size = inode->i_sb->s_maxbytes;
 	loff_t limit = rlimit(RLIMIT_FSIZE);
 
 	if (limit != RLIM_INFINITY) {

@@ -2923,7 +2910,15 @@ static int generic_write_check_limits(struct file *file, loff_t pos,
 		*count = min(*count, limit - pos);
 	}
 
-	return generic_access_check_limits(file, pos, count);
+	if (!(file->f_flags & O_LARGEFILE))
+		max_size = MAX_NON_LFS;
+
+	if (unlikely(pos >= max_size))
+		return -EFBIG;
+
+	*count = min(*count, max_size - pos);
+
+	return 0;
 }
 
 /*

@@ -2963,7 +2958,7 @@ EXPORT_SYMBOL(generic_write_checks);
 /*
  * Performs necessary checks before doing a clone.
  *
- * Can adjust amount of bytes to clone.
+ * Can adjust amount of bytes to clone via @req_count argument.
  * Returns appropriate error code that caller should return or
  * zero in case the clone should be allowed.
  */

@@ -3001,10 +2996,6 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 		return -EINVAL;
 	count = min(count, size_in - (uint64_t)pos_in);
 
-	ret = generic_access_check_limits(file_in, pos_in, &count);
-	if (ret)
-		return ret;
-
 	ret = generic_write_check_limits(file_out, pos_out, &count);
 	if (ret)
 		return ret;

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 +53 -0

index 44361928bbb0..aac71aef4c61 100644
@@ -3056,6 +3056,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 via @req_count argument.
+ * 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 */
+	size_in = i_size_read(inode_in);
+	if (pos_in >= size_in)
+		count = 0;
+	else
+		count = min(count, size_in - (uint64_t)pos_in);
+
+	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

The combination of file_remove_privs() and file_update_mtime() is quite common in filesystem ->write_iter() methods.

Modelled after the helper file_accessed(), introduce file_modified() and use it from generic_remap_file_range_prep().

Note that the order of calling file_remove_privs() before file_update_mtime() in the helper was matched to the more common order by filesystems and not the current order in generic_remap_file_range_prep().

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

---
 fs/inode.c         | 20 ++++++++++++++++++++
 fs/read_write.c    | 21 +++------------------
 include/linux/fs.h |  2 ++
 3 files changed, 25 insertions(+), 18 deletions(-)

fs/inode.c +4 -0

index df6542ec3b88..2885f2f2c7a5 100644
@@ -1899,6 +1899,26 @@ int file_update_time(struct file *file)
 }
 EXPORT_SYMBOL(file_update_time);
 
+/* Caller must hold the file's inode lock */
+int file_modified(struct file *file)
+{
+	int err;

fs/inode.c +6 -0

+		return 0;
+
+	return file_update_time(file);
+}
+EXPORT_SYMBOL(file_modified);
+
 int inode_needs_sync(struct inode *inode)
 {
 	if (IS_SYNC(inode))

fs/read_write.c +3 -18

index b0fb1176b628..cec7e7b1f693 100644
@@ -1980,25 +1980,10 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
 		return ret;
 
 	/* If can't alter the file contents, we're done. */
-	if (!(remap_flags & REMAP_FILE_DEDUP)) {
-		/* Update the timestamps, since we can alter file contents. */
-		if (!(file_out->f_mode & FMODE_NOCMTIME)) {
-			ret = file_update_time(file_out);
-			if (ret)
-				return ret;
-		}
+	if (!(remap_flags & REMAP_FILE_DEDUP))
+		ret = file_modified(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);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL(generic_remap_file_range_prep);
 

include/linux/fs.h +2 -0

index e4d382c4342a..79ffa2958bd8 100644
@@ -2177,6 +2177,8 @@ static inline void file_accessed(struct file *file)
 		touch_atime(&file->f_path);
 }
 
+extern int file_modified(struct file *file);
+
 int sync_inode(struct inode *inode, struct writeback_control *wbc);
 int sync_inode_metadata(struct inode *inode, int wait);
 

Signature · 1 line

2.17.1

Quoted 29 lines · Amir Goldstein

The combination of file_remove_privs() and file_update_mtime() is quite common in filesystem ->write_iter() methods.

Modelled after the helper file_accessed(), introduce file_modified() and use it from generic_remap_file_range_prep().

Note that the order of calling file_remove_privs() before file_update_mtime() in the helper was matched to the more common order by filesystems and not the current order in generic_remap_file_range_prep().

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

---
 fs/inode.c         | 20 ++++++++++++++++++++
 fs/read_write.c    | 21 +++------------------
 include/linux/fs.h |  2 ++
 3 files changed, 25 insertions(+), 18 deletions(-)

fs/inode.c +4 -0

index df6542ec3b88..2885f2f2c7a5 100644
@@ -1899,6 +1899,26 @@ int file_update_time(struct file *file)
 }
 EXPORT_SYMBOL(file_update_time);
 
+/* Caller must hold the file's inode lock */
+int file_modified(struct file *file)
+{
+     int err;

fs/inode.c int file_update_time(struct file *file) · line 1915

Quoted 10 lines

fs/inode.c +10 -0

+
+     /*
+      * Clear the security bits if the process is not being run by root.
+      * This keeps people from modifying setuid and setgid binaries.
+      */
+     err = file_remove_privs(file);
+     if (err)
+             return err;
+
+     if (likely(file->f_mode & FMODE_NOCMTIME))

I would not have thought NOCMTIME is likely?

Maybe it is for io requests coming from overlayfs, but for regular uses I don't think that's true.

Quoted 44 lines · Darrick J. Wong

Quoted 29 lines · Amir Goldstein

The combination of file_remove_privs() and file_update_mtime() is quite common in filesystem ->write_iter() methods.

Modelled after the helper file_accessed(), introduce file_modified() and use it from generic_remap_file_range_prep().

Note that the order of calling file_remove_privs() before file_update_mtime() in the helper was matched to the more common order by filesystems and not the current order in generic_remap_file_range_prep().

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

---
 fs/inode.c         | 20 ++++++++++++++++++++
 fs/read_write.c    | 21 +++------------------
 include/linux/fs.h |  2 ++
 3 files changed, 25 insertions(+), 18 deletions(-)

fs/inode.c +4 -0

index df6542ec3b88..2885f2f2c7a5 100644
@@ -1899,6 +1899,26 @@ int file_update_time(struct file *file)
 }
 EXPORT_SYMBOL(file_update_time);
 
+/* Caller must hold the file's inode lock */
+int file_modified(struct file *file)
+{
+     int err;

fs/inode.c int file_update_time(struct file *file) · line 1915

Quoted 10 lines

fs/inode.c +10 -0

+
+     /*
+      * Clear the security bits if the process is not being run by root.
+      * This keeps people from modifying setuid and setgid binaries.
+      */
+     err = file_remove_privs(file);
+     if (err)
+             return err;
+
+     if (likely(file->f_mode & FMODE_NOCMTIME))

I would not have thought NOCMTIME is likely?

Quoted 2 lines

Maybe it is for io requests coming from overlayfs, but for regular uses I don't think that's true.

Nope that's a typo. Good spotting.
Overlayfs doesn't set FMODE_NOCMTIME (yet). Only xfs does from XFS_IOC_OPEN_BY_HANDLE, but I think Dave said that is a deprecated API. so should have been very_unlikely().

Quoted 44 lines · Darrick J. Wong

Quoted 29 lines · Amir Goldstein

The combination of file_remove_privs() and file_update_mtime() is quite common in filesystem ->write_iter() methods.

Modelled after the helper file_accessed(), introduce file_modified() and use it from generic_remap_file_range_prep().

Note that the order of calling file_remove_privs() before file_update_mtime() in the helper was matched to the more common order by filesystems and not the current order in generic_remap_file_range_prep().

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

---
 fs/inode.c         | 20 ++++++++++++++++++++
 fs/read_write.c    | 21 +++------------------
 include/linux/fs.h |  2 ++
 3 files changed, 25 insertions(+), 18 deletions(-)

fs/inode.c +4 -0

index df6542ec3b88..2885f2f2c7a5 100644
@@ -1899,6 +1899,26 @@ int file_update_time(struct file *file)
 }
 EXPORT_SYMBOL(file_update_time);
 
+/* Caller must hold the file's inode lock */
+int file_modified(struct file *file)
+{
+     int err;

fs/inode.c int file_update_time(struct file *file) · line 1915

Quoted 10 lines

fs/inode.c +10 -0

+
+     /*
+      * Clear the security bits if the process is not being run by root.
+      * This keeps people from modifying setuid and setgid binaries.
+      */
+     err = file_remove_privs(file);
+     if (err)
+             return err;
+
+     if (likely(file->f_mode & FMODE_NOCMTIME))

I would not have thought NOCMTIME is likely?

Quoted 2 lines

Maybe it is for io requests coming from overlayfs, but for regular uses I don't think that's true.

Nope that's a typo. Good spotting.
Overlayfs doesn't set FMODE_NOCMTIME (yet). Only xfs does from XFS_IOC_OPEN_BY_HANDLE, but I think Dave said that is a deprecated API. so should have been very_unlikely().

Note that by using the helper, the order of calling file_remove_privs() after file_update_mtime() in xfs_file_aio_write_checks() has changed.

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

---
 fs/xfs/xfs_file.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 76748255f843..916a35cae5e9 100644

fs/xfs/xfs_file.c +1 -1

-	return 0;
+	return file_modified(file);
 }
 
 static int

Signature · 1 line

2.17.1

Quoted 13 lines · Amir Goldstein

Note that by using the helper, the order of calling file_remove_privs() after file_update_mtime() in xfs_file_aio_write_checks() has changed.

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

---
 fs/xfs/xfs_file.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 76748255f843..916a35cae5e9 100644

fs/xfs/xfs_file.c xfs_file_aio_write_checks( · line 370

Quoted 4 lines

fs/xfs/xfs_file.c +0 -1

@@ -367,20 +367,7 @@ xfs_file_aio_write_checks(
       * lock above.  Eventually we should look into a way to avoid
       * the pointless lock roundtrip.
       */
-     if (likely(!(file->f_mode & FMODE_NOCMTIME))) {

...especially since here we think NOCMTIME is likely /not/ to be set.

fs/xfs/xfs_file.c xfs_file_aio_write_checks( · line 382

Quoted 12 lines

fs/xfs/xfs_file.c +0 -12

-             error = file_update_time(file);
-             if (error)
-                     return error;
-     }
-
-     /*
-      * If we're writing the file then make sure to clear the setuid and
-      * setgid bits if the process is not being run by root.  This keeps
-      * people from modifying setuid and setgid binaries.
-      */
-     if (!IS_NOSEC(inode))
-             return file_remove_privs(file);

Hm, file_modified doesn't have the !IS_NOSEC check guarding file_remove_privs, are you sure it's ok to remove the suid bits unconditionally? Even if SB_NOSEC (and therefore S_NOSEC) are set?

Quoted 35 lines · Darrick J. Wong

Quoted 13 lines · Amir Goldstein

Note that by using the helper, the order of calling file_remove_privs() after file_update_mtime() in xfs_file_aio_write_checks() has changed.

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

---
 fs/xfs/xfs_file.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

fs/xfs/xfs_file.c +0 -0

index 76748255f843..916a35cae5e9 100644

fs/xfs/xfs_file.c xfs_file_aio_write_checks( · line 370

Quoted 4 lines

fs/xfs/xfs_file.c +0 -1

@@ -367,20 +367,7 @@ xfs_file_aio_write_checks(
       * lock above.  Eventually we should look into a way to avoid
       * the pointless lock roundtrip.
       */
-     if (likely(!(file->f_mode & FMODE_NOCMTIME))) {

...especially since here we think NOCMTIME is likely /not/ to be set.

Quoted 12 lines

fs/xfs/xfs_file.c +0 -12

-             error = file_update_time(file);
-             if (error)
-                     return error;
-     }
-
-     /*
-      * If we're writing the file then make sure to clear the setuid and
-      * setgid bits if the process is not being run by root.  This keeps
-      * people from modifying setuid and setgid binaries.
-      */
-     if (!IS_NOSEC(inode))
-             return file_remove_privs(file);

Quoted 4 lines

Hm, file_modified doesn't have the !IS_NOSEC check guarding file_remove_privs, are you sure it's ok to remove the suid bits unconditionally? Even if SB_NOSEC (and therefore S_NOSEC) are set?

file_remove_privs() has its own IS_NOSEC() check.

Thanks,
Amir.

Because generic_copy_file_range doesn't hold the destination inode lock throughout the copy, strip setuid bits before and after copy.

The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to generic_file_read_iter().

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

---
 fs/read_write.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

fs/read_write.c +21 -2

index cec7e7b1f693..706ea5f276a7 100644
@@ -1590,8 +1590,27 @@ 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);
+	struct inode *inode_out = file_inode(file_out);
+	int ret, err;
+
+	/* Should inode_out lock be held throughout the copy operation? */
+	inode_lock(inode_out);
+	err = file_modified(file_out);
+	inode_unlock(inode_out);
+	if (err)
+		return err;
+
+	ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out,
+			       len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
+
+	file_accessed(file_in);
+
+	/* To be on the safe side, remove privs also after copy */
+	inode_lock(inode_out);
+	err = file_modified(file_out);
+	inode_unlock(inode_out);
+
+	return err ?: ret;
 }
 EXPORT_SYMBOL(generic_copy_file_range);
 

Signature · 1 line

2.17.1

Quoted 3 lines · Amir Goldstein

Because generic_copy_file_range doesn't hold the destination inode lock throughout the copy, strip setuid bits before and after copy.

Quoted 5 lines

The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to generic_file_read_iter().

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

Looks reasonable,

Because ceph doesn't hold destination inode lock throughout the copy, strip setuid bits before and after copy.

The destination inode mtime is updated before and after the copy and the source inode atime is updated after the copy, similar to the filesystem ->read_iter() implementation.

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

---
 fs/ceph/file.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

fs/ceph/file.c +17 -1

index e87f7b2023af..8a70708e1aca 100644
@@ -1947,6 +1947,15 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
                goto out;
        }
 
+       /* Should dst_inode lock be held throughout the copy operation? */
+       inode_lock(dst_inode);
+       ret = file_modified(dst_file);
+       inode_unlock(dst_inode);
+       if (ret < 0) {
+               dout("failed to modify dst file before copy (%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

@@ -2097,6 +2106,14 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
 out:
        ceph_free_cap_flush(prealloc_cf);
 
+       file_accessed(src_file);
+       /* To be on the safe side, remove privs also after copy */
+       inode_lock(dst_inode);
+       err = file_modified(dst_file);
+       inode_unlock(dst_inode);
+       if (err < 0)
+               dout("failed to modify dst file after copy (%zd)\n", err);
+
        return ret;
 }
 
--

2.17.1

cifs has both source and destination inodes locked throughout the copy. Like ->write_iter(), we update mtime and strip setuid bits of destination file before copy and like ->read_iter(), we update atime of source file after copy.

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

---
 fs/cifs/cifsfs.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

fs/cifs/cifsfs.c +8 -4

index c65823270313..ab6c5c24146d 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,12 @@ 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 = file_modified(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;
+
+       file_accessed(src_file);
 
        /* force revalidate of size and timestamps of target file now
         * that target is updated on the server
--

2.17.1

Like ->write_iter(), we update mtime and strip setuid of dst file before copy and like ->read_iter(), we update atime of src file after copy.

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

---
 fs/fuse/file.c | 5 +++++
 1 file changed, 5 insertions(+)

fs/fuse/file.c +5 -1

index e03901ae729b..7f33d68f66d9 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 = file_modified(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);

@@ -3169,6 +3173,7 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in,
                clear_bit(FUSE_I_SIZE_UNSTABLE, &fi_out->state);
 
        inode_unlock(inode_out);
+       file_accessed(file_in);
 
        return err;
 }
--

2.17.1

Hi Miklos,

Could we get an ACK on this patch.
It is a prerequisite for merging the cross-device copy_file_range work.

It depends on a new helper introduced here:
https://lore.kernel.org/linux-fsdevel/CAOQ4uxjbcSWX1hUcuXbn8hFH3QYB+5bAC9Z1yCwJdR=T-GGtCg@mail.gmail.com/T/#m1569878c41f39fac3aadb3832a30659c323b582a

Like ->write_iter(), we update mtime and strip setuid of dst file before copy and like ->read_iter(), we update atime of src file after copy.

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

---
 fs/nfs/nfs42proc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

fs/nfs/nfs42proc.c +6 -4

index 5196bfa7894d..c37a8e5116c6 100644
@@ -345,10 +345,13 @@ 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 = file_modified(dst);
+               if (!err)
+                       err = _nfs42_proc_copy(src, src_lock,
+                                              dst, dst_lock,
+                                              &args, &res);
                inode_unlock(file_inode(dst));
+               file_accessed(src);
 
                if (err >= 0)
                        break;
--

2.17.1

Hi Olga,Anna,Trond

Could we get an ACK on this patch.
It is a prerequisite for merging the cross-device copy_file_range work.

It depends on a new helper introduced here:
https://lore.kernel.org/linux-fsdevel/CAOQ4uxjbcSWX1hUcuXbn8hFH3QYB+5bAC9Z1yCwJdR=T-GGtCg@mail.gmail.com/T/#m1569878c41f39fac3aadb3832a30659c323b582a

Thanks,
Amir,

Quoted 30 lines · Amir Goldstein

Like ->write_iter(), we update mtime and strip setuid of dst file before
copy and like ->read_iter(), we update atime of src file after copy.

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

---
 fs/nfs/nfs42proc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

fs/nfs/nfs42proc.c +6 -3

index 5196bfa7894d..c37a8e5116c6 100644
@@ -345,10 +345,13 @@ 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 = file_modified(dst);
+               if (!err)
+                       err = _nfs42_proc_copy(src, src_lock,
+                                              dst, dst_lock,
+                                              &args, &res);
                inode_unlock(file_inode(dst));
+               file_accessed(src);
 

Quoted 5 lines

fs/nfs/nfs42proc.c +0 -0

                if (err >= 0)

                        break;

--
2.17.1

Quoted 44 lines · Amir Goldstein

Hi Olga,Anna,Trond

Could we get an ACK on this patch.
It is a prerequisite for merging the cross-device copy_file_range work.

It depends on a new helper introduced here:
https://lore.kernel.org/linux-fsdevel/CAOQ4uxjbcSWX1hUcuXbn8hFH3QYB+5bAC9Z1yCwJdR=T-GGtCg@mail.gmail.com/T/#m1569878c41f39fac3aadb3832a30659c323b582a

Thanks,
Amir,

Quoted 30 lines · Amir Goldstein

Like ->write_iter(), we update mtime and strip setuid of dst file before
copy and like ->read_iter(), we update atime of src file after copy.

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

---
 fs/nfs/nfs42proc.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

fs/nfs/nfs42proc.c +6 -3

index 5196bfa7894d..c37a8e5116c6 100644
@@ -345,10 +345,13 @@ 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 = file_modified(dst);
+               if (!err)
+                       err = _nfs42_proc_copy(src, src_lock,
+                                              dst, dst_lock,
+                                              &args, &res);
                inode_unlock(file_inode(dst));
+               file_accessed(src);
 

Quoted 5 lines

Quoted 5 lines

fs/nfs/nfs42proc.c +0 -0

                if (err >= 0)

                        break;

--
2.17.1

Please drop this patch. In the NFS protocol, the client is not expected to mess with atime or mtime other than when the user explicitly sets it through a call to utimensat() or similar. The server takes care of any a/mtime updates as and when the file is read/written to.

Ditto for suid/sgid clearing.

Motto: "No file_accessed() or file_remove_privs() calls, please. We're NFS."

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

Reviewed-by Darrick J. Wong <[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 8a70708e1aca..e9614d686301 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;
 

@@ -2126,7 +2128,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 ab6c5c24146d..83956452c108 100644
@@ -1154,7 +1154,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 7f33d68f66d9..eab00cd089e8 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 = file_modified(file_out);

@@ -3187,7 +3190,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 +2 -1

index 706ea5f276a7..d8930bb735cb 100644
@@ -1618,7 +1618,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

fs/read_write.c +2 -6

                return file_out->f_op->copy_file_range(file_in, pos_in,
                                                       file_out, pos_out,
                                                       len, flags);

@@ -1641,10 +1652,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))

@@ -1667,7 +1674,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
         * Try cloning first, this is supported by more file systems, and
         * more efficient if both clone and copy are supported (e.g. NFS).
         */
-       if (file_in->f_op->remap_file_range) {
+       if (file_in->f_op->remap_file_range &&
+           file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) {
                loff_t cloned;
 
                cloned = file_in->f_op->remap_file_range(file_in, pos_in,
--

2.17.1

Quoted 120 lines · Amir Goldstein

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

Reviewed-by Darrick J. Wong <[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 8a70708e1aca..e9614d686301 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;
 

@@ -2126,7 +2128,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 ab6c5c24146d..83956452c108 100644
@@ -1154,7 +1154,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 7f33d68f66d9..eab00cd089e8 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 = file_modified(file_out);

@@ -3187,7 +3190,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 +2 -1

index 706ea5f276a7..d8930bb735cb 100644
@@ -1618,7 +1618,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

fs/read_write.c static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, · line 1632

Quoted 10 lines

fs/read_write.c +10 -0

+        * 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)

While I'm not sure how much I care (vs wanting at least this much of cross device copy available) but in NFS there are several NFS file_system_type defined which would disallow a copy between them (like nfs4_remote_fs_type, nfs4_remote_referral_fs_type, and good old nfs4_fs_type).

One idea would be to push the check into the filesystems themselves.

Quoted 139 lines · Olga Kornievskaia

Quoted 120 lines · Amir Goldstein

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

Reviewed-by Darrick J. Wong <[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 8a70708e1aca..e9614d686301 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;
 

@@ -2126,7 +2128,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 ab6c5c24146d..83956452c108 100644
@@ -1154,7 +1154,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 7f33d68f66d9..eab00cd089e8 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 = file_modified(file_out);

@@ -3187,7 +3190,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 +2 -1

index 706ea5f276a7..d8930bb735cb 100644
@@ -1618,7 +1618,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

fs/read_write.c static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, · line 1632

Quoted 10 lines

fs/read_write.c +10 -0

+        * 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)

While I'm not sure how much I care (vs wanting at least this much of cross device copy available) but in NFS there are several NFS file_system_type defined which would disallow a copy between them (like nfs4_remote_fs_type, nfs4_remote_referral_fs_type, and good old nfs4_fs_type).

Quoted 2 lines

One idea would be to push the check into the filesystems themselves.

That will require more delicate patches to filesystems. Are you saying there is a *good* reason to do that now? Is nfs copy offload expected to be between different types of nfs file_system_type?

Quoted 127 lines · Amir Goldstein

Quoted 124 lines · Olga Kornievskaia

Quoted 122 lines · Amir Goldstein

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

Reviewed-by Darrick J. Wong <[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 8a70708e1aca..e9614d686301 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;
 

@@ -2126,7 +2128,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 ab6c5c24146d..83956452c108 100644
@@ -1154,7 +1154,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 7f33d68f66d9..eab00cd089e8 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 = file_modified(file_out);

@@ -3187,7 +3190,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 +4 -1

index 706ea5f276a7..d8930bb735cb 100644
@@ -1618,7 +1618,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

Quoted 8 lines

Quoted 8 lines

fs/read_write.c static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, · line 1632

Quoted 8 lines

fs/read_write.c +8 -0

+        * 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)

Quoted 7 lines

Quoted 7 lines

While I'm not sure how much I care (vs wanting at least this much of cross device copy available) but in NFS there are several NFS file_system_type defined which would disallow a copy between them (like nfs4_remote_fs_type, nfs4_remote_referral_fs_type, and good old nfs4_fs_type).

Quoted 7 lines

Quoted 2 lines

One idea would be to push the check into the filesystems themselves.

That will require more delicate patches to filesystems. Are you saying there is a *good* reason to do that now? Is nfs copy offload expected to be between different types of nfs file_system_type?

So I had to setup a test case to perhaps give you a good reason. An NFS server might have an export that's a referral to another server. In this case the NFS client gets an ERR_MOVED and would mount the 2nd server. It shows up as a submount and it would have a different file system type. Having that check would prevent the NFS client from doing an NFS copy_file_range between those 2 servers and instead VFS would fallback to the generic_copy_file_range.

So why is hard(er) to push the check that ->s_types are the same for the input and output files into the filesystems like it's done in this patch with the ->i_sb checks?