runtime: always call stackfree on the system stack

Currently when the garbage collector frees stacks of dead goroutines
in markrootFreeGStacks, it calls stackfree on a regular user stack.
This is a problem, since stackfree manipulates the stack cache in the
per-P mcache, so if it grows the stack or gets preempted in the middle
of manipulating the stack cache (which are both possible since it's on
a user stack), it can easily corrupt the stack cache.

Fix this by calling markrootFreeGStacks on the system stack, so that
all calls to stackfree happen on the system stack. To prevent this bug
in the future, mark stack functions that manipulate the mcache as
go:systemstack.

Fixes #15853.

Change-Id: Ic0d1c181efb342f134285a152560c3a074f14a3d
Reviewed-on: https://go-review.googlesource.com/23511
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go
index 2d0cbd203c219f10348a2e6a8aecf74b9ccf6d25..00b96fd00beed2d03157cd7f3965196d23b149fe 100644 (file)
--- a/src/runtime/mgcmark.go
+++ b/src/runtime/mgcmark.go
@@ -174,7 +174,9 @@ func markroot(gcw *gcWork, i uint32) {
                // Only do this once per GC cycle; preferably
                // concurrently.
                if !work.markrootDone {
-                       markrootFreeGStacks()
+                       // Switch to the system stack so we can call
+                       // stackfree.
+                       systemstack(markrootFreeGStacks)
                }
 
        case baseSpans <= i && i < baseStacks:

diff --git a/src/runtime/stack.go b/src/runtime/stack.go
index 8e344cdf03585c3d07e9c9ee95b3d7dfcb28c744..ee2797e14433007c8edfdcc8d50d0b390833aadc 100644 (file)
--- a/src/runtime/stack.go
+++ b/src/runtime/stack.go
@@ -251,6 +251,8 @@ func stackpoolfree(x gclinkptr, order uint8) {
 
 // stackcacherefill/stackcacherelease implement a global pool of stack segments.
 // The pool is required to prevent unlimited growth of per-thread caches.
+//
+//go:systemstack
 func stackcacherefill(c *mcache, order uint8) {
        if stackDebug >= 1 {
                print("stackcacherefill order=", order, "\n")
@@ -272,6 +274,7 @@ func stackcacherefill(c *mcache, order uint8) {
        c.stackcache[order].size = size
 }
 
+//go:systemstack
 func stackcacherelease(c *mcache, order uint8) {
        if stackDebug >= 1 {
                print("stackcacherelease order=", order, "\n")
@@ -290,6 +293,7 @@ func stackcacherelease(c *mcache, order uint8) {
        c.stackcache[order].size = size
 }
 
+//go:systemstack
 func stackcache_clear(c *mcache) {
        if stackDebug >= 1 {
                print("stackcache clear\n")
@@ -308,6 +312,12 @@ func stackcache_clear(c *mcache) {
        unlock(&stackpoolmu)
 }
 
+// stackalloc allocates an n byte stack.
+//
+// stackalloc must run on the system stack because it uses per-P
+// resources and must not split the stack.
+//
+//go:systemstack
 func stackalloc(n uint32) (stack, []stkbar) {
        // Stackalloc must be called on scheduler stack, so that we
        // never try to grow the stack during the code that stackalloc runs.
@@ -405,6 +415,12 @@ func stackalloc(n uint32) (stack, []stkbar) {
        return stack{uintptr(v), uintptr(v) + top}, *(*[]stkbar)(unsafe.Pointer(&stkbarSlice))
 }
 
+// stackfree frees an n byte stack allocation at stk.
+//
+// stackfree must run on the system stack because it uses per-P
+// resources and must not split the stack.
+//
+//go:systemstack
 func stackfree(stk stack, n uintptr) {
        gp := getg()
        v := unsafe.Pointer(stk.lo)