Skip to content
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

waitqueue clarifications for SGX platform #111595

Merged
merged 2 commits into from
Oct 6, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions library/std/src/sys/sgx/waitqueue/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ mod unsafe_list;

use crate::num::NonZeroUsize;
use crate::ops::{Deref, DerefMut};
use crate::panic::{self, AssertUnwindSafe};
use crate::time::Duration;

use super::abi::thread;
Expand Down Expand Up @@ -147,7 +148,8 @@ impl WaitQueue {
/// Adds the calling thread to the `WaitVariable`'s wait queue, then wait
/// until a wakeup event.
///
/// This function does not return until this thread has been awoken.
/// This function does not return until this thread has been awoken. When `before_wait` panics,
/// this function will abort.
pub fn wait<T, F: FnOnce()>(mut guard: SpinMutexGuard<'_, WaitVariable<T>>, before_wait: F) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does a panic in before_wait result in UB? If so can you make this function unsafe?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, can we catch a panic and rtabort instead?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is an option. Adding a drop guard (value whose drop impl aborts and which is forgotten after calling before_wait to prevent dropping it) is another option.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::thread::PanicGuard is an example of that.

// very unsafe: check requirements of UnsafeList::push
unsafe {
Expand All @@ -157,8 +159,13 @@ impl WaitQueue {
}));
let entry = guard.queue.inner.push(&mut entry);
drop(guard);
before_wait();
if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) {
rtabort!("Panic before wait on wakeup event")
}
while !entry.lock().wake {
// `entry.wake` is only set in `notify_one` and `notify_all` functions. Both ensure
// the entry is removed from the queue _before_ setting this bool. There are no
// other references to `entry`.
// don't panic, this would invalidate `entry` during unwinding
let eventset = rtunwrap!(Ok, usercalls::wait(EV_UNPARK, WAIT_INDEFINITE));
rtassert!(eventset & EV_UNPARK == EV_UNPARK);
Expand All @@ -169,6 +176,7 @@ 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.
/// When `before_wait` panics, this function will abort.
pub fn wait_timeout<T, F: FnOnce()>(
lock: &SpinMutex<WaitVariable<T>>,
timeout: Duration,
Expand All @@ -181,9 +189,13 @@ impl WaitQueue {
wake: false,
}));
let entry_lock = lock.lock().queue.inner.push(&mut entry);
before_wait();
if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) {
rtabort!("Panic before wait on wakeup event or timeout")
}
usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake);
// acquire the wait queue's lock first to avoid deadlock.
// acquire the wait queue's lock first to avoid deadlock
// and ensure no other function can simultaneously access the list
// (e.g., `notify_one` or `notify_all`)
let mut guard = lock.lock();
let success = entry_lock.lock().wake;
if !success {
Expand All @@ -204,8 +216,8 @@ impl WaitQueue {
) -> Result<WaitGuard<'_, T>, SpinMutexGuard<'_, WaitVariable<T>>> {
// SAFETY: lifetime of the pop() return value is limited to the map
// closure (The closure return value is 'static). The underlying
// stack frame won't be freed until after the WaitGuard created below
// is dropped.
// stack frame won't be freed until after the lock on the queue is released
// (i.e., `guard` is dropped).
unsafe {
let tcs = guard.queue.inner.pop().map(|entry| -> Tcs {
let mut entry_guard = entry.lock();
Expand All @@ -231,7 +243,7 @@ impl WaitQueue {
) -> Result<WaitGuard<'_, T>, SpinMutexGuard<'_, WaitVariable<T>>> {
// SAFETY: lifetime of the pop() return values are limited to the
// while loop body. The underlying stack frames won't be freed until
// after the WaitGuard created below is dropped.
// after the lock on the queue is released (i.e., `guard` is dropped).
unsafe {
let mut count = 0;
while let Some(entry) = guard.queue.inner.pop() {
Expand Down