mm: various improvements to PageRef #450
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
00xc
reviewed
Aug 29, 2024
|
I think I don't fully understand why implementing |
Data races are only UB if they happen through normal reads or writes. If we implement the reads/writes in assembly, data races aren't UB. |
p4zuu
approved these changes
Aug 30, 2024
joergroedel
requested changes
Sep 11, 2024
Previously PageRef::new was unsound because it allowed the caller to pass in any virtual address and dereferene it through the returned PageRef. allocate_file_page_ref is the only caller of PageRef::new, so moving the alloc step into PageRef::new doesn't break anything. Signed-off-by: Tom Dohrmann <[email protected]>
After the previous patch, this function did nothing but call another function, so we might as well remove this function. Signed-off-by: Tom Dohrmann <[email protected]>
These are safe methods to write to the pointed to memory that take care of any bounds checks. Making these into methods also makes it easier to change to their implementations to be more sound. Signed-off-by: Tom Dohrmann <[email protected]>
Previously PageRef had AsMut<[u8; PAGE_SIZE]> and AsRef<[u8; PAGE_SIZE]> implementations that provided access the the bytes in the page. Unfortunately, this is not sound because PageRef doesn't have unique ownership over the page: An aliasing page can be create by calling .clone() and the page is also aliased when it's mapped into a page table. For these reasons, non-atomic accesses could potentially cause a data race and those are UB. This leaves two solutions for implementing the methods for PageRef: 1. Only use Relaxed atomic accesses. The codegen for this is generally quite bad because LLVM will access each byte individually. 2. Implement them in assembly. The upside of this is that if we control the assembly, so there's no compiler that will tell us what we can and can't do. For this patch, I've opted for the second solution. Signed-off-by: Tom Dohrmann <[email protected]>
drain is shorter and faster :) Signed-off-by: Tom Dohrmann <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes some soundness issues around PageRef and adds save abstractions for reading, writing, and filling a PageRef.