Auto merge of rust-lang#3829 - tiif:edgecase, r=oli-obk

epoll: handle edge case for epoll_ctl

There is a test case that revealed that our implementation differs from the real system:
 - Set up an epoll watching the FD
- Call epoll_wait
- Set up another epoll watching the same FD
- Call epoll_wait on the first epoll. Nothing should be reported!

This happened because, in ``epoll_ctl``, we used ``check_and_update_readiness``, which is a function that would return notification for all epoll file description that registered a particular file description. But we shouldn't do that because no notification should be returned if there is no I/O activity between two ``epoll_wait`` (every first ``epoll_wait`` that happens after ``epoll_ctl`` is an exception, we should return notification that reflects the readiness of file description).

r? `@oli-obk`

Author: bors

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

@@ -269,10 +269,10 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let mut interest_list = epoll_file_description.interest_list.borrow_mut();
         let ready_list = &epoll_file_description.ready_list;
 
-        let Some(file_descriptor) = this.machine.fds.get(fd) else {
+        let Some(fd_ref) = this.machine.fds.get(fd) else {
             return Ok(Scalar::from_i32(this.fd_not_found()?));
         };
-        let id = file_descriptor.get_id();
+        let id = fd_ref.get_id();
 
         if op == epoll_ctl_add || op == epoll_ctl_mod {
             // Read event bitmask and data from epoll_event passed by caller.
@@ -332,7 +332,6 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 }
             }
 
-            let id = file_descriptor.get_id();
             // Create an epoll_interest.
             let interest = Rc::new(RefCell::new(EpollEventInterest {
                 file_descriptor: fd,
@@ -344,7 +343,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
             if op == epoll_ctl_add {
                 // Insert an epoll_interest to global epoll_interest list.
                 this.machine.epoll_interests.insert_epoll_interest(id, Rc::downgrade(&interest));
-                interest_list.insert(epoll_key, interest);
+                interest_list.insert(epoll_key, Rc::clone(&interest));
             } else {
                 // Directly modify the epoll_interest so the global epoll_event_interest table
                 // will be updated too.
@@ -353,9 +352,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                 epoll_interest.data = data;
             }
 
-            // Readiness will be updated immediately when the epoll_event_interest is added or modified.
-            this.check_and_update_readiness(&file_descriptor)?;
-
+            // Notification will be returned for current epfd if there is event in the file
+            // descriptor we registered.
+            check_and_update_one_event_interest(&fd_ref, interest, id, this)?;
             return Ok(Scalar::from_i32(0));
         } else if op == epoll_ctl_del {
             let epoll_key = (id, fd);
@@ -489,25 +488,9 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
         let id = fd_ref.get_id();
         // Get a list of EpollEventInterest that is associated to a specific file description.
         if let Some(epoll_interests) = this.machine.epoll_interests.get_epoll_interest(id) {
-            let epoll_ready_events = fd_ref.get_epoll_ready_events()?;
-            // Get the bitmask of ready events.
-            let ready_events = epoll_ready_events.get_event_bitmask(this);
-
             for weak_epoll_interest in epoll_interests {
                 if let Some(epoll_interest) = weak_epoll_interest.upgrade() {
-                    // This checks if any of the events specified in epoll_event_interest.events
-                    // match those in ready_events.
-                    let epoll_event_interest = epoll_interest.borrow();
-                    let flags = epoll_event_interest.events & ready_events;
-                    // If there is any event that we are interested in being specified as ready,
-                    // insert an epoll_return to the ready list.
-                    if flags != 0 {
-                        let epoll_key = (id, epoll_event_interest.file_descriptor);
-                        let ready_list = &mut epoll_event_interest.ready_list.borrow_mut();
-                        let event_instance =
-                            EpollEventInstance::new(flags, epoll_event_interest.data);
-                        ready_list.insert(epoll_key, event_instance);
-                    }
+                    check_and_update_one_event_interest(fd_ref, epoll_interest, id, this)?;
                 }
             }
         }
@@ -532,3 +515,30 @@ fn ready_list_next(
     }
     return None;
 }
+
+/// This helper function checks whether an epoll notification should be triggered for a specific
+/// epoll_interest and, if necessary, triggers the notification. Unlike check_and_update_readiness,
+/// this function sends a notification to only one epoll instance.
+fn check_and_update_one_event_interest<'tcx>(
+    fd_ref: &FileDescriptionRef,
+    interest: Rc<RefCell<EpollEventInterest>>,
+    id: FdId,
+    ecx: &MiriInterpCx<'tcx>,
+) -> InterpResult<'tcx> {
+    // Get the bitmask of ready events for a file description.
+    let ready_events_bitmask = fd_ref.get_epoll_ready_events()?.get_event_bitmask(ecx);
+    let epoll_event_interest = interest.borrow();
+    // This checks if any of the events specified in epoll_event_interest.events
+    // match those in ready_events.
+    let flags = epoll_event_interest.events & ready_events_bitmask;
+    // If there is any event that we are interested in being specified as ready,
+    // insert an epoll_return to the ready list.
+    if flags != 0 {
+        let epoll_key = (id, epoll_event_interest.file_descriptor);
+        let ready_list = &mut epoll_event_interest.ready_list.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);
+    }
+    Ok(())
+}

src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.rs

@@ -0,0 +1,18 @@
+//@only-target-linux
+
+// This is a test for registering unsupported fd with epoll.
+// Register epoll fd with epoll is allowed in real system, but we do not support this.
+fn main() {
+    // Create two epoll instance.
+    let epfd0 = unsafe { libc::epoll_create1(0) };
+    assert_ne!(epfd0, -1);
+    let epfd1 = unsafe { libc::epoll_create1(0) };
+    assert_ne!(epfd1, -1);
+
+    // Register epoll with epoll.
+    let mut ev =
+        libc::epoll_event { events: (libc::EPOLLIN | libc::EPOLLET) as _, u64: epfd1 as u64 };
+    let res = unsafe { libc::epoll_ctl(epfd0, libc::EPOLL_CTL_ADD, epfd1, &mut ev) };
+    //~^ERROR: epoll does not support this file description
+    assert_eq!(res, 0);
+}

src/tools/miri/tests/fail-dep/libc/libc_epoll_unsupported_fd.stderr

@@ -0,0 +1,14 @@
+error: unsupported operation: epoll: epoll does not support this file description
+  --> $DIR/libc_epoll_unsupported_fd.rs:LL:CC
+   |
+LL |     let res = unsafe { libc::epoll_ctl(epfd0, libc::EPOLL_CTL_ADD, epfd1, &mut ev) };
+   |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ epoll: epoll does not support this file description
+   |
+   = help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support
+   = note: BACKTRACE:
+   = note: inside `main` at $DIR/libc_epoll_unsupported_fd.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
+

src/tools/miri/tests/pass-dep/libc/libc-epoll.rs

@@ -22,6 +22,7 @@ fn main() {
     test_epoll_lost_events();
     test_ready_list_fetching_logic();
     test_epoll_ctl_epfd_equal_fd();
+    test_epoll_ctl_notification();
 }
 
 // Using `as` cast since `EPOLLET` wraps around
@@ -644,3 +645,41 @@ fn test_epoll_ctl_epfd_equal_fd() {
     assert_eq!(e.raw_os_error(), Some(libc::EINVAL));
     assert_eq!(res, -1);
 }
+
+// We previously used check_and_update_readiness the moment a file description is registered in an
+// epoll instance. But this has an unfortunate side effect of returning notification to another
+// epfd that shouldn't receive notification.
+fn test_epoll_ctl_notification() {
+    // Create an epoll instance.
+    let epfd0 = unsafe { libc::epoll_create1(0) };
+    assert_ne!(epfd0, -1);
+
+    // Create a socketpair instance.
+    let mut fds = [-1, -1];
+    let res = unsafe { libc::socketpair(libc::AF_UNIX, libc::SOCK_STREAM, 0, fds.as_mut_ptr()) };
+    assert_eq!(res, 0);
+
+    // Register one side of the socketpair with epoll.
+    let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[0] as u64 };
+    let res = unsafe { libc::epoll_ctl(epfd0, libc::EPOLL_CTL_ADD, fds[0], &mut ev) };
+    assert_eq!(res, 0);
+
+    // epoll_wait to clear notification for epfd0.
+    let expected_event = u32::try_from(libc::EPOLLOUT).unwrap();
+    let expected_value = fds[0] as u64;
+    check_epoll_wait::<1>(epfd0, &[(expected_event, expected_value)]);
+
+    // Create another epoll instance.
+    let epfd1 = unsafe { libc::epoll_create1(0) };
+    assert_ne!(epfd1, -1);
+
+    // Register the same file description for epfd1.
+    let mut ev = libc::epoll_event { events: EPOLL_IN_OUT_ET, u64: fds[0] as u64 };
+    let res = unsafe { libc::epoll_ctl(epfd1, libc::EPOLL_CTL_ADD, fds[0], &mut ev) };
+    assert_eq!(res, 0);
+    check_epoll_wait::<1>(epfd1, &[(expected_event, expected_value)]);
+
+    // Previously this epoll_wait will receive a notification, but we shouldn't return notification
+    // for this epfd, because there is no I/O event between the two epoll_wait.
+    check_epoll_wait::<1>(epfd0, &[]);
+}