runtime: prevent preemption while timer is in timerModifying

Currently if a goroutine is preempted while owning a timer in the
timerModifying state, it could self-deadlock. When the goroutine is
preempted and calls into the scheduler, it could call checkTimers. If
checkTimers encounters the timerModifying timer and calls runtimer on
it, then runtimer will spin, waiting for that timer to leave the
timerModifying state, which it never will.

So far we got lucky that for the most part that there were no preemption
points while timerModifying is happening, however CL 221077 seems to
have introduced one, leading to sporadic self-deadlocks.

This change disables preemption explicitly while a goroutines holds a
timer in timerModifying. Since only checkTimers (and thus runtimer) is
called from the scheduler, this is sufficient to prevent
preemption-based self-deadlocks.

Fixes #38070.
Updates #37894.

Change-Id: Idbfac310889c92773023733ff7e2ff87e9896f0c
Reviewed-on: https://go-review.googlesource.com/c/go/+/225497
Run-TryBot: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/runtime/time.go b/src/runtime/time.go
index 50e3d4b60bd11644f16e6c07a451b9e648dee394..208fbf64c7cb618016d2233018e06b4e87c04b83 100644 (file)
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -292,6 +292,9 @@ func deltimer(t *timer) bool {
        for {
                switch s := atomic.Load(&t.status); s {
                case timerWaiting, timerModifiedLater:
+                       // Prevent preemption while the timer is in timerModifying.
+                       // This could lead to a self-deadlock. See #38070.
+                       mp := acquirem()
                        if atomic.Cas(&t.status, s, timerModifying) {
                                // Must fetch t.pp before changing status,
                                // as cleantimers in another goroutine
@@ -300,11 +303,17 @@ func deltimer(t *timer) bool {
                                if !atomic.Cas(&t.status, timerModifying, timerDeleted) {
                                        badTimer()
                                }
+                               releasem(mp)
                                atomic.Xadd(&tpp.deletedTimers, 1)
                                // Timer was not yet run.
                                return true
+                       } else {
+                               releasem(mp)
                        }
                case timerModifiedEarlier:
+                       // Prevent preemption while the timer is in timerModifying.
+                       // This could lead to a self-deadlock. See #38070.
+                       mp := acquirem()
                        if atomic.Cas(&t.status, s, timerModifying) {
                                // Must fetch t.pp before setting status
                                // to timerDeleted.
@@ -313,9 +322,12 @@ func deltimer(t *timer) bool {
                                if !atomic.Cas(&t.status, timerModifying, timerDeleted) {
                                        badTimer()
                                }
+                               releasem(mp)
                                atomic.Xadd(&tpp.deletedTimers, 1)
                                // Timer was not yet run.
                                return true
+                       } else {
+                               releasem(mp)
                        }
                case timerDeleted, timerRemoving, timerRemoved:
                        // Timer was already run.
@@ -398,25 +410,39 @@ func modtimer(t *timer, when, period int64, f func(interface{}, uintptr), arg in
 
        status := uint32(timerNoStatus)
        wasRemoved := false
+       var mp *m
 loop:
        for {
                switch status = atomic.Load(&t.status); status {
                case timerWaiting, timerModifiedEarlier, timerModifiedLater:
+                       // Prevent preemption while the timer is in timerModifying.
+                       // This could lead to a self-deadlock. See #38070.
+                       mp = acquirem()
                        if atomic.Cas(&t.status, status, timerModifying) {
                                break loop
                        }
+                       releasem(mp)
                case timerNoStatus, timerRemoved:
+                       // Prevent preemption while the timer is in timerModifying.
+                       // This could lead to a self-deadlock. See #38070.
+                       mp = acquirem()
+
                        // Timer was already run and t is no longer in a heap.
                        // Act like addtimer.
                        if atomic.Cas(&t.status, status, timerModifying) {
                                wasRemoved = true
                                break loop
                        }
+                       releasem(mp)
                case timerDeleted:
+                       // Prevent preemption while the timer is in timerModifying.
+                       // This could lead to a self-deadlock. See #38070.
+                       mp = acquirem()
                        if atomic.Cas(&t.status, status, timerModifying) {
                                atomic.Xadd(&t.pp.ptr().deletedTimers, -1)
                                break loop
                        }
+                       releasem(mp)
                case timerRunning, timerRemoving, timerMoving:
                        // The timer is being run or moved, by a different P.
                        // Wait for it to complete.
@@ -444,6 +470,7 @@ loop:
                if !atomic.Cas(&t.status, timerModifying, timerWaiting) {
                        badTimer()
                }
+               releasem(mp)
                wakeNetPoller(when)
        } else {
                // The timer is in some other P's heap, so we can't change
@@ -476,6 +503,7 @@ loop:
                if !atomic.Cas(&t.status, timerModifying, newStatus) {
                        badTimer()
                }
+               releasem(mp)
 
                // If the new status is earlier, wake up the poller.
                if newStatus == timerModifiedEarlier {