Skip to content
This repository has been archived by the owner on Mar 7, 2021. It is now read-only.

Implement seek on FileOperations #113

Merged
merged 3 commits into from
Aug 25, 2019
Merged

Implement seek on FileOperations #113

merged 3 commits into from
Aug 25, 2019

Conversation

alex
Copy link
Member

@alex alex commented Aug 4, 2019

Refs #110

@alex alex force-pushed the ll_seek branch 2 times, most recently from 65826da to d098fc6 Compare August 5, 2019 02:05
@alex alex requested a review from geofft August 5, 2019 02:15
src/chrdev.rs Outdated Show resolved Hide resolved
src/chrdev.rs Outdated Show resolved Hide resolved
@geofft
Copy link
Collaborator

geofft commented Aug 5, 2019

(LGTM otherwise)

@alex
Copy link
Member Author

alex commented Aug 5, 2019

The kernel seems to just use loff_t for all these values, even when a negative makes no sense. Opinions?

@alex
Copy link
Member Author

alex commented Aug 5, 2019

Updated to make from_ptr unsafe

@alex alex force-pushed the ll_seek branch 2 times, most recently from 4cdf5c2 to 597b464 Compare August 20, 2019 00:51
@geofft
Copy link
Collaborator

geofft commented Aug 25, 2019

Quoting this in the main comment thread so it doesn't get lost in line-item review.

Is as u64 the right thing here? (What does native C code do?)

OK, the answer is miserable:

  • struct file stores the offset in the signed variable loff_t f_pos.
  • For rare files like /dev/kmem that can be larger than the maximum signed long, there's a flag FMODE_UNSIGNED_OFFSET in struct file::f_mode that indicates that the offset is to be treated as an unsigned instead of a signed. That means, don't complain about a "negative" offset (it will be treated as positive by the read/write code). Introduced in torvalds/linux@4a3956c7, changed into its current form in torvalds/linux@cccb5a1e, vulnerability fixed in torvalds/linux@e8905ec2.
  • The built-in generic code in fs/read_write.c (see default_llseek and generic_file_llseek_size/vfs_setpos) will calculate the final offset as a signed (loff_t), then do a signed comparison to zero if FMODE_UNSIGNED_OFFSET isn't set and raise EINVAL. Presumably it is UB to do SEEK_CUR while you're over in offsets greater than 2^63. (Yikes.)

I think the right thing to do here is to punt on handling FMODE_UNSIGNED_OFFSET files for now, raise EINVAL on a SeekFrom::Start(n) if n < 0, and encourage calling code to do the same if the final seek position makes no sense (either before the start or after the end).

src/chrdev.rs Outdated Show resolved Hide resolved
src/chrdev.rs Show resolved Hide resolved
@alex alex merged commit baed97f into master Aug 25, 2019
@alex alex deleted the ll_seek branch August 25, 2019 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants