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