Fix over synchronization of epoll

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;

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();
+}

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
+