From: Luis Henriques <hidden> Date: 2021-02-16 18:55:06
Amir Goldstein [off-list ref] writes:
On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
quoted
Ugh. And I guess overlayfs may have a similar problem.
Not exactly.
Generally speaking, overlayfs should call vfs_copy_file_range()
with the flags it got from layer above, so if called from nfsd it
will allow cross fs copy and when called from syscall it won't.
There are some corner cases where overlayfs could benefit from
COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but
let's leave those for now. Just leave overlayfs code as is.
Got it, thanks for clarifying.
quoted
quoted
quoted
This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
is internal to kernel users.
FWIW, you may want to look at the loop in ovl_copy_up_data()
for improvements to nfsd_copy_file_range().
We can move the check out to copy_file_range syscall:
if (flags != 0)
return -EINVAL;
Leave the fallback from all filesystems and check for the
COPY_FILE_SPLICE flag inside generic_copy_file_range().
Ok, the diff bellow is just to make sure I understood your suggestion.
The patch will also need to:
- change nfs and overlayfs calls to vfs_copy_file_range() so that they
use the new flag.
- check flags in generic_copy_file_checks() to make sure only valid flags
are used (COPY_FILE_SPLICE at the moment).
Also, where should this flag be defined? include/uapi/linux/fs.h?
Grep for REMAP_FILE_
Same header file, same Documentation rst file.
That looks strange, because you are duplicating the logic in
do_copy_file_range(). Maybe better:
if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
return -EINVAL;
if (flags & COPY_FILE_SPLICE)
return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
My initial reasoning for duplicating the logic in do_copy_file_range() was
to allow the generic_copy_file_range() callers to be left unmodified and
allow the filesystems to default to this implementation.
With this change, I guess that the calls to generic_copy_file_range() from
the different filesystems can be dropped, as in my initial patch, as they
will always get -EINVAL. The other option would be to set the
COPY_FILE_SPLICE flag in those calls, but that would get us back to the
problem we're trying to solve.
I don't understand the problem.
What exactly is wrong with the code I suggested?
Why should any filesystem be changed?
Maybe I am missing something.
Ok, I have to do a full brain reboot and start all over.
Before that, I picked the code you suggested and tested it. I've mounted
a cephfs filesystem and used xfs_io to execute a 'copy_range' command
using /sys/kernel/debug/sched_features as source. The result was a
0-sized file in cephfs. And the reason is thevfs_copy_file_range()
early exit in:
if (len == 0)
return 0;
'len' is set in generic_copy_file_checks().
This means that we're not solving the original problem anymore (probably
since v1 of this patch, haven't checked).
Also, re-reading Trond's emails, I read: "... also disallowing the copy
from, say, an XFS formatted partition to an ext4 partition". Isn't that
*exactly* what we're trying to do here? I.e. _prevent_ these copies from
happening so that tracefs files can't be CFR'ed?
/me stops now and waits to see if the morning brings some sun :-)
Cheers,
--
Luis
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-02-16 19:21:32
On Tue, Feb 16, 2021 at 8:54 PM Luis Henriques [off-list ref] wrote:
Amir Goldstein [off-list ref] writes:
quoted
On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
quoted
Ugh. And I guess overlayfs may have a similar problem.
Not exactly.
Generally speaking, overlayfs should call vfs_copy_file_range()
with the flags it got from layer above, so if called from nfsd it
will allow cross fs copy and when called from syscall it won't.
There are some corner cases where overlayfs could benefit from
COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but
let's leave those for now. Just leave overlayfs code as is.
Got it, thanks for clarifying.
quoted
quoted
quoted
This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
is internal to kernel users.
FWIW, you may want to look at the loop in ovl_copy_up_data()
for improvements to nfsd_copy_file_range().
We can move the check out to copy_file_range syscall:
if (flags != 0)
return -EINVAL;
Leave the fallback from all filesystems and check for the
COPY_FILE_SPLICE flag inside generic_copy_file_range().
Ok, the diff bellow is just to make sure I understood your suggestion.
The patch will also need to:
- change nfs and overlayfs calls to vfs_copy_file_range() so that they
use the new flag.
- check flags in generic_copy_file_checks() to make sure only valid flags
are used (COPY_FILE_SPLICE at the moment).
Also, where should this flag be defined? include/uapi/linux/fs.h?
Grep for REMAP_FILE_
Same header file, same Documentation rst file.
That looks strange, because you are duplicating the logic in
do_copy_file_range(). Maybe better:
if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
return -EINVAL;
if (flags & COPY_FILE_SPLICE)
return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
My initial reasoning for duplicating the logic in do_copy_file_range() was
to allow the generic_copy_file_range() callers to be left unmodified and
allow the filesystems to default to this implementation.
With this change, I guess that the calls to generic_copy_file_range() from
the different filesystems can be dropped, as in my initial patch, as they
will always get -EINVAL. The other option would be to set the
COPY_FILE_SPLICE flag in those calls, but that would get us back to the
problem we're trying to solve.
I don't understand the problem.
What exactly is wrong with the code I suggested?
Why should any filesystem be changed?
Maybe I am missing something.
Ok, I have to do a full brain reboot and start all over.
Before that, I picked the code you suggested and tested it. I've mounted
a cephfs filesystem and used xfs_io to execute a 'copy_range' command
using /sys/kernel/debug/sched_features as source. The result was a
0-sized file in cephfs. And the reason is thevfs_copy_file_range()
early exit in:
if (len == 0)
return 0;
'len' is set in generic_copy_file_checks().
Good point.. I guess we will need to do all the checks earlier in
generic_copy_file_checks() including the logic of:
if (file_in->f_op->remap_file_range &&
file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)
This means that we're not solving the original problem anymore (probably
since v1 of this patch, haven't checked).
Also, re-reading Trond's emails, I read: "... also disallowing the copy
from, say, an XFS formatted partition to an ext4 partition". Isn't that
*exactly* what we're trying to do here? I.e. _prevent_ these copies from
happening so that tracefs files can't be CFR'ed?
We want to address the report which means calls coming from
copy_file_range() syscall.
Trond's use case is vfs_copy_file_range() coming from nfsd.
When he writes about copy from XFS to ext4, he means an
NFS client is issuing server side copy (on same or different NFS mounts)
and the NFS server is executing nfsd_copy_file_range() on a source
file that happens to be on XFS and destination happens to be on ext4.
We can undo the copy_file_range() syscall change of behavior from
v5.3 without regressing the NFS use case.
We just need to be careful and look at all the affected code paths.
Thanks,
Amir.
From: Anna Schumaker <hidden> Date: 2021-02-16 19:29:20
On Tue, Feb 16, 2021 at 2:22 PM Amir Goldstein [off-list ref] wrote:
On Tue, Feb 16, 2021 at 8:54 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
quoted
Ugh. And I guess overlayfs may have a similar problem.
Not exactly.
Generally speaking, overlayfs should call vfs_copy_file_range()
with the flags it got from layer above, so if called from nfsd it
will allow cross fs copy and when called from syscall it won't.
There are some corner cases where overlayfs could benefit from
COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but
let's leave those for now. Just leave overlayfs code as is.
Got it, thanks for clarifying.
quoted
quoted
quoted
This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
is internal to kernel users.
FWIW, you may want to look at the loop in ovl_copy_up_data()
for improvements to nfsd_copy_file_range().
We can move the check out to copy_file_range syscall:
if (flags != 0)
return -EINVAL;
Leave the fallback from all filesystems and check for the
COPY_FILE_SPLICE flag inside generic_copy_file_range().
Ok, the diff bellow is just to make sure I understood your suggestion.
The patch will also need to:
- change nfs and overlayfs calls to vfs_copy_file_range() so that they
use the new flag.
- check flags in generic_copy_file_checks() to make sure only valid flags
are used (COPY_FILE_SPLICE at the moment).
Also, where should this flag be defined? include/uapi/linux/fs.h?
Grep for REMAP_FILE_
Same header file, same Documentation rst file.
That looks strange, because you are duplicating the logic in
do_copy_file_range(). Maybe better:
if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
return -EINVAL;
if (flags & COPY_FILE_SPLICE)
return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
My initial reasoning for duplicating the logic in do_copy_file_range() was
to allow the generic_copy_file_range() callers to be left unmodified and
allow the filesystems to default to this implementation.
With this change, I guess that the calls to generic_copy_file_range() from
the different filesystems can be dropped, as in my initial patch, as they
will always get -EINVAL. The other option would be to set the
COPY_FILE_SPLICE flag in those calls, but that would get us back to the
problem we're trying to solve.
I don't understand the problem.
What exactly is wrong with the code I suggested?
Why should any filesystem be changed?
Maybe I am missing something.
Ok, I have to do a full brain reboot and start all over.
Before that, I picked the code you suggested and tested it. I've mounted
a cephfs filesystem and used xfs_io to execute a 'copy_range' command
using /sys/kernel/debug/sched_features as source. The result was a
0-sized file in cephfs. And the reason is thevfs_copy_file_range()
early exit in:
if (len == 0)
return 0;
'len' is set in generic_copy_file_checks().
Good point.. I guess we will need to do all the checks earlier in
generic_copy_file_checks() including the logic of:
if (file_in->f_op->remap_file_range &&
file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)
quoted
This means that we're not solving the original problem anymore (probably
since v1 of this patch, haven't checked).
Also, re-reading Trond's emails, I read: "... also disallowing the copy
from, say, an XFS formatted partition to an ext4 partition". Isn't that
*exactly* what we're trying to do here? I.e. _prevent_ these copies from
happening so that tracefs files can't be CFR'ed?
We want to address the report which means calls coming from
copy_file_range() syscall.
Trond's use case is vfs_copy_file_range() coming from nfsd.
When he writes about copy from XFS to ext4, he means an
NFS client is issuing server side copy (on same or different NFS mounts)
and the NFS server is executing nfsd_copy_file_range() on a source
file that happens to be on XFS and destination happens to be on ext4.
NFS also supports a server-to-server copy where the destination server
mounts the source server and reads the data to be copied. Please don't
break that either :)
Anna
We can undo the copy_file_range() syscall change of behavior from
v5.3 without regressing the NFS use case.
We just need to be careful and look at all the affected code paths.
Thanks,
Amir.
From: Steve French <smfrench@gmail.com> Date: 2021-02-16 19:32:36
On Tue, Feb 16, 2021 at 1:29 PM Anna Schumaker
[off-list ref] wrote:
On Tue, Feb 16, 2021 at 2:22 PM Amir Goldstein [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 8:54 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
quoted
Ugh. And I guess overlayfs may have a similar problem.
Not exactly.
Generally speaking, overlayfs should call vfs_copy_file_range()
with the flags it got from layer above, so if called from nfsd it
will allow cross fs copy and when called from syscall it won't.
There are some corner cases where overlayfs could benefit from
COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but
let's leave those for now. Just leave overlayfs code as is.
Got it, thanks for clarifying.
quoted
quoted
quoted
This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
is internal to kernel users.
FWIW, you may want to look at the loop in ovl_copy_up_data()
for improvements to nfsd_copy_file_range().
We can move the check out to copy_file_range syscall:
if (flags != 0)
return -EINVAL;
Leave the fallback from all filesystems and check for the
COPY_FILE_SPLICE flag inside generic_copy_file_range().
Ok, the diff bellow is just to make sure I understood your suggestion.
The patch will also need to:
- change nfs and overlayfs calls to vfs_copy_file_range() so that they
use the new flag.
- check flags in generic_copy_file_checks() to make sure only valid flags
are used (COPY_FILE_SPLICE at the moment).
Also, where should this flag be defined? include/uapi/linux/fs.h?
Grep for REMAP_FILE_
Same header file, same Documentation rst file.
That looks strange, because you are duplicating the logic in
do_copy_file_range(). Maybe better:
if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
return -EINVAL;
if (flags & COPY_FILE_SPLICE)
return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
My initial reasoning for duplicating the logic in do_copy_file_range() was
to allow the generic_copy_file_range() callers to be left unmodified and
allow the filesystems to default to this implementation.
With this change, I guess that the calls to generic_copy_file_range() from
the different filesystems can be dropped, as in my initial patch, as they
will always get -EINVAL. The other option would be to set the
COPY_FILE_SPLICE flag in those calls, but that would get us back to the
problem we're trying to solve.
I don't understand the problem.
What exactly is wrong with the code I suggested?
Why should any filesystem be changed?
Maybe I am missing something.
Ok, I have to do a full brain reboot and start all over.
Before that, I picked the code you suggested and tested it. I've mounted
a cephfs filesystem and used xfs_io to execute a 'copy_range' command
using /sys/kernel/debug/sched_features as source. The result was a
0-sized file in cephfs. And the reason is thevfs_copy_file_range()
early exit in:
if (len == 0)
return 0;
'len' is set in generic_copy_file_checks().
Good point.. I guess we will need to do all the checks earlier in
generic_copy_file_checks() including the logic of:
if (file_in->f_op->remap_file_range &&
file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)
quoted
This means that we're not solving the original problem anymore (probably
since v1 of this patch, haven't checked).
Also, re-reading Trond's emails, I read: "... also disallowing the copy
from, say, an XFS formatted partition to an ext4 partition". Isn't that
*exactly* what we're trying to do here? I.e. _prevent_ these copies from
happening so that tracefs files can't be CFR'ed?
We want to address the report which means calls coming from
copy_file_range() syscall.
Trond's use case is vfs_copy_file_range() coming from nfsd.
When he writes about copy from XFS to ext4, he means an
NFS client is issuing server side copy (on same or different NFS mounts)
and the NFS server is executing nfsd_copy_file_range() on a source
file that happens to be on XFS and destination happens to be on ext4.
NFS also supports a server-to-server copy where the destination server
mounts the source server and reads the data to be copied. Please don't
break that either :)
This is a case we will eventually need to support for cifs (SMB3) as well.
--
Thanks,
Steve
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-02-16 19:41:41
On Tue, Feb 16, 2021 at 9:31 PM Steve French [off-list ref] wrote:
On Tue, Feb 16, 2021 at 1:29 PM Anna Schumaker
[off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 2:22 PM Amir Goldstein [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 8:54 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
quoted
Ugh. And I guess overlayfs may have a similar problem.
Not exactly.
Generally speaking, overlayfs should call vfs_copy_file_range()
with the flags it got from layer above, so if called from nfsd it
will allow cross fs copy and when called from syscall it won't.
There are some corner cases where overlayfs could benefit from
COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but
let's leave those for now. Just leave overlayfs code as is.
Got it, thanks for clarifying.
quoted
quoted
quoted
This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
is internal to kernel users.
FWIW, you may want to look at the loop in ovl_copy_up_data()
for improvements to nfsd_copy_file_range().
We can move the check out to copy_file_range syscall:
if (flags != 0)
return -EINVAL;
Leave the fallback from all filesystems and check for the
COPY_FILE_SPLICE flag inside generic_copy_file_range().
Ok, the diff bellow is just to make sure I understood your suggestion.
The patch will also need to:
- change nfs and overlayfs calls to vfs_copy_file_range() so that they
use the new flag.
- check flags in generic_copy_file_checks() to make sure only valid flags
are used (COPY_FILE_SPLICE at the moment).
Also, where should this flag be defined? include/uapi/linux/fs.h?
Grep for REMAP_FILE_
Same header file, same Documentation rst file.
That looks strange, because you are duplicating the logic in
do_copy_file_range(). Maybe better:
if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
return -EINVAL;
if (flags & COPY_FILE_SPLICE)
return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
My initial reasoning for duplicating the logic in do_copy_file_range() was
to allow the generic_copy_file_range() callers to be left unmodified and
allow the filesystems to default to this implementation.
With this change, I guess that the calls to generic_copy_file_range() from
the different filesystems can be dropped, as in my initial patch, as they
will always get -EINVAL. The other option would be to set the
COPY_FILE_SPLICE flag in those calls, but that would get us back to the
problem we're trying to solve.
I don't understand the problem.
What exactly is wrong with the code I suggested?
Why should any filesystem be changed?
Maybe I am missing something.
Ok, I have to do a full brain reboot and start all over.
Before that, I picked the code you suggested and tested it. I've mounted
a cephfs filesystem and used xfs_io to execute a 'copy_range' command
using /sys/kernel/debug/sched_features as source. The result was a
0-sized file in cephfs. And the reason is thevfs_copy_file_range()
early exit in:
if (len == 0)
return 0;
'len' is set in generic_copy_file_checks().
Good point.. I guess we will need to do all the checks earlier in
generic_copy_file_checks() including the logic of:
if (file_in->f_op->remap_file_range &&
file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)
quoted
This means that we're not solving the original problem anymore (probably
since v1 of this patch, haven't checked).
Also, re-reading Trond's emails, I read: "... also disallowing the copy
from, say, an XFS formatted partition to an ext4 partition". Isn't that
*exactly* what we're trying to do here? I.e. _prevent_ these copies from
happening so that tracefs files can't be CFR'ed?
We want to address the report which means calls coming from
copy_file_range() syscall.
Trond's use case is vfs_copy_file_range() coming from nfsd.
When he writes about copy from XFS to ext4, he means an
NFS client is issuing server side copy (on same or different NFS mounts)
and the NFS server is executing nfsd_copy_file_range() on a source
file that happens to be on XFS and destination happens to be on ext4.
NFS also supports a server-to-server copy where the destination server
mounts the source server and reads the data to be copied. Please don't
break that either :)
As long as the copy is via nfsd_copy_file_range() and not from the syscall
it should not regress.
This is a case we will eventually need to support for cifs (SMB3) as well.
samba already does server side copy very well without needing any support
from the kernel.
nfsd also doesn't *need* to use vfs_copy_file_range() it can use kernel APIs
like the loop in ovl_copy_up_data(). But it does, so we should not regress it.
samba/nfsd can try to use copy_file_range() and it will work if the
source/target
fs support it. Otherwise, the server can perfectly well do the copy via other
available interfaces, just like userspace copy tools.
Thanks,
Amir.
From: Steve French <smfrench@gmail.com> Date: 2021-02-16 21:16:19
On Tue, Feb 16, 2021 at 1:40 PM Amir Goldstein [off-list ref] wrote:
On Tue, Feb 16, 2021 at 9:31 PM Steve French [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 1:29 PM Anna Schumaker
[off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 2:22 PM Amir Goldstein [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 8:54 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
quoted
Ugh. And I guess overlayfs may have a similar problem.
Not exactly.
Generally speaking, overlayfs should call vfs_copy_file_range()
with the flags it got from layer above, so if called from nfsd it
will allow cross fs copy and when called from syscall it won't.
There are some corner cases where overlayfs could benefit from
COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but
let's leave those for now. Just leave overlayfs code as is.
Got it, thanks for clarifying.
quoted
quoted
quoted
This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
is internal to kernel users.
FWIW, you may want to look at the loop in ovl_copy_up_data()
for improvements to nfsd_copy_file_range().
We can move the check out to copy_file_range syscall:
if (flags != 0)
return -EINVAL;
Leave the fallback from all filesystems and check for the
COPY_FILE_SPLICE flag inside generic_copy_file_range().
Ok, the diff bellow is just to make sure I understood your suggestion.
The patch will also need to:
- change nfs and overlayfs calls to vfs_copy_file_range() so that they
use the new flag.
- check flags in generic_copy_file_checks() to make sure only valid flags
are used (COPY_FILE_SPLICE at the moment).
Also, where should this flag be defined? include/uapi/linux/fs.h?
Grep for REMAP_FILE_
Same header file, same Documentation rst file.
That looks strange, because you are duplicating the logic in
do_copy_file_range(). Maybe better:
if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
return -EINVAL;
if (flags & COPY_FILE_SPLICE)
return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
My initial reasoning for duplicating the logic in do_copy_file_range() was
to allow the generic_copy_file_range() callers to be left unmodified and
allow the filesystems to default to this implementation.
With this change, I guess that the calls to generic_copy_file_range() from
the different filesystems can be dropped, as in my initial patch, as they
will always get -EINVAL. The other option would be to set the
COPY_FILE_SPLICE flag in those calls, but that would get us back to the
problem we're trying to solve.
I don't understand the problem.
What exactly is wrong with the code I suggested?
Why should any filesystem be changed?
Maybe I am missing something.
Ok, I have to do a full brain reboot and start all over.
Before that, I picked the code you suggested and tested it. I've mounted
a cephfs filesystem and used xfs_io to execute a 'copy_range' command
using /sys/kernel/debug/sched_features as source. The result was a
0-sized file in cephfs. And the reason is thevfs_copy_file_range()
early exit in:
if (len == 0)
return 0;
'len' is set in generic_copy_file_checks().
Good point.. I guess we will need to do all the checks earlier in
generic_copy_file_checks() including the logic of:
if (file_in->f_op->remap_file_range &&
file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)
quoted
This means that we're not solving the original problem anymore (probably
since v1 of this patch, haven't checked).
Also, re-reading Trond's emails, I read: "... also disallowing the copy
from, say, an XFS formatted partition to an ext4 partition". Isn't that
*exactly* what we're trying to do here? I.e. _prevent_ these copies from
happening so that tracefs files can't be CFR'ed?
We want to address the report which means calls coming from
copy_file_range() syscall.
Trond's use case is vfs_copy_file_range() coming from nfsd.
When he writes about copy from XFS to ext4, he means an
NFS client is issuing server side copy (on same or different NFS mounts)
and the NFS server is executing nfsd_copy_file_range() on a source
file that happens to be on XFS and destination happens to be on ext4.
NFS also supports a server-to-server copy where the destination server
mounts the source server and reads the data to be copied. Please don't
break that either :)
As long as the copy is via nfsd_copy_file_range() and not from the syscall
it should not regress.
quoted
This is a case we will eventually need to support for cifs (SMB3) as well.
samba already does server side copy very well without needing any support
from the kernel.
nfsd also doesn't *need* to use vfs_copy_file_range() it can use kernel APIs
like the loop in ovl_copy_up_data(). But it does, so we should not regress it.
samba/nfsd can try to use copy_file_range() and it will work if the
source/target
fs support it. Otherwise, the server can perfectly well do the copy via other
available interfaces, just like userspace copy tools.
I was thinking about cifsd ("ksmbd") the kernel server from
Namjae/Sergey etc. which is making excellent progress.
--
Thanks,
Steve
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-02-17 08:09:55
On Tue, Feb 16, 2021 at 11:15 PM Steve French [off-list ref] wrote:
On Tue, Feb 16, 2021 at 1:40 PM Amir Goldstein [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 9:31 PM Steve French [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 1:29 PM Anna Schumaker
[off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 2:22 PM Amir Goldstein [off-list ref] wrote:
quoted
On Tue, Feb 16, 2021 at 8:54 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
On Tue, Feb 16, 2021 at 6:41 PM Luis Henriques [off-list ref] wrote:
quoted
Amir Goldstein [off-list ref] writes:
quoted
quoted
Ugh. And I guess overlayfs may have a similar problem.
Not exactly.
Generally speaking, overlayfs should call vfs_copy_file_range()
with the flags it got from layer above, so if called from nfsd it
will allow cross fs copy and when called from syscall it won't.
There are some corner cases where overlayfs could benefit from
COPY_FILE_SPLICE (e.g. copy from lower file to upper file), but
let's leave those for now. Just leave overlayfs code as is.
Got it, thanks for clarifying.
quoted
quoted
quoted
This is easy to solve with a flag COPY_FILE_SPLICE (or something) that
is internal to kernel users.
FWIW, you may want to look at the loop in ovl_copy_up_data()
for improvements to nfsd_copy_file_range().
We can move the check out to copy_file_range syscall:
if (flags != 0)
return -EINVAL;
Leave the fallback from all filesystems and check for the
COPY_FILE_SPLICE flag inside generic_copy_file_range().
Ok, the diff bellow is just to make sure I understood your suggestion.
The patch will also need to:
- change nfs and overlayfs calls to vfs_copy_file_range() so that they
use the new flag.
- check flags in generic_copy_file_checks() to make sure only valid flags
are used (COPY_FILE_SPLICE at the moment).
Also, where should this flag be defined? include/uapi/linux/fs.h?
Grep for REMAP_FILE_
Same header file, same Documentation rst file.
That looks strange, because you are duplicating the logic in
do_copy_file_range(). Maybe better:
if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
return -EINVAL;
if (flags & COPY_FILE_SPLICE)
return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
My initial reasoning for duplicating the logic in do_copy_file_range() was
to allow the generic_copy_file_range() callers to be left unmodified and
allow the filesystems to default to this implementation.
With this change, I guess that the calls to generic_copy_file_range() from
the different filesystems can be dropped, as in my initial patch, as they
will always get -EINVAL. The other option would be to set the
COPY_FILE_SPLICE flag in those calls, but that would get us back to the
problem we're trying to solve.
I don't understand the problem.
What exactly is wrong with the code I suggested?
Why should any filesystem be changed?
Maybe I am missing something.
Ok, I have to do a full brain reboot and start all over.
Before that, I picked the code you suggested and tested it. I've mounted
a cephfs filesystem and used xfs_io to execute a 'copy_range' command
using /sys/kernel/debug/sched_features as source. The result was a
0-sized file in cephfs. And the reason is thevfs_copy_file_range()
early exit in:
if (len == 0)
return 0;
'len' is set in generic_copy_file_checks().
Good point.. I guess we will need to do all the checks earlier in
generic_copy_file_checks() including the logic of:
if (file_in->f_op->remap_file_range &&
file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)
quoted
This means that we're not solving the original problem anymore (probably
since v1 of this patch, haven't checked).
Also, re-reading Trond's emails, I read: "... also disallowing the copy
from, say, an XFS formatted partition to an ext4 partition". Isn't that
*exactly* what we're trying to do here? I.e. _prevent_ these copies from
happening so that tracefs files can't be CFR'ed?
We want to address the report which means calls coming from
copy_file_range() syscall.
Trond's use case is vfs_copy_file_range() coming from nfsd.
When he writes about copy from XFS to ext4, he means an
NFS client is issuing server side copy (on same or different NFS mounts)
and the NFS server is executing nfsd_copy_file_range() on a source
file that happens to be on XFS and destination happens to be on ext4.
NFS also supports a server-to-server copy where the destination server
mounts the source server and reads the data to be copied. Please don't
break that either :)
As long as the copy is via nfsd_copy_file_range() and not from the syscall
it should not regress.
quoted
This is a case we will eventually need to support for cifs (SMB3) as well.
samba already does server side copy very well without needing any support
from the kernel.
nfsd also doesn't *need* to use vfs_copy_file_range() it can use kernel APIs
like the loop in ovl_copy_up_data(). But it does, so we should not regress it.
samba/nfsd can try to use copy_file_range() and it will work if the
source/target
fs support it. Otherwise, the server can perfectly well do the copy via other
available interfaces, just like userspace copy tools.
I was thinking about cifsd ("ksmbd") the kernel server from
Namjae/Sergey etc. which is making excellent progress.
You are missing my point.
Never mind which server. The server does not *need* to rely on
vfs_copy_file_range() to copy files from XFS to ext4.
The server is very capable of implementing the fallback generic copy
in case source/target fs do not support native {copy,remap}_file_range().
w.r.t semantics of copy_file_range() syscall vs. the fallback to userespace
'cp' tool (check source file size before copy or not), please note that the
semantics of CIFS_IOC_COPYCHUNK_FILE are that of the former:
rc = cifs_file_copychunk_range(xid, src_file.file, 0, dst_file, 0,
src_inode->i_size, 0);
It will copy zero bytes if advertised source file size if zero.
NFS server side copy semantics are currently de-facto the same
because both the client and the server will have to pass through this
line in vfs_copy_file_range():
if (len == 0)
return 0;
IMO, and this opinion was voiced by several other filesystem developers,
the shortend copy semantics are the correct semantics for copy_file_range()
syscall as well as for vfs_copy_file_range() for internal kernel users.
I guess what this means is that if the 'cp' tool ever tries an opportunistic
copy_file_range() syscall (e.g. --cfr=auto), it may result in zero size copy.
Thanks,
Amir.
From: Luis Henriques <hidden> Date: 2021-02-17 17:26:41
A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystems copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices"). It also introduces a flag (COPY_FILE_SPLICE) that can be used
by filesystems calling directly into the vfs copy_file_range to override
these restrictions. Right now, only NFS needs to set this flag.
Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
Link: https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/
Link: https://lore.kernel.org/linux-fsdevel/CANMq1KDZuxir2LM5jOTm0xx+BnvW=ZmpsG47CyHFJwnw7zSX6Q@mail.gmail.com/
Link: https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/
Reported-by: Nicolas Boichat <redacted>
Signed-off-by: Luis Henriques <redacted>
---
Ok, I've tried to address all the issues and comments. Hopefully this v3
is a bit closer to the final fix.
Changes since v2
- do all the required checks earlier, in generic_copy_file_checks(),
adding new checks for ->remap_file_range
- new COPY_FILE_SPLICE flag
- don't remove filesystem's fallback to generic_copy_file_range()
- updated commit changelog (and subject)
Changes since v1 (after Amir review)
- restored do_copy_file_range() helper
- return -EOPNOTSUPP if fs doesn't implement CFR
- updated commit description
fs/nfsd/vfs.c | 3 ++-
fs/read_write.c | 44 +++++++++++++++++++++++++++++++++++++++++---
include/linux/fs.h | 7 +++++++
3 files changed, 50 insertions(+), 4 deletions(-)
@@ -1427,6 +1454,14 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,loff_tsize_in;intret;+/* Only check f_ops if we're not trying to clone */+if(!file_in->f_op->remap_file_range||+(file_inode(file_in)->i_sb==file_inode(file_out)->i_sb)){+ret=fops_copy_file_checks(file_in,file_out,flags);+if(ret)+returnret;+}+ret=generic_file_rw_checks(file_in,file_out);if(ret)returnret;
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-02-17 20:48:38
On Wed, Feb 17, 2021 at 7:25 PM Luis Henriques [off-list ref] wrote:
quoted hunk
A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystems copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices"). It also introduces a flag (COPY_FILE_SPLICE) that can be used
by filesystems calling directly into the vfs copy_file_range to override
these restrictions. Right now, only NFS needs to set this flag.
Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
Link: https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/
Link: https://lore.kernel.org/linux-fsdevel/CANMq1KDZuxir2LM5jOTm0xx+BnvW=ZmpsG47CyHFJwnw7zSX6Q@mail.gmail.com/
Link: https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/
Reported-by: Nicolas Boichat <redacted>
Signed-off-by: Luis Henriques <redacted>
---
Ok, I've tried to address all the issues and comments. Hopefully this v3
is a bit closer to the final fix.
Changes since v2
- do all the required checks earlier, in generic_copy_file_checks(),
adding new checks for ->remap_file_range
- new COPY_FILE_SPLICE flag
- don't remove filesystem's fallback to generic_copy_file_range()
- updated commit changelog (and subject)
Changes since v1 (after Amir review)
- restored do_copy_file_range() helper
- return -EOPNOTSUPP if fs doesn't implement CFR
- updated commit description
fs/nfsd/vfs.c | 3 ++-
fs/read_write.c | 44 +++++++++++++++++++++++++++++++++++++++++---
include/linux/fs.h | 7 +++++++
3 files changed, 50 insertions(+), 4 deletions(-)
Suggest:
if (!file_in->f_op->copy_file_range) {
if (file_in->f_op->copy_file_range !=
file_out->f_op->copy_file_range)
return -EXDEV;
} else if (file_in->f_op->remap_file_range) {
if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
return -EXDEV;
} else {
return -EOPNOTSUPP;
}
return 0;
}
quoted hunk
+
/*
* Performs necessary checks before doing a file copy
*
@@ -1427,6 +1454,14 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, loff_t size_in; int ret;+ /* Only check f_ops if we're not trying to clone */+ if (!file_in->f_op->remap_file_range ||+ (file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)) {+ ret = fops_copy_file_checks(file_in, file_out, flags);+ if (ret)+ return ret;+ }+
and then you don't need this special casing of clone here.
quoted hunk
ret = generic_file_rw_checks(file_in, file_out);
if (ret)
return ret;
@@ -1474,9 +1509,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, { ssize_t ret;- if (flags != 0)- return -EINVAL;- ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, flags); if (unlikely(ret))
@@ -1511,6 +1543,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, ret = cloned; goto done; }+ ret = fops_copy_file_checks(file_in, file_out, flags);+ if (ret)+ return ret;
and you don't need this here (right?)
and you can remove the checks for same i_sb and same copy_file_range
op that were already tested from vfs_copy_file_range().
Hope I am not missing anything.
Thanks,
Amir.
From: Nicolas Boichat <hidden> Date: 2021-02-18 00:59:54
On Thu, Feb 18, 2021 at 1:25 AM Luis Henriques [off-list ref] wrote:
A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystems copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices").
Note that you also fix intra-filesystem copy_file_range on these
generated filesystems. This is IMHO great, but needs to be mentioned
in the commit message.
It also introduces a flag (COPY_FILE_SPLICE) that can be used
by filesystems calling directly into the vfs copy_file_range to override
these restrictions. Right now, only NFS needs to set this flag.
Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
So technically this fixes something much older, presumably ever since
copy_file_range was introduced.
Tested-by: Nicolas Boichat <redacted>
but I guess you should not add to the next revision, I'll keep testing
further revisions ,-)
quoted hunk
Signed-off-by: Luis Henriques <redacted>
---
Ok, I've tried to address all the issues and comments. Hopefully this v3
is a bit closer to the final fix.
Changes since v2
- do all the required checks earlier, in generic_copy_file_checks(),
adding new checks for ->remap_file_range
- new COPY_FILE_SPLICE flag
- don't remove filesystem's fallback to generic_copy_file_range()
- updated commit changelog (and subject)
Changes since v1 (after Amir review)
- restored do_copy_file_range() helper
- return -EOPNOTSUPP if fs doesn't implement CFR
- updated commit description
fs/nfsd/vfs.c | 3 ++-
fs/read_write.c | 44 +++++++++++++++++++++++++++++++++++++++++---
include/linux/fs.h | 7 +++++++
3 files changed, 50 insertions(+), 4 deletions(-)
+ unsigned int flags)
+{
+ if (WARN_ON_ONCE(flags & ~COPY_FILE_SPLICE))
+ return -EINVAL;
+
+ if (flags & COPY_FILE_SPLICE)
+ return 0;
+ /*
+ * We got here from userspace, so forbid copies if copy_file_range isn't
+ * implemented or if we're doing a cross-fs copy.
+ */
+ if (!file_out->f_op->copy_file_range)
+ return -EOPNOTSUPP;
After this is merged, should this be added as an error code to the man page?
+ else if (file_out->f_op->copy_file_range !=
+ file_in->f_op->copy_file_range)
Just note, this could be a cross-fs copy (just not a cross-fs_type copy).
quoted hunk
+ return -EXDEV;
+
+ return 0;
+}
+
/*
* Performs necessary checks before doing a file copy
*
@@ -1427,6 +1454,14 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, loff_t size_in; int ret;+ /* Only check f_ops if we're not trying to clone */+ if (!file_in->f_op->remap_file_range ||+ (file_inode(file_in)->i_sb == file_inode(file_out)->i_sb)) {+ ret = fops_copy_file_checks(file_in, file_out, flags);+ if (ret)+ return ret;+ }+ ret = generic_file_rw_checks(file_in, file_out); if (ret) return ret;
@@ -1474,9 +1509,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, { ssize_t ret;- if (flags != 0)- return -EINVAL;- ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, flags); if (unlikely(ret))
@@ -1511,6 +1543,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, ret = cloned; goto done; }+ ret = fops_copy_file_checks(file_in, file_out, flags);+ if (ret)+ return ret; } ret = do_copy_file_range(file_in, pos_in, file_out, pos_out, len,
From: Olga Kornievskaia <hidden> Date: 2021-02-18 05:36:58
On Wed, Feb 17, 2021 at 3:30 PM Luis Henriques [off-list ref] wrote:
A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystems copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices"). It also introduces a flag (COPY_FILE_SPLICE) that can be used
by filesystems calling directly into the vfs copy_file_range to override
these restrictions. Right now, only NFS needs to set this flag.
Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
Link: https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/
Link: https://lore.kernel.org/linux-fsdevel/CANMq1KDZuxir2LM5jOTm0xx+BnvW=ZmpsG47CyHFJwnw7zSX6Q@mail.gmail.com/
Link: https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/
Reported-by: Nicolas Boichat <redacted>
Signed-off-by: Luis Henriques <redacted>
---
Ok, I've tried to address all the issues and comments. Hopefully this v3
is a bit closer to the final fix.
Changes since v2
- do all the required checks earlier, in generic_copy_file_checks(),
adding new checks for ->remap_file_range
- new COPY_FILE_SPLICE flag
- don't remove filesystem's fallback to generic_copy_file_range()
- updated commit changelog (and subject)
Changes since v1 (after Amir review)
- restored do_copy_file_range() helper
- return -EOPNOTSUPP if fs doesn't implement CFR
- updated commit description
In my testing, this patch breaks NFS server-to-server copy file.
@@ -1427,6 +1454,14 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in,loff_tsize_in;intret;+/* Only check f_ops if we're not trying to clone */+if(!file_in->f_op->remap_file_range||+(file_inode(file_in)->i_sb==file_inode(file_out)->i_sb)){+ret=fops_copy_file_checks(file_in,file_out,flags);+if(ret)+returnret;+}+ret=generic_file_rw_checks(file_in,file_out);if(ret)returnret;
From: Amir Goldstein <amir73il@gmail.com> Date: 2021-02-18 07:01:44
On Thu, Feb 18, 2021 at 7:33 AM Olga Kornievskaia [off-list ref] wrote:
On Wed, Feb 17, 2021 at 3:30 PM Luis Henriques [off-list ref] wrote:
quoted
A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystems copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices"). It also introduces a flag (COPY_FILE_SPLICE) that can be used
by filesystems calling directly into the vfs copy_file_range to override
these restrictions. Right now, only NFS needs to set this flag.
Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
Link: https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/
Link: https://lore.kernel.org/linux-fsdevel/CANMq1KDZuxir2LM5jOTm0xx+BnvW=ZmpsG47CyHFJwnw7zSX6Q@mail.gmail.com/
Link: https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/
Reported-by: Nicolas Boichat <redacted>
Signed-off-by: Luis Henriques <redacted>
---
Ok, I've tried to address all the issues and comments. Hopefully this v3
is a bit closer to the final fix.
Changes since v2
- do all the required checks earlier, in generic_copy_file_checks(),
adding new checks for ->remap_file_range
- new COPY_FILE_SPLICE flag
- don't remove filesystem's fallback to generic_copy_file_range()
- updated commit changelog (and subject)
Changes since v1 (after Amir review)
- restored do_copy_file_range() helper
- return -EOPNOTSUPP if fs doesn't implement CFR
- updated commit description
In my testing, this patch breaks NFS server-to-server copy file.
Hi Olga,
Can you please provide more details on the failed tests.
Does it fail on the client between two nfs mounts or does it fail
on the server? If the latter, between which two filesystems on the server?
Thanks,
Amir.
From: Olga Kornievskaia <hidden> Date: 2021-02-18 17:15:14
On Thu, Feb 18, 2021 at 1:48 AM Amir Goldstein [off-list ref] wrote:
On Thu, Feb 18, 2021 at 7:33 AM Olga Kornievskaia [off-list ref] wrote:
quoted
On Wed, Feb 17, 2021 at 3:30 PM Luis Henriques [off-list ref] wrote:
quoted
A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystems copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices"). It also introduces a flag (COPY_FILE_SPLICE) that can be used
by filesystems calling directly into the vfs copy_file_range to override
these restrictions. Right now, only NFS needs to set this flag.
Fixes: 5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices")
Link: https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/
Link: https://lore.kernel.org/linux-fsdevel/CANMq1KDZuxir2LM5jOTm0xx+BnvW=ZmpsG47CyHFJwnw7zSX6Q@mail.gmail.com/
Link: https://lore.kernel.org/linux-fsdevel/20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid/
Reported-by: Nicolas Boichat <redacted>
Signed-off-by: Luis Henriques <redacted>
---
Ok, I've tried to address all the issues and comments. Hopefully this v3
is a bit closer to the final fix.
Changes since v2
- do all the required checks earlier, in generic_copy_file_checks(),
adding new checks for ->remap_file_range
- new COPY_FILE_SPLICE flag
- don't remove filesystem's fallback to generic_copy_file_range()
- updated commit changelog (and subject)
Changes since v1 (after Amir review)
- restored do_copy_file_range() helper
- return -EOPNOTSUPP if fs doesn't implement CFR
- updated commit description
In my testing, this patch breaks NFS server-to-server copy file.
Hi Olga,
Can you please provide more details on the failed tests.
Does it fail on the client between two nfs mounts or does it fail
on the server? If the latter, between which two filesystems on the server?
It was a pilot error. V3 worked. I'm having some other issues with
server to server copy code but they seem to be unrelated to this. I
will test the new v6 versions when it comes out.
From: Christoph Hellwig <hch@infradead.org> Date: 2021-02-18 07:49:55
On Wed, Feb 17, 2021 at 05:26:54PM +0000, Luis Henriques wrote:
A regression has been reported by Nicolas Boichat, found while using the
copy_file_range syscall to copy a tracefs file. Before commit
5dae222a5ff0 ("vfs: allow copy_file_range to copy across devices") the
kernel would return -EXDEV to userspace when trying to copy a file across
different filesystems. After this commit, the syscall doesn't fail anymore
and instead returns zero (zero bytes copied), as this file's content is
generated on-the-fly and thus reports a size of zero.
This patch restores some cross-filesystems copy restrictions that existed
prior to commit 5dae222a5ff0 ("vfs: allow copy_file_range to copy across
devices"). It also introduces a flag (COPY_FILE_SPLICE) that can be used
by filesystems calling directly into the vfs copy_file_range to override
these restrictions. Right now, only NFS needs to set this flag.
No need for the flag. Jyst fall back to splicing in the only caller
that wants it.
From: Andreas Dilger <hidden> Date: 2021-02-18 00:51:40
On Feb 17, 2021, at 1:08 AM, Amir Goldstein [off-list ref] wrote:
You are missing my point.
Never mind which server. The server does not *need* to rely on
vfs_copy_file_range() to copy files from XFS to ext4.
The server is very capable of implementing the fallback generic copy
in case source/target fs do not support native {copy,remap}_file_range().
w.r.t semantics of copy_file_range() syscall vs. the fallback to userespace
'cp' tool (check source file size before copy or not), please note that the
semantics of CIFS_IOC_COPYCHUNK_FILE are that of the former:
rc = cifs_file_copychunk_range(xid, src_file.file, 0, dst_file, 0,
src_inode->i_size, 0);
It will copy zero bytes if advertised source file size if zero.
NFS server side copy semantics are currently de-facto the same
because both the client and the server will have to pass through this
line in vfs_copy_file_range():
if (len == 0)
return 0;
IMO, and this opinion was voiced by several other filesystem developers,
the shortend copy semantics are the correct semantics for copy_file_range()
syscall as well as for vfs_copy_file_range() for internal kernel users.
I guess what this means is that if the 'cp' tool ever tries an opportunistic
copy_file_range() syscall (e.g. --cfr=auto), it may result in zero size copy.
Having a syscall that does the "wrong thing" when called on two files
doesn't make sense. Expecting userspace to check whether source/target
files supports CFR is also not practical. This is trivial for the
kernel to determine and return -EOPNOTSUPP to the caller if the source
file (procfs/sysfs/etc) does not work with CFR properly.
Applications must already handle -EOPNOTSUPP with a fallback, but
expecting all applications that may call copy_file_range() to be
properly coded to handle corner cases is just asking for trouble.
That is doubly true given that an existing widely-used tool like
cp and mv are using this syscall if it is available in the kernel.
Cheers, Andreas
On Wed, Feb 17, 2021 at 05:50:35PM -0700, Andreas Dilger wrote:
On Feb 17, 2021, at 1:08 AM, Amir Goldstein [off-list ref] wrote:
quoted
You are missing my point.
Never mind which server. The server does not *need* to rely on
vfs_copy_file_range() to copy files from XFS to ext4.
The server is very capable of implementing the fallback generic copy
in case source/target fs do not support native {copy,remap}_file_range().
w.r.t semantics of copy_file_range() syscall vs. the fallback to userespace
'cp' tool (check source file size before copy or not), please note that the
semantics of CIFS_IOC_COPYCHUNK_FILE are that of the former:
rc = cifs_file_copychunk_range(xid, src_file.file, 0, dst_file, 0,
src_inode->i_size, 0);
It will copy zero bytes if advertised source file size if zero.
NFS server side copy semantics are currently de-facto the same
because both the client and the server will have to pass through this
line in vfs_copy_file_range():
if (len == 0)
return 0;
IMO, and this opinion was voiced by several other filesystem developers,
the shortend copy semantics are the correct semantics for copy_file_range()
syscall as well as for vfs_copy_file_range() for internal kernel users.
I guess what this means is that if the 'cp' tool ever tries an opportunistic
copy_file_range() syscall (e.g. --cfr=auto), it may result in zero size copy.
Having a syscall that does the "wrong thing" when called on two files
doesn't make sense. Expecting userspace to check whether source/target
files supports CFR is also not practical. This is trivial for the
kernel to determine and return -EOPNOTSUPP to the caller if the source
file (procfs/sysfs/etc) does not work with CFR properly.
How does the kernel "know" that a specific file in a specific filesystem
will not work with CFR "properly"? That goes back to the original patch
which tried to label each and every filesystem type with a
"supported/not supported" type of flag, which was going to be a mess,
especially as it seems that this might be a file-specific thing, not a
filesystem-specific thing.
The goal of the patch _should_ be that the kernel figure it out itself,
but so far no one seems to be able to explain how that can be done :(
So, any hints?
thanks,
greg k-h