From 3e089b0e16db2779de0c5ca7d54a462d78354fb7 Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 07:44:01 -0400 Subject: [PATCH 1/4] epoll: add data_race test This test demonstrates the need to synchronize the clock of the thread waking up from an epoll_wait from the thread that issued the epoll awake event. --- .../fail-dep/libc/libc-epoll-blocking.rs | 79 +++++++++++++++++++ .../fail-dep/libc/libc-epoll-blocking.stderr | 29 +++++++ 2 files changed, 108 insertions(+) create mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs create mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs new file mode 100644 index 000000000000..fd9d1875daf8 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs @@ -0,0 +1,79 @@ +//@only-target: linux +// test_epoll_race depends on a deterministic schedule. +//@compile-flags: -Zmiri-preemption-rate=0 + +use std::convert::TryInto; +use std::thread; + +fn main() { + test_epoll_race(); +} + +// Using `as` cast since `EPOLLET` wraps around +const EPOLL_IN_OUT_ET: u32 = (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _; + +#[track_caller] +fn check_epoll_wait( + epfd: i32, + expected_notifications: &[(u32, u64)], + timeout: i32, +) { + let epoll_event = libc::epoll_event { events: 0, u64: 0 }; + let mut array: [libc::epoll_event; N] = [epoll_event; N]; + let maxsize = N; + let array_ptr = array.as_mut_ptr(); + let res = unsafe { libc::epoll_wait(epfd, array_ptr, maxsize.try_into().unwrap(), timeout) }; + if res < 0 { + panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); + } + assert_eq!( + res, + expected_notifications.len().try_into().unwrap(), + "got wrong number of notifications" + ); + let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; + for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { + let event = return_event.events; + let data = return_event.u64; + assert_eq!(event, expected_event.0, "got wrong events"); + assert_eq!(data, expected_event.1, "got wrong data"); + } +} + +// This test shows a data_race before epoll had vector clocks added. +fn test_epoll_race() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create an eventfd instance. + let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; + let fd = unsafe { libc::eventfd(0, flags) }; + + // Register eventfd with the epoll instance. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; + assert_eq!(res, 0); + + static mut VAL: u8 = 0; + let thread1 = thread::spawn(move || { + // Write to the static mut variable. + unsafe { VAL = 1 }; + // Write to the eventfd instance. + let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; + // read returns number of bytes that have been read, which is always 8. + assert_eq!(res, 8); + }); + thread::yield_now(); + // epoll_wait for the event to happen. + let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); + let expected_value = u64::try_from(fd).unwrap(); + check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)], -1); + // Read from the static mut variable. + #[allow(static_mut_refs)] + unsafe { + assert_eq!(VAL, 1) //~ ERROR: Data race detected + }; + thread1.join().unwrap(); +} diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr new file mode 100644 index 000000000000..27974981cea7 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr @@ -0,0 +1,29 @@ +error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here + --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC + | +LL | assert_eq!(VAL, 1) + | ^^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here + | +help: and (1) occurred earlier here + --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC + | +LL | unsafe { VAL = 1 }; + | ^^^^^^^ + = help: retags occur on all (re)borrows and as well as when references are copied or moved + = help: retags permit optimizations that insert speculative reads or writes + = help: therefore from the perspective of data races, a retag has the same implications as a read or write + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE (of the first span): + = note: inside `test_epoll_race` at RUSTLIB/core/src/macros/mod.rs:LL:CC +note: inside `main` + --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC + | +LL | test_epoll_race(); + | ^^^^^^^^^^^^^^^^^ + = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + From 1b622f46728a51b6582b02b84407e5f64262479b Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 07:57:36 -0400 Subject: [PATCH 2/4] epoll: remove unnecessary instructions A couple of instructions were left over from an earlier rebase it would seem. They don't impact the logic but the ready_list type is about to change in the next commit. Rather than modify one of these lines in the commit that changes ready_list, only to have these lines removed later on, remove them now. They don't impact the tests results. --- src/tools/miri/src/shims/unix/linux/epoll.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index ff0709dd5ac1..13c44334d584 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -207,9 +207,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { ); } - let mut epoll_instance = Epoll::default(); - epoll_instance.ready_list = Rc::new(RefCell::new(BTreeMap::new())); - let fd = this.machine.fds.insert_new(Epoll::default()); Ok(Scalar::from_i32(fd)) } From 86bb1373aa94aec8d9d4c8fb692cd352eca01d90 Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 08:04:25 -0400 Subject: [PATCH 3/4] epoll: add vector clock to the epoll ready_list This adds a VClock to the epoll implementation's ready_list and has this VClock synced from the thread that updates an event in the ready_list and then has the VClocks of any threads being made runnable again, out of the calls to epoll_wait, synced from it. --- src/tools/miri/src/shims/unix/linux/epoll.rs | 37 ++++++--- .../fail-dep/libc/libc-epoll-blocking.rs | 79 ------------------- .../fail-dep/libc/libc-epoll-blocking.stderr | 29 ------- .../pass-dep/libc/libc-epoll-blocking.rs | 41 +++++++++- 4 files changed, 67 insertions(+), 119 deletions(-) delete mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs delete mode 100644 src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 13c44334d584..88fab8aa010a 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -4,6 +4,7 @@ use std::io; use std::rc::{Rc, Weak}; use std::time::Duration; +use crate::concurrency::VClock; use crate::shims::unix::fd::{FdId, FileDescriptionRef, WeakFileDescriptionRef}; use crate::shims::unix::*; use crate::*; @@ -19,7 +20,7 @@ struct Epoll { /// and file descriptor value. // This is an Rc because EpollInterest need to hold a reference to update // it. - ready_list: Rc>>, + ready_list: Rc, /// A list of thread ids blocked on this epoll instance. thread_id: RefCell>, } @@ -63,7 +64,7 @@ pub struct EpollEventInterest { /// data: u64, /// Ready list of the epoll instance under which this EpollEventInterest is registered. - ready_list: Rc>>, + ready_list: Rc, /// The epoll file description that this EpollEventInterest is registered under. weak_epfd: WeakFileDescriptionRef, } @@ -88,6 +89,12 @@ pub struct EpollReadyEvents { pub epollerr: bool, } +#[derive(Debug, Default)] +struct ReadyList { + mapping: RefCell>, + clock: RefCell, +} + impl EpollReadyEvents { pub fn new() -> Self { EpollReadyEvents { @@ -127,7 +134,7 @@ impl EpollReadyEvents { } impl Epoll { - fn get_ready_list(&self) -> Rc>> { + fn get_ready_list(&self) -> Rc { Rc::clone(&self.ready_list) } } @@ -374,7 +381,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { drop(epoll_interest); // Remove related epoll_interest from ready list. - ready_list.borrow_mut().remove(&epoll_key); + ready_list.mapping.borrow_mut().remove(&epoll_key); // Remove dangling EpollEventInterest from its global table. // .unwrap() below should succeed because the file description id must have registered @@ -469,7 +476,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { .downcast::() .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; let binding = epoll_file_description.get_ready_list(); - ready_list_empty = binding.borrow_mut().is_empty(); + ready_list_empty = binding.mapping.borrow_mut().is_empty(); thread_ids = epoll_file_description.thread_id.borrow_mut(); } if timeout == 0 || !ready_list_empty { @@ -558,9 +565,15 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { // holds a strong ref to epoll_interest. let epfd = epoll_interest.borrow().weak_epfd.upgrade().unwrap(); // FIXME: We can randomly pick a thread to unblock. - if let Some(thread_id) = - epfd.downcast::().unwrap().thread_id.borrow_mut().pop() - { + + let epoll = epfd.downcast::().unwrap(); + + // Synchronize running thread to the epoll ready list. + if let Some(clock) = &this.release_clock() { + epoll.ready_list.clock.borrow_mut().join(clock); + } + + if let Some(thread_id) = epoll.thread_id.borrow_mut().pop() { waiter.push(thread_id); }; } @@ -614,7 +627,7 @@ fn check_and_update_one_event_interest<'tcx>( // insert an epoll_return to the ready list. if flags != 0 { let epoll_key = (id, epoll_event_interest.fd_num); - let ready_list = &mut epoll_event_interest.ready_list.borrow_mut(); + let ready_list = &mut epoll_event_interest.ready_list.mapping.borrow_mut(); let event_instance = EpollEventInstance::new(flags, epoll_event_interest.data); // Triggers the notification by inserting it to the ready list. ready_list.insert(epoll_key, event_instance); @@ -641,7 +654,11 @@ fn blocking_epoll_callback<'tcx>( .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; let ready_list = epoll_file_description.get_ready_list(); - let mut ready_list = ready_list.borrow_mut(); + + // Synchronize waking thread from the epoll ready list. + ecx.acquire_clock(&ready_list.clock.borrow()); + + let mut ready_list = ready_list.mapping.borrow_mut(); let mut num_of_events: i32 = 0; let mut array_iter = ecx.project_array_fields(events)?; diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs deleted file mode 100644 index fd9d1875daf8..000000000000 --- a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.rs +++ /dev/null @@ -1,79 +0,0 @@ -//@only-target: linux -// test_epoll_race depends on a deterministic schedule. -//@compile-flags: -Zmiri-preemption-rate=0 - -use std::convert::TryInto; -use std::thread; - -fn main() { - test_epoll_race(); -} - -// Using `as` cast since `EPOLLET` wraps around -const EPOLL_IN_OUT_ET: u32 = (libc::EPOLLIN | libc::EPOLLOUT | libc::EPOLLET) as _; - -#[track_caller] -fn check_epoll_wait( - epfd: i32, - expected_notifications: &[(u32, u64)], - timeout: i32, -) { - let epoll_event = libc::epoll_event { events: 0, u64: 0 }; - let mut array: [libc::epoll_event; N] = [epoll_event; N]; - let maxsize = N; - let array_ptr = array.as_mut_ptr(); - let res = unsafe { libc::epoll_wait(epfd, array_ptr, maxsize.try_into().unwrap(), timeout) }; - if res < 0 { - panic!("epoll_wait failed: {}", std::io::Error::last_os_error()); - } - assert_eq!( - res, - expected_notifications.len().try_into().unwrap(), - "got wrong number of notifications" - ); - let slice = unsafe { std::slice::from_raw_parts(array_ptr, res.try_into().unwrap()) }; - for (return_event, expected_event) in slice.iter().zip(expected_notifications.iter()) { - let event = return_event.events; - let data = return_event.u64; - assert_eq!(event, expected_event.0, "got wrong events"); - assert_eq!(data, expected_event.1, "got wrong data"); - } -} - -// This test shows a data_race before epoll had vector clocks added. -fn test_epoll_race() { - // Create an epoll instance. - let epfd = unsafe { libc::epoll_create1(0) }; - assert_ne!(epfd, -1); - - // Create an eventfd instance. - let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; - let fd = unsafe { libc::eventfd(0, flags) }; - - // Register eventfd with the epoll instance. - let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd as u64 }; - let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; - assert_eq!(res, 0); - - static mut VAL: u8 = 0; - let thread1 = thread::spawn(move || { - // Write to the static mut variable. - unsafe { VAL = 1 }; - // Write to the eventfd instance. - let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); - let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; - // read returns number of bytes that have been read, which is always 8. - assert_eq!(res, 8); - }); - thread::yield_now(); - // epoll_wait for the event to happen. - let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); - let expected_value = u64::try_from(fd).unwrap(); - check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)], -1); - // Read from the static mut variable. - #[allow(static_mut_refs)] - unsafe { - assert_eq!(VAL, 1) //~ ERROR: Data race detected - }; - thread1.join().unwrap(); -} diff --git a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr b/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr deleted file mode 100644 index 27974981cea7..000000000000 --- a/src/tools/miri/tests/fail-dep/libc/libc-epoll-blocking.stderr +++ /dev/null @@ -1,29 +0,0 @@ -error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here - --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC - | -LL | assert_eq!(VAL, 1) - | ^^^^^^^^^^^^^^^^^^ Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) retag read of type `u8` on thread `main` at ALLOC. (2) just happened here - | -help: and (1) occurred earlier here - --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC - | -LL | unsafe { VAL = 1 }; - | ^^^^^^^ - = help: retags occur on all (re)borrows and as well as when references are copied or moved - = help: retags permit optimizations that insert speculative reads or writes - = help: therefore from the perspective of data races, a retag has the same implications as a read or write - = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior - = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information - = note: BACKTRACE (of the first span): - = note: inside `test_epoll_race` at RUSTLIB/core/src/macros/mod.rs:LL:CC -note: inside `main` - --> tests/fail-dep/libc/libc-epoll-blocking.rs:LL:CC - | -LL | test_epoll_race(); - | ^^^^^^^^^^^^^^^^^ - = note: this error originates in the macro `assert_eq` (in Nightly builds, run with -Z macro-backtrace for more info) - -note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace - -error: aborting due to 1 previous error - diff --git a/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs b/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs index eb38529ae57d..d7675a40163c 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-epoll-blocking.rs @@ -1,5 +1,5 @@ //@only-target: linux -// test_epoll_block_then_unblock depends on a deterministic schedule. +// test_epoll_block_then_unblock and test_epoll_race depend on a deterministic schedule. //@compile-flags: -Zmiri-preemption-rate=0 use std::convert::TryInto; @@ -12,6 +12,7 @@ fn main() { test_epoll_block_without_notification(); test_epoll_block_then_unblock(); test_notification_after_timeout(); + test_epoll_race(); } // Using `as` cast since `EPOLLET` wraps around @@ -137,3 +138,41 @@ fn test_notification_after_timeout() { let expected_value = fds[0] as u64; check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)], 10); } + +// This test shows a data_race before epoll had vector clocks added. +fn test_epoll_race() { + // Create an epoll instance. + let epfd = unsafe { libc::epoll_create1(0) }; + assert_ne!(epfd, -1); + + // Create an eventfd instance. + let flags = libc::EFD_NONBLOCK | libc::EFD_CLOEXEC; + let fd = unsafe { libc::eventfd(0, flags) }; + + // Register eventfd with the epoll instance. + let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fd as u64 }; + let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fd, &mut ev) }; + assert_eq!(res, 0); + + static mut VAL: u8 = 0; + let thread1 = thread::spawn(move || { + // Write to the static mut variable. + unsafe { VAL = 1 }; + // Write to the eventfd instance. + let sized_8_data: [u8; 8] = 1_u64.to_ne_bytes(); + let res = unsafe { libc::write(fd, sized_8_data.as_ptr() as *const libc::c_void, 8) }; + // read returns number of bytes that have been read, which is always 8. + assert_eq!(res, 8); + }); + thread::yield_now(); + // epoll_wait for the event to happen. + let expected_event = u32::try_from(libc::EPOLLIN | libc::EPOLLOUT).unwrap(); + let expected_value = u64::try_from(fd).unwrap(); + check_epoll_wait::<8>(epfd, &[(expected_event, expected_value)], -1); + // Read from the static mut variable. + #[allow(static_mut_refs)] + unsafe { + assert_eq!(VAL, 1) + }; + thread1.join().unwrap(); +} From 81202c8b13c6ff64c14fd740757ba3e283adfec7 Mon Sep 17 00:00:00 2001 From: Frank Rehwinkel Date: Wed, 2 Oct 2024 08:13:11 -0400 Subject: [PATCH 4/4] epoll: remove extraneous clone of ready_list A simplification that doesn't impact the epoll implementation's logic. It is not necessary to clone the ready_list before reading its `is_empty` state. This avoids the clone step but more importantly avoids the invisible drop step of the clone. --- src/tools/miri/src/shims/unix/linux/epoll.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/tools/miri/src/shims/unix/linux/epoll.rs b/src/tools/miri/src/shims/unix/linux/epoll.rs index 88fab8aa010a..c4bd38c47e54 100644 --- a/src/tools/miri/src/shims/unix/linux/epoll.rs +++ b/src/tools/miri/src/shims/unix/linux/epoll.rs @@ -475,8 +475,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { let epoll_file_description = epfd .downcast::() .ok_or_else(|| err_unsup_format!("non-epoll FD passed to `epoll_wait`"))?; - let binding = epoll_file_description.get_ready_list(); - ready_list_empty = binding.mapping.borrow_mut().is_empty(); + ready_list_empty = epoll_file_description.ready_list.mapping.borrow().is_empty(); thread_ids = epoll_file_description.thread_id.borrow_mut(); } if timeout == 0 || !ready_list_empty {