runtime: prevent sigprof during all stack barrier ops

A sigprof during stack barrier insertion or removal can crash if it
detects an inconsistency between the stkbar array and the stack
itself. Currently we protect against this when scanning another G's
stack using stackLock, but we don't protect against it when unwinding
stack barriers for a recover or a memmove to the stack.

This commit cleans up and improves the stack locking code. It
abstracts out the lock and unlock operations. It uses the lock
consistently everywhere we perform stack operations, and pushes the
lock/unlock down closer to where the stack barrier operations happen
to make it more obvious what it's protecting. Finally, it modifies
sigprof so that instead of spinning until it acquires the lock, it
simply doesn't perform a traceback if it can't acquire it. This is
necessary to prevent self-deadlock.

Updates #11863, which introduced stackLock to fix some of these
issues, but didn't go far enough.

Updates #12528.

Change-Id: I9d1fa88ae3744d31ba91500c96c6988ce1a3a349
Reviewed-on: https://go-review.googlesource.com/17036
Reviewed-by: Russ Cox <rsc@golang.org>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go
index 07eb1901a049acdb253e7eb25d645b0c02e7b665..6deb09adbf9fe92cd4e788a827ac0f65321759d2 100644 (file)
--- a/src/runtime/mgcmark.go
+++ b/src/runtime/mgcmark.go
@@ -620,6 +620,8 @@ func scanstack(gp *g) {
                        throw("g already has stack barriers")
                }
 
+               gcLockStackBarriers(gp)
+
        case _GCmarktermination:
                if int(gp.stkbarPos) == len(gp.stkbar) {
                        // gp hit all of the stack barriers (or there
@@ -675,6 +677,9 @@ func scanstack(gp *g) {
        if gcphase == _GCmarktermination {
                gcw.dispose()
        }
+       if gcphase == _GCmark {
+               gcUnlockStackBarriers(gp)
+       }
        gp.gcscanvalid = true
 }
 

diff --git a/src/runtime/mstkbar.go b/src/runtime/mstkbar.go
index 964e09ed0a26e486be50c801a6d983ad304d5ded..6f5b4598538a2cb1b47640d795456746aba15b67 100644 (file)
--- a/src/runtime/mstkbar.go
+++ b/src/runtime/mstkbar.go
@@ -106,6 +106,7 @@
 package runtime
 
 import (
+       "runtime/internal/atomic"
        "runtime/internal/sys"
        "unsafe"
 )
@@ -199,6 +200,8 @@ func gcRemoveStackBarriers(gp *g) {
                print("hit ", gp.stkbarPos, " stack barriers, goid=", gp.goid, "\n")
        }
 
+       gcLockStackBarriers(gp)
+
        // Remove stack barriers that we didn't hit.
        for _, stkbar := range gp.stkbar[gp.stkbarPos:] {
                gcRemoveStackBarrier(gp, stkbar)
@@ -208,6 +211,8 @@ func gcRemoveStackBarriers(gp *g) {
        // adjust them.
        gp.stkbarPos = 0
        gp.stkbar = gp.stkbar[:0]
+
+       gcUnlockStackBarriers(gp)
 }
 
 // gcRemoveStackBarrier removes a single stack barrier. It is the
@@ -254,6 +259,7 @@ func gcPrintStkbars(stkbar []stkbar) {
 //
 //go:nosplit
 func gcUnwindBarriers(gp *g, sp uintptr) {
+       gcLockStackBarriers(gp)
        // On LR machines, if there is a stack barrier on the return
        // from the frame containing sp, this will mark it as hit even
        // though it isn't, but it's okay to be conservative.
@@ -262,6 +268,7 @@ func gcUnwindBarriers(gp *g, sp uintptr) {
                gcRemoveStackBarrier(gp, gp.stkbar[gp.stkbarPos])
                gp.stkbarPos++
        }
+       gcUnlockStackBarriers(gp)
        if debugStackBarrier && gp.stkbarPos != before {
                print("skip barriers below ", hex(sp), " in goid=", gp.goid, ": ")
                gcPrintStkbars(gp.stkbar[before:gp.stkbarPos])
@@ -284,3 +291,25 @@ func setNextBarrierPC(pc uintptr) {
        gp := getg()
        gp.stkbar[gp.stkbarPos].savedLRVal = pc
 }
+
+// gcLockStackBarriers synchronizes with tracebacks of gp's stack
+// during sigprof for installation or removal of stack barriers. It
+// blocks until any current sigprof is done tracebacking gp's stack
+// and then disallows profiling tracebacks of gp's stack.
+//
+// This is necessary because a sigprof during barrier installation or
+// removal could observe inconsistencies between the stkbar array and
+// the stack itself and crash.
+func gcLockStackBarriers(gp *g) {
+       for !atomic.Cas(&gp.stackLock, 0, 1) {
+               osyield()
+       }
+}
+
+func gcTryLockStackBarriers(gp *g) bool {
+       return atomic.Cas(&gp.stackLock, 0, 1)
+}
+
+func gcUnlockStackBarriers(gp *g) {
+       atomic.Store(&gp.stackLock, 0)
+}

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index aaabfae7b864963216b38b3c4a68593b0edadd73..f47c4bef5b2f3f7401a50c89b3e49b3d5bdf623a 100644 (file)
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -735,13 +735,7 @@ func scang(gp *g) {
                        // the goroutine until we're done.
                        if castogscanstatus(gp, s, s|_Gscan) {
                                if !gp.gcscandone {
-                                       // Coordinate with traceback
-                                       // in sigprof.
-                                       for !atomic.Cas(&gp.stackLock, 0, 1) {
-                                               osyield()
-                                       }
                                        scanstack(gp)
-                                       atomic.Store(&gp.stackLock, 0)
                                        gp.gcscandone = true
                                }
                                restartg(gp)
@@ -2846,11 +2840,6 @@ func sigprof(pc, sp, lr uintptr, gp *g, mp *m) {
        // Profiling runs concurrently with GC, so it must not allocate.
        mp.mallocing++
 
-       // Coordinate with stack barrier insertion in scanstack.
-       for !atomic.Cas(&gp.stackLock, 0, 1) {
-               osyield()
-       }
-
        // Define that a "user g" is a user-created goroutine, and a "system g"
        // is one that is m->g0 or m->gsignal.
        //
@@ -2917,8 +2906,18 @@ func sigprof(pc, sp, lr uintptr, gp *g, mp *m) {
        // transition. We simply require that g and SP match and that the PC is not
        // in gogo.
        traceback := true
+       haveStackLock := false
        if gp == nil || sp < gp.stack.lo || gp.stack.hi < sp || setsSP(pc) {
                traceback = false
+       } else if gp.m.curg != nil {
+               if gcTryLockStackBarriers(gp.m.curg) {
+                       haveStackLock = true
+               } else {
+                       // Stack barriers are being inserted or
+                       // removed, so we can't get a consistent
+                       // traceback right now.
+                       traceback = false
+               }
        }
        var stk [maxCPUProfStack]uintptr
        n := 0
@@ -2961,7 +2960,9 @@ func sigprof(pc, sp, lr uintptr, gp *g, mp *m) {
                        }
                }
        }
-       atomic.Store(&gp.stackLock, 0)
+       if haveStackLock {
+               gcUnlockStackBarriers(gp.m.curg)
+       }
 
        if prof.hz != 0 {
                // Simple cas-lock to coordinate with setcpuprofilerate.