Auto merge of rust-lang#3946 - FrankReh:fix-over-synchronization-of-epoll, r=RalfJung

Fix over synchronization of epoll

Fixes rust-lang#3944.

The clock used by epoll is now per event generated, rather than by the `epoll's` ready_list.

The same epoll tests that existed before are unchanged and still pass. Also the `tokio` test case we had worked on last week still passes with this change.

This change does beg the question of how the epoll event states should change. Perhaps rather than expose public crate bool fields, so setters should be provided that include a clock parameter or an optional clock parameter. Also should all the epoll event possibilities have their clock sync tested the way these commit lay out testing. In this first go around, only the pipe's EPOLLIN is tested. The EPOLLOUT might deserve testing too, as would the eventfd. Any future source of epoll events would also fit into that category.

Author: bors

src/tools/miri/src/shims/unix/linux/epoll.rs

@@ -32,11 +32,13 @@ pub struct EpollEventInstance {
     events: u32,
     /// Original data retrieved from `epoll_event` during `epoll_ctl`.
     data: u64,
+    /// The release clock associated with this event.
+    clock: VClock,
 }
 
 impl EpollEventInstance {
     pub fn new(events: u32, data: u64) -> EpollEventInstance {
-        EpollEventInstance { events, data }
+        EpollEventInstance { events, data, clock: Default::default() }
     }
 }
 
@@ -92,7 +94,6 @@ pub struct EpollReadyEvents {
 #[derive(Debug, Default)]
 struct ReadyList {
     mapping: RefCell<BTreeMap<(FdId, i32), EpollEventInstance>>,
-    clock: RefCell<VClock>,
 }
 
 impl EpollReadyEvents {
@@ -567,11 +568,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
 
                         let epoll = epfd.downcast::<Epoll>().unwrap();
 
-                        // Synchronize running thread to the epoll ready list.
-                        this.release_clock(|clock| {
-                            epoll.ready_list.clock.borrow_mut().join(clock);
-                        });
-
                         if let Some(thread_id) = epoll.thread_id.borrow_mut().pop() {
                             waiter.push(thread_id);
                         };
@@ -627,7 +623,11 @@ fn check_and_update_one_event_interest<'tcx>(
     if flags != 0 {
         let epoll_key = (id, epoll_event_interest.fd_num);
         let ready_list = &mut epoll_event_interest.ready_list.mapping.borrow_mut();
-        let event_instance = EpollEventInstance::new(flags, epoll_event_interest.data);
+        let mut event_instance = EpollEventInstance::new(flags, epoll_event_interest.data);
+        // If we are tracking data races, remember the current clock so we can sync with it later.
+        ecx.release_clock(|clock| {
+            event_instance.clock.join(clock);
+        });
         // Triggers the notification by inserting it to the ready list.
         ready_list.insert(epoll_key, event_instance);
         interp_ok(true)
@@ -654,9 +654,6 @@ fn blocking_epoll_callback<'tcx>(
 
     let ready_list = epoll_file_description.get_ready_list();
 
-    // 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)?;
@@ -670,6 +667,9 @@ fn blocking_epoll_callback<'tcx>(
                 ],
                 &des.1,
             )?;
+            // Synchronize waking thread with the event of interest.
+            ecx.acquire_clock(&epoll_event_instance.clock);
+
             num_of_events = num_of_events.strict_add(1);
         } else {
             break;

src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.rs

@@ -0,0 +1,97 @@
+//! This ensures that when an epoll_wait wakes up and there are multiple events,
+//! and we only read one of them, we do not synchronize with the other events
+//! and therefore still report a data race for things that need to see the second event
+//! to be considered synchronized.
+//@only-target: linux
+// ensure deterministic schedule
+//@compile-flags: -Zmiri-preemption-rate=0
+
+use std::convert::TryInto;
+use std::thread;
+use std::thread::spawn;
+
+#[track_caller]
+fn check_epoll_wait<const N: usize>(epfd: i32, expected_notifications: &[(u32, u64)]) {
+    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(), 0) };
+    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");
+    }
+}
+
+fn main() {
+    // Create an epoll instance.
+    let epfd = unsafe { libc::epoll_create1(0) };
+    assert_ne!(epfd, -1);
+
+    // Create two socketpair instances.
+    let mut fds_a = [-1, -1];
+    let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds_a.as_mut_ptr()) };
+    assert_eq!(res, 0);
+
+    let mut fds_b = [-1, -1];
+    let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds_b.as_mut_ptr()) };
+    assert_eq!(res, 0);
+
+    // Register both pipe read ends.
+    let mut ev = libc::epoll_event {
+        events: (libc::EPOLLIN | libc::EPOLLET) as _,
+        u64: u64::try_from(fds_a[1]).unwrap(),
+    };
+    let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds_a[1], &mut ev) };
+    assert_eq!(res, 0);
+
+    let mut ev = libc::epoll_event {
+        events: (libc::EPOLLIN | libc::EPOLLET) as _,
+        u64: u64::try_from(fds_b[1]).unwrap(),
+    };
+    let res = unsafe { libc::epoll_ctl(epfd, libc::EPOLL_CTL_ADD, fds_b[1], &mut ev) };
+    assert_eq!(res, 0);
+
+    static mut VAL_ONE: u8 = 40; // This one will be read soundly.
+    static mut VAL_TWO: u8 = 50; // This one will be read unsoundly.
+    let thread1 = spawn(move || {
+        unsafe { VAL_ONE = 41 };
+
+        let data = "abcde".as_bytes().as_ptr();
+        let res = unsafe { libc::write(fds_a[0], data as *const libc::c_void, 5) };
+        assert_eq!(res, 5);
+
+        unsafe { VAL_TWO = 51 };
+
+        let res = unsafe { libc::write(fds_b[0], data as *const libc::c_void, 5) };
+        assert_eq!(res, 5);
+    });
+    thread::yield_now();
+
+    // With room for one event: check result from epoll_wait.
+    let expected_event = u32::try_from(libc::EPOLLIN).unwrap();
+    let expected_value = u64::try_from(fds_a[1]).unwrap();
+    check_epoll_wait::<1>(epfd, &[(expected_event, expected_value)]);
+
+    // Since we only received one event, we have synchronized with
+    // the write to VAL_ONE but not with the one to VAL_TWO.
+    unsafe {
+        assert_eq!({ VAL_ONE }, 41) // This one is not UB
+    };
+    unsafe {
+        assert_eq!({ VAL_TWO }, 51) //~ERROR: Data race detected
+    };
+
+    thread1.join().unwrap();
+}

src/tools/miri/tests/fail-dep/libc/libc-epoll-data-race.stderr

@@ -0,0 +1,20 @@
+error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) non-atomic read on thread `main` at ALLOC. (2) just happened here
+  --> tests/fail-dep/libc/libc-epoll-data-race.rs:LL:CC
+   |
+LL |         assert_eq!({ VAL_TWO }, 51)
+   |                      ^^^^^^^ Data race detected between (1) non-atomic write on thread `unnamed-ID` and (2) non-atomic read on thread `main` at ALLOC. (2) just happened here
+   |
+help: and (1) occurred earlier here
+  --> tests/fail-dep/libc/libc-epoll-data-race.rs:LL:CC
+   |
+LL |         unsafe { VAL_TWO = 51 };
+   |                  ^^^^^^^^^^^^
+   = 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 `main` at tests/fail-dep/libc/libc-epoll-data-race.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to 1 previous error
+