| Message ID | 20210126135012.1.If45b7cdc3ff707bc1efa17f5366057d60603c45f@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs: generic_copy_file_checks: Do not adjust count based on file size | expand |
On Wed, Jan 27, 2021 at 7:38 AM Dave Chinner <david@fromorbit.com> wrote: > > On Tue, Jan 26, 2021 at 01:50:22PM +0800, Nicolas Boichat wrote: > > copy_file_range (which calls generic_copy_file_checks) uses the > > inode file size to adjust the copy count parameter. This breaks > > with special filesystems like procfs/sysfs, where the file size > > appears to be zero, but content is actually returned when a read > > operation is performed. > > > > This commit ignores the source file size, and makes copy_file_range > > match the end of file behaviour documented in POSIX's "read", > > where 0 is returned to mark EOF. This would allow "cp" and other > > standard tools to make use of copy_file_range with the exact same > > behaviour as they had in the past. > > > > Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > Nack. Thanks Dave and Al for the detailed explanations. > > As I've explained, this is intentional and bypassing it is not a > work around for enabling cfr on filesystems that produce ephemeral, > volatile read-once data using seq-file pipes that masquerade as > regular files with zero size. These files are behaving like pipes > and only work because the VFS has to support read() and friends from > pipes that don't publish the amount of data they contain to the VFS > inode. > > copy_file_range() does not support such behaviour. > > copy_file_range() -writes- data, so we have to check that those > writes do not extend past boundaries that the destination inode > imposes on the operation. e.g. maximum offset limits, whether the > ranges overlap in the same file, etc. > > Hence we need to know how much data there is present to copy before > we can check if it is safe to perform the -write- of the data we are > going to read. Hence we cannot safely support data sources that > cannot tell us how much data is present before we start the copy > operation. > > IOWs, these source file EOF restrictions are required by the write > side of copy_file_range(), not the read side. > > > --- > > This can be reproduced with this simple test case: > > #define _GNU_SOURCE > > #include <fcntl.h> > > #include <stdio.h> > > #include <stdlib.h> > > #include <sys/stat.h> > > #include <unistd.h> > > > > int > > main(int argc, char **argv) > > { > > int fd_in, fd_out; > > loff_t ret; > > > > fd_in = open("/proc/version", O_RDONLY); > > fd_out = open("version", O_CREAT | O_WRONLY | O_TRUNC, 0644); > > > > do { > > ret = copy_file_range(fd_in, NULL, fd_out, NULL, 1024, 0); > > printf("%d bytes copied\n", (int)ret); > > } while (ret > 0); > > > > return 0; > > } > > > > Without this patch, `version` output file is empty, and no bytes > > are copied: > > 0 bytes copied > > $ ls -l /proc/version > -r--r--r-- 1 root root 0 Jan 20 17:25 /proc/version > $ > > It's a zero length file. > > sysfs does this just fine - it's regular files have a size of > at least PAGE_SIZE rather than zero, and so copy_file_range works > just fine on them: > > $ ls -l /sys/block/nvme0n1/capability > -r--r--r-- 1 root root 4096 Jan 27 08:41 /sys/block/nvme0n1/capability > $ cat /sys/block/nvme0n1/capability > 50 > $ xfs_io -f -c "copy_range -s 0 -d 0 -l 4096 /sys/block/nvme0n1/capability" /tmp/foo > $ sudo cat /tmp/foo > 50 > > And the behaviour is exactly as you'd expect a read() loop to copy > the file to behave: > > openat(AT_FDCWD, "/tmp/foo", O_RDWR|O_CREAT, 0600) = 3 > .... > openat(AT_FDCWD, "/sys/block/nvme0n1/capability", O_RDONLY) = 4 > copy_file_range(4, [0], 3, [0], 4096, 0) = 3 > copy_file_range(4, [3], 3, [3], 4093, 0) = 0 > close(4) > > See? Inode size of 4096 means there's a maximum of 4kB of data that > can be read from this file. copy_file_range() now behaves exactly > as read() would, returning a short copy and then 0 bytes to indicate > EOF. Unless the content happens to be larger than PAGE_SIZE, then copy_file_range would only copy the beginning of the file. And as Al explained, this will still break in case of short writes. > > If you want ephemeral data pipes masquerading as regular files to > work with copy_file_range, then the filesystem implementation needs > to provide the VFS with a data size that indicates the maximum > amount of data that the pipe can produce in a continuous read loop. > Otherwise we cannot validate the range of the write we may be asked > to perform... > > > Under the hood, Go 1.15 uses `copy_file_range` syscall to optimize the > > copy operation. However, that fails to copy any content when the input > > file is from sysfs/tracefs, with an apparent size of 0 (but there is > > still content when you `cat` it, of course). > > Libraries using copy_file_range() must be prepared for it to fail > and fall back to normal copy mechanisms. How is userspace suppose to detect that? (checking for 0 file size won't work with the example above) > Of course, with these > special zero length files that contain ephemeral data, userspace can't > actually tell that they contain data from userspace using stat(). So > as far as userspace is concerned, copy_file_range() correctly > returned zero bytes copied from a zero byte long file and there's > nothing more to do. > > This zero length file behaviour is, fundamentally, a kernel > filesystem implementation bug, not a copy_file_range() bug. Okay, so, based on this and Al's reply, I see 2 things we can do: 1. Go should probably not use copy_file_range in a common library function, as I don't see any easy way to detect this scenario currently (detect 0 size? sure, but that won't work with the example you provide above). And the man page should document this behaviour more explicitly to prevent further incorrect usage. 2. Can procfs/sysfs/debugfs and friends explicitly prevent usage of copy_file_range? (based on Al's reply, there seems to be no way to implement it correctly as seeking in such files will not work in case of short writes) Thanks, > > Cheers, > > Dave. > -- > Dave Chinner > david@fromorbit.com
On Thu, Jan 28, 2021 at 08:46:04AM +0800, Nicolas Boichat wrote: > On Wed, Jan 27, 2021 at 7:38 AM Dave Chinner <david@fromorbit.com> wrote: > > > > On Tue, Jan 26, 2021 at 01:50:22PM +0800, Nicolas Boichat wrote: > > > copy_file_range (which calls generic_copy_file_checks) uses the > > > inode file size to adjust the copy count parameter. This breaks > > > with special filesystems like procfs/sysfs, where the file size > > > appears to be zero, but content is actually returned when a read > > > operation is performed. > > > > > > This commit ignores the source file size, and makes copy_file_range > > > match the end of file behaviour documented in POSIX's "read", > > > where 0 is returned to mark EOF. This would allow "cp" and other > > > standard tools to make use of copy_file_range with the exact same > > > behaviour as they had in the past. > > > > > > Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") > > > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> > > > > Nack. > > Thanks Dave and Al for the detailed explanations. > > > > > As I've explained, this is intentional and bypassing it is not a > > work around for enabling cfr on filesystems that produce ephemeral, > > volatile read-once data using seq-file pipes that masquerade as > > regular files with zero size. These files are behaving like pipes > > and only work because the VFS has to support read() and friends from > > pipes that don't publish the amount of data they contain to the VFS > > inode. > > > > copy_file_range() does not support such behaviour. > > > > copy_file_range() -writes- data, so we have to check that those > > writes do not extend past boundaries that the destination inode > > imposes on the operation. e.g. maximum offset limits, whether the > > ranges overlap in the same file, etc. > > > > Hence we need to know how much data there is present to copy before > > we can check if it is safe to perform the -write- of the data we are > > going to read. Hence we cannot safely support data sources that > > cannot tell us how much data is present before we start the copy > > operation. > > > > IOWs, these source file EOF restrictions are required by the write > > side of copy_file_range(), not the read side. > > > > > --- > > > This can be reproduced with this simple test case: > > > #define _GNU_SOURCE > > > #include <fcntl.h> > > > #include <stdio.h> > > > #include <stdlib.h> > > > #include <sys/stat.h> > > > #include <unistd.h> > > > > > > int > > > main(int argc, char **argv) > > > { > > > int fd_in, fd_out; > > > loff_t ret; > > > > > > fd_in = open("/proc/version", O_RDONLY); > > > fd_out = open("version", O_CREAT | O_WRONLY | O_TRUNC, 0644); > > > > > > do { > > > ret = copy_file_range(fd_in, NULL, fd_out, NULL, 1024, 0); > > > printf("%d bytes copied\n", (int)ret); > > > } while (ret > 0); > > > > > > return 0; > > > } > > > > > > Without this patch, `version` output file is empty, and no bytes > > > are copied: > > > 0 bytes copied > > > > $ ls -l /proc/version > > -r--r--r-- 1 root root 0 Jan 20 17:25 /proc/version > > $ > > > > It's a zero length file. > > > > sysfs does this just fine - it's regular files have a size of > > at least PAGE_SIZE rather than zero, and so copy_file_range works > > just fine on them: > > > > $ ls -l /sys/block/nvme0n1/capability > > -r--r--r-- 1 root root 4096 Jan 27 08:41 /sys/block/nvme0n1/capability > > $ cat /sys/block/nvme0n1/capability > > 50 > > $ xfs_io -f -c "copy_range -s 0 -d 0 -l 4096 /sys/block/nvme0n1/capability" /tmp/foo > > $ sudo cat /tmp/foo > > 50 > > > > And the behaviour is exactly as you'd expect a read() loop to copy > > the file to behave: > > > > openat(AT_FDCWD, "/tmp/foo", O_RDWR|O_CREAT, 0600) = 3 > > .... > > openat(AT_FDCWD, "/sys/block/nvme0n1/capability", O_RDONLY) = 4 > > copy_file_range(4, [0], 3, [0], 4096, 0) = 3 > > copy_file_range(4, [3], 3, [3], 4093, 0) = 0 > > close(4) > > > > See? Inode size of 4096 means there's a maximum of 4kB of data that > > can be read from this file. copy_file_range() now behaves exactly > > as read() would, returning a short copy and then 0 bytes to indicate > > EOF. > > Unless the content happens to be larger than PAGE_SIZE, then > copy_file_range would only copy the beginning of the file. And as Al > explained, this will still break in case of short writes. > > > > > If you want ephemeral data pipes masquerading as regular files to > > work with copy_file_range, then the filesystem implementation needs > > to provide the VFS with a data size that indicates the maximum > > amount of data that the pipe can produce in a continuous read loop. > > Otherwise we cannot validate the range of the write we may be asked > > to perform... > > > > > Under the hood, Go 1.15 uses `copy_file_range` syscall to optimize the > > > copy operation. However, that fails to copy any content when the input > > > file is from sysfs/tracefs, with an apparent size of 0 (but there is > > > still content when you `cat` it, of course). > > > > Libraries using copy_file_range() must be prepared for it to fail > > and fall back to normal copy mechanisms. > > How is userspace suppose to detect that? (checking for 0 file size > won't work with the example above) > > > Of course, with these > > special zero length files that contain ephemeral data, userspace can't > > actually tell that they contain data from userspace using stat(). So > > as far as userspace is concerned, copy_file_range() correctly > > returned zero bytes copied from a zero byte long file and there's > > nothing more to do. > > > > This zero length file behaviour is, fundamentally, a kernel > > filesystem implementation bug, not a copy_file_range() bug. > > Okay, so, based on this and Al's reply, I see 2 things we can do: > 1. Go should probably not use copy_file_range in a common library > function, as I don't see any easy way to detect this scenario > currently (detect 0 size? sure, but that won't work with the example > you provide above). And the man page should document this behaviour > more explicitly to prevent further incorrect usage. > 2. Can procfs/sysfs/debugfs and friends explicitly prevent usage of > copy_file_range? (based on Al's reply, there seems to be no way to > implement it correctly as seeking in such files will not work in case > of short writes) One /could/ make those three provide a phony CFR implementation that would return -EOPNOTSUPP, though like others have said, it's weird to have regular files that aren't quite regular. Not sure where that leaves them, though... --D > > Thanks, > > > > > Cheers, > > > > Dave. > > -- > > Dave Chinner > > david@fromorbit.com
On Thu, Jan 28, 2021 at 1:57 PM Darrick J. Wong <djwong@kernel.org> wrote: > > On Thu, Jan 28, 2021 at 08:46:04AM +0800, Nicolas Boichat wrote: [snip] > > Okay, so, based on this and Al's reply, I see 2 things we can do: > > 1. Go should probably not use copy_file_range in a common library > > function, as I don't see any easy way to detect this scenario > > currently (detect 0 size? sure, but that won't work with the example > > you provide above). And the man page should document this behaviour > > more explicitly to prevent further incorrect usage. > > 2. Can procfs/sysfs/debugfs and friends explicitly prevent usage of > > copy_file_range? (based on Al's reply, there seems to be no way to > > implement it correctly as seeking in such files will not work in case > > of short writes) > > One /could/ make those three provide a phony CFR implementation that > would return -EOPNOTSUPP, though like others have said, it's weird to > have regular files that aren't quite regular. Not sure where that > leaves them, though... Not that simple, as the issue happens on cross-filesystem operations where file_operations->copy_file_range is not called (and also, that'd require modifying operations for every single generated file... Anyway, made an attempt here: https://lore.kernel.org/linux-fsdevel/20210212044405.4120619-1-drinkcat@chromium.org/T/#t > --D > > > > > Thanks, > > > > > > > > Cheers, > > > > > > Dave. > > > -- > > > Dave Chinner > > > david@fromorbit.com
diff --git a/fs/read_write.c b/fs/read_write.c index 75f764b43418..7236146f6ad7 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1424,7 +1424,6 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, struct inode *inode_in = file_inode(file_in); struct inode *inode_out = file_inode(file_out); uint64_t count = *req_count; - loff_t size_in; int ret; ret = generic_file_rw_checks(file_in, file_out); @@ -1442,13 +1441,6 @@ static int generic_copy_file_checks(struct file *file_in, loff_t pos_in, if (pos_in + count < pos_in || pos_out + count < pos_out) return -EOVERFLOW; - /* Shorten the copy to EOF */ - size_in = i_size_read(inode_in); - if (pos_in >= size_in) - count = 0; - else - count = min(count, size_in - (uint64_t)pos_in); - ret = generic_write_check_limits(file_out, pos_out, &count); if (ret) return ret;
copy_file_range (which calls generic_copy_file_checks) uses the inode file size to adjust the copy count parameter. This breaks with special filesystems like procfs/sysfs, where the file size appears to be zero, but content is actually returned when a read operation is performed. This commit ignores the source file size, and makes copy_file_range match the end of file behaviour documented in POSIX's "read", where 0 is returned to mark EOF. This would allow "cp" and other standard tools to make use of copy_file_range with the exact same behaviour as they had in the past. Fixes: 96e6e8f4a68d ("vfs: add missing checks to copy_file_range") Signed-off-by: Nicolas Boichat <drinkcat@chromium.org> --- This can be reproduced with this simple test case: #define _GNU_SOURCE #include <fcntl.h> #include <stdio.h> #include <stdlib.h> #include <sys/stat.h> #include <unistd.h> int main(int argc, char **argv) { int fd_in, fd_out; loff_t ret; fd_in = open("/proc/version", O_RDONLY); fd_out = open("version", O_CREAT | O_WRONLY | O_TRUNC, 0644); do { ret = copy_file_range(fd_in, NULL, fd_out, NULL, 1024, 0); printf("%d bytes copied\n", (int)ret); } while (ret > 0); return 0; } Without this patch, `version` output file is empty, and no bytes are copied: 0 bytes copied With this patch, the loop runs twice and the content of the file is copied: 315 bytes copied 0 bytes copied We hit this issue when upgrading Go compiler from 1.13 to 1.15 [1], as we use Go's `io.Copy` to copy the content of `/sys/kernel/debug/tracing/trace` to a temporary file. Under the hood, Go 1.15 uses `copy_file_range` syscall to optimize the copy operation. However, that fails to copy any content when the input file is from sysfs/tracefs, with an apparent size of 0 (but there is still content when you `cat` it, of course). [1] http://issuetracker.google.com/issues/178332739 fs/read_write.c | 8 -------- 1 file changed, 8 deletions(-)