fs: fixes for major copy_file_range() issues

From: Dave Chinner <[email protected]>

The man page says:

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

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

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

fs/read_write.c +0 -0

index 4dae0399c75a..09d1816cf3cf 100644

fs/read_write.c +0 -1

--

2.19.1

Quoted 9 lines

From: Dave Chinner <[email protected]>

The man page says:

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

fs/read_write.c ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, · line 1587

Quoted 4 lines

fs/read_write.c +4 -0

+       /* If the source range crosses EOF, fail the copy */
+       if (pos_in >= i_size(inode_in) || pos_in + len > i_size(inode_in))
+               return -EINVAL;
+

i_size_read()...

Otherwise

Reviewed-by Amir Goldstein <[email protected]>

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

Quoted 4 lines

i_size_read()...

Otherwise

Reviewed-by Amir Goldstein <[email protected]>

Looks like this doesn't even compile?

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop:

        while (len > 0) {
                ret = copy_file_range(... len ...)
                ...
                off_in += ret;
                off_out += ret;
                len -= ret;
        }

Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF.

So, from an application perspective, did the copy succeed or did it fail?

Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF.

IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented.

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

Quoted 7 lines

Quoted 5 lines

i_size_read()...

Otherwise

Reviewed-by Amir Goldstein <[email protected]>

Looks like this doesn't even compile?

It's fixed in a later patch that consolidates the checks into a generic check function, but I'm not sure why my "compile every patch" script didn't catch this.

Cheers,

-Dave.
--
Dave Chinner
[email protected]

Quoted 48 lines · Dave Chinner

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop:

        while (len > 0) {
                ret = copy_file_range(... len ...)
                ...
                off_in += ret;
                off_out += ret;
                len -= ret;
        }

Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF.

So, from an application perspective, did the copy succeed or did it fail?

Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF.

IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented.

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

There are two checks to consider
1. pos_in >= EOF should return EINVAL
2. however what's perhaps should be relaxed is pos_in+len >= EOF should return a short copy.

Having check#1 enforced allows to us to differentiate between a real error and a short copy.

That's what the code does right now and *exactly what I'm trying to fix* because it EINVAL is ambiguous and not an indicator that we've reached the end of the source file. EINVAL can indicate several different errors, so it really has to be treated as a "copy failed" error by applications.

Have a look at read/pread() - they return 0 in this case to indicate a short read, and the value of zero is explicitly defined as meaning "read position is beyond EOF". Applications know straight away that there is no more data to be read and there was no error, so can terminate on a successful short read.

We need to allow applications to terminate copy loops on a successful short copy.

IOWs, applications need to either:

- get an immediate error saying the range is invalid rather than doing a short copy (as per the man page); or
- have an explicit marker to say "no more data to be copied"

Applications need the "no more data to copy" case to be explicit and unambiguous so they can make sane decisions about whether a short copy was successful because the file was shorter than expected or whether a short copy was a result of a real error being encountered. The current behaviour is largely unusable for applications because they have to guess at the reason for EINVAL part way through a copy....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 53 lines · Olga Kornievskaia

Quoted 48 lines · Dave Chinner

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop:

        while (len > 0) {
                ret = copy_file_range(... len ...)
                ...
                off_in += ret;
                off_out += ret;
                len -= ret;
        }

Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF.

So, from an application perspective, did the copy succeed or did it fail?

Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF.

IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented.

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

Quoted 7 lines

There are two checks to consider
1. pos_in >= EOF should return EINVAL
2. however what's perhaps should be relaxed is pos_in+len >= EOF should return a short copy.

Having check#1 enforced allows to us to differentiate between a real error and a short copy.

That's what the code does right now and *exactly what I'm trying to fix* because it EINVAL is ambiguous and not an indicator that we've reached the end of the source file. EINVAL can indicate several different errors, so it really has to be treated as a "copy failed" error by applications.

Have a look at read/pread() - they return 0 in this case to indicate a short read, and the value of zero is explicitly defined as meaning "read position is beyond EOF". Applications know straight away that there is no more data to be read and there was no error, so can terminate on a successful short read.

We need to allow applications to terminate copy loops on a successful short copy. IOWs, applications need to either:

- get an immediate error saying the range is invalid rather than doing a short copy (as per the man page); or
- have an explicit marker to say "no more data to be copied"

Applications need the "no more data to copy" case to be explicit and unambiguous so they can make sane decisions about whether a short copy was successful because the file was shorter than expected or whether a short copy was a result of a real error being encountered. The current behaviour is largely unusable for applications because they have to guess at the reason for EINVAL part way through a copy....

Quoted 82 lines · Dave Chinner

Quoted 53 lines · Olga Kornievskaia

Quoted 48 lines · Dave Chinner

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop:

        while (len > 0) {
                ret = copy_file_range(... len ...)
                ...
                off_in += ret;
                off_out += ret;
                len -= ret;
        }

Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF.

So, from an application perspective, did the copy succeed or did it fail?

Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF.

IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented.

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

Quoted 7 lines

There are two checks to consider
1. pos_in >= EOF should return EINVAL
2. however what's perhaps should be relaxed is pos_in+len >= EOF should return a short copy.

Having check#1 enforced allows to us to differentiate between a real error and a short copy.

That's what the code does right now and *exactly what I'm trying to fix* because it EINVAL is ambiguous and not an indicator that we've reached the end of the source file. EINVAL can indicate several different errors, so it really has to be treated as a "copy failed" error by applications.

Have a look at read/pread() - they return 0 in this case to indicate a short read, and the value of zero is explicitly defined as meaning "read position is beyond EOF". Applications know straight away that there is no more data to be read and there was no error, so can terminate on a successful short read.

We need to allow applications to terminate copy loops on a successful short copy. IOWs, applications need to either:

- get an immediate error saying the range is invalid rather than doing a short copy (as per the man page); or
- have an explicit marker to say "no more data to be copied"

Quoted 8 lines

Applications need the "no more data to copy" case to be explicit and unambiguous so they can make sane decisions about whether a short copy was successful because the file was shorter than expected or whether a short copy was a result of a real error being encountered. The current behaviour is largely unusable for applications because they have to guess at the reason for EINVAL part way through a copy....

Dave,

I went a head and implemented the desired behavior.
However, while testing I observed that the desired behavior is already the existing behavior. For example, trying to copy 10 bytes from a 2 bytes file, xfs_io copy loop ends as expected:
copy_file_range(4, [0], 3, [0], 10, 0) = 2
copy_file_range(4, [2], 3, [2], 8, 0) = 0

This was tested on ext4 and xfs with reflink on recent kernel as well as on v4.20-rc1 (era of original patch set).

Where and how did you observe the EINVAL behavior described above? (besides man page that is). There are even xfstests (which you modified) that verify the return 0 for past EOF behavior.

For now, I am just dropping this patch from the patch series. Let me know if I am missing something.

Quoted 105 lines · Amir Goldstein

Quoted 82 lines · Dave Chinner

Quoted 53 lines · Olga Kornievskaia

Quoted 48 lines · Dave Chinner

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop:

        while (len > 0) {
                ret = copy_file_range(... len ...)
                ...
                off_in += ret;
                off_out += ret;
                len -= ret;
        }

Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF.

So, from an application perspective, did the copy succeed or did it fail?

Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF.

IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented.

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

Quoted 7 lines

There are two checks to consider
1. pos_in >= EOF should return EINVAL
2. however what's perhaps should be relaxed is pos_in+len >= EOF should return a short copy.

Having check#1 enforced allows to us to differentiate between a real error and a short copy.

That's what the code does right now and *exactly what I'm trying to fix* because it EINVAL is ambiguous and not an indicator that we've reached the end of the source file. EINVAL can indicate several different errors, so it really has to be treated as a "copy failed" error by applications.

Have a look at read/pread() - they return 0 in this case to indicate a short read, and the value of zero is explicitly defined as meaning "read position is beyond EOF". Applications know straight away that there is no more data to be read and there was no error, so can terminate on a successful short read.

We need to allow applications to terminate copy loops on a successful short copy. IOWs, applications need to either:

- get an immediate error saying the range is invalid rather than doing a short copy (as per the man page); or
- have an explicit marker to say "no more data to be copied"

Quoted 8 lines

Applications need the "no more data to copy" case to be explicit and unambiguous so they can make sane decisions about whether a short copy was successful because the file was shorter than expected or whether a short copy was a result of a real error being encountered. The current behaviour is largely unusable for applications because they have to guess at the reason for EINVAL part way through a copy....

Dave,

I went a head and implemented the desired behavior.
However, while testing I observed that the desired behavior is already the existing behavior. For example, trying to copy 10 bytes from a 2 bytes file, xfs_io copy loop ends as expected:
copy_file_range(4, [0], 3, [0], 10, 0) = 2
copy_file_range(4, [2], 3, [2], 8, 0) = 0

This was tested on ext4 and xfs with reflink on recent kernel as well as on v4.20-rc1 (era of original patch set).

Quoted 6 lines

Where and how did you observe the EINVAL behavior described above? (besides man page that is). There are even xfstests (which you modified) that verify the return 0 for past EOF behavior.

For now, I am just dropping this patch from the patch series. Let me know if I am missing something.

The was fixing inconsistency in what the man page specified (ie., it must fail with EINVAL if offsets are out of range) which was never enforced by the code. The patch then could be to fix the existing semantics (man page) of the system call.

Copy file range range is not only read and write but rather lseek+read+write and if somebody specifies an incorrect offset to the

lseek the system call should fail. Thus I still think that copy file range should enforce that specifying a source offset beyond the end of the file should fail with EINVAL.

If the copy file range returned 0 bytes does it mean it's a stopping condition, not according to the current semantics.

Quoted 114 lines · Olga Kornievskaia

Quoted 105 lines · Amir Goldstein

Quoted 82 lines · Dave Chinner

Quoted 53 lines · Olga Kornievskaia

Quoted 48 lines · Dave Chinner

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop:

        while (len > 0) {
                ret = copy_file_range(... len ...)
                ...
                off_in += ret;
                off_out += ret;
                len -= ret;
        }

Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF.

So, from an application perspective, did the copy succeed or did it fail?

Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF.

IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented.

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

Quoted 7 lines

There are two checks to consider
1. pos_in >= EOF should return EINVAL
2. however what's perhaps should be relaxed is pos_in+len >= EOF should return a short copy.

Having check#1 enforced allows to us to differentiate between a real error and a short copy.

That's what the code does right now and *exactly what I'm trying to fix* because it EINVAL is ambiguous and not an indicator that we've reached the end of the source file. EINVAL can indicate several different errors, so it really has to be treated as a "copy failed" error by applications.

Have a look at read/pread() - they return 0 in this case to indicate a short read, and the value of zero is explicitly defined as meaning "read position is beyond EOF". Applications know straight away that there is no more data to be read and there was no error, so can terminate on a successful short read.

We need to allow applications to terminate copy loops on a successful short copy. IOWs, applications need to either:

- get an immediate error saying the range is invalid rather than doing a short copy (as per the man page); or
- have an explicit marker to say "no more data to be copied"

Quoted 8 lines

Applications need the "no more data to copy" case to be explicit and unambiguous so they can make sane decisions about whether a short copy was successful because the file was shorter than expected or whether a short copy was a result of a real error being encountered. The current behaviour is largely unusable for applications because they have to guess at the reason for EINVAL part way through a copy....

Dave,

I went a head and implemented the desired behavior.
However, while testing I observed that the desired behavior is already the existing behavior. For example, trying to copy 10 bytes from a 2 bytes file, xfs_io copy loop ends as expected:
copy_file_range(4, [0], 3, [0], 10, 0) = 2
copy_file_range(4, [2], 3, [2], 8, 0) = 0

This was tested on ext4 and xfs with reflink on recent kernel as well as on v4.20-rc1 (era of original patch set).

Quoted 6 lines

Where and how did you observe the EINVAL behavior described above? (besides man page that is). There are even xfstests (which you modified) that verify the return 0 for past EOF behavior.

For now, I am just dropping this patch from the patch series. Let me know if I am missing something.

Quoted 7 lines

The was fixing inconsistency in what the man page specified (ie., it must fail with EINVAL if offsets are out of range) which was never enforced by the code. The patch then could be to fix the existing semantics (man page) of the system call.

Copy file range range is not only read and write but rather lseek+read+write and if somebody specifies an incorrect offset to the

Nope. it is like either read+write or pread+pwrite.

Quoted 3 lines

lseek the system call should fail. Thus I still think that copy file range should enforce that specifying a source offset beyond the end of the file should fail with EINVAL.

You appear to be out numbered by reviewers that think copy_file_range(2) should behave like pread(2) and return 0 when offf_in >= size_in.

Quoted 3 lines

If the copy file range returned 0 bytes does it mean it's a stopping condition, not according to the current semantics.

Yes. Same as read(2)/pread(2).

Thanks,
Amir.

Quoted 135 lines · Amir Goldstein

Quoted 114 lines · Olga Kornievskaia

Quoted 105 lines · Amir Goldstein

Quoted 82 lines · Dave Chinner

Quoted 53 lines · Olga Kornievskaia

Quoted 48 lines · Dave Chinner

Quoted 5 lines · Christoph Hellwig

Quoted 4 lines · Amir Goldstein

Quoted 4 lines

From: Dave Chinner <[email protected]>

The man page says:

Quoted 8 lines

Quoted 5 lines

Quoted 5 lines

EINVAL Requested range extends beyond the end of the source file

But the current behaviour is that copy_file_range does a short copy up to the source file EOF. Fix the kernel behaviour to match the behaviour described in the man page.

I think the behavior implemented is a lot more useful than the one documented..

The current behaviour is really nasty. Because copy_file_range() can return short copies, the caller has to implement a loop to ensure the range hey want get copied. When the source range you are trying to copy overlaps source EOF, this loop:

        while (len > 0) {
                ret = copy_file_range(... len ...)
                ...
                off_in += ret;
                off_out += ret;
                len -= ret;
        }

Currently the fallback code copies up to the end of the source file on the first copy and then fails the second copy with EINVAL because the source range is now completely beyond EOF.

So, from an application perspective, did the copy succeed or did it fail?

Existing tools that exercise copy_file_range (like xfs_io) consider this a failure, because the second copy_file_range() call returns EINVAL and not some "there is no more to copy" marker like read() returning 0 bytes when attempting to read beyond EOF.

IOWs, we cannot tell the difference between a real error and a short copy because the input range spans EOF and it was silently shortened. That's the API problem we need to fix here - the existing behaviour is really crappy for applications. Erroring out immmediately is one solution, and it's what the man page says should happen so that is what I implemented.

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

Quoted 7 lines

There are two checks to consider
1. pos_in >= EOF should return EINVAL
2. however what's perhaps should be relaxed is pos_in+len >= EOF should return a short copy.

Having check#1 enforced allows to us to differentiate between a real error and a short copy.

That's what the code does right now and *exactly what I'm trying to fix* because it EINVAL is ambiguous and not an indicator that we've reached the end of the source file. EINVAL can indicate several different errors, so it really has to be treated as a "copy failed" error by applications.

Have a look at read/pread() - they return 0 in this case to indicate a short read, and the value of zero is explicitly defined as meaning "read position is beyond EOF". Applications know straight away that there is no more data to be read and there was no error, so can terminate on a successful short read.

We need to allow applications to terminate copy loops on a successful short copy. IOWs, applications need to either:

- get an immediate error saying the range is invalid rather than doing a short copy (as per the man page); or
- have an explicit marker to say "no more data to be copied"

Quoted 8 lines

Applications need the "no more data to copy" case to be explicit and unambiguous so they can make sane decisions about whether a short copy was successful because the file was shorter than expected or whether a short copy was a result of a real error being encountered. The current behaviour is largely unusable for applications because they have to guess at the reason for EINVAL part way through a copy....

Dave,

I went a head and implemented the desired behavior.
However, while testing I observed that the desired behavior is already the existing behavior. For example, trying to copy 10 bytes from a 2 bytes file, xfs_io copy loop ends as expected:
copy_file_range(4, [0], 3, [0], 10, 0) = 2
copy_file_range(4, [2], 3, [2], 8, 0) = 0

This was tested on ext4 and xfs with reflink on recent kernel as well as on v4.20-rc1 (era of original patch set).

Quoted 6 lines

Where and how did you observe the EINVAL behavior described above? (besides man page that is). There are even xfstests (which you modified) that verify the return 0 for past EOF behavior.

For now, I am just dropping this patch from the patch series. Let me know if I am missing something.

Quoted 7 lines

The was fixing inconsistency in what the man page specified (ie., it must fail with EINVAL if offsets are out of range) which was never enforced by the code. The patch then could be to fix the existing semantics (man page) of the system call.

Copy file range range is not only read and write but rather lseek+read+write and if somebody specifies an incorrect offset to the

Nope. it is like either read+write or pread+pwrite.

Quoted 3 lines

lseek the system call should fail. Thus I still think that copy file range should enforce that specifying a source offset beyond the end of the file should fail with EINVAL.

You appear to be out numbered by reviewers that think copy_file_range(2) should behave like pread(2) and return 0 when offf_in >= size_in.

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

If the copy file range returned 0 bytes does it mean it's a stopping condition, not according to the current semantics.

Yes. Same as read(2)/pread(2).

If that's the case, then it's great. Perhaps it's the fact that the copy_file_range man page doesn't talk about it that makes it confusing.

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

I agree with this statement. So we don't we implement these semantics?

Quoted 5 lines · Christoph Hellwig

Quoted 3 lines

Realistically, though, I think an attempt to read beyond EOF for the copy should result in behaviour like read() (i.e. return 0 bytes), not EINVAL. The existing behaviour needs to change, though.

I agree with this statement. So we don't we implement these semantics?

No, we don't.

I will rework the patch series to make attempts to copy beyond the end of the source file return 0 to indicate that there is no more data to copy rather than return an error.

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

---

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

fs/read_write.c +32 -3

index 09d1816cf3cf..50114694c98b 100644
@@ -1540,6 +1540,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

@@ -1611,9 +1641,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 -1

index c95c0807471f..a4478764cf63 100644
@@ -1874,6 +1874,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,
--

2.19.1

Quoted 9 lines · Dave Chinner

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

Quoted 6 lines

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

---

Looks good.

Reviewed-by Amir Goldstein <[email protected]>

Question:
2 years ago you suggested that I covert the overlayfs copy up code that does a do_direct_splice() with a loop of vfs_copy_file_range():
https://marc.info/?l=linux-fsdevel&m=147369468521525&w=2
We ended up with a slightly different solution, but with your recent changes, I can get back to your original proposal.

Back then, I wondered whether it makes sense to push the killable loop of shorter do_direct_splice() calls into the vfs helper. What do you think about adding this to generic_copy_file_range() now? (I can do that after your changes are merged).

The fact that userspace *can* enter a very long unkillable loop with current copy_file_range() syscall doesn't mean that we *should* persist this situation. After all, fixing the brokenness of the existing interface is what you set out to do.

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.

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

fs/overlayfs/file.c +1 -1

index 84dd957efa24..68736e5d6a56 100644
@@ -486,8 +486,15 @@ static ssize_t ovl_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 ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
+       ssize_t ret;

Quoted 9 lines · Dave Chinner

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

Quoted 6 lines

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.

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

You may add

Reviewed-by Amir Goldstein <[email protected]>

After fixing the overlayfs issue below.
...

Quoted 17 lines

fs/overlayfs/file.c +7 -1

index 84dd957efa24..68736e5d6a56 100644
@@ -486,8 +486,15 @@ static ssize_t ovl_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 ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
+       ssize_t ret;
+
+       ret =  ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
                            OVL_COPY);
+
+       if (ret == -EOPNOTSUPP)
+               ret = generic_copy_file_range(file_in, pos_in, file_out,
+                                       pos_out, len, flags);

fs/overlayfs/file.c static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in, · line 499

Quoted 3 lines

fs/overlayfs/file.c +1 -0

+       return ret;
 }
 

This is unneeded, because ovl_copyfile(OVL_COPY) is implemented by calling vfs_copy_file_range() (on the underlying files) and it is not possible
to get EOPNOTSUPP from vfs_copy_file_range().

Quoted 40 lines · Amir Goldstein

Quoted 9 lines · Dave Chinner

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

Quoted 6 lines

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.

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

You may add

Reviewed-by Amir Goldstein <[email protected]>

After fixing the overlayfs issue below.
...

Quoted 17 lines

fs/overlayfs/file.c +7 -1

index 84dd957efa24..68736e5d6a56 100644
@@ -486,8 +486,15 @@ static ssize_t ovl_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 ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
+       ssize_t ret;
+
+       ret =  ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
                            OVL_COPY);
+
+       if (ret == -EOPNOTSUPP)
+               ret = generic_copy_file_range(file_in, pos_in, file_out,
+                                       pos_out, len, flags);

Quoted 8 lines

fs/overlayfs/file.c static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in, · line 499

Quoted 3 lines

fs/overlayfs/file.c +1 -0

+       return ret;
 }
 

This is unneeded, because ovl_copyfile(OVL_COPY) is implemented by calling vfs_copy_file_range() (on the underlying files) and it is not possible
to get EOPNOTSUPP from vfs_copy_file_range().

Except that it is possible. e.g. If the underlying filesystem tries a copy offload, gets a "not supported" failure from the remote server and then doesn't implement a fallback.

Quoted 51 lines · Dave Chinner

Quoted 40 lines · Amir Goldstein

Quoted 9 lines · Dave Chinner

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

Quoted 6 lines

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.

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

You may add

Reviewed-by Amir Goldstein <[email protected]>

After fixing the overlayfs issue below.
...

Quoted 17 lines

fs/overlayfs/file.c +7 -1

index 84dd957efa24..68736e5d6a56 100644
@@ -486,8 +486,15 @@ static ssize_t ovl_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 ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
+       ssize_t ret;
+
+       ret =  ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
                            OVL_COPY);
+
+       if (ret == -EOPNOTSUPP)
+               ret = generic_copy_file_range(file_in, pos_in, file_out,
+                                       pos_out, len, flags);

Quoted 8 lines

fs/overlayfs/file.c static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in, · line 499

Quoted 3 lines

fs/overlayfs/file.c +1 -0

+       return ret;
 }
 

This is unneeded, because ovl_copyfile(OVL_COPY) is implemented by calling vfs_copy_file_range() (on the underlying files) and it is not possible
to get EOPNOTSUPP from vfs_copy_file_range().

Quoted 4 lines

Except that it is possible. e.g. If the underlying filesystem tries a copy offload, gets a "not supported" failure from the remote server and then doesn't implement a fallback.

I'm in the opinion that ovl_copy_file_range() and do_copy_file_range() are a like. If you choose not to fallback in the latter to generic_copy_file_range() for misbehaving filesystem and WARN_ON this case, there is no reason for overlayfs to cover up for the misbehaving underlying filesystem.

If you want to cover up for misbehaving filesystem, please do it in do_copy_file_range() and drop the WARN_ON_ONCE().
Come to think about it, I understand your reasoning for pushing generic_copy_file_range() down to filesystems so they can fallback to it in several error conditions.
I do not follow the reasoning of NOT falling back to generic_copy_file_range() in vfs if EOPNOTSUPP is returned from filesystem. IOW, if we want to cover up for misbehaving filesystem, this would have been a more robust code:

Quoted 63 lines · Amir Goldstein

Quoted 51 lines · Dave Chinner

Quoted 40 lines · Amir Goldstein

Quoted 9 lines · Dave Chinner

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

Quoted 6 lines

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.

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

You may add

Reviewed-by Amir Goldstein <[email protected]>

After fixing the overlayfs issue below.
...

Quoted 17 lines

fs/overlayfs/file.c +7 -1

index 84dd957efa24..68736e5d6a56 100644
@@ -486,8 +486,15 @@ static ssize_t ovl_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 ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
+       ssize_t ret;
+
+       ret =  ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
                            OVL_COPY);
+
+       if (ret == -EOPNOTSUPP)
+               ret = generic_copy_file_range(file_in, pos_in, file_out,
+                                       pos_out, len, flags);

Quoted 8 lines

fs/overlayfs/file.c static ssize_t ovl_copy_file_range(struct file *file_in, loff_t pos_in, · line 499

Quoted 3 lines

fs/overlayfs/file.c +1 -0

+       return ret;
 }
 

This is unneeded, because ovl_copyfile(OVL_COPY) is implemented by calling vfs_copy_file_range() (on the underlying files) and it is not possible
to get EOPNOTSUPP from vfs_copy_file_range().

Quoted 4 lines

Except that it is possible. e.g. If the underlying filesystem tries a copy offload, gets a "not supported" failure from the remote server and then doesn't implement a fallback.

I'm in the opinion that ovl_copy_file_range() and do_copy_file_range() are a like. If you choose not to fallback in the latter to generic_copy_file_range() for misbehaving filesystem and WARN_ON this case, there is no reason for overlayfs to cover up for the misbehaving underlying filesystem.

Quoted 9 lines

If you want to cover up for misbehaving filesystem, please do it in do_copy_file_range() and drop the WARN_ON_ONCE().
Come to think about it, I understand your reasoning for pushing generic_copy_file_range() down to filesystems so they can fallback to it in several error conditions.
I do not follow the reasoning of NOT falling back to generic_copy_file_range() in vfs if EOPNOTSUPP is returned from filesystem. IOW, if we want to cover up for misbehaving filesystem, this would have been a more robust code:

Since when have we defined a filesystem returning -EOPNOTSUPP as a "misbehaving filesystem"?

Userspace has to handle errors in
copy_file_range() with it's own fallback copy code (i.e. it cannot rely on the kernel actually supporting copy_file_range at all). Hence it's perfectly fine for a filesystem implementation to encode "offload or fail entirely" semantics if they want.

Yes, I've been shouted at by developers quite recently who *demanded* that copy_file_range (and other offloads like fallocate(ZERO_RANGE)) *fail* if they cannot "offload" the operation to make it "fast". The application developers want to use different algorithms if the kernel offload isn't any faster than userspace doing the dumb thing and phsyically pushing bytes around itself.

I've pushed back on this as much as I can, but it doesn't change the fact that for many situations doing do_splice_direct() is exactly the wrong thing to do (e.g. because copy_file_range() on a TB+ scale file couldn't be offloaded by the filesystem because the server said EOPNOTSUPP)

IOWs, for some filesystems or situations where it makes sense to have fail-fast semantics and leave the decision of what to do next in the hands of the userspace application that has the context necessary to determine what the best action to take is. And to do that, we need to give control of the fallback to the filesystems.

Flexibility is what is needed here, not a dumb, hard coded "the VFS always know what's right for you" policy that triggers when nobody really wants it to.

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.

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

---
 fs/ceph/file.c      | 17 ++++++++++++++++-
 fs/cifs/cifsfs.c    |  4 ++++
 fs/fuse/file.c      | 17 ++++++++++++++++-
 fs/nfs/nfs4file.c   |  4 ++++

 fs/overlayfs/file.c |  9 ++++++++-
 fs/read_write.c     | 24 +++++++++++++++---------
 6 files changed, 63 insertions(+), 12 deletions(-)

fs/ceph/file.c +1 -1

index 189df668b6a0..cf29f0410dcb 100644
@@ -1885,7 +1885,7 @@ 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,
+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)

{

fs/ceph/file.c +15 -0

@@ -2096,6 +2096,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,

fs/ceph/file.c +0 -0

 	.release = ceph_release,

fs/cifs/cifsfs.c +4 -0

index 865706edb307..5ef4baec6234 100644
@@ -1141,6 +1141,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 +1 -1

index b52f9baaa3e7..b86fb0298739 100644
@@ -3024,7 +3024,7 @@ 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,
+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)

{

fs/fuse/file.c +15 -0

@@ -3100,6 +3100,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,

fs/fuse/file.c +0 -0

 	.read_iter	= fuse_file_read_iter,

fs/nfs/nfs4file.c +4 -0

index 46d691ba04bc..d7766a6eb0f4 100644
@@ -141,6 +141,10 @@ static ssize_t nfs4_copy_file_range(struct file *file_in,
 loff_t pos_in,
 	ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count);
 	if (ret == -EAGAIN)
 		goto retry;
+
+	if (ret == -EOPNOTSUPP)
+		ret = generic_copy_file_range(file_in, pos_in, file_out,
+					pos_out, count, flags);
 	return ret;
 }

fs/overlayfs/file.c +8 -1

index 84dd957efa24..68736e5d6a56 100644
@@ -486,8 +486,15 @@ static ssize_t ovl_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 ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
+	ssize_t ret;
+
+	ret =  ovl_copyfile(file_in, pos_in, file_out, pos_out, len, flags,
 			    OVL_COPY);
+
+	if (ret == -EOPNOTSUPP)
+		ret = generic_copy_file_range(file_in, pos_in, file_out,
+					pos_out, len, flags);
+	return ret;
 }
 

static loff_t ovl_remap_file_range(struct file *file_in, loff_t pos_in,

fs/read_write.c +12 -0

index 50114694c98b..44339b44accc 100644
@@ -1570,6 +1570,18 @@ 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

fs/read_write.c +3 -9

@@ -1634,15 +1646,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);

From: Dave Chinner <[email protected]>

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.

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

---

From: Dave Chinner <[email protected]>

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.

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

---
 fs/read_write.c    | 27 ++++++------------
 include/linux/fs.h |  3 ++
 mm/filemap.c       | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 18 deletions(-)

fs/read_write.c +9 -18

index 44339b44accc..69809345977e 100644
@@ -1578,7 +1578,7 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
 		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,
+	return generic_copy_file_range(file_in, pos_in, file_out, pos_out,
 					len, flags);
 }
 

@@ -1598,10 +1598,14 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
 	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 (inode_in->i_sb != inode_out->i_sb)
+		return -EXDEV;
+
+	ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
+					flags);
+	if (ret < 0)
+		return ret;
 
 	ret = rw_verify_area(READ, file_in, &pos_in, len);
 	if (unlikely(ret))

@@ -1611,22 +1615,9 @@ 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;
 
-	/* If the source range crosses EOF, fail the copy */
-	if (pos_in >= i_size(inode_in) || pos_in + len > i_size(inode_in))
-		return -EINVAL;
-
 	file_start_write(file_out);
 
 	/*

include/linux/fs.h +3 -0

index a4478764cf63..0d9d2d93d4df 100644
@@ -3022,6 +3022,9 @@ 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_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 +27 -0

index 81adec8ee02c..0a170425935b 100644
@@ -2975,6 +2975,75 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
 	return 0;
 }
 
+
+/*
+ * Performs necessary checks before doing a file copy
+ *
+ * Can adjust amount of bytes to copy
+ * 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;
+	uint64_t bcount;
+	loff_t size_in, size_out;
+	loff_t bs = inode_out->i_sb->s_blocksize;
+	int 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 +32 -0

+
+	/* Ensure offsets don't wrap. */
+	if (pos_in + count < pos_in || pos_out + count < pos_out)
+		return -EOVERFLOW;
+
+	size_in = i_size_read(inode_in);
+	size_out = i_size_read(inode_out);
+
+	/* If the source range crosses EOF, fail the copy */
+	if (pos_in >= size_in)
+		return -EINVAL;
+	if (pos_in + count > size_in)
+		return -EINVAL;
+
+	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;
+
+	/* 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.19.1

From: Dave Chinner <[email protected]>

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.

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

---
 fs/read_write.c    | 27 ++++++------------
 include/linux/fs.h |  3 ++
 mm/filemap.c       | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 18 deletions(-)

fs/read_write.c +9 -18

index 44339b44accc..69809345977e 100644
@@ -1578,7 +1578,7 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
                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,
+       return generic_copy_file_range(file_in, pos_in, file_out, pos_out,
                                        len, flags);
 }
 

@@ -1598,10 +1598,14 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
        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 (inode_in->i_sb != inode_out->i_sb)
+               return -EXDEV;
+
+       ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len,
+                                       flags);
+       if (ret < 0)
+               return ret;
 
        ret = rw_verify_area(READ, file_in, &pos_in, len);
        if (unlikely(ret))

@@ -1611,22 +1615,9 @@ 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;
 
-       /* If the source range crosses EOF, fail the copy */
-       if (pos_in >= i_size(inode_in) || pos_in + len > i_size(inode_in))
-               return -EINVAL;
-
        file_start_write(file_out);
 
        /*

include/linux/fs.h +3 -0

index a4478764cf63..0d9d2d93d4df 100644
@@ -3022,6 +3022,9 @@ 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_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 +9 -0

index 81adec8ee02c..0a170425935b 100644
@@ -2975,6 +2975,75 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
        return 0;
 }
 
+
+/*
+ * Performs necessary checks before doing a file copy
+ *
+ * Can adjust amount of bytes to copy
+ * 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,

mm/filemap.c +50 -1

+
+       /* 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;
+
+       /* Don't copy dirs, pipes, sockets... */
+       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;
+
+       if (!(file_in->f_mode & FMODE_READ) ||
+           !(file_out->f_mode & FMODE_WRITE) ||
+           (file_out->f_flags & O_APPEND))
+               return -EBADF;
+
+       /* Ensure offsets don't wrap. */
+       if (pos_in + count < pos_in || pos_out + count < pos_out)
+               return -EOVERFLOW;
+
+       size_in = i_size_read(inode_in);
+       size_out = i_size_read(inode_out);
+
+       /* If the source range crosses EOF, fail the copy */
+       if (pos_in >= size_in)
+               return -EINVAL;
+       if (pos_in + count > size_in)
+               return -EINVAL;
+
+       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;
+
+       /* 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)
--

2.19.1

Quoted 29 lines · Dave Chinner

include/linux/fs.h +3 -0

@@ -3022,6 +3022,9 @@ 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_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 +9 -0

index 81adec8ee02c..0a170425935b 100644
@@ -2975,6 +2975,75 @@ int generic_remap_checks(struct file *file_in, loff_t pos_in,
        return 0;
 }
 
+
+/*
+ * Performs necessary checks before doing a file copy
+ *
+ * Can adjust amount of bytes to copy
+ * 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,

mm/filemap.c int generic_remap_checks(struct file *file_in, loff_t pos_in, · line 2996

Quoted 10 lines

mm/filemap.c +10 -0

+                        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;
+       uint64_t bcount;
+       loff_t size_in, size_out;
+       loff_t bs = inode_out->i_sb->s_blocksize;
+       int ret;

I got compile warnings:

mm/filemap.c: In function ‘generic_copy_file_checks’:
mm/filemap.c:2995:9: warning: unused variable ‘bs’ [-Wunused-variable]

  loff_t bs = inode_out->i_sb->s_blocksize;
         ^

mm/filemap.c:2993:11: warning: unused variable ‘bcount’ [-Wunused-variable]
uint64_t bcount;

(quoted diff) +4 -0

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

Dave Chinner <[email protected]> writes:

<snip>

Quoted 4 lines

(quoted diff) +4 -0

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

<snip>

Review

Quoted 5 lines

(quoted diff) +5 -0

+	/* 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;

I was wondering if, with the above check, it would make sense to also have an extra patch changing some filesystems (ceph, nfs and cifs) to simply return -EOPNOTSUPP (instead of -EINVAL) when inode_in == inode_out. Something like the diff below (not tested!).

This caught my attention when I was running the latest generic xfstests on ceph and realised that I had some new failures due to the recently added copy_file_range support in fsx by Darrick. The failures were caused by the usage of the same fd both as source and destination.

Cheers,

Signature · 1 line

Luis

fs/ceph/file.c +1 -1

index 189df668b6a0..c22ac60ec0ba 100644
@@ -1904,7 +1904,7 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off,
 	bool do_final_copy = false;
 
 	if (src_inode == dst_inode)
-		return -EINVAL;
+		return -EOPNOTSUPP;
 	if (ceph_snap(dst_inode) != CEPH_NOSNAP)
 		return -EROFS;
 

fs/cifs/cifsfs.c +1 -1

index 865706edb307..d4f63eae531e 100644
@@ -1068,7 +1068,7 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	cifs_dbg(FYI, "copychunk range\n");
 
 	if (src_inode == target_inode) {
-		rc = -EINVAL;
+		rc = -EOPNOTSUPP;
 		goto out;
 	}
 

fs/nfs/nfs4file.c +1 -1

index 46d691ba04bc..910a2abade92 100644
@@ -136,7 +136,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
 	ssize_t ret;
 
 	if (file_inode(file_in) == file_inode(file_out))
-		return -EINVAL;
+		return -EOPNOTSUPP;
 retry:
 	ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count);
 	if (ret == -EAGAIN)

Dave Chinner <[email protected]> writes:

<snip>

Quoted 4 lines

(quoted diff) +4 -0

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

<snip>

Review

Quoted 5 lines

(quoted diff) +5 -0

+     /* 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;

I was wondering if, with the above check, it would make sense to also have an extra patch changing some filesystems (ceph, nfs and cifs) to simply return -EOPNOTSUPP (instead of -EINVAL) when inode_in == inode_out. Something like the diff below (not tested!).

This caught my attention when I was running the latest generic xfstests on ceph and realised that I had some new failures due to the recently added copy_file_range support in fsx by Darrick. The failures were caused by the usage of the same fd both as source and destination.

Cheers,
--
Luis

fs/ceph/file.c +1 -1

index 189df668b6a0..c22ac60ec0ba 100644
@@ -1904,7 +1904,7 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off,
        bool do_final_copy = false;
 
        if (src_inode == dst_inode)
-               return -EINVAL;
+               return -EOPNOTSUPP;
        if (ceph_snap(dst_inode) != CEPH_NOSNAP)
                return -EROFS;
 

fs/cifs/cifsfs.c +1 -1

index 865706edb307..d4f63eae531e 100644
@@ -1068,7 +1068,7 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
        cifs_dbg(FYI, "copychunk range\n");
 
        if (src_inode == target_inode) {
-               rc = -EINVAL;
+               rc = -EOPNOTSUPP;
                goto out;
        }
 

fs/nfs/nfs4file.c +0 -0

index 46d691ba04bc..910a2abade92 100644

fs/nfs/nfs4file.c +0 -0

 retry:
        ret = nfs42_proc_copy(file_in, pos_in, file_out, pos_out, count);
        if (ret == -EAGAIN)

fs/nfs/nfs4file.c static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, · line 139

Quoted 12 lines · Luis Henriques

I was wondering if, with the above check, it would make sense to also have an extra patch changing some filesystems (ceph, nfs and cifs) to simply return -EOPNOTSUPP (instead of -EINVAL) when inode_in == inode_out. Something like the diff below (not tested!).

+++ b/fs/nfs/nfs4file.c

fs/nfs/nfs4file.c +1 -1

@@ -136,7 +136,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
        ssize_t ret;
 
        if (file_inode(file_in) == file_inode(file_out))
-               return -EINVAL;
+               return -EOPNOTSUPP;

Please don't change the NFS bits. This is against the NFS specifications. RFC 7862 15.2.3

(snippet)
SAVED_FH and CURRENT_FH must be different files. If SAVED_FH and
CURRENT_FH refer to the same file, the operation MUST fail with NFS4ERR_INVAL.

Quoted 17 lines · Olga Kornievskaia

fs/nfs/nfs4file.c static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, · line 139

Quoted 12 lines · Luis Henriques

I was wondering if, with the above check, it would make sense to also have an extra patch changing some filesystems (ceph, nfs and cifs) to simply return -EOPNOTSUPP (instead of -EINVAL) when inode_in == inode_out. Something like the diff below (not tested!).

+++ b/fs/nfs/nfs4file.c

fs/nfs/nfs4file.c +1 -1

@@ -136,7 +136,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
        ssize_t ret;
 
        if (file_inode(file_in) == file_inode(file_out))
-               return -EINVAL;
+               return -EOPNOTSUPP;

Please don't change the NFS bits. This is against the NFS specifications. RFC 7862 15.2.3

Quoted 4 lines

(snippet)
SAVED_FH and CURRENT_FH must be different files. If SAVED_FH and
CURRENT_FH refer to the same file, the operation MUST fail with NFS4ERR_INVAL.

I don't see how that applies. That refers to a requirement _in the protocol_ that determines what the server MUST do if the client sends it two FHs which refer to the same file.

What we're talking about here is how a Linux filesystem behaves when receiving a copy_file_range() referring to the same file. As long as the Linux filesystem doesn't react by sending out one of these invalid protocol messages, I don't see the problem.

Quoted 24 lines · Matthew Wilcox

Quoted 17 lines · Olga Kornievskaia

Review static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, · line 139

Quoted 12 lines · Luis Henriques

I was wondering if, with the above check, it would make sense to also have an extra patch changing some filesystems (ceph, nfs and cifs) to simply return -EOPNOTSUPP (instead of -EINVAL) when inode_in == inode_out. Something like the diff below (not tested!).

+++ b/fs/nfs/nfs4file.c

(quoted diff) +1 -1

@@ -136,7 +136,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in,
        ssize_t ret;
 
        if (file_inode(file_in) == file_inode(file_out))
-               return -EINVAL;
+               return -EOPNOTSUPP;

Please don't change the NFS bits. This is against the NFS specifications. RFC 7862 15.2.3

Quoted 4 lines

(snippet)
SAVED_FH and CURRENT_FH must be different files. If SAVED_FH and
CURRENT_FH refer to the same file, the operation MUST fail with NFS4ERR_INVAL.

Quoted 8 lines

I don't see how that applies. That refers to a requirement _in the protocol_ that determines what the server MUST do if the client sends it two FHs which refer to the same file.

What we're talking about here is how a Linux filesystem behaves when receiving a copy_file_range() referring to the same file. As long as the Linux filesystem doesn't react by sending out one of these invalid protocol messages, I don't see the problem.

Ok then this should be changed to call generic_copy_file_range() not returning the EOPNOTSUPP since there is no longer fallback in vfs to call the generic_copy_file_range() and in turn responsibility of each file system.

From f66a07e22dc93827bdafc1666d4980edc986bce4 Mon Sep 17 00:00:00 2001

From: Dave Chinner <[email protected]>

Similar to FI_DEDUPERANGE, make copy_file_range() check that we have write permissions to the destination inode.

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

---

From: Dave Chinner <[email protected]>

Similar to FI_DEDUPERANGE, make copy_file_range() check that we have

write permissions to the destination inode.

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

---
 mm/filemap.c | 5 +++++
 1 file changed, 5 insertions(+)

mm/filemap.c +0 -0

index 0a170425935b..876df5275514 100644

mm/filemap.c +3 -0

+	if (ret < 0)
+		return ret;
+
 	/* Ensure offsets don't wrap. */
 	if (pos_in + count < pos_in || pos_out + count < pos_out)
 		return -EOVERFLOW;

Signature · 1 line

2.19.1

Quoted 3 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Similar to FI_DEDUPERANGE, make copy_file_range() check that we have

TLDR: No, it's not similar to FIDEDUPERANGE -- the use of inode_permission() in allow_file_dedupe() is to enable callers to dedupe into a file for which the caller has write permissions but opened the file O_RDONLY.

[Please keep reading...]

Quoted 12 lines

write permissions to the destination inode.

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

---
 mm/filemap.c | 5 +++++
 1 file changed, 5 insertions(+)

mm/filemap.c +0 -0

index 0a170425935b..876df5275514 100644

mm/filemap.c int generic_copy_file_checks(struct file *file_in, loff_t pos_in, · line 3017

Quoted 5 lines

mm/filemap.c +2 -0

@@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
 	    (file_out->f_flags & O_APPEND))
 		return -EBADF;
 
+	/* may sure we really are allowed to write to the destination inode */
+	ret = inode_permission(inode_out, MAY_WRITE);

What's the difference between security_file_permission and inode_permission, and when do we call them for a regular open-write-close sequence? Hmmm, let me take a look:

We also cannot dedupe into a file that becomes immutable after we open it for write, but we can dedupe into a file that loses its write permissions after we open it.

op: after +immutable? after chmod a-w?
write yes yes
clonerange no yes
dedupe no maybe
newcopyrange no no

My reaction: I don't think that writes should be allowed after an administrator marks a file immutable (but that's a separate issue) but I do think we should be consistent in allowing copying into a file that has lost its write permissions after we opened the file for write, like we do for write() and the remap ioct....

Quoted 9 lines · Darrick J. Wong

Quoted 3 lines · Dave Chinner

From: Dave Chinner <[email protected]>

Similar to FI_DEDUPERANGE, make copy_file_range() check that we have

TLDR: No, it's not similar to FIDEDUPERANGE -- the use of inode_permission() in allow_file_dedupe() is to enable callers to dedupe into a file for which the caller has write permissions but opened the file O_RDONLY.

What a grotty, nasty hack.

Quoted 23 lines

[Please keep reading...]

Quoted 12 lines

write permissions to the destination inode.

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

---
 mm/filemap.c | 5 +++++
 1 file changed, 5 insertions(+)

mm/filemap.c +0 -0

index 0a170425935b..876df5275514 100644

mm/filemap.c int generic_copy_file_checks(struct file *file_in, loff_t pos_in, · line 3017

Quoted 5 lines

mm/filemap.c +2 -0

@@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
 	    (file_out->f_flags & O_APPEND))
 		return -EBADF;
 
+	/* may sure we really are allowed to write to the destination inode */
+	ret = inode_permission(inode_out, MAY_WRITE);

What's the difference between security_file_permission and inode_permission, and when do we call them for a regular open-write-close sequence? Hmmm, let me take a look:

.....

Quoted 3 lines

We also cannot dedupe into a file that becomes immutable after we open it for write, but we can dedupe into a file that loses its write permissions after we open it.

It's more nuanced than that - dedupe will proceed after write permissions have been removed only if you are root or own the file, otherwise it will fail.

Updated summary:

Quoted 11 lines

op: after +immutable? after chmod a-w?
write yes yes
clonerange no yes
dedupe no maybe
newcopyrange no no

My reaction: I don't think that writes should be allowed after an administrator marks a file immutable (but that's a separate issue) but I do think we should be consistent in allowing copying into a file that has lost its write permissions after we opened the file for write, like we do for write() and the remap ioct....

If we want to allow copying to files we don't actually have permission to write to anymore, then I'll remove this from the test, the man page and the code. But, quite frankly, I don't trust remote server side copies to follow the same permission models as the client side OS, so I think we have to treat copy_file_range differently to a normal write syscall....

From: Dave Chinner <[email protected]>

Similar to FI_DEDUPERANGE, make copy_file_range() check that we have write permissions to the destination inode.

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

---
 mm/filemap.c | 5 +++++
 1 file changed, 5 insertions(+)

mm/filemap.c +5 -0

index 0a170425935b..876df5275514 100644
@@ -3013,6 +3013,11 @@ int generic_copy_file_checks(struct file *file_in, loff_t pos_in,
 	    (file_out->f_flags & O_APPEND))
 		return -EBADF;
 
+	/* may sure we really are allowed to write to the destination inode */
+	ret = inode_permission(inode_out, MAY_WRITE);
+	if (ret < 0)
+		return ret;
+
 	/* Ensure offsets don't wrap. */
 	if (pos_in + count < pos_in || pos_out + count < pos_out)
 		return -EOVERFLOW;

Signature · 1 line

2.19.1

From: Dave Chinner <[email protected]>

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.

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

---

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

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

fs/read_write.c +0 -0

index 3b101183ea19..3288db1d5f21 100644
@@ -1576,6 +1576,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
 {
        ssize_t ret;
 

Quoted 22 lines · Dave Chinner

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

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

fs/read_write.c +0 -0

index 3b101183ea19..3288db1d5f21 100644
@@ -1576,6 +1576,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
 {
        ssize_t ret;
 

fs/read_write.c static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, · 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;
+       }
+

If there is a consistency about who is responsible of calling file_accessed() and file_update_time() it eludes me. grep tells me that they are mostly handled by filesystem code or generic helpers called by filesystem code and not in the vfs helpers.

FMODE_NOCMTIME seems like an xfs specific flag (for DMAPI?), which most generic callers of file_update_time() completely ignore. This seems like another argument in favor of leaving the responsibility of the timestamp updates to the filesystem.

Maybe I am missing something?

Thanks,
Amir.

Quoted 45 lines · Amir Goldstein

Quoted 22 lines · Dave Chinner

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

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

fs/read_write.c +0 -0

index 3b101183ea19..3288db1d5f21 100644
@@ -1576,6 +1576,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
 {
        ssize_t ret;
 

fs/read_write.c static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, · 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;
+       }
+

If there is a consistency about who is responsible of calling file_accessed() and file_update_time() it eludes me. grep tells me that they are mostly handled by filesystem code or generic helpers called by filesystem code and not in the vfs helpers.

FMODE_NOCMTIME seems like an xfs specific flag (for DMAPI?), which most generic callers of file_update_time() completely ignore. This seems like another argument in favor of leaving the responsibility of the timestamp updates to the filesystem.

Quoted 2 lines

Maybe I am missing something?

I had similar question before about who is responsible for doing the checks. I agree that attributes should be updated for the case when no filesystem support exist for copy_file_range() but this code does it for all the cases. I also wonder if it's appropriate to update the attributes before the copy is actually done?

Quoted 2 lines

Thanks,
Amir.

Quoted 22 lines · Dave Chinner

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

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

fs/read_write.c +0 -0

index 3b101183ea19..3288db1d5f21 100644
@@ -1576,6 +1576,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
 {
        ssize_t ret;
 

fs/read_write.c static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, · 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;
+       }
+

If there is a consistency about who is responsible of calling file_accessed() and file_update_time() it eludes me. grep tells me that they are mostly handled by filesystem code or generic helpers called by filesystem code and not in the vfs helpers.

FMODE_NOCMTIME seems like an xfs specific flag (for DMAPI?), which

most generic callers of file_update_time() completely ignore.

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.

From: Dave Chinner <[email protected]>

before we can enable cross-device copies into copy_file_range(), we have to ensure that ->remap_file_range() implemenations will correctly reject attempts to do cross filesystem clones. Currently

these checks are done above calls to ->remap_file_range(), but we need to drive them inwards so that we get EXDEV protection for all callers of ->remap_file_range().

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

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

fs/read_write.c +13 -0

index 3288db1d5f21..174cf92eea1d 100644
@@ -1909,6 +1909,19 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
        bool same_inode = (inode_in == inode_out);
        int ret;
 
+       /*
+        * 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. We check this here rather than at the ioctl layers because
+        * this is effectively a limitation of the fielsystem implementations,
+        * not so much the API itself. Further, ->remap_file_range() can be
+        * called from syscalls that don't have cross device copy restrictions
+        * (such as copy_file_range()) and so we need to catch them before we
+        * do any damage.
+        */
+       if (inode_in->i_sb != inode_out->i_sb)
+               return -EXDEV;
+
        /* Don't touch certain kinds of inodes */
        if (IS_IMMUTABLE(inode_out))
                return -EPERM;

@@ -2013,14 +2026,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
        if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

before we can enable cross-device copies into copy_file_range(), we have to ensure that ->remap_file_range() implemenations will correctly reject attempts to do cross filesystem clones. Currently

But you only fixed remap_file_range() implemenations of xfs and ocfs2...

Quoted 37 lines

these checks are done above calls to ->remap_file_range(), but we need to drive them inwards so that we get EXDEV protection for all callers of ->remap_file_range().

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

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

fs/read_write.c +13 -0

index 3288db1d5f21..174cf92eea1d 100644
@@ -1909,6 +1909,19 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
        bool same_inode = (inode_in == inode_out);
        int ret;
 
+       /*
+        * 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. We check this here rather than at the ioctl layers because
+        * this is effectively a limitation of the fielsystem implementations,
+        * not so much the API itself. Further, ->remap_file_range() can be
+        * called from syscalls that don't have cross device copy restrictions
+        * (such as copy_file_range()) and so we need to catch them before we
+        * do any damage.
+        */
+       if (inode_in->i_sb != inode_out->i_sb)
+               return -EXDEV;
+
        /* Don't touch certain kinds of inodes */
        if (IS_IMMUTABLE(inode_out))
                return -EPERM;

@@ -2013,14 +2026,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
        if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
                return -EINVAL;
 

fs/read_write.c loff_t do_clone_file_range(struct file *file_in, loff_t pos_in, · line 2023

Quoted 8 lines

fs/read_write.c +0 -8

-       /*
-        * 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)
-               return -EXDEV;
-

That leaves {nfs42,cifs,btrfs}_remap_file_range() exposed to passing files not of their own fs type let alone same sb when do_clone_file_range() is called from ovl_copy_up_data().

Thanks,
Amir.

Quoted 47 lines · Amir Goldstein

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

before we can enable cross-device copies into copy_file_range(), we have to ensure that ->remap_file_range() implemenations will correctly reject attempts to do cross filesystem clones. Currently

But you only fixed remap_file_range() implemenations of xfs and ocfs2...

Quoted 37 lines

these checks are done above calls to ->remap_file_range(), but we need to drive them inwards so that we get EXDEV protection for all callers of ->remap_file_range().

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

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

fs/read_write.c +13 -0

index 3288db1d5f21..174cf92eea1d 100644
@@ -1909,6 +1909,19 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
        bool same_inode = (inode_in == inode_out);
        int ret;
 
+       /*
+        * 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. We check this here rather than at the ioctl layers because
+        * this is effectively a limitation of the fielsystem implementations,
+        * not so much the API itself. Further, ->remap_file_range() can be
+        * called from syscalls that don't have cross device copy restrictions
+        * (such as copy_file_range()) and so we need to catch them before we
+        * do any damage.
+        */
+       if (inode_in->i_sb != inode_out->i_sb)
+               return -EXDEV;
+
        /* Don't touch certain kinds of inodes */
        if (IS_IMMUTABLE(inode_out))
                return -EPERM;

@@ -2013,14 +2026,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
        if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
                return -EINVAL;
 

Quoted 9 lines

fs/read_write.c loff_t do_clone_file_range(struct file *file_in, loff_t pos_in, · line 2023

Quoted 8 lines

fs/read_write.c +0 -8

-       /*
-        * 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)
-               return -EXDEV;
-

I think this is sort of backwards -- the checks should stay in do_clone_file_range, and vfs_copy_file_range should be calling that instead of directly calling ->remap_range():

vfs_copy_file_range()

{
	file_start_write(...);
	ret = do_clone_file_range(...);
	if (ret > 0)
		return ret;
	ret = do_copy_file_range(...);
	file_end_write(...);
	return ret;
}

Quoted 63 lines · Darrick J. Wong

Quoted 47 lines · Amir Goldstein

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

before we can enable cross-device copies into copy_file_range(), we have to ensure that ->remap_file_range() implemenations will correctly reject attempts to do cross filesystem clones. Currently

But you only fixed remap_file_range() implemenations of xfs and ocfs2...

Quoted 37 lines

these checks are done above calls to ->remap_file_range(), but we need to drive them inwards so that we get EXDEV protection for all callers of ->remap_file_range().

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

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

fs/read_write.c +13 -0

index 3288db1d5f21..174cf92eea1d 100644
@@ -1909,6 +1909,19 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
        bool same_inode = (inode_in == inode_out);
        int ret;
 
+       /*
+        * 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. We check this here rather than at the ioctl layers because
+        * this is effectively a limitation of the fielsystem implementations,
+        * not so much the API itself. Further, ->remap_file_range() can be
+        * called from syscalls that don't have cross device copy restrictions
+        * (such as copy_file_range()) and so we need to catch them before we
+        * do any damage.
+        */
+       if (inode_in->i_sb != inode_out->i_sb)
+               return -EXDEV;
+
        /* Don't touch certain kinds of inodes */
        if (IS_IMMUTABLE(inode_out))
                return -EPERM;

@@ -2013,14 +2026,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
        if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
                return -EINVAL;
 

Quoted 9 lines

fs/read_write.c loff_t do_clone_file_range(struct file *file_in, loff_t pos_in, · line 2023

Quoted 8 lines

fs/read_write.c +0 -8

-       /*
-        * 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)
-               return -EXDEV;
-

I think this is sort of backwards -- the checks should stay in do_clone_file_range, and vfs_copy_file_range should be calling that instead of directly calling ->remap_range():

vfs_copy_file_range()

Quoted 9 lines

{
	file_start_write(...);
	ret = do_clone_file_range(...);
	if (ret > 0)
		return ret;
	ret = do_copy_file_range(...);
	file_end_write(...);
	return ret;
}

I'm already confused by the way we weave in and out of "vfs_/do_*" functions, and this just makes it worse.

Just what the hell is supposed to be in a "vfs_" prefixed function, and why the hell is it considered a "vfs" level function if we then export it's internal functions for individual filesystems to use?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 18 lines

Quoted 5 lines

I think this is sort of backwards -- the checks should stay in do_clone_file_range, and vfs_copy_file_range should be calling that instead of directly calling ->remap_range():

vfs_copy_file_range()

Quoted 9 lines

{
    file_start_write(...);
    ret = do_clone_file_range(...);
    if (ret > 0)
            return ret;
    ret = do_copy_file_range(...);
    file_end_write(...);
    return ret;
}

I'm already confused by the way we weave in and out of "vfs_/do_*" functions, and this just makes it worse.

Quoted 3 lines

Just what the hell is supposed to be in a "vfs_" prefixed function, and why the hell is it considered a "vfs" level function if we then export it's internal functions for individual filesystems to use?

I /think/ vfs_ functions are file_start_write()/file_end_write() wrappers around a similarly named function that lacks the freeze protection??

(AFAICT Amir made that split so that overlayfs could use these functions, though I do not know if everything vfs_ was made that way /specifically/ for overlayfs or if that's the way things have been and ovlfs simply takes advantage of it...)

Guhhh, none of this is documented......

From there on, I guess things kinda grew organically.

Quoted 6 lines · Dave Chinner

From: Dave Chinner <[email protected]>

before we can enable cross-device copies into copy_file_range(), we have to ensure that ->remap_file_range() implemenations will correctly reject attempts to do cross filesystem clones. Currently

But you only fixed remap_file_range() implemenations of xfs and ocfs2...

Quoted 37 lines

these checks are done above calls to ->remap_file_range(), but we need to drive them inwards so that we get EXDEV protection for all callers of ->remap_file_range().

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

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

fs/read_write.c +13 -0

index 3288db1d5f21..174cf92eea1d 100644
@@ -1909,6 +1909,19 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
        bool same_inode = (inode_in == inode_out);
        int ret;
 
+       /*
+        * 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. We check this here rather than at the ioctl layers because
+        * this is effectively a limitation of the fielsystem implementations,
+        * not so much the API itself. Further, ->remap_file_range() can be
+        * called from syscalls that don't have cross device copy restrictions
+        * (such as copy_file_range()) and so we need to catch them before we
+        * do any damage.
+        */
+       if (inode_in->i_sb != inode_out->i_sb)
+               return -EXDEV;
+
        /* Don't touch certain kinds of inodes */
        if (IS_IMMUTABLE(inode_out))
                return -EPERM;

@@ -2013,14 +2026,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
        if (!S_ISREG(inode_in->i_mode) || !S_ISREG(inode_out->i_mode))
                return -EINVAL;
 

fs/read_write.c loff_t do_clone_file_range(struct file *file_in, loff_t pos_in, · line 2023

Quoted 8 lines

fs/read_write.c +0 -8

-       /*
-        * 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)
-               return -EXDEV;
-

That leaves {nfs42,cifs,btrfs}_remap_file_range() exposed to passing files not of their own fs type let alone same sb when do_clone_file_range() is called from ovl_copy_up_data().

From: Dave Chinner <[email protected]>

before we can enable cross-device copies into copy_file_range(), we have to ensure that ->remap_file_range() implemenations will correctly reject attempts to do cross filesystem clones. Currently these checks are done above calls to ->remap_file_range(), but we need to drive them inwards so that we get EXDEV protection for all callers of ->remap_file_range().

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

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

fs/read_write.c +0 -0

index 3288db1d5f21..174cf92eea1d 100644

fs/read_write.c +8 -8

+	 * not so much the API itself. Further, ->remap_file_range() can be
+	 * called from syscalls that don't have cross device copy restrictions
+	 * (such as copy_file_range()) and so we need to catch them before we
+	 * do any damage.
+	 */
+	if (inode_in->i_sb != inode_out->i_sb)
+		return -EXDEV;
+
 	/* Don't touch certain kinds of inodes */
 	if (IS_IMMUTABLE(inode_out))
 		return -EPERM;

@@ -2013,14 +2026,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
 	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)
-		return -EXDEV;
-
 	if (!(file_in->f_mode & FMODE_READ) ||
 	    !(file_out->f_mode & FMODE_WRITE) ||
 	    (file_out->f_flags & O_APPEND))

Signature · 1 line

2.19.1

From: Dave Chinner <[email protected]>

before we can enable cross-device copies into copy_file_range(), we have to ensure that ->remap_file_range() implemenations will correctly reject attempts to do cross filesystem clones. Currently these checks are done above calls to ->remap_file_range(), but we need to drive them inwards so that we get EXDEV protection for all callers of ->remap_file_range().

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

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

fs/read_write.c +13 -9

index 3288db1d5f21..174cf92eea1d 100644
@@ -1909,6 +1909,19 @@ int generic_remap_file_range_prep(struct file *file_in, loff_t pos_in,
        bool same_inode = (inode_in == inode_out);
        int ret;
 
+       /*
+        * 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. We check this here rather than at the ioctl layers because
+        * this is effectively a limitation of the fielsystem implementations,
+        * not so much the API itself. Further, ->remap_file_range() can be
+        * called from syscalls that don't have cross device copy restrictions
+        * (such as copy_file_range()) and so we need to catch them before we
+        * do any damage.
+        */
+       if (inode_in->i_sb != inode_out->i_sb)
+               return -EXDEV;
+
        /* Don't touch certain kinds of inodes */
        if (IS_IMMUTABLE(inode_out))
                return -EPERM;

@@ -2013,14 +2026,6 @@ loff_t do_clone_file_range(struct file *file_in, loff_t pos_in,
        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)
-               return -EXDEV;
-
        if (!(file_in->f_mode & FMODE_READ) ||
            !(file_out->f_mode & FMODE_WRITE) ||
            (file_out->f_flags & O_APPEND))
--

2.19.1