Paths

Table of Contentst

fix copy_file_range(2) so that it handles the case where input offset + len would wrap around
ClosedPublic
Actions

Authored by rmacklem on Sep 28 2020, 1:19 AM.

Details

Reviewers

asomers
kib

Commits

rS366278: Make copy_file_range(2) Linux compatible for overflow of offset + len.

Summary

Without this patch, if a call to copy_file_range(2) specifies an input file
outset + len that would wrap around, EINVAL is returned.
I thought that was the Linux behaviour, but recent testing showed that
Linux accepts this case and does the copy_file_range() to EOF.

This patch changes the FreeBSD code to exhibit the same behaviour as
Linux for this case.

Test Plan

Tested with a little file copy program and a patched variant
of "cp", where the len argument for copy_file_range() is
SSIZE_MAX.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 33860

Event Timeline

rmacklem created this revision.Sep 28 2020, 1:19 AM

Herald added a subscriber: imp. · View Herald TranscriptSep 28 2020, 1:19 AM

rmacklem requested review of this revision.Sep 28 2020, 1:19 AM

Harbormaster completed remote builds in B33844: Diff 77581.Sep 28 2020, 1:19 AM

asomers requested changes to this revision.Sep 28 2020, 4:17 AM

asomers added inline comments.

sys/kern/vfs_vnops.c
2808	Why do you still return EINVAL if the output offset + len would wrap around? I would think that the common case would be `copy_file_range(infd, some_potentially_nonzero_offset, outfd, some_potentially_nonzero_offset, SIZE_MAX, 0)` to copy to the EOF of the input file. In that case, you should allow outoffp to wrap, too.

This revision now requires changes to proceed.Sep 28 2020, 4:17 AM

rmacklem added inline comments.Sep 28 2020, 3:19 PM

sys/kern/vfs_vnops.c
2808	If outoffp wraps around it goes negative. I can't see VOP_WRITE() with a negative file offset being a good idea. I agree that, usually, the copy will end before the wrap around happens (is there a real file system that can handle a sparse file of size INT64_MAX?), but I think a wraparound is conceivable?

Change handling of a wrap around of *outoffp + len
to clip "len" instead of return EINVAL.

Harbormaster completed remote builds in B33859: Diff 77601.Sep 28 2020, 4:15 PM

rmacklem marked an inline comment as done.Sep 28 2020, 4:17 PM

rmacklem added inline comments.

sys/kern/vfs_vnops.c
2820	Although allowing it to wrap around doesn't seem like a good idea, doing a further clip of "len" seems ok. This will allow SSIZE_MAX to be used for "copy to EOF" when the output offset > input offset.

Add a check for len being clipped down to 0.
No sense in doing the VOP call for this case.

Harbormaster completed remote builds in B33860: Diff 77604.Sep 28 2020, 4:30 PM

kib added inline comments.Sep 28 2020, 4:44 PM

sys/kern/vfs_vnops.c
2806	What if uval + len overflow so that the sum is less then either of addends ?

rmacklem added inline comments.Sep 29 2020, 12:49 AM

sys/kern/vfs_vnops.c
2806	I don't think this can happen. *inoffp is off_t (signed 64bit) and has been already checked for non-negative, so it must be <= INT64_MAX. len has been clipped to SSIZE_MAX, so it can't be greater than INT64_MAX. --> The sum of these can't exceed UINT64_MAX. Sound right?

Treating overflow of inoffp and outoffp identically makes sense to me.

This revision is now accepted and ready to land.Sep 29 2020, 1:06 AM

Closed by commit rS366278: Make copy_file_range(2) Linux compatible for overflow of offset + len. (authored by rmacklem). · Explain WhySep 30 2020, 2:18 AM

This revision was automatically updated to reflect the committed changes.

rmacklem added a commit: rS366278: Make copy_file_range(2) Linux compatible for overflow of offset + len..

Revision Contents
Changeset List

Path

Size

sys/

kern/

	vfs_vnops.c
	vfs_vnops.c.sav3

24 lines

Diff 77604

View Options

fix copy_file_range(2) so that it handles the case where input offset + len would wrap aroundClosedPublicActions