From c4b02659c16d2ad0ac36d2c8602edd002e559f7a Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Thu, 11 Jun 2020 17:44:21 -0700 Subject: [PATCH 1/7] Enable some timeouts in SGX platform This would partially resolve https://github.com/fortanix/rust-sgx/issues/31 --- src/libstd/sync/condvar.rs | 4 - src/libstd/sync/mpsc/mod.rs | 9 -- src/libstd/sys/sgx/abi/usercalls/mod.rs | 24 ++++- src/libstd/sys/sgx/condvar.rs | 6 +- src/libstd/sys/sgx/mod.rs | 45 +++++++++- src/libstd/sys/sgx/thread.rs | 6 +- src/libstd/sys/sgx/waitqueue.rs | 113 ++++++++++++++++++++++-- src/libstd/thread/mod.rs | 3 - 8 files changed, 179 insertions(+), 31 deletions(-) diff --git a/src/libstd/sync/condvar.rs b/src/libstd/sync/condvar.rs index 77e521eae9afe..6778ab9dcde4a 100644 --- a/src/libstd/sync/condvar.rs +++ b/src/libstd/sync/condvar.rs @@ -695,7 +695,6 @@ mod tests { #[test] #[cfg_attr(target_os = "emscripten", ignore)] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn wait_timeout_wait() { let m = Arc::new(Mutex::new(())); let c = Arc::new(Condvar::new()); @@ -715,7 +714,6 @@ mod tests { #[test] #[cfg_attr(target_os = "emscripten", ignore)] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn wait_timeout_while_wait() { let m = Arc::new(Mutex::new(())); let c = Arc::new(Condvar::new()); @@ -740,7 +738,6 @@ mod tests { #[test] #[cfg_attr(target_os = "emscripten", ignore)] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn wait_timeout_while_wake() { let pair = Arc::new((Mutex::new(false), Condvar::new())); let pair_copy = pair.clone(); @@ -764,7 +761,6 @@ mod tests { #[test] #[cfg_attr(target_os = "emscripten", ignore)] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn wait_timeout_wake() { let m = Arc::new(Mutex::new(())); let c = Arc::new(Condvar::new()); diff --git a/src/libstd/sync/mpsc/mod.rs b/src/libstd/sync/mpsc/mod.rs index e70204d6839fc..db765bde5438e 100644 --- a/src/libstd/sync/mpsc/mod.rs +++ b/src/libstd/sync/mpsc/mod.rs @@ -2088,7 +2088,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn oneshot_single_thread_recv_timeout() { let (tx, rx) = channel(); tx.send(()).unwrap(); @@ -2099,7 +2098,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn stress_recv_timeout_two_threads() { let (tx, rx) = channel(); let stress = stress_factor() + 100; @@ -2130,7 +2128,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn recv_timeout_upgrade() { let (tx, rx) = channel::<()>(); let timeout = Duration::from_millis(1); @@ -2142,7 +2139,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn stress_recv_timeout_shared() { let (tx, rx) = channel(); let stress = stress_factor() + 100; @@ -2173,7 +2169,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn very_long_recv_timeout_wont_panic() { let (tx, rx) = channel::<()>(); let join_handle = @@ -2196,7 +2191,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn shared_recv_timeout() { let (tx, rx) = channel(); let total = 5; @@ -2426,7 +2420,6 @@ mod sync_tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn recv_timeout() { let (tx, rx) = sync_channel::(1); assert_eq!(rx.recv_timeout(Duration::from_millis(1)), Err(RecvTimeoutError::Timeout)); @@ -2518,7 +2511,6 @@ mod sync_tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn stress_recv_timeout_two_threads() { let (tx, rx) = sync_channel::(0); @@ -2544,7 +2536,6 @@ mod sync_tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn stress_recv_timeout_shared() { const AMT: u32 = 1000; const NTHREADS: u32 = 8; diff --git a/src/libstd/sys/sgx/abi/usercalls/mod.rs b/src/libstd/sys/sgx/abi/usercalls/mod.rs index ae803ee47a6cb..b223da9d7e4a2 100644 --- a/src/libstd/sys/sgx/abi/usercalls/mod.rs +++ b/src/libstd/sys/sgx/abi/usercalls/mod.rs @@ -1,5 +1,6 @@ use crate::cmp; use crate::io::{Error as IoError, IoSlice, IoSliceMut, Result as IoResult}; +use crate::sys::rand::rdrand64; use crate::time::Duration; pub(crate) mod alloc; @@ -149,7 +150,28 @@ pub fn exit(panic: bool) -> ! { /// Usercall `wait`. See the ABI documentation for more information. #[unstable(feature = "sgx_platform", issue = "56975")] -pub fn wait(event_mask: u64, timeout: u64) -> IoResult { +pub fn wait(event_mask: u64, mut timeout: u64) -> IoResult { + if timeout != WAIT_NO && timeout != WAIT_INDEFINITE { + // We don't want people to rely on accuracy of timeouts to make + // security decisions in an SGX enclave. That's why we add a random + // amount not exceeding +/- 10% to the timeout value to discourage + // people from relying on accuracy of timeouts while providing a way + // to make things work in other cases. Note that in the SGX threat + // model the enclave runner which is serving the wait usercall is not + // trusted to ensure accurate timeouts. + let base = cmp::max(1, timeout / 10) * 2 + 1; + let zero = base / 2; + match rdrand64() % base { + jitter if jitter > zero => { + timeout = timeout.checked_add(jitter - zero).unwrap_or(timeout) + } + jitter if jitter < zero => { + timeout = timeout.checked_sub(zero - jitter).unwrap_or(timeout) + } + _ => {} + }; + timeout = cmp::min(u64::MAX - 1, cmp::max(1, timeout)); + } unsafe { raw::wait(event_mask, timeout).from_sgx_result() } } diff --git a/src/libstd/sys/sgx/condvar.rs b/src/libstd/sys/sgx/condvar.rs index 9c5c086184d68..ed6dbcf497147 100644 --- a/src/libstd/sys/sgx/condvar.rs +++ b/src/libstd/sys/sgx/condvar.rs @@ -31,8 +31,10 @@ impl Condvar { mutex.lock() } - pub unsafe fn wait_timeout(&self, _mutex: &Mutex, _dur: Duration) -> bool { - rtabort!("timeout not supported in SGX"); + pub unsafe fn wait_timeout(&self, mutex: &Mutex, dur: Duration) -> bool { + let success = WaitQueue::wait_timeout(&self.inner, dur, || mutex.unlock()); + mutex.lock(); + success } #[inline] diff --git a/src/libstd/sys/sgx/mod.rs b/src/libstd/sys/sgx/mod.rs index 397dd496ae8af..dff9d3b1f07ea 100644 --- a/src/libstd/sys/sgx/mod.rs +++ b/src/libstd/sys/sgx/mod.rs @@ -110,6 +110,42 @@ pub fn decode_error_kind(code: i32) -> ErrorKind { } } +// This function makes an effort to sleep at least as long as `duration`. +// Note that in general there is no guarantee about accuracy of time and +// timeouts in SGX model. The enclave runner serving usercalls may lie about +// current time and/or ignore timeout values. +// +// FIXME: note these caveats in documentation of all public types that use this +// function in their execution path. +pub fn wait_timeout_sgx(event_mask: u64, duration: crate::time::Duration) { + use self::abi::usercalls; + use crate::cmp; + use crate::io::ErrorKind; + use crate::time::Instant; + + let start = Instant::now(); + let mut remaining = duration; + loop { + let timeout = cmp::min((u64::MAX - 1) as u128, remaining.as_nanos()) as u64; + match usercalls::wait(event_mask, timeout) { + Ok(eventset) => { + if event_mask != 0 { + rtassert!(eventset & event_mask == event_mask); + return; + } + rtabort!("expected usercalls::wait() to return Err, found Ok."); + } + Err(e) => { + rtassert!(e.kind() == ErrorKind::TimedOut || e.kind() == ErrorKind::WouldBlock) + } + } + remaining = match duration.checked_sub(start.elapsed()) { + Some(remaining) => remaining, + None => break, + } + } +} + // This enum is used as the storage for a bunch of types which can't actually // exist. #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)] @@ -137,8 +173,8 @@ pub extern "C" fn __rust_abort() { abort_internal(); } -pub fn hashmap_random_keys() -> (u64, u64) { - fn rdrand64() -> u64 { +pub mod rand { + pub fn rdrand64() -> u64 { unsafe { let mut ret: u64 = 0; for _ in 0..10 { @@ -149,7 +185,10 @@ pub fn hashmap_random_keys() -> (u64, u64) { rtabort!("Failed to obtain random data"); } } - (rdrand64(), rdrand64()) +} + +pub fn hashmap_random_keys() -> (u64, u64) { + (self::rand::rdrand64(), self::rand::rdrand64()) } pub use crate::sys_common::{AsInner, FromInner, IntoInner}; diff --git a/src/libstd/sys/sgx/thread.rs b/src/libstd/sys/sgx/thread.rs index 9b515eb82de35..8ff0f1fde91c2 100644 --- a/src/libstd/sys/sgx/thread.rs +++ b/src/libstd/sys/sgx/thread.rs @@ -1,6 +1,8 @@ #![cfg_attr(test, allow(dead_code))] // why is this necessary? + use crate::ffi::CStr; use crate::io; +use crate::sys::wait_timeout_sgx; use crate::time::Duration; use super::abi::usercalls; @@ -73,8 +75,8 @@ impl Thread { // FIXME: could store this pointer in TLS somewhere } - pub fn sleep(_dur: Duration) { - rtabort!("can't sleep"); // FIXME + pub fn sleep(dur: Duration) { + wait_timeout_sgx(0, dur); } pub fn join(self) { diff --git a/src/libstd/sys/sgx/waitqueue.rs b/src/libstd/sys/sgx/waitqueue.rs index 6e50f161b3b41..71d1c22cd6191 100644 --- a/src/libstd/sys/sgx/waitqueue.rs +++ b/src/libstd/sys/sgx/waitqueue.rs @@ -1,4 +1,3 @@ -use crate::num::NonZeroUsize; /// A simple queue implementation for synchronization primitives. /// /// This queue is used to implement condition variable and mutexes. @@ -10,7 +9,10 @@ use crate::num::NonZeroUsize; /// Since userspace may send spurious wake-ups, the wakeup event state is /// recorded in the enclave. The wakeup event state is protected by a spinlock. /// The queue and associated wait state are stored in a `WaitVariable`. +use crate::num::NonZeroUsize; use crate::ops::{Deref, DerefMut}; +use crate::sys::wait_timeout_sgx; +use crate::time::Duration; use super::abi::thread; use super::abi::usercalls; @@ -158,6 +160,37 @@ impl WaitQueue { } } + /// Adds the calling thread to the `WaitVariable`'s wait queue, then wait + /// until a wakeup event or timeout. If event was observed, returns true. + /// If not, it will remove the calling thread from the wait queue. + pub fn wait_timeout( + lock: &SpinMutex>, + timeout: Duration, + before_wait: F, + ) -> bool { + // very unsafe: check requirements of UnsafeList::push + unsafe { + let mut entry = UnsafeListEntry::new(SpinMutex::new(WaitEntry { + tcs: thread::current(), + wake: false, + })); + let entry_lock = lock.lock().queue.inner.push(&mut entry); + before_wait(); + // don't panic, this would invalidate `entry` during unwinding + wait_timeout_sgx(EV_UNPARK, timeout); + // acquire the wait queue's lock first to avoid deadlock. + let mut guard = lock.lock(); + let entry_guard = entry_lock.lock(); + let success = entry_guard.wake; + if !success { + // nobody is waking us up, so remove the entry from the wait queue. + drop(entry_guard); + guard.queue.inner.remove(&mut entry); + } + success + } + } + /// Either find the next waiter on the wait queue, or return the mutex /// guard unchanged. /// @@ -325,6 +358,31 @@ mod unsafe_list { Some((*first.as_ptr()).value.as_ref().unwrap()) } } + + /// Removes an entry from the list. + /// + /// # Safety + /// + /// The caller must ensure that entry has been pushed prior to this + /// call and has not moved since push. + pub unsafe fn remove(&mut self, entry: &mut UnsafeListEntry) { + rtassert!(!self.is_empty()); + // BEFORE: + // /----\ next ---> /-----\ next ---> /----\ + // ... |prev| |entry| |next| ... + // \----/ <--- prev \-----/ <--- prev \----/ + // + // AFTER: + // /----\ next ---> /----\ + // ... |prev| |next| ... + // \----/ <--- prev \----/ + let mut prev = entry.prev; + let mut next = entry.next; + prev.as_mut().next = next; + next.as_mut().prev = prev; + entry.next = NonNull::dangling(); + entry.prev = NonNull::dangling(); + } } #[cfg(test)] @@ -354,6 +412,51 @@ mod unsafe_list { } } + #[test] + fn push_remove() { + unsafe { + let mut node = UnsafeListEntry::new(1234); + let mut list = UnsafeList::new(); + assert_eq!(list.push(&mut node), &1234); + list.remove(&mut node); + assert_empty(&mut list); + } + } + + #[test] + fn push_remove_pop() { + unsafe { + let mut node1 = UnsafeListEntry::new(11); + let mut node2 = UnsafeListEntry::new(12); + let mut node3 = UnsafeListEntry::new(13); + let mut node4 = UnsafeListEntry::new(14); + let mut node5 = UnsafeListEntry::new(15); + let mut list = UnsafeList::new(); + assert_eq!(list.push(&mut node1), &11); + assert_eq!(list.push(&mut node2), &12); + assert_eq!(list.push(&mut node3), &13); + assert_eq!(list.push(&mut node4), &14); + assert_eq!(list.push(&mut node5), &15); + + list.remove(&mut node1); + assert_eq!(list.pop().unwrap(), &12); + list.remove(&mut node3); + assert_eq!(list.pop().unwrap(), &14); + list.remove(&mut node5); + assert_empty(&mut list); + + assert_eq!(list.push(&mut node1), &11); + assert_eq!(list.pop().unwrap(), &11); + assert_empty(&mut list); + + assert_eq!(list.push(&mut node3), &13); + assert_eq!(list.push(&mut node4), &14); + list.remove(&mut node3); + list.remove(&mut node4); + assert_empty(&mut list); + } + } + #[test] fn complex_pushes_pops() { unsafe { @@ -474,7 +577,7 @@ mod spin_mutex { use super::*; use crate::sync::Arc; use crate::thread; - use crate::time::{Duration, SystemTime}; + use crate::time::Duration; #[test] fn sleep() { @@ -485,11 +588,7 @@ mod spin_mutex { *mutex2.lock() = 1; }); - // "sleep" for 50ms - // FIXME: https://github.com/fortanix/rust-sgx/issues/31 - let start = SystemTime::now(); - let max = Duration::from_millis(50); - while start.elapsed().unwrap() < max {} + thread::sleep(Duration::from_millis(50)); assert_eq!(*guard, 0); drop(guard); diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index 3134a5967566b..9bcb89d81443d 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -1743,7 +1743,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn test_park_timeout_unpark_not_called() { for _ in 0..10 { thread::park_timeout(Duration::from_millis(10)); @@ -1751,7 +1750,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn test_park_timeout_unpark_called_other_thread() { for _ in 0..10 { let th = thread::current(); @@ -1766,7 +1764,6 @@ mod tests { } #[test] - #[cfg_attr(target_env = "sgx", ignore)] // FIXME: https://github.com/fortanix/rust-sgx/issues/31 fn sleep_ms_smoke() { thread::sleep(Duration::from_millis(2)); } From d7dc64bdfea4fbf8974774800ab51e04eaa4f082 Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Fri, 12 Jun 2020 12:06:41 -0700 Subject: [PATCH 2/7] Handle spurious wakeups in wait_timeout_sgx --- src/libstd/sys/sgx/mod.rs | 16 ++++++++++++---- src/libstd/sys/sgx/thread.rs | 2 +- src/libstd/sys/sgx/waitqueue.rs | 2 +- 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/libstd/sys/sgx/mod.rs b/src/libstd/sys/sgx/mod.rs index dff9d3b1f07ea..b72e4fb06f35b 100644 --- a/src/libstd/sys/sgx/mod.rs +++ b/src/libstd/sys/sgx/mod.rs @@ -115,9 +115,15 @@ pub fn decode_error_kind(code: i32) -> ErrorKind { // timeouts in SGX model. The enclave runner serving usercalls may lie about // current time and/or ignore timeout values. // +// Once the event is observed, `stop` will be used to determine whether or not +// we should continue to wait. +// // FIXME: note these caveats in documentation of all public types that use this // function in their execution path. -pub fn wait_timeout_sgx(event_mask: u64, duration: crate::time::Duration) { +pub fn wait_timeout_sgx(event_mask: u64, duration: crate::time::Duration, stop: F) +where + F: Fn() -> bool, +{ use self::abi::usercalls; use crate::cmp; use crate::io::ErrorKind; @@ -129,11 +135,13 @@ pub fn wait_timeout_sgx(event_mask: u64, duration: crate::time::Duration) { let timeout = cmp::min((u64::MAX - 1) as u128, remaining.as_nanos()) as u64; match usercalls::wait(event_mask, timeout) { Ok(eventset) => { - if event_mask != 0 { - rtassert!(eventset & event_mask == event_mask); + if event_mask == 0 { + rtabort!("expected usercalls::wait() to return Err, found Ok."); + } + rtassert!(eventset & event_mask == event_mask); + if stop() { return; } - rtabort!("expected usercalls::wait() to return Err, found Ok."); } Err(e) => { rtassert!(e.kind() == ErrorKind::TimedOut || e.kind() == ErrorKind::WouldBlock) diff --git a/src/libstd/sys/sgx/thread.rs b/src/libstd/sys/sgx/thread.rs index 8ff0f1fde91c2..5636a6f7eabde 100644 --- a/src/libstd/sys/sgx/thread.rs +++ b/src/libstd/sys/sgx/thread.rs @@ -76,7 +76,7 @@ impl Thread { } pub fn sleep(dur: Duration) { - wait_timeout_sgx(0, dur); + wait_timeout_sgx(0, dur, || true); } pub fn join(self) { diff --git a/src/libstd/sys/sgx/waitqueue.rs b/src/libstd/sys/sgx/waitqueue.rs index 71d1c22cd6191..36b3f5bcc41d8 100644 --- a/src/libstd/sys/sgx/waitqueue.rs +++ b/src/libstd/sys/sgx/waitqueue.rs @@ -177,7 +177,7 @@ impl WaitQueue { let entry_lock = lock.lock().queue.inner.push(&mut entry); before_wait(); // don't panic, this would invalidate `entry` during unwinding - wait_timeout_sgx(EV_UNPARK, timeout); + wait_timeout_sgx(EV_UNPARK, timeout, || entry_lock.lock().wake); // acquire the wait queue's lock first to avoid deadlock. let mut guard = lock.lock(); let entry_guard = entry_lock.lock(); From c5d1fcd2309b6903fed82aba6e0fdc2fa85bc874 Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Fri, 12 Jun 2020 13:41:46 -0700 Subject: [PATCH 3/7] Allow more ui tests for SGX --- src/test/ui/issues/issue-59020.rs | 1 - src/test/ui/issues/issue-9396.rs | 1 - src/test/ui/mpsc_stress.rs | 2 +- src/test/ui/sleep.rs | 1 - src/test/ui/tcp-stress.rs | 1 - 5 files changed, 1 insertion(+), 5 deletions(-) diff --git a/src/test/ui/issues/issue-59020.rs b/src/test/ui/issues/issue-59020.rs index e7544934da0c8..a2b11764a2fc6 100644 --- a/src/test/ui/issues/issue-59020.rs +++ b/src/test/ui/issues/issue-59020.rs @@ -1,7 +1,6 @@ // edition:2018 // run-pass // ignore-emscripten no threads support -// ignore-sgx no thread sleep support use std::thread; use std::time::Duration; diff --git a/src/test/ui/issues/issue-9396.rs b/src/test/ui/issues/issue-9396.rs index 27b5185377d97..3e7e9a51cdd3a 100644 --- a/src/test/ui/issues/issue-9396.rs +++ b/src/test/ui/issues/issue-9396.rs @@ -2,7 +2,6 @@ #![allow(unused_must_use)] #![allow(deprecated)] // ignore-emscripten no threads support -// ignore-sgx no thread sleep support use std::sync::mpsc::{TryRecvError, channel}; use std::thread; diff --git a/src/test/ui/mpsc_stress.rs b/src/test/ui/mpsc_stress.rs index bce5fdcd119f7..81c000839bd4d 100644 --- a/src/test/ui/mpsc_stress.rs +++ b/src/test/ui/mpsc_stress.rs @@ -1,7 +1,6 @@ // run-pass // compile-flags:--test // ignore-emscripten -// ignore-sgx no thread sleep support use std::sync::mpsc::channel; use std::sync::mpsc::TryRecvError; @@ -37,6 +36,7 @@ impl Barrier { fn wait(self) { self.shared.fetch_add(1, Ordering::SeqCst); while self.shared.load(Ordering::SeqCst) != self.count { + thread::yield_now(); } } } diff --git a/src/test/ui/sleep.rs b/src/test/ui/sleep.rs index 757578b84750c..3b3a4a4f3250c 100644 --- a/src/test/ui/sleep.rs +++ b/src/test/ui/sleep.rs @@ -1,6 +1,5 @@ // run-pass // ignore-emscripten no threads support -// ignore-sgx no thread sleep support use std::thread::{self, sleep}; use std::time::Duration; diff --git a/src/test/ui/tcp-stress.rs b/src/test/ui/tcp-stress.rs index 1f1948fa8fa2c..08b47dc531857 100644 --- a/src/test/ui/tcp-stress.rs +++ b/src/test/ui/tcp-stress.rs @@ -4,7 +4,6 @@ // ignore-emscripten no threads or sockets support // ignore-netbsd system ulimit (Too many open files) // ignore-openbsd system ulimit (Too many open files) -// ignore-sgx no thread sleep support use std::io::prelude::*; use std::net::{TcpListener, TcpStream}; From 3442d23c1a12f1f01a0e07b6bec72b58998f49ef Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Thu, 18 Jun 2020 12:50:10 -0700 Subject: [PATCH 4/7] Improve wait_timeout_sgx, simplify usercalls::wait --- src/libstd/sys/sgx/abi/usercalls/mod.rs | 17 +++---- src/libstd/sys/sgx/mod.rs | 67 ++++++++++++++++++------- 2 files changed, 56 insertions(+), 28 deletions(-) diff --git a/src/libstd/sys/sgx/abi/usercalls/mod.rs b/src/libstd/sys/sgx/abi/usercalls/mod.rs index b223da9d7e4a2..69ff7ebf9a19d 100644 --- a/src/libstd/sys/sgx/abi/usercalls/mod.rs +++ b/src/libstd/sys/sgx/abi/usercalls/mod.rs @@ -1,4 +1,5 @@ use crate::cmp; +use crate::convert::TryFrom; use crate::io::{Error as IoError, IoSlice, IoSliceMut, Result as IoResult}; use crate::sys::rand::rdrand64; use crate::time::Duration; @@ -159,17 +160,11 @@ pub fn wait(event_mask: u64, mut timeout: u64) -> IoResult { // to make things work in other cases. Note that in the SGX threat // model the enclave runner which is serving the wait usercall is not // trusted to ensure accurate timeouts. - let base = cmp::max(1, timeout / 10) * 2 + 1; - let zero = base / 2; - match rdrand64() % base { - jitter if jitter > zero => { - timeout = timeout.checked_add(jitter - zero).unwrap_or(timeout) - } - jitter if jitter < zero => { - timeout = timeout.checked_sub(zero - jitter).unwrap_or(timeout) - } - _ => {} - }; + if let Ok(timeout_signed) = i64::try_from(timeout) { + let tenth = 1 + timeout_signed / 10; + let deviation = (rdrand64() as i64).checked_rem(tenth).unwrap_or(0); + timeout = timeout_signed.saturating_add(deviation) as _; + } timeout = cmp::min(u64::MAX - 1, cmp::max(1, timeout)); } unsafe { raw::wait(event_mask, timeout).from_sgx_result() } diff --git a/src/libstd/sys/sgx/mod.rs b/src/libstd/sys/sgx/mod.rs index b72e4fb06f35b..1c957d8ff8032 100644 --- a/src/libstd/sys/sgx/mod.rs +++ b/src/libstd/sys/sgx/mod.rs @@ -110,43 +110,76 @@ pub fn decode_error_kind(code: i32) -> ErrorKind { } } -// This function makes an effort to sleep at least as long as `duration`. -// Note that in general there is no guarantee about accuracy of time and -// timeouts in SGX model. The enclave runner serving usercalls may lie about -// current time and/or ignore timeout values. +// This function makes an effort to wait for a non-spurious event at least as +// long as `duration`. Note that in general there is no guarantee about accuracy +// of time and timeouts in SGX model. The enclave runner serving usercalls may +// lie about current time and/or ignore timeout values. // -// Once the event is observed, `stop` will be used to determine whether or not -// we should continue to wait. +// Once the event is observed, `woken_up` will be used to determine whether or +// not the event was spurious. // // FIXME: note these caveats in documentation of all public types that use this // function in their execution path. -pub fn wait_timeout_sgx(event_mask: u64, duration: crate::time::Duration, stop: F) +pub fn wait_timeout_sgx(event_mask: u64, duration: crate::time::Duration, woken_up: F) where F: Fn() -> bool, { use self::abi::usercalls; use crate::cmp; use crate::io::ErrorKind; - use crate::time::Instant; - - let start = Instant::now(); - let mut remaining = duration; - loop { - let timeout = cmp::min((u64::MAX - 1) as u128, remaining.as_nanos()) as u64; + use crate::time::{Duration, Instant}; + + // Calls the wait usercall and checks the result. Returns true if event was + // returned, and false if WouldBlock/TimedOut was returned. + // If duration is None, it will use WAIT_NO. + fn wait_checked(event_mask: u64, duration: Option) -> bool { + let timeout = duration.map_or(usercalls::raw::WAIT_NO, |duration| { + cmp::min((u64::MAX - 1) as u128, duration.as_nanos()) as u64 + }); match usercalls::wait(event_mask, timeout) { Ok(eventset) => { if event_mask == 0 { rtabort!("expected usercalls::wait() to return Err, found Ok."); } rtassert!(eventset & event_mask == event_mask); - if stop() { - return; - } + true } Err(e) => { - rtassert!(e.kind() == ErrorKind::TimedOut || e.kind() == ErrorKind::WouldBlock) + rtassert!(e.kind() == ErrorKind::TimedOut || e.kind() == ErrorKind::WouldBlock); + false } } + } + + match wait_checked(event_mask, Some(duration)) { + false => return, // timed out + true if woken_up() => return, // woken up + true => {} // spurious event + } + + // Drain all cached events. + // Note that `event_mask != 0` is implied if we get here. + loop { + match wait_checked(event_mask, None) { + false => break, // no more cached events + true if woken_up() => return, // woken up + true => {} // spurious event + } + } + + // Continue waiting, but take note of time spent waiting so we don't wait + // forever. We intentionally don't call `Instant::now()` before this point + // to avoid the cost of the `insecure_time` usercall in case there are no + // spurious wakeups. + + let start = Instant::now(); + let mut remaining = duration; + loop { + match wait_checked(event_mask, Some(remaining)) { + false => return, // timed out + true if woken_up() => return, // woken up + true => {} // spurious event + } remaining = match duration.checked_sub(start.elapsed()) { Some(remaining) => remaining, None => break, From c457b67af394c37826f75d73cca10319ee96b910 Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Wed, 1 Jul 2020 12:45:11 -0700 Subject: [PATCH 5/7] Remove unnecessary check in SGX wait usercall --- src/libstd/sys/sgx/abi/usercalls/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libstd/sys/sgx/abi/usercalls/mod.rs b/src/libstd/sys/sgx/abi/usercalls/mod.rs index 69ff7ebf9a19d..6ee147d1704ad 100644 --- a/src/libstd/sys/sgx/abi/usercalls/mod.rs +++ b/src/libstd/sys/sgx/abi/usercalls/mod.rs @@ -161,11 +161,10 @@ pub fn wait(event_mask: u64, mut timeout: u64) -> IoResult { // model the enclave runner which is serving the wait usercall is not // trusted to ensure accurate timeouts. if let Ok(timeout_signed) = i64::try_from(timeout) { - let tenth = 1 + timeout_signed / 10; + let tenth = timeout_signed / 10; let deviation = (rdrand64() as i64).checked_rem(tenth).unwrap_or(0); timeout = timeout_signed.saturating_add(deviation) as _; } - timeout = cmp::min(u64::MAX - 1, cmp::max(1, timeout)); } unsafe { raw::wait(event_mask, timeout).from_sgx_result() } } From 1466598e19321bc6b97aef8271a317e78211d54d Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Fri, 10 Jul 2020 19:57:31 -0700 Subject: [PATCH 6/7] Address review comments --- src/libstd/sys/sgx/mod.rs | 31 +++++++++++++-------------- src/libstd/sys/sgx/thread.rs | 5 ++--- src/libstd/sys/sgx/waitqueue.rs | 37 +++++++++++++++------------------ src/test/ui/mpsc_stress.rs | 1 + 4 files changed, 35 insertions(+), 39 deletions(-) diff --git a/src/libstd/sys/sgx/mod.rs b/src/libstd/sys/sgx/mod.rs index 1c957d8ff8032..c412053112bc5 100644 --- a/src/libstd/sys/sgx/mod.rs +++ b/src/libstd/sys/sgx/mod.rs @@ -115,12 +115,9 @@ pub fn decode_error_kind(code: i32) -> ErrorKind { // of time and timeouts in SGX model. The enclave runner serving usercalls may // lie about current time and/or ignore timeout values. // -// Once the event is observed, `woken_up` will be used to determine whether or -// not the event was spurious. -// -// FIXME: note these caveats in documentation of all public types that use this -// function in their execution path. -pub fn wait_timeout_sgx(event_mask: u64, duration: crate::time::Duration, woken_up: F) +// Once the event is observed, `should_wake_up` will be used to determine +// whether or not the event was spurious. +pub fn usercall_wait_timeout(event_mask: u64, duration: crate::time::Duration, should_wake_up: F) where F: Fn() -> bool, { @@ -141,7 +138,9 @@ where if event_mask == 0 { rtabort!("expected usercalls::wait() to return Err, found Ok."); } - rtassert!(eventset & event_mask == event_mask); + // A matching event is one whose bits are equal to or a subset + // of `event_mask`. + rtassert!(eventset & !event_mask == 0); true } Err(e) => { @@ -152,18 +151,18 @@ where } match wait_checked(event_mask, Some(duration)) { - false => return, // timed out - true if woken_up() => return, // woken up - true => {} // spurious event + false => return, // timed out + true if should_wake_up() => return, // woken up + true => {} // spurious event } // Drain all cached events. // Note that `event_mask != 0` is implied if we get here. loop { match wait_checked(event_mask, None) { - false => break, // no more cached events - true if woken_up() => return, // woken up - true => {} // spurious event + false => break, // no more cached events + true if should_wake_up() => return, // woken up + true => {} // spurious event } } @@ -176,9 +175,9 @@ where let mut remaining = duration; loop { match wait_checked(event_mask, Some(remaining)) { - false => return, // timed out - true if woken_up() => return, // woken up - true => {} // spurious event + false => return, // timed out + true if should_wake_up() => return, // woken up + true => {} // spurious event } remaining = match duration.checked_sub(start.elapsed()) { Some(remaining) => remaining, diff --git a/src/libstd/sys/sgx/thread.rs b/src/libstd/sys/sgx/thread.rs index 5636a6f7eabde..58b6f4346bc14 100644 --- a/src/libstd/sys/sgx/thread.rs +++ b/src/libstd/sys/sgx/thread.rs @@ -1,8 +1,7 @@ #![cfg_attr(test, allow(dead_code))] // why is this necessary? - use crate::ffi::CStr; use crate::io; -use crate::sys::wait_timeout_sgx; +use crate::sys::usercall_wait_timeout; use crate::time::Duration; use super::abi::usercalls; @@ -76,7 +75,7 @@ impl Thread { } pub fn sleep(dur: Duration) { - wait_timeout_sgx(0, dur, || true); + usercall_wait_timeout(0, dur, || true); } pub fn join(self) { diff --git a/src/libstd/sys/sgx/waitqueue.rs b/src/libstd/sys/sgx/waitqueue.rs index 36b3f5bcc41d8..c8ccab2247a9c 100644 --- a/src/libstd/sys/sgx/waitqueue.rs +++ b/src/libstd/sys/sgx/waitqueue.rs @@ -1,17 +1,17 @@ -/// A simple queue implementation for synchronization primitives. -/// -/// This queue is used to implement condition variable and mutexes. -/// -/// Users of this API are expected to use the `WaitVariable` type. Since -/// that type is not `Sync`, it needs to be protected by e.g., a `SpinMutex` to -/// allow shared access. -/// -/// Since userspace may send spurious wake-ups, the wakeup event state is -/// recorded in the enclave. The wakeup event state is protected by a spinlock. -/// The queue and associated wait state are stored in a `WaitVariable`. +//! A simple queue implementation for synchronization primitives. +//! +//! This queue is used to implement condition variable and mutexes. +//! +//! Users of this API are expected to use the `WaitVariable` type. Since +//! that type is not `Sync`, it needs to be protected by e.g., a `SpinMutex` to +//! allow shared access. +//! +//! Since userspace may send spurious wake-ups, the wakeup event state is +//! recorded in the enclave. The wakeup event state is protected by a spinlock. +//! The queue and associated wait state are stored in a `WaitVariable`. use crate::num::NonZeroUsize; use crate::ops::{Deref, DerefMut}; -use crate::sys::wait_timeout_sgx; +use crate::sys::usercall_wait_timeout; use crate::time::Duration; use super::abi::thread; @@ -176,15 +176,12 @@ impl WaitQueue { })); let entry_lock = lock.lock().queue.inner.push(&mut entry); before_wait(); - // don't panic, this would invalidate `entry` during unwinding - wait_timeout_sgx(EV_UNPARK, timeout, || entry_lock.lock().wake); + usercall_wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake); // acquire the wait queue's lock first to avoid deadlock. let mut guard = lock.lock(); - let entry_guard = entry_lock.lock(); - let success = entry_guard.wake; + let success = entry_lock.lock().wake; if !success { - // nobody is waking us up, so remove the entry from the wait queue. - drop(entry_guard); + // nobody is waking us up, so remove our entry from the wait queue. guard.queue.inner.remove(&mut entry); } success @@ -363,8 +360,8 @@ mod unsafe_list { /// /// # Safety /// - /// The caller must ensure that entry has been pushed prior to this - /// call and has not moved since push. + /// The caller must ensure that `entry` has been pushed onto `self` + /// prior to this call and has not moved since then. pub unsafe fn remove(&mut self, entry: &mut UnsafeListEntry) { rtassert!(!self.is_empty()); // BEFORE: diff --git a/src/test/ui/mpsc_stress.rs b/src/test/ui/mpsc_stress.rs index 81c000839bd4d..a889542fec0be 100644 --- a/src/test/ui/mpsc_stress.rs +++ b/src/test/ui/mpsc_stress.rs @@ -36,6 +36,7 @@ impl Barrier { fn wait(self) { self.shared.fetch_add(1, Ordering::SeqCst); while self.shared.load(Ordering::SeqCst) != self.count { + #[cfg(target_env = "sgx")] thread::yield_now(); } } From 85c25aed510ce599504b172f7c7bef280e91637b Mon Sep 17 00:00:00 2001 From: Mohsen Zohrevandi Date: Wed, 15 Jul 2020 15:48:36 -0700 Subject: [PATCH 7/7] Move usercall_wait_timeout to abi::usercalls::wait_timeout --- src/libstd/sys/sgx/abi/usercalls/mod.rs | 74 +++++++++++++++++++++++- src/libstd/sys/sgx/mod.rs | 76 ------------------------- src/libstd/sys/sgx/thread.rs | 3 +- src/libstd/sys/sgx/waitqueue.rs | 3 +- 4 files changed, 74 insertions(+), 82 deletions(-) diff --git a/src/libstd/sys/sgx/abi/usercalls/mod.rs b/src/libstd/sys/sgx/abi/usercalls/mod.rs index 6ee147d1704ad..73f1b951e7430 100644 --- a/src/libstd/sys/sgx/abi/usercalls/mod.rs +++ b/src/libstd/sys/sgx/abi/usercalls/mod.rs @@ -1,8 +1,8 @@ use crate::cmp; use crate::convert::TryFrom; -use crate::io::{Error as IoError, IoSlice, IoSliceMut, Result as IoResult}; +use crate::io::{Error as IoError, ErrorKind, IoSlice, IoSliceMut, Result as IoResult}; use crate::sys::rand::rdrand64; -use crate::time::Duration; +use crate::time::{Duration, Instant}; pub(crate) mod alloc; #[macro_use] @@ -169,6 +169,76 @@ pub fn wait(event_mask: u64, mut timeout: u64) -> IoResult { unsafe { raw::wait(event_mask, timeout).from_sgx_result() } } +/// This function makes an effort to wait for a non-spurious event at least as +/// long as `duration`. Note that in general there is no guarantee about accuracy +/// of time and timeouts in SGX model. The enclave runner serving usercalls may +/// lie about current time and/or ignore timeout values. +/// +/// Once the event is observed, `should_wake_up` will be used to determine +/// whether or not the event was spurious. +#[unstable(feature = "sgx_platform", issue = "56975")] +pub fn wait_timeout(event_mask: u64, duration: Duration, should_wake_up: F) +where + F: Fn() -> bool, +{ + // Calls the wait usercall and checks the result. Returns true if event was + // returned, and false if WouldBlock/TimedOut was returned. + // If duration is None, it will use WAIT_NO. + fn wait_checked(event_mask: u64, duration: Option) -> bool { + let timeout = duration.map_or(raw::WAIT_NO, |duration| { + cmp::min((u64::MAX - 1) as u128, duration.as_nanos()) as u64 + }); + match wait(event_mask, timeout) { + Ok(eventset) => { + if event_mask == 0 { + rtabort!("expected wait() to return Err, found Ok."); + } + rtassert!(eventset != 0 && eventset & !event_mask == 0); + true + } + Err(e) => { + rtassert!(e.kind() == ErrorKind::TimedOut || e.kind() == ErrorKind::WouldBlock); + false + } + } + } + + match wait_checked(event_mask, Some(duration)) { + false => return, // timed out + true if should_wake_up() => return, // woken up + true => {} // spurious event + } + + // Drain all cached events. + // Note that `event_mask != 0` is implied if we get here. + loop { + match wait_checked(event_mask, None) { + false => break, // no more cached events + true if should_wake_up() => return, // woken up + true => {} // spurious event + } + } + + // Continue waiting, but take note of time spent waiting so we don't wait + // forever. We intentionally don't call `Instant::now()` before this point + // to avoid the cost of the `insecure_time` usercall in case there are no + // spurious wakeups. + + let start = Instant::now(); + let mut remaining = duration; + loop { + match wait_checked(event_mask, Some(remaining)) { + false => return, // timed out + true if should_wake_up() => return, // woken up + true => {} // spurious event + } + remaining = match duration.checked_sub(start.elapsed()) { + Some(remaining) => remaining, + None => break, + } + } +} + /// Usercall `send`. See the ABI documentation for more information. #[unstable(feature = "sgx_platform", issue = "56975")] pub fn send(event_set: u64, tcs: Option) -> IoResult<()> { diff --git a/src/libstd/sys/sgx/mod.rs b/src/libstd/sys/sgx/mod.rs index c412053112bc5..7a3a3eb2049b3 100644 --- a/src/libstd/sys/sgx/mod.rs +++ b/src/libstd/sys/sgx/mod.rs @@ -110,82 +110,6 @@ pub fn decode_error_kind(code: i32) -> ErrorKind { } } -// This function makes an effort to wait for a non-spurious event at least as -// long as `duration`. Note that in general there is no guarantee about accuracy -// of time and timeouts in SGX model. The enclave runner serving usercalls may -// lie about current time and/or ignore timeout values. -// -// Once the event is observed, `should_wake_up` will be used to determine -// whether or not the event was spurious. -pub fn usercall_wait_timeout(event_mask: u64, duration: crate::time::Duration, should_wake_up: F) -where - F: Fn() -> bool, -{ - use self::abi::usercalls; - use crate::cmp; - use crate::io::ErrorKind; - use crate::time::{Duration, Instant}; - - // Calls the wait usercall and checks the result. Returns true if event was - // returned, and false if WouldBlock/TimedOut was returned. - // If duration is None, it will use WAIT_NO. - fn wait_checked(event_mask: u64, duration: Option) -> bool { - let timeout = duration.map_or(usercalls::raw::WAIT_NO, |duration| { - cmp::min((u64::MAX - 1) as u128, duration.as_nanos()) as u64 - }); - match usercalls::wait(event_mask, timeout) { - Ok(eventset) => { - if event_mask == 0 { - rtabort!("expected usercalls::wait() to return Err, found Ok."); - } - // A matching event is one whose bits are equal to or a subset - // of `event_mask`. - rtassert!(eventset & !event_mask == 0); - true - } - Err(e) => { - rtassert!(e.kind() == ErrorKind::TimedOut || e.kind() == ErrorKind::WouldBlock); - false - } - } - } - - match wait_checked(event_mask, Some(duration)) { - false => return, // timed out - true if should_wake_up() => return, // woken up - true => {} // spurious event - } - - // Drain all cached events. - // Note that `event_mask != 0` is implied if we get here. - loop { - match wait_checked(event_mask, None) { - false => break, // no more cached events - true if should_wake_up() => return, // woken up - true => {} // spurious event - } - } - - // Continue waiting, but take note of time spent waiting so we don't wait - // forever. We intentionally don't call `Instant::now()` before this point - // to avoid the cost of the `insecure_time` usercall in case there are no - // spurious wakeups. - - let start = Instant::now(); - let mut remaining = duration; - loop { - match wait_checked(event_mask, Some(remaining)) { - false => return, // timed out - true if should_wake_up() => return, // woken up - true => {} // spurious event - } - remaining = match duration.checked_sub(start.elapsed()) { - Some(remaining) => remaining, - None => break, - } - } -} - // This enum is used as the storage for a bunch of types which can't actually // exist. #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Debug, Hash)] diff --git a/src/libstd/sys/sgx/thread.rs b/src/libstd/sys/sgx/thread.rs index 58b6f4346bc14..5895f70436efa 100644 --- a/src/libstd/sys/sgx/thread.rs +++ b/src/libstd/sys/sgx/thread.rs @@ -1,7 +1,6 @@ #![cfg_attr(test, allow(dead_code))] // why is this necessary? use crate::ffi::CStr; use crate::io; -use crate::sys::usercall_wait_timeout; use crate::time::Duration; use super::abi::usercalls; @@ -75,7 +74,7 @@ impl Thread { } pub fn sleep(dur: Duration) { - usercall_wait_timeout(0, dur, || true); + usercalls::wait_timeout(0, dur, || true); } pub fn join(self) { diff --git a/src/libstd/sys/sgx/waitqueue.rs b/src/libstd/sys/sgx/waitqueue.rs index c8ccab2247a9c..070afa55f3019 100644 --- a/src/libstd/sys/sgx/waitqueue.rs +++ b/src/libstd/sys/sgx/waitqueue.rs @@ -11,7 +11,6 @@ //! The queue and associated wait state are stored in a `WaitVariable`. use crate::num::NonZeroUsize; use crate::ops::{Deref, DerefMut}; -use crate::sys::usercall_wait_timeout; use crate::time::Duration; use super::abi::thread; @@ -176,7 +175,7 @@ impl WaitQueue { })); let entry_lock = lock.lock().queue.inner.push(&mut entry); before_wait(); - usercall_wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake); + usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake); // acquire the wait queue's lock first to avoid deadlock. let mut guard = lock.lock(); let success = entry_lock.lock().wake;