runtime: release timersLock while running timer

Dan Scales pointed out a theoretical deadlock in the runtime.

The timer code runs timer functions while holding the timers lock for a P.
The scavenger queues up a timer function that calls wakeScavenger,
which acquires the scavenger lock.

The scavengeSleep function acquires the scavenger lock,
then calls resetTimer which can call addInitializedTimer
which acquires the timers lock for the current P.

So there is a potential deadlock, in that the scavenger lock and
the timers lock for some P may both be acquired in different order.
It's not clear to me whether this deadlock can ever actually occur.

Issue 35532 describes another possible deadlock.

The pollSetDeadline function acquires pd.lock for some poll descriptor,
and in some cases calls resettimer which can in some cases acquire
the timers lock for the current P.

The timer code runs timer functions while holding the timers lock for a P.
The timer function for poll descriptors winds up in netpolldeadlineimpl
which acquires pd.lock.

So again there is a potential deadlock, in that the pd lock for some
poll descriptor and the timers lock for some P may both be acquired in
different order. I think this can happen if we change the deadline
for a network connection exactly as the former deadline expires.

Looking at the code, I don't see any reason why we have to hold
the timers lock while running a timer function.
This CL implements that change.

Updates #6239
Updates #27707
Fixes #35532

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

1 files changed, 9 insertions, 2 deletions

diff --git a/src/runtime/time.go b/src/runtime/time.go
index 47b326289b..ed044b3b4c 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -1011,6 +1011,8 @@ func nobarrierWakeTime(pp *p) int64 {
 // Returns 0 if it ran a timer, -1 if there are no more timers, or the time
 // when the first timer should run.
 // The caller must have locked the timers for pp.
+// If a timer is run, this will temporarily unlock the timers.
+//go:systemstack
 func runtimer(pp *p, now int64) int64 {
 	for {
 		t := pp.timers[0]
@@ -1027,6 +1029,8 @@ func runtimer(pp *p, now int64) int64 {
 			if !atomic.Cas(&t.status, s, timerRunning) {
 				continue
 			}
+			// Note that runOneTimer may temporarily unlock
+			// pp.timersLock.
 			runOneTimer(pp, t, now)
 			return 0
 
@@ -1081,6 +1085,8 @@ func runtimer(pp *p, now int64) int64 {
 
 // runOneTimer runs a single timer.
 // The caller must have locked the timers for pp.
+// This will temporarily unlock the timers while running the timer function.
+//go:systemstack
 func runOneTimer(pp *p, t *timer, now int64) {
 	if raceenabled {
 		if pp.timerRaceCtx == 0 {
@@ -1122,11 +1128,12 @@ func runOneTimer(pp *p, t *timer, now int64) {
 		gp.racectx = pp.timerRaceCtx
 	}
 
-	// Note that since timers are locked here, f may not call
-	// addtimer or resettimer.
+	unlock(&pp.timersLock)
 
 	f(arg, seq)
 
+	lock(&pp.timersLock)
+
 	if raceenabled {
 		gp := getg()
 		gp.racectx = 0