runtime: in traceback, only jump stack if M doesn't change

CL 424257 modified gentraceback to switch gp when jumping from a
system stack to a user stack to simplify reasoning through the rest of
the function. This has the unintended side-effect of also switching
all references to gp.m. The vast majority of the time, g0.m and curg.m
are the same across a stack switch, making this a no-op, but there's
at least one case where this isn't true: if a profiling signal happens
in execute between setting mp.curg and setting gp.m. In this case,
mp.curg.m is briefly nil, which can cause gentraceback to crash with a
nil pointer dereference. We see this failure (surprisingly
frequently!) in profiling tests in the morestack=mayMoreStackPreempt
testing mode (#48297).

Fix this by making only jumping stacks if doing so will not switch Ms.
This restores the original property that gp.m doesn't change across
the stack jump, and makes gentraceback a little more conservative
about jumping stacks.

Fixes #54885.

Change-Id: Ib1524c41c748eeff35896e0f3abf9a7efbe5969f
Reviewed-on: https://go-review.googlesource.com/c/go/+/428656
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Austin Clements <austin@google.com>

diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go
index 4cc5eb91c820180bcb4b5c4784f68eaa6660a773..286e9c610ec932d9f390a63ac40f5d92df371086 100644 (file)
--- a/src/runtime/traceback.go
+++ b/src/runtime/traceback.go
@@ -159,7 +159,10 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in
                if frame.fp == 0 {
                        // Jump over system stack transitions. If we're on g0 and there's a user
                        // goroutine, try to jump. Otherwise this is a regular call.
-                       if flags&_TraceJumpStack != 0 && gp == gp.m.g0 && gp.m.curg != nil {
+                       // We also defensively check that this won't switch M's on us,
+                       // which could happen at critical points in the scheduler.
+                       // This ensures gp.m doesn't change from a stack jump.
+                       if flags&_TraceJumpStack != 0 && gp == gp.m.g0 && gp.m.curg != nil && gp.m.curg.m == gp.m {
                                switch f.funcID {
                                case funcID_morestack:
                                        // morestack does not return normally -- newstack()