Auto merge of rust-lang#2228 - cbeuw:futex-fix, r=RalfJung

Prevent futex_wait from actually waiting if a concurrent waker was executed before us

Fixes rust-lang#2223

Two SC fences were placed in `futex_wake` (after the caller has changed `addr`), and in `futex_wait` (before we read `addr`). This guarantees that `futex_wait` sees the value written to `addr` before the last `futex_wake` call, should one exists, and avoid going into sleep with no one else to wake us up.
https://github.com/rust-lang/miri/blob/ada7b72a879d79aaa06f0a2a95edd520615da1a2/src/concurrency/weak_memory.rs#L324-L326

Earlier I proposed to use `fetch_add(0)` to read the latest value in MO, though this isn't the proper way to do it and breaks aliasing: syscall caller may pass in a `*const` from a `&` and Miri complains about write to a `SharedReadOnly` location, causing this test to fail.
https://github.com/rust-lang/miri/blob/ada7b72a879d79aaa06f0a2a95edd520615da1a2/tests/pass/concurrency/linux-futex.rs#L56-L68

src/shims/unix/linux/sync.rs

@@ -126,28 +126,63 @@ pub fn futex<'tcx>(
  Align::from_bytes(4).unwrap(),
  CheckInAllocMsg::MemoryAccessTest,
  )?;
+ // There may be a concurrent thread changing the value of addr
+ // and then invoking the FUTEX_WAKE syscall. It is critical that the
+ // effects of this and the other thread are correctly observed,
+ // otherwise we will deadlock.
+ //
+ // There are two scenarios to consider:
+ // 1. If we (FUTEX_WAIT) execute first, we'll push ourselves into
+ // the waiters queue and go to sleep. They (addr write & FUTEX_WAKE)
+ // will see us in the queue and wake us up.
+ // 2. If they (addr write & FUTEX_WAKE) execute first, we must observe
+ // addr's new value. If we see an outdated value that happens to equal
+ // the expected val, then we'll put ourselves to sleep with no one to wake us
+ // up, so we end up with a deadlock. This is prevented by having a SeqCst
+ // fence inside FUTEX_WAKE syscall, and another SeqCst fence
+ // below, the atomic read on addr after the SeqCst fence is guaranteed
+ // not to see any value older than the addr write immediately before
+ // calling FUTEX_WAKE. We'll see futex_val != val and return without
+ // sleeping.
+ //
+ // Note that the fences do not create any happens-before relationship.
+ // The read sees the write immediately before the fence not because
+ // one happens after the other, but is instead due to a guarantee unique
+ // to SeqCst fences that restricts what an atomic read placed AFTER the
+ // fence can see. The read still has to be atomic, otherwise it's a data
+ // race. This guarantee cannot be achieved with acquire-release fences
+ // since they only talk about reads placed BEFORE a fence - and places
+ // no restrictions on what the read itself can see, only that there is
+ // a happens-before between the fences IF the read happens to see the
+ // right value. This is useless to us, since we need the read itself
+ // to see an up-to-date value.
+ //
+ // The above case distinction is valid since both FUTEX_WAIT and FUTEX_WAKE
+ // contain a SeqCst fence, therefore inducting a total order between the operations.
+ // It is also critical that the fence, the atomic load, and the comparison in FUTEX_WAIT
+ // altogether happen atomically. If the other thread's fence in FUTEX_WAKE
+ // gets interleaved after our fence, then we lose the guarantee on the
+ // atomic load being up-to-date; if the other thread's write on addr and FUTEX_WAKE
+ // call are interleaved after the load but before the comparison, then we get a TOCTOU
+ // race condition, and go to sleep thinking the other thread will wake us up,
+ // even though they have already finished.
+ //
+ // Thankfully, preemptions cannot happen inside a Miri shim, so we do not need to
+ // do anything special to guarantee fence-load-comparison atomicity.
+ this.atomic_fence(&[], AtomicFenceOp::SeqCst)?;
  // Read an `i32` through the pointer, regardless of any wrapper types.
  // It's not uncommon for `addr` to be passed as another type than `*mut i32`, such as `*const AtomicI32`.
  // FIXME: this fails if `addr` is not a pointer type.
- // The atomic ordering for futex(https://man7.org/linux/man-pages/man2/futex.2.html):
- // "The load of the value of the futex word is an
- // atomic memory access (i.e., using atomic machine instructions
- // of the respective architecture). This load, the comparison
- // with the expected value, and starting to sleep are performed
- // atomically and totally ordered with respect to other futex
- // operations on the same futex word."
- // SeqCst is total order over all operations.
- // FIXME: check if this should be changed when weak memory orders are added.
  let futex_val = this
  .read_scalar_at_offset_atomic(
  &addr.into(),
  0,
  this.machine.layouts.i32,
- AtomicReadOp::SeqCst,
+ AtomicReadOp::Relaxed,
  )?
  .to_i32()?;
  if val == futex_val {
- // The value still matches, so we block the trait make it wait for FUTEX_WAKE.
+ // The value still matches, so we block the thread make it wait for FUTEX_WAKE.
  this.block_thread(thread);
  this.futex_wait(addr_usize, thread, bitset);
  // Succesfully waking up from FUTEX_WAIT always returns zero.
@@ -203,6 +238,10 @@ pub fn futex<'tcx>(
  this.write_scalar(Scalar::from_machine_isize(-1, this), dest)?;
  return Ok(());
  }
+ // Together with the SeqCst fence in futex_wait, this makes sure that futex_wait
+ // will see the latest value on addr which could be changed by our caller
+ // before doing the syscall.
+ this.atomic_fence(&[], AtomicFenceOp::SeqCst)?;
  let mut n = 0;
  for _ in 0..val {
  if let Some(thread) = this.futex_wake(addr_usize, bitset) {

tests/pass/concurrency/linux-futex.rs

@@ -6,6 +6,8 @@ extern crate libc;
 
 use std::mem::MaybeUninit;
 use std::ptr;
+use std::sync::atomic::AtomicI32;
+use std::sync::atomic::Ordering;
 use std::thread;
 use std::time::{Duration, Instant};
 
@@ -206,6 +208,47 @@ fn wait_wake_bitset() {
  t.join().unwrap();
 }
 
+const FREE: i32 = 0;
+const HELD: i32 = 1;
+fn concurrent_wait_wake() {
+ static FUTEX: AtomicI32 = AtomicI32::new(0);
+ for _ in 0..20 {
+ // Suppose the main thread is holding a lock implemented using futex...
+ FUTEX.store(HELD, Ordering::Relaxed);
+
+ let t = thread::spawn(move || {
+ // If this syscall runs first, then we'll be woken up by
+ // the main thread's FUTEX_WAKE, and all is fine.
+ //
+ // If this sycall runs after the main thread's store
+ // and FUTEX_WAKE, the syscall must observe that
+ // the FUTEX is FREE != HELD and return without waiting
+ // or we'll deadlock.
+ unsafe {
+ libc::syscall(
+ libc::SYS_futex,
+ &FUTEX as *const AtomicI32,
+ libc::FUTEX_WAIT,
+ HELD,
+ ptr::null::<libc::timespec>(),
+ );
+ }
+ });
+
+ FUTEX.store(FREE, Ordering::Relaxed);
+ unsafe {
+ libc::syscall(
+ libc::SYS_futex,
+ &FUTEX as *const AtomicI32,
+ libc::FUTEX_WAKE,
+ 1,
+ );
+ }
+
+ t.join().unwrap();
+ }
+}
+
 fn main() {
  wake_nobody();
  wake_dangling();
@@ -214,4 +257,5 @@ fn main() {
  wait_absolute_timeout();
  wait_wake();
  wait_wake_bitset();
+ concurrent_wait_wake();
 }