From 81d384cfe9d38e940e73ad7dad3a8e4de5f06086 Mon Sep 17 00:00:00 2001 From: Jamie Liu Date: Mon, 7 Mar 2022 10:40:14 -0800 Subject: [PATCH] Fix race between epoll readiness check and re-readying. PiperOrigin-RevId: 432982236 --- pkg/sentry/kernel/epoll/epoll.go | 40 +++++++++++++++++++++++----- pkg/sentry/vfs/epoll.go | 45 +++++++++++++++++++++++++------- 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/pkg/sentry/kernel/epoll/epoll.go b/pkg/sentry/kernel/epoll/epoll.go index fa0be9b5a..fca9f7a91 100644 --- a/pkg/sentry/kernel/epoll/epoll.go +++ b/pkg/sentry/kernel/epoll/epoll.go @@ -77,6 +77,8 @@ type pollEntry struct { // in-struct pointers. Instead, EventPoll will properly set this field // in its loading logic. curList *pollEntryList `state:"nosave"` + + readySeq uint32 } // WeakRefGone implements refs.WeakRefUser.WeakRefGone. @@ -130,6 +132,12 @@ type EventPoll struct { readyList pollEntryList waitingList pollEntryList disabledList pollEntryList + + // readySeq is used to detect calls to pollEntry.NotifyEvent() while + // eventsAvailable() or ReadEvents() are running with listsMu unlocked. + // readySeq is protected by both mu and listsMu; reading requires either + // mutex to be locked, but mutation requires both mutexes to be locked. + readySeq uint32 } // cycleMu is used to serialize all the cycle checks. This is only used when @@ -198,6 +206,7 @@ func (e *EventPoll) eventsAvailable() bool { ) e.listsMu.Lock() readyList.PushBackList(&e.readyList) + e.readySeq++ e.listsMu.Unlock() if readyList.Empty() { return false @@ -205,8 +214,16 @@ func (e *EventPoll) eventsAvailable() bool { defer func() { e.listsMu.Lock() e.readyList.PushFrontList(&readyList) - for entry := waitingList.Front(); entry != nil; entry = entry.Next() { - entry.curList = &e.waitingList + var next *pollEntry + for entry := waitingList.Front(); entry != nil; entry = next { + next = entry.Next() + if entry.readySeq == e.readySeq { + // entry.NotifyEvent() was called while we were running. + waitingList.Remove(entry) + e.readyList.PushBack(entry) + } else { + entry.curList = &e.waitingList + } } e.waitingList.PushBackList(&waitingList) e.listsMu.Unlock() @@ -257,13 +274,14 @@ func (e *EventPoll) ReadEvents(max int) []linux.EpollEvent { // pollEntry.NotifyEvent() doesn't touch pollEntryEntry. var ( readyList pollEntryList - requeueList pollEntryList waitingList pollEntryList + requeueList pollEntryList disabledList pollEntryList ret []linux.EpollEvent ) e.listsMu.Lock() readyList.PushBackList(&e.readyList) + e.readySeq++ e.listsMu.Unlock() if readyList.Empty() { return nil @@ -271,10 +289,18 @@ func (e *EventPoll) ReadEvents(max int) []linux.EpollEvent { 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 + var next *pollEntry + for entry := waitingList.Front(); entry != nil; entry = next { + next = entry.Next() + if entry.readySeq == e.readySeq { + // entry.NotifyEvent() was called while we were running. + waitingList.Remove(entry) + e.readyList.PushBack(entry) + } else { + entry.curList = &e.waitingList + } } + e.readyList.PushBackList(&requeueList) e.waitingList.PushBackList(&waitingList) for entry := disabledList.Front(); entry != nil; entry = entry.Next() { entry.curList = &e.disabledList @@ -332,6 +358,8 @@ func (p *pollEntry) NotifyEvent(waiter.EventMask) { e.listsMu.Lock() + p.readySeq = e.readySeq + if p.curList == &e.waitingList { e.waitingList.Remove(p) e.readyList.PushBack(p) diff --git a/pkg/sentry/vfs/epoll.go b/pkg/sentry/vfs/epoll.go index e338e5f77..6041f8ff6 100644 --- a/pkg/sentry/vfs/epoll.go +++ b/pkg/sentry/vfs/epoll.go @@ -47,7 +47,7 @@ type EpollInstance struct { // EpollInstance for monitoring. interest map[epollInterestKey]*epollInterest - // readyMu protects ready, epollInterest.ready, and + // readyMu protects ready, readySeq, epollInterest.ready, and // epollInterest.epollInterestEntry. ready is analogous to Linux's struct // eventpoll::lock. readyMu sync.Mutex `state:"nosave"` @@ -61,6 +61,12 @@ type EpollInstance struct { // because it focuses on a set of file descriptors that are already known // to be ready." - epoll_wait(2) ready epollInterestList + + // readySeq is used to detect calls to epollInterest.NotifyEvent() while + // Readiness() or ReadEvents() are running with readyMu unlocked. readySeq + // is protected by both interestMu and readyMu; reading requires either + // mutex to be locked, but mutation requires both mutexes to be locked. + readySeq uint32 } // +stateify savable @@ -93,10 +99,12 @@ type epollInterest struct { // 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.readyMu. + // ready is true if epollInterestEntry is linked into epoll.ready. readySeq + // is the value of epoll.readySeq when NotifyEvent() was last called. + // ready, epollInterestEntry, and readySeq are protected by epoll.readyMu. ready bool epollInterestEntry + readySeq uint32 // userData is the struct epoll_event::data associated with this // epollInterest. userData is protected by epoll.interestMu. @@ -156,6 +164,7 @@ func (ep *EpollInstance) Readiness(mask waiter.EventMask) waiter.EventMask { ) ep.readyMu.Lock() ready.PushBackList(&ep.ready) + ep.readySeq++ ep.readyMu.Unlock() if ready.Empty() { return 0 @@ -163,8 +172,16 @@ func (ep *EpollInstance) Readiness(mask waiter.EventMask) waiter.EventMask { defer func() { ep.readyMu.Lock() ep.ready.PushFrontList(&ready) - for epi := notReady.Front(); epi != nil; epi = epi.Next() { - epi.ready = false + var next *epollInterest + for epi := notReady.Front(); epi != nil; epi = next { + next = epi.Next() + if epi.readySeq == ep.readySeq { + // epi.NotifyEvent() was called while we were running. + notReady.Remove(epi) + ep.ready.PushBack(epi) + } else { + epi.ready = false + } } ep.readyMu.Unlock() }() @@ -369,6 +386,7 @@ func (epi *epollInterest) NotifyEvent(waiter.EventMask) { epi.ready = true epi.epoll.ready.PushBack(epi) } + epi.readySeq = epi.epoll.readySeq epi.epoll.readyMu.Unlock() if newReady { epi.epoll.q.Notify(waiter.ReadableEvents) @@ -399,11 +417,12 @@ func (ep *EpollInstance) ReadEvents(events []linux.EpollEvent, maxEvents int) [] defer ep.interestMu.Unlock() var ( ready epollInterestList - requeue epollInterestList notReady epollInterestList + requeue epollInterestList ) ep.readyMu.Lock() ready.PushBackList(&ep.ready) + ep.readySeq++ ep.readyMu.Unlock() if ready.Empty() { return nil @@ -414,10 +433,18 @@ func (ep *EpollInstance) ReadEvents(events []linux.EpollEvent, maxEvents int) [] // 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 + var next *epollInterest + for epi := notReady.Front(); epi != nil; epi = next { + next = epi.Next() + if epi.readySeq == ep.readySeq { + // epi.NotifyEvent() was called while we were running. + notReady.Remove(epi) + ep.ready.PushBack(epi) + } else { + epi.ready = false + } } + ep.ready.PushBackList(&requeue) ep.readyMu.Unlock() }()