-
Notifications
You must be signed in to change notification settings - Fork 109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add the Allowed type to libtock_platform. #222
Merged
Merged
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
/// Because the kernel may modify shared memory and place any bit pattern into | ||
/// that memory, we can only read types out of shared memory if every bit | ||
/// pattern is a valid value of that type. `AllowReadable` types are safe to | ||
/// read out of a shared buffer. | ||
pub unsafe trait AllowReadable {} | ||
|
||
unsafe impl AllowReadable for i8 {} | ||
unsafe impl AllowReadable for i16 {} | ||
unsafe impl AllowReadable for i32 {} | ||
unsafe impl AllowReadable for i64 {} | ||
unsafe impl AllowReadable for i128 {} | ||
unsafe impl AllowReadable for isize {} | ||
unsafe impl AllowReadable for u8 {} | ||
unsafe impl AllowReadable for u16 {} | ||
unsafe impl AllowReadable for u32 {} | ||
unsafe impl AllowReadable for u64 {} | ||
unsafe impl AllowReadable for u128 {} | ||
unsafe impl AllowReadable for usize {} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
/// An individual value that has been shared with the kernel using the `allow` | ||
/// system call. | ||
// Design note: Allowed's implementation does not directly use the 'b lifetime. | ||
// Platform uses 'b to prevent the Allowed from accessing the buffer after the | ||
// buffer becomes invalid. | ||
pub struct Allowed<'b, T: 'b> { | ||
// Safety properties: | ||
// 1. `buffer` remains valid and usable for the lifetime of this Allowed | ||
// instance. | ||
// 2. Read and write accesses of `buffer`'s pointee must be performed as a | ||
// volatile operation, as the kernel may mutate buffer's pointee at any | ||
// time. | ||
// 3. The value `buffer` points to may have an arbitrary bit pattern in | ||
// it, so reading from `buffer` is only safe if the type contained | ||
// within is AllowReadable. | ||
buffer: core::ptr::NonNull<T>, | ||
|
||
// We need to use the 'b lifetime in Allowed to prevent an "unused lifetime" | ||
// compiler error. We use it here. Note that the phantom data must be an | ||
// &mut rather than a shared reference in order to make Allowed invariant | ||
// with respect to T. Invariance is required because Allowed allows us to | ||
// mutate the value in buffer, and invariance is the required property to do | ||
// so without allowing callers to produce dangling references. This is | ||
// documented at https://doc.rust-lang.org/nomicon/subtyping.html. | ||
_phantom: core::marker::PhantomData<&'b mut T>, | ||
} | ||
|
||
// Allowed's API is based on that of core::cell::Cell, but removes some methods | ||
// that are not safe for use with shared memory. | ||
// | ||
// Internally, Allowed performs accesses to the shared memory using volatile | ||
// reads and writes through raw pointers. We avoid constructing references to | ||
// shared memory because that leads to undefined behavior (there is some | ||
// background on this in the following discussion: | ||
// https://github.com/rust-lang/unsafe-code-guidelines/issues/33). Tock runs on | ||
// single-threaded platforms, some of which lack atomic instructions, so we only | ||
// need to be able to deconflict races between the kernel (which will never | ||
// interrupt an instruction's execution) and this process. Therefore volatile | ||
// accesses are sufficient to deconflict races. | ||
impl<'b, T: 'b> Allowed<'b, T> { | ||
// Allowed can only be constructed by the Platform. It is constructed after | ||
// the `allow` system call, and as such must accept a raw pointer rather | ||
// than a reference. The caller must make sure the following are true: | ||
// 1. buffer points to a valid instance of type T | ||
// 2. There are no references to buffer's pointee | ||
// 3. buffer remains usable until the Allowed's lifetime has ended. | ||
#[allow(dead_code)] // TODO: Remove when Platform is implemented | ||
pub(crate) unsafe fn new(buffer: core::ptr::NonNull<T>) -> Allowed<'b, T> { | ||
Allowed { | ||
buffer, | ||
_phantom: core::marker::PhantomData, | ||
} | ||
} | ||
|
||
// Sets the value in the buffer. Note that unlike `core::cell::Cell::set`, | ||
// `Allowed::set` will not drop the existing value. To drop the existing | ||
// value, use `replace` and drop the return value. | ||
pub fn set(&self, value: T) { | ||
unsafe { | ||
core::ptr::write_volatile(self.buffer.as_ptr(), value); | ||
} | ||
} | ||
} | ||
|
||
impl<'b, T: crate::AllowReadable + 'b> Allowed<'b, T> { | ||
pub fn replace(&self, value: T) -> T { | ||
let current = unsafe { core::ptr::read_volatile(self.buffer.as_ptr()) }; | ||
unsafe { | ||
core::ptr::write_volatile(self.buffer.as_ptr(), value); | ||
} | ||
current | ||
} | ||
} | ||
|
||
impl<'b, T: crate::AllowReadable + Copy + 'b> Allowed<'b, T> { | ||
pub fn get(&self) -> T { | ||
unsafe { core::ptr::read_volatile(self.buffer.as_ptr()) } | ||
} | ||
} | ||
|
||
impl<'b, T: crate::AllowReadable + Default + 'b> Allowed<'b, T> { | ||
pub fn take(&self) -> T { | ||
self.replace(T::default()) | ||
} | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,184 @@ | ||
use crate::allows::test_util::DropCheck; | ||
use crate::Allowed; | ||
use core::marker::PhantomData; | ||
use core::ptr::NonNull; | ||
|
||
// How do we simulate accesses to the shared buffer by the kernel? | ||
// | ||
// Well, a naive way would be to mutate the `buffer` variable directly. Because | ||
// Allowed accesses the memory through a *mut pointer, such a test would compile | ||
// and run fine with the current Rust compiler. As far as I can tell, it would | ||
// not hit any behavior documented as undefined at | ||
// https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html, | ||
// nor would it cause rustc to generate LLVM bitcode that encounters undefined | ||
// behavior. | ||
// | ||
// However, the naive approach will throw an "undefined behavior" error when run | ||
// under Miri (e.g. with `cargo miri test`), which uses the stacked borrows | ||
// model [1]. In particular, accessing the `buffer` variable directly pops the | ||
// SharedRW off buffer's borrow stack, which prevents Allowed from using its | ||
// *mut pointer to access `buffer` afterwards. It is likely that Rust will adopt | ||
// the stacked borrows model as its formal model for borrow validity, and in | ||
// that case accessing `buffer` in that manner will become undefined behavior. | ||
// In addition, running these tests under Miri is highly valuable, as this is | ||
// tricky code to get correct and an unsound API may be hard to fix. | ||
// | ||
// Instead, we explicitly refer to buffer through use of a KernelPtr that | ||
// simulates the pointer that `allow()` would hand to the Tock kernel. As far as | ||
// the stacked borrows model is concerned, accesses through the KernelPtr | ||
// variable behave identically to mutations performed by the kernel. This | ||
// pattern allows us to use `cargo miri test` to not only execute the unit | ||
// tests, but to test whether Allowed would encounter undefined behavior when | ||
// interacting with a real Tock kernel. | ||
// | ||
// [1] https://plv.mpi-sws.org/rustbelt/stacked-borrows/paper.pdf | ||
struct KernelPtr<'b, T: 'b> { | ||
ptr: NonNull<T>, | ||
|
||
// We need to consume the 'b lifetime. This is very similar to Allowed's | ||
// implementation. | ||
_phantom: PhantomData<&'b mut T>, | ||
} | ||
|
||
impl<'b, T: 'b> KernelPtr<'b, T> { | ||
// The constructor for KernelPtr; simulates allow(). Returns both the | ||
// Allowed instance the Platform would return and a KernelPtr the test can | ||
// use to simulate a kernel. | ||
pub fn allow(buffer: &'b mut T) -> (Allowed<'b, T>, KernelPtr<'b, T>) { | ||
let ptr = NonNull::new(buffer).unwrap(); | ||
// Discard buffer *without* creating a reference to it, as would be done | ||
// if we called drop(). | ||
let _ = buffer; | ||
// All 3 preconditions of Allowed::new are satisfied by the fact that | ||
// `buffer` is directly derived from a &'b mut T. | ||
let allowed = unsafe { Allowed::new(ptr) }; | ||
let kernel_ptr = KernelPtr { | ||
ptr, | ||
_phantom: PhantomData, | ||
}; | ||
(allowed, kernel_ptr) | ||
} | ||
|
||
// Replaces the value in the buffer with a new one. Does not drop the | ||
// existing value. | ||
pub fn set(&self, value: T) { | ||
unsafe { | ||
core::ptr::write(self.ptr.as_ptr(), value); | ||
} | ||
} | ||
} | ||
|
||
impl<'b, T: Copy + 'b> KernelPtr<'b, T> { | ||
// Copies the contained value out of the buffer. | ||
pub fn get(&self) -> T { | ||
unsafe { core::ptr::read(self.ptr.as_ptr()) } | ||
} | ||
} | ||
|
||
impl<'b, 'f: 'b> KernelPtr<'b, DropCheck<'f>> { | ||
// Retrieves the value of the contained DropCheck. | ||
pub fn value(&self) -> usize { | ||
let drop_check = unsafe { core::ptr::read(self.ptr.as_ptr()) }; | ||
let value = drop_check.value; | ||
core::mem::forget(drop_check); | ||
value | ||
} | ||
} | ||
|
||
#[test] | ||
fn set() { | ||
let dropped1 = core::cell::Cell::new(false); | ||
let dropped2 = core::cell::Cell::new(false); | ||
let dropped3 = core::cell::Cell::new(false); | ||
let mut buffer = DropCheck { | ||
flag: Some(&dropped1), | ||
value: 1, | ||
}; | ||
let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer); | ||
assert_eq!(kernel_ptr.value(), 1); | ||
assert_eq!(dropped1.get(), false); | ||
|
||
// Simulate the kernel replacing the value in buffer. We don't drop the | ||
// existing value. | ||
kernel_ptr.set(DropCheck { | ||
flag: Some(&dropped2), | ||
value: 2, | ||
}); | ||
allowed.set(DropCheck { | ||
flag: Some(&dropped3), | ||
value: 3, | ||
}); | ||
assert_eq!(kernel_ptr.value(), 3); | ||
assert_eq!(dropped1.get(), false); | ||
assert_eq!(dropped2.get(), false); | ||
assert_eq!(dropped3.get(), false); | ||
} | ||
|
||
jrvanwhy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#[test] | ||
fn replace() { | ||
let dropped1 = core::cell::Cell::new(false); | ||
let dropped2 = core::cell::Cell::new(false); | ||
let dropped3 = core::cell::Cell::new(false); | ||
let mut buffer = DropCheck { | ||
flag: Some(&dropped1), | ||
value: 1, | ||
}; | ||
let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer); | ||
assert_eq!(kernel_ptr.value(), 1); | ||
assert_eq!(dropped1.get(), false); | ||
|
||
// Simulate the kernel replacing the value in buffer. We don't drop the | ||
// existing value. | ||
kernel_ptr.set(DropCheck { | ||
flag: Some(&dropped2), | ||
value: 2, | ||
}); | ||
let returned = allowed.replace(DropCheck { | ||
flag: Some(&dropped3), | ||
value: 3, | ||
}); | ||
assert_eq!(returned.value, 2); | ||
assert_eq!(kernel_ptr.value(), 3); | ||
assert_eq!(dropped1.get(), false); | ||
assert_eq!(dropped2.get(), false); | ||
assert_eq!(dropped3.get(), false); | ||
} | ||
|
||
#[test] | ||
fn get() { | ||
// We can't use DropCheck because Drop and Copy cannot both be implemented | ||
// on a single type. | ||
let mut buffer = 1; | ||
let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer); | ||
assert_eq!(kernel_ptr.get(), 1); | ||
kernel_ptr.set(2); | ||
assert_eq!(allowed.get(), 2); | ||
assert_eq!(kernel_ptr.get(), 2); | ||
} | ||
|
||
#[test] | ||
fn take() { | ||
let dropped1 = core::cell::Cell::new(false); | ||
let dropped2 = core::cell::Cell::new(false); | ||
let dropped3 = core::cell::Cell::new(false); | ||
let mut buffer = DropCheck { | ||
flag: Some(&dropped1), | ||
value: 1, | ||
}; | ||
let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer); | ||
assert_eq!(kernel_ptr.value(), 1); | ||
assert_eq!(dropped1.get(), false); | ||
|
||
// Simulate the kernel replacing the value in buffer. We don't drop the | ||
// existing value. | ||
kernel_ptr.set(DropCheck { | ||
flag: Some(&dropped2), | ||
value: 2, | ||
}); | ||
let returned = allowed.take(); | ||
assert_eq!(returned.value, 2); | ||
assert_eq!(kernel_ptr.value(), 0); | ||
assert_eq!(dropped1.get(), false); | ||
assert_eq!(dropped2.get(), false); | ||
assert_eq!(dropped3.get(), false); | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
mod allow_readable; | ||
mod allowed; | ||
|
||
pub use allow_readable::AllowReadable; | ||
pub use allowed::Allowed; | ||
|
||
#[cfg(test)] | ||
mod allowed_tests; | ||
#[cfg(test)] | ||
mod test_util; |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
//! Contains testing utilities for libtock_platform::allowed. | ||
|
||
// A value that can be placed in an Allowed that checks whether it has been | ||
// dropped. | ||
#[derive(Default)] | ||
pub(crate) struct DropCheck<'f> { | ||
pub flag: Option<&'f core::cell::Cell<bool>>, | ||
pub value: usize, | ||
} | ||
|
||
impl<'f> Drop for DropCheck<'f> { | ||
fn drop(&mut self) { | ||
if let Some(flag) = self.flag { | ||
flag.set(true); | ||
} | ||
} | ||
} | ||
|
||
// Note: DropCheck cannot be safely used in a non-test context, as DropCheck | ||
// does not satisfy AllowReadable's "every bit pattern is valid" requirement. | ||
// However, in the tests we aren't sharing the buffer with a real Tock kernel, | ||
// and instead we guarantee that only valid instances of DropCheck are loaded | ||
// into the buffer. | ||
unsafe impl<'f> crate::AllowReadable for DropCheck<'f> {} | ||
|
||
// Verify that DropCheck works, as none of the tests require it to actually set | ||
// the drop flag. | ||
#[test] | ||
fn drop_check() { | ||
let flag = core::cell::Cell::new(false); | ||
let drop_check = DropCheck { | ||
flag: Some(&flag), | ||
value: 0, | ||
}; | ||
drop(drop_check); | ||
assert_eq!(flag.get(), true); | ||
} |
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this by modifying
replace()
, and it does seem to be generally true.However, this behavior is definitely somewhat confusing, as the caller of replace cannot know whether the destructor will be called for the item originally inserted from the userspace side, or for the item inserted by the kernel.
It feels like it would be a mistake for any userspace implementation to rely on drop being called for values that have been
Allowed
, but I read this comment as suggesting that doing so is possible. Perhaps it should be reworded? Alternatively, perhaps I am misunderstanding?