From 5e6fc2f225c4c9811b6086578149be4944364dde Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Fri, 4 Mar 2022 11:05:24 -0800 Subject: [PATCH] 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 | 17 +++++ pkg/sentry/kernel/epoll/epoll.go | 100 +++++++++++++++++++------- pkg/sentry/vfs/epoll.go | 118 +++++++++++++++++++++++-------- pkg/sentry/vfs/vfs.go | 5 +- test/syscalls/linux/BUILD | 1 + test/syscalls/linux/epoll.cc | 52 +++++++++++++- 6 files changed, 233 insertions(+), 60 deletions(-) diff --git a/pkg/ilist/list.go b/pkg/ilist/list.go index 557051d18..7405ed6cb 100644 --- a/pkg/ilist/list.go +++ b/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 diff --git a/pkg/sentry/kernel/epoll/epoll.go b/pkg/sentry/kernel/epoll/epoll.go index f46acafc7..fa0be9b5a 100644 --- a/pkg/sentry/kernel/epoll/epoll.go +++ b/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 } diff --git a/pkg/sentry/vfs/epoll.go b/pkg/sentry/vfs/epoll.go index b6d35bffd..e338e5f77 100644 --- a/pkg/sentry/vfs/epoll.go +++ b/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 } diff --git a/pkg/sentry/vfs/vfs.go b/pkg/sentry/vfs/vfs.go index 9c1ee549a..a8b8d9ae5 100644 --- a/pkg/sentry/vfs/vfs.go +++ b/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 diff --git a/test/syscalls/linux/BUILD b/test/syscalls/linux/BUILD index ef0143343..6f2f70bcc 100644 --- a/test/syscalls/linux/BUILD +++ b/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", diff --git a/test/syscalls/linux/epoll.cc b/test/syscalls/linux/epoll.cc index 95e1dabf8..b4a612cbd 100644 --- a/test/syscalls/linux/epoll.cc +++ b/test/syscalls/linux/epoll.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -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