runtime: always keep global reference to mp until mexit completes

Ms are allocated via standard heap allocation (`new(m)`), which means we
must keep them alive (i.e., reachable by the GC) until we are completely
done using them.

Ms are primarily reachable through runtime.allm. However, runtime.mexit
drops the M from allm fairly early, long before it is done using the M
structure. If that was the last reference to the M, it is now at risk of
being freed by the GC and used for some other allocation, leading to
memory corruption.

Ms with a Go-allocated stack coincidentally already keep a reference to
the M in sched.freem, so that the stack can be freed lazily. This
reference has the side effect of keeping this Ms reachable. However, Ms
with an OS stack skip this and are at risk of corruption.

Fix this lifetime by extending sched.freem use to all Ms, with the value
of mp.freeWait determining whether the stack needs to be freed or not.

Fixes #56243.

Change-Id: Ic0c01684775f5646970df507111c9abaac0ba52e
Reviewed-on: https://go-review.googlesource.com/c/go/+/443716
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

1 files changed, 28 insertions, 20 deletions

diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 02390375b5..4285ff6b7c 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -1582,19 +1582,18 @@ func mexit(osStack bool) {
 	}
 	throw("m not found in allm")
 found:
-	if !osStack {
-		// Delay reaping m until it's done with the stack.
-		//
-		// If this is using an OS stack, the OS will free it
-		// so there's no need for reaping.
-		atomic.Store(&mp.freeWait, 1)
-		// Put m on the free list, though it will not be reaped until
-		// freeWait is 0. Note that the free list must not be linked
-		// through alllink because some functions walk allm without
-		// locking, so may be using alllink.
-		mp.freelink = sched.freem
-		sched.freem = mp
-	}
+	// Delay reaping m until it's done with the stack.
+	//
+	// Put mp on the free list, though it will not be reaped while freeWait
+	// is freeMWait. mp is no longer reachable via allm, so even if it is
+	// on an OS stack, we must keep a reference to mp alive so that the GC
+	// doesn't free mp while we are still using it.
+	//
+	// Note that the free list must not be linked through alllink because
+	// some functions walk allm without locking, so may be using alllink.
+	mp.freeWait.Store(freeMWait)
+	mp.freelink = sched.freem
+	sched.freem = mp
 	unlock(&sched.lock)
 
 	atomic.Xadd64(&ncgocall, int64(mp.ncgocall))
@@ -1624,6 +1623,9 @@ found:
 	mdestroy(mp)
 
 	if osStack {
+		// No more uses of mp, so it is safe to drop the reference.
+		mp.freeWait.Store(freeMRef)
+
 		// Return from mstart and let the system thread
 		// library free the g0 stack and terminate the thread.
 		return
@@ -1795,19 +1797,25 @@ func allocm(pp *p, fn func(), id int64) *m {
 		lock(&sched.lock)
 		var newList *m
 		for freem := sched.freem; freem != nil; {
-			if freem.freeWait != 0 {
+			wait := freem.freeWait.Load()
+			if wait == freeMWait {
 				next := freem.freelink
 				freem.freelink = newList
 				newList = freem
 				freem = next
 				continue
 			}
-			// stackfree must be on the system stack, but allocm is
-			// reachable off the system stack transitively from
-			// startm.
-			systemstack(func() {
-				stackfree(freem.g0.stack)
-			})
+			// Free the stack if needed. For freeMRef, there is
+			// nothing to do except drop freem from the sched.freem
+			// list.
+			if wait == freeMStack {
+				// stackfree must be on the system stack, but allocm is
+				// reachable off the system stack transitively from
+				// startm.
+				systemstack(func() {
+					stackfree(freem.g0.stack)
+				})
+			}
 			freem = freem.freelink
 		}
 		sched.freem = newList