Don't hold EpollInstance.mu while calling FileDescription.Readiness().

EpollInstance.mu is also renamed to readyMu to more accurately reflect its
purpose.

The same fix is also applied to VFS1's epoll implementation.

This is more consistent with Linux, although Linux has additional complexity to
support concurrent queueing of readied FDs in its equivalent of
epollInterest.NotifyEvent() (fs/eventpoll.c:ep_poll_callback()). Compare:
- VFS2 EpollInstance.Readiness() and Linux __ep_eventpoll_poll() =>
  ep_start/done_scan()
- VFS2 EpollInstance.ReadEvents() and Linux ep_poll() => ep_send_events() =>
  ep_start/done_scan()

PiperOrigin-RevId: 432488493

pkg/ilist/list.go

@@ -120,6 +120,23 @@ func (l *List) PushFront(e Element) {
 	l.head = e
 }
 
+// PushFrontList inserts list m at the start of list l, emptying m.
+//
+//go:nosplit
+func (l *List) PushFrontList(m *List) {
+	if l.head == nil {
+		l.head = m.head
+		l.tail = m.tail
+	} else if m.head != nil {
+		ElementMapper{}.linkerFor(l.head).SetPrev(m.tail)
+		ElementMapper{}.linkerFor(m.tail).SetNext(l.head)
+
+		l.head = m.head
+	}
+	m.head = nil
+	m.tail = nil
+}
+
 // PushBack inserts the element e at the back of list l.
 //
 //go:nosplit

pkg/sentry/kernel/epoll/epoll.go

@@ -183,9 +183,36 @@ func (*EventPoll) Write(context.Context, *fs.File, usermem.IOSequence, int64) (i
 
 // eventsAvailable determines if 'e' has events available for delivery.
 func (e *EventPoll) eventsAvailable() bool {
-	e.listsMu.Lock()
+	e.mu.Lock()
+	defer e.mu.Unlock()
 
-	for it := e.readyList.Front(); it != nil; {
+	// We can't call fs.File.Readiness() while holding e.listsMu due to lock
+	// ordering requirements. Instead, hold e.mu to prevent changes to the set
+	// of pollEntries, then temporarily move all pollEntries already on
+	// e.readyList to a local list that we can iterate without holding
+	// e.listsMu. pollEntry.curList is left set to &e.readyList so that
+	// pollEntry.NotifyEvent() doesn't touch pollEntryEntry.
+	var (
+		readyList   pollEntryList
+		waitingList pollEntryList
+	)
+	e.listsMu.Lock()
+	readyList.PushBackList(&e.readyList)
+	e.listsMu.Unlock()
+	if readyList.Empty() {
+		return false
+	}
+	defer func() {
+		e.listsMu.Lock()
+		e.readyList.PushFrontList(&readyList)
+		for entry := waitingList.Front(); entry != nil; entry = entry.Next() {
+			entry.curList = &e.waitingList
+		}
+		e.waitingList.PushBackList(&waitingList)
+		e.listsMu.Unlock()
+	}()
+
+	for it := readyList.Front(); it != nil; {
 		entry := it
 		it = it.Next()
 
@@ -193,18 +220,15 @@ func (e *EventPoll) eventsAvailable() bool {
 		// ready for delivery.
 		ready := entry.id.File.Readiness(entry.mask)
 		if ready != 0 {
-			e.listsMu.Unlock()
 			return true
 		}
 
-		// Entry is not ready, so move it to waiting list.
-		e.readyList.Remove(entry)
-		e.waitingList.PushBack(entry)
-		entry.curList = &e.waitingList
+		// Entry is not ready, so move it to waiting list. entry.curList will
+		// be updated with e.listsMu locked in the deferred function above.
+		readyList.Remove(entry)
+		waitingList.PushBack(entry)
 	}
 
-	e.listsMu.Unlock()
-
 	return false
 }
 
@@ -222,13 +246,45 @@ func (e *EventPoll) Readiness(mask waiter.EventMask) waiter.EventMask {
 
 // ReadEvents returns up to max available events.
 func (e *EventPoll) ReadEvents(max int) []linux.EpollEvent {
-	var local pollEntryList
-	var ret []linux.EpollEvent
+	e.mu.Lock()
+	defer e.mu.Unlock()
 
+	// We can't call fs.File.Readiness() while holding e.listsMu due to lock
+	// ordering requirements. Instead, hold e.mu to prevent changes to the set
+	// of pollEntries, then temporarily move all pollEntries already on
+	// e.readyList to a local list that we can iterate without holding
+	// e.listsMu. pollEntry.curList is left set to &e.readyList so that
+	// pollEntry.NotifyEvent() doesn't touch pollEntryEntry.
+	var (
+		readyList    pollEntryList
+		requeueList  pollEntryList
+		waitingList  pollEntryList
+		disabledList pollEntryList
+		ret          []linux.EpollEvent
+	)
 	e.listsMu.Lock()
+	readyList.PushBackList(&e.readyList)
+	e.listsMu.Unlock()
+	if readyList.Empty() {
+		return nil
+	}
+	defer func() {
+		e.listsMu.Lock()
+		e.readyList.PushFrontList(&readyList)
+		e.readyList.PushBackList(&requeueList)
+		for entry := waitingList.Front(); entry != nil; entry = entry.Next() {
+			entry.curList = &e.waitingList
+		}
+		e.waitingList.PushBackList(&waitingList)
+		for entry := disabledList.Front(); entry != nil; entry = entry.Next() {
+			entry.curList = &e.disabledList
+		}
+		e.disabledList.PushBackList(&disabledList)
+		e.listsMu.Unlock()
+	}()
 
 	// Go through all entries we believe may be ready.
-	for it := e.readyList.Front(); it != nil && len(ret) < max; {
+	for it := readyList.Front(); it != nil && len(ret) < max; {
 		entry := it
 		it = it.Next()
 
@@ -237,10 +293,8 @@ func (e *EventPoll) ReadEvents(max int) []linux.EpollEvent {
 		// entry.
 		ready := entry.id.File.Readiness(entry.mask) & entry.mask
 		if ready == 0 {
-			e.readyList.Remove(entry)
-			e.waitingList.PushBack(entry)
-			entry.curList = &e.waitingList
-
+			readyList.Remove(entry)
+			waitingList.PushBack(entry)
 			continue
 		}
 
@@ -256,22 +310,16 @@ func (e *EventPoll) ReadEvents(max int) []linux.EpollEvent {
 		// list so that its readiness can be checked the next time
 		// around; however, we must move it to the end of the list so
 		// that other events can be delivered as well.
-		e.readyList.Remove(entry)
+		readyList.Remove(entry)
 		if entry.flags&OneShot != 0 {
-			e.disabledList.PushBack(entry)
-			entry.curList = &e.disabledList
+			disabledList.PushBack(entry)
 		} else if entry.flags&EdgeTriggered != 0 {
-			e.waitingList.PushBack(entry)
-			entry.curList = &e.waitingList
+			waitingList.PushBack(entry)
 		} else {
-			local.PushBack(entry)
+			requeueList.PushBack(entry)
 		}
 	}
 
-	e.readyList.PushBackList(&local)
-
-	e.listsMu.Unlock()
-
 	return ret
 }
 

pkg/sentry/vfs/epoll.go

@@ -38,13 +38,19 @@ type EpollInstance struct {
 	// q holds waiters on this EpollInstance.
 	q waiter.Queue
 
-	// interest is the set of file descriptors that are registered with the
-	// EpollInstance for monitoring. interest is protected by interestMu.
+	// interestMu protects interest and most fields in registered
+	// epollInterests. interestMu is analogous to Linux's struct
+	// eventpoll::mtx.
 	interestMu sync.Mutex `state:"nosave"`
-	interest   map[epollInterestKey]*epollInterest
 
-	// mu protects fields in registered epollInterests.
-	mu sync.Mutex `state:"nosave"`
+	// interest is the set of file descriptors that are registered with the
+	// EpollInstance for monitoring.
+	interest map[epollInterestKey]*epollInterest
+
+	// readyMu protects ready, epollInterest.ready, and
+	// epollInterest.epollInterestEntry. ready is analogous to Linux's struct
+	// eventpoll::lock.
+	readyMu sync.Mutex `state:"nosave"`
 
 	// ready is the set of file descriptors that may be "ready" for I/O. Note
 	// that this must be an ordered list, not a map: "If more than maxevents
@@ -79,20 +85,21 @@ type epollInterest struct {
 	// key is the file to which this epollInterest applies. key is immutable.
 	key epollInterestKey
 
-	// waiter is registered with key.file. entry is protected by epoll.mu.
+	// waiter is registered with key.file. entry is protected by
+	// epoll.interestMu.
 	waiter waiter.Entry
 
 	// mask is the event mask associated with this registration, including
-	// flags EPOLLET and EPOLLONESHOT. mask is protected by epoll.mu.
+	// flags EPOLLET and EPOLLONESHOT. mask is protected by epoll.interestMu.
 	mask uint32
 
 	// ready is true if epollInterestEntry is linked into epoll.ready. ready
-	// and epollInterestEntry are protected by epoll.mu.
+	// and epollInterestEntry are protected by epoll.readyMu.
 	ready bool
 	epollInterestEntry
 
 	// userData is the struct epoll_event::data associated with this
-	// epollInterest. userData is protected by epoll.mu.
+	// epollInterest. userData is protected by epoll.interestMu.
 	userData [2]int32
 }
 
@@ -134,19 +141,45 @@ func (ep *EpollInstance) Readiness(mask waiter.EventMask) waiter.EventMask {
 	if mask&waiter.ReadableEvents == 0 {
 		return 0
 	}
-	ep.mu.Lock()
+
+	// We can't call FileDescription.Readiness() while holding ep.readyMu.
+	// Instead, hold ep.interestMu to prevent changes to the set of
+	// epollInterests, then temporarily move all epollInterests already on
+	// ep.ready to a local list that we can iterate without holding ep.readyMu.
+	// epollInterest.ready is left set to true so that
+	// epollInterest.NotifyEvent() doesn't touch epollInterestEntry.
+	ep.interestMu.Lock()
+	defer ep.interestMu.Unlock()
+	var (
+		ready    epollInterestList
+		notReady epollInterestList
+	)
+	ep.readyMu.Lock()
+	ready.PushBackList(&ep.ready)
+	ep.readyMu.Unlock()
+	if ready.Empty() {
+		return 0
+	}
+	defer func() {
+		ep.readyMu.Lock()
+		ep.ready.PushFrontList(&ready)
+		for epi := notReady.Front(); epi != nil; epi = epi.Next() {
+			epi.ready = false
+		}
+		ep.readyMu.Unlock()
+	}()
+
 	var next *epollInterest
-	for epi := ep.ready.Front(); epi != nil; epi = next {
+	for epi := ready.Front(); epi != nil; epi = next {
 		next = epi.Next()
 		wmask := waiter.EventMaskFromLinux(epi.mask)
 		if epi.key.file.Readiness(wmask)&wmask != 0 {
-			ep.mu.Unlock()
 			return waiter.ReadableEvents
 		}
-		ep.ready.Remove(epi)
-		epi.ready = false
+		// epi.key.file was readied spuriously; leave it off of ep.ready.
+		ready.Remove(epi)
+		notReady.PushBack(epi)
 	}
-	ep.mu.Unlock()
 	return 0
 }
 
@@ -279,10 +312,8 @@ func (ep *EpollInstance) ModifyInterest(file *FileDescription, num int32, event
 
 	// Update epi for the next call to ep.ReadEvents().
 	mask := event.Events | linux.EPOLLERR | linux.EPOLLHUP
-	ep.mu.Lock()
 	epi.mask = mask
 	epi.userData = event.Data
-	ep.mu.Unlock()
 
 	// Re-register with the new mask.
 	file.EventUnregister(&epi.waiter)
@@ -332,13 +363,13 @@ func (ep *EpollInstance) DeleteInterest(file *FileDescription, num int32) error
 // NotifyEvent implements waiter.EventListener.NotifyEvent.
 func (epi *epollInterest) NotifyEvent(waiter.EventMask) {
 	newReady := false
-	epi.epoll.mu.Lock()
+	epi.epoll.readyMu.Lock()
 	if !epi.ready {
 		newReady = true
 		epi.ready = true
 		epi.epoll.ready.PushBack(epi)
 	}
-	epi.epoll.mu.Unlock()
+	epi.epoll.readyMu.Unlock()
 	if newReady {
 		epi.epoll.q.Notify(waiter.ReadableEvents)
 	}
@@ -347,32 +378,61 @@ func (epi *epollInterest) NotifyEvent(waiter.EventMask) {
 // Preconditions: ep.interestMu must be locked.
 func (ep *EpollInstance) removeLocked(epi *epollInterest) {
 	delete(ep.interest, epi.key)
-	ep.mu.Lock()
+	ep.readyMu.Lock()
 	if epi.ready {
 		epi.ready = false
 		ep.ready.Remove(epi)
 	}
-	ep.mu.Unlock()
+	ep.readyMu.Unlock()
 }
 
 // ReadEvents appends up to maxReady events to events and returns the updated
 // slice of events.
 func (ep *EpollInstance) ReadEvents(events []linux.EpollEvent, maxEvents int) []linux.EpollEvent {
+	// We can't call FileDescription.Readiness() while holding ep.readyMu.
+	// Instead, hold ep.interestMu to prevent changes to the set of
+	// epollInterests, then temporarily move all epollInterests already on
+	// ep.ready to a local list that we can iterate without holding ep.readyMu.
+	// epollInterest.ready is left set to true so that
+	// epollInterest.NotifyEvent() doesn't touch epollInterestEntry.
+	ep.interestMu.Lock()
+	defer ep.interestMu.Unlock()
+	var (
+		ready    epollInterestList
+		requeue  epollInterestList
+		notReady epollInterestList
+	)
+	ep.readyMu.Lock()
+	ready.PushBackList(&ep.ready)
+	ep.readyMu.Unlock()
+	if ready.Empty() {
+		return nil
+	}
+	defer func() {
+		ep.readyMu.Lock()
+		// epollInterests that we never checked are re-inserted at the start of
+		// ep.ready. epollInterests that were ready are re-inserted at the end
+		// for reasons described by EpollInstance.ready.
+		ep.ready.PushFrontList(&ready)
+		ep.ready.PushBackList(&requeue)
+		for epi := notReady.Front(); epi != nil; epi = epi.Next() {
+			epi.ready = false
+		}
+		ep.readyMu.Unlock()
+	}()
+
 	i := 0
-	// Hot path: avoid defer.
-	ep.mu.Lock()
 	var next *epollInterest
-	var requeue epollInterestList
-	for epi := ep.ready.Front(); epi != nil; epi = next {
+	for epi := ready.Front(); epi != nil; epi = next {
 		next = epi.Next()
 		// Regardless of what else happens, epi is initially removed from the
 		// ready list.
-		ep.ready.Remove(epi)
+		ready.Remove(epi)
 		wmask := waiter.EventMaskFromLinux(epi.mask)
 		ievents := epi.key.file.Readiness(wmask) & wmask
 		if ievents == 0 {
 			// Leave epi off the ready list.
-			epi.ready = false
+			notReady.PushBack(epi)
 			continue
 		}
 		// Determine what we should do with epi.
@@ -384,7 +444,7 @@ func (ep *EpollInstance) ReadEvents(events []linux.EpollEvent, maxEvents int) []
 			fallthrough
 		case epi.mask&linux.EPOLLET != 0:
 			// Leave epi off the ready list.
-			epi.ready = false
+			notReady.PushBack(epi)
 		default:
 			// Queue epi to be moved to the end of the ready list.
 			requeue.PushBack(epi)
@@ -399,7 +459,5 @@ func (ep *EpollInstance) ReadEvents(events []linux.EpollEvent, maxEvents int) []
 			break
 		}
 	}
-	ep.ready.PushBackList(&requeue)
-	ep.mu.Unlock()
 	return events
 }

pkg/sentry/vfs/vfs.go

@@ -18,14 +18,13 @@
 //
 // EpollInstance.interestMu
 //   FileDescription.epollMu
-//     FilesystemImpl/FileDescriptionImpl locks
+//     Locks acquired by FilesystemImpl/FileDescriptionImpl methods
 //       VirtualFilesystem.mountMu
 //         Dentry.mu
 //           Locks acquired by FilesystemImpls between Prepare{Delete,Rename}Dentry and Commit{Delete,Rename*}Dentry
 //         VirtualFilesystem.filesystemsMu
 //       fdnotifier.notifier.mu
-//         EpollInstance.mu
-//           Locks acquired by FileDescriptionImpl.Readiness
+//         EpollInstance.readyMu
 //       Inotify.mu
 //         Watches.mu
 //           Inotify.evMu

test/syscalls/linux/BUILD

@@ -610,6 +610,7 @@ cc_binary(
         "//test/util:file_descriptor",
         gtest,
         "//test/util:posix_error",
+        "//test/util:signal_util",
         "//test/util:temp_path",
         "//test/util:test_main",
         "//test/util:test_util",

test/syscalls/linux/epoll.cc

@@ -21,6 +21,7 @@
 #include <string.h>
 #include <sys/epoll.h>
 #include <sys/eventfd.h>
+#include <sys/signalfd.h>
 #include <time.h>
 #include <unistd.h>
 
@@ -29,6 +30,7 @@
 #include "test/util/eventfd_util.h"
 #include "test/util/file_descriptor.h"
 #include "test/util/posix_error.h"
+#include "test/util/signal_util.h"
 #include "test/util/temp_path.h"
 #include "test/util/test_util.h"
 #include "test/util/thread_util.h"
@@ -533,7 +535,7 @@ TEST(EpollTest, DoubleLayerEpoll) {
   }
 }
 
-TEST(EPollTest, RegularFiles) {
+TEST(EpollTest, RegularFiles) {
   auto epollfd = ASSERT_NO_ERRNO_AND_VALUE(NewEpollFD());
 
   struct epoll_event event;
@@ -546,6 +548,54 @@ TEST(EPollTest, RegularFiles) {
               SyscallFailsWithErrno(EPERM));
 }
 
+// Regression test for b/222369818.
+TEST(EpollTest, ReadyMutexCircularity) {
+  constexpr int kSignal = SIGUSR1;
+  sigset_t set;
+  sigemptyset(&set);
+  sigaddset(&set, kSignal);
+  auto cleanup_sigmask =
+      ASSERT_NO_ERRNO_AND_VALUE(ScopedSignalMask(SIG_BLOCK, set));
+  int sigfd_raw;
+  ASSERT_THAT(sigfd_raw = signalfd(-1 /* fd */, &set, SFD_NONBLOCK),
+              SyscallSucceeds());
+  FileDescriptor sigfd(sigfd_raw);
+
+  auto epollfd = ASSERT_NO_ERRNO_AND_VALUE(NewEpollFD());
+  ASSERT_NO_ERRNO(RegisterEpollFD(epollfd.get(), sigfd.get(), EPOLLIN, 0));
+
+  // The test passes if this does not deadlock.
+  constexpr int kIterations = 25000;
+  auto pid = getpid();
+  auto tid = gettid();
+  DisableSave ds;
+  ScopedThread sender_thread([&] {
+    for (int i = 0; i < kIterations; i++) {
+      ASSERT_THAT(tgkill(pid, tid, kSignal), SyscallSucceeds());
+    }
+  });
+  int num_signals = 0;
+  signalfd_siginfo info;
+  while (true) {
+    struct epoll_event ev;
+    int ret = RetryEINTR(epoll_wait)(epollfd.get(), &ev, 1, 1000 /* timeout */);
+    ASSERT_THAT(ret, SyscallSucceeds());
+    if (ret == 0) {
+      break;
+    }
+    ASSERT_THAT(read(sigfd.get(), &info, sizeof(info)),
+                SyscallSucceedsWithValue(sizeof(info)));
+    num_signals++;
+  }
+  EXPECT_GT(num_signals, 0);
+  sender_thread.Join();
+  // epoll_wait() may have timed out before sender_thread finished executing
+  // (possible on slower platforms like ptrace), so read from sigfd (which is
+  // non-blocking) one more time to potentially dequeue the signal before
+  // unmasking it in cleanup_sigmask's destructor.
+  read(sigfd.get(), &info, sizeof(info));
+}
+
 }  // namespace
 
 }  // namespace testing