runtime: always update stack bounds on cgocallback

callbackUpdateSystemStack contains a fast path to exit early without
update if SP is already within the g0.stack bounds.

This is not safe, as a subsequent call may have new stack bounds that
only partially overlap the old stack bounds. In this case it is possible
to see an SP that is in the old stack bounds, but very close to the
bottom of the bounds due to the partial overlap. In that case we're very
likely to "run out" of space on the system stack.

We only need to do this on extra Ms, as normal Ms have precise bounds
defined when we allocated the stack.

TSAN annotations are added to x_cgo_getstackbounds because bounds is a
pointer into the Go stack. The stack can be reused when an old thread
exits and a new thread starts, but TSAN can't see the synchronization
there. This isn't a new case, but we are now calling more often.

Fixes #62440.

Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest
Change-Id: I5389050494987b7668d0b317fb92f85e61d798ac
Reviewed-on: https://go-review.googlesource.com/c/go/+/584597
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/runtime/cgo/gcc_stack_darwin.c b/src/runtime/cgo/gcc_stack_darwin.c
index 0a9038eb3bcb85dafb0036f25852b68a3081176e..28364c7420e992fecc6955a813b8a8751a2284ec 100644 (file)
--- a/src/runtime/cgo/gcc_stack_darwin.c
+++ b/src/runtime/cgo/gcc_stack_darwin.c
@@ -15,6 +15,11 @@ x_cgo_getstackbound(uintptr bounds[2])
        p = pthread_self();
        addr = pthread_get_stackaddr_np(p); // high address (!)
        size = pthread_get_stacksize_np(p);
+
+       // bounds points into the Go stack. TSAN can't see the synchronization
+       // in Go around stack reuse.
+       _cgo_tsan_acquire();
        bounds[0] = (uintptr)addr - size;
        bounds[1] = (uintptr)addr;
+       _cgo_tsan_release();
 }

diff --git a/src/runtime/cgo/gcc_stack_unix.c b/src/runtime/cgo/gcc_stack_unix.c
index 67efd9bc6314127b3a8ec1dbd921094df2c44f1b..eb1d7f9ec5f862662a1714f1c98affd7428bcccc 100644 (file)
--- a/src/runtime/cgo/gcc_stack_unix.c
+++ b/src/runtime/cgo/gcc_stack_unix.c
@@ -37,6 +37,10 @@ x_cgo_getstackbound(uintptr bounds[2])
 #endif
        pthread_attr_destroy(&attr);
 
+       // bounds points into the Go stack. TSAN can't see the synchronization
+       // in Go around stack reuse.
+       _cgo_tsan_acquire();
        bounds[0] = (uintptr)addr;
        bounds[1] = (uintptr)addr + size;
+       _cgo_tsan_release();
 }

diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go
index 8f09b6831b5d1ba3760a478342f88fd05a0c89ad..071643614bd8ebcae3444e24c80924fe81ff02be 100644 (file)
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -221,15 +221,18 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
 //go:nosplit
 func callbackUpdateSystemStack(mp *m, sp uintptr, signal bool) {
        g0 := mp.g0
-       if sp > g0.stack.lo && sp <= g0.stack.hi {
-               // Stack already in bounds, nothing to do.
-               return
-       }
 
-       if mp.ncgo > 0 {
+       inBound := sp > g0.stack.lo && sp <= g0.stack.hi
+       if mp.ncgo > 0 && !inBound {
                // ncgo > 0 indicates that this M was in Go further up the stack
-               // (it called C and is now receiving a callback). It is not
-               // safe for the C call to change the stack out from under us.
+               // (it called C and is now receiving a callback).
+               //
+               // !inBound indicates that we were called with SP outside the
+               // expected system stack bounds (C changed the stack out from
+               // under us between the cgocall and cgocallback?).
+               //
+               // It is not safe for the C call to change the stack out from
+               // under us, so throw.
 
                // Note that this case isn't possible for signal == true, as
                // that is always passing a new M from needm.
@@ -247,12 +250,26 @@ func callbackUpdateSystemStack(mp *m, sp uintptr, signal bool) {
                exit(2)
        }
 
+       if !mp.isextra {
+               // We allocated the stack for standard Ms. Don't replace the
+               // stack bounds with estimated ones when we already initialized
+               // with the exact ones.
+               return
+       }
+
        // This M does not have Go further up the stack. However, it may have
        // previously called into Go, initializing the stack bounds. Between
        // that call returning and now the stack may have changed (perhaps the
        // C thread is running a coroutine library). We need to update the
        // stack bounds for this case.
        //
+       // N.B. we need to update the stack bounds even if SP appears to
+       // already be in bounds. Our "bounds" may actually be estimated dummy
+       // bounds (below). The actual stack bounds could have shifted but still
+       // have partial overlap with our dummy bounds. If we failed to update
+       // in that case, we could find ourselves seemingly called near the
+       // bottom of the stack bounds, where we quickly run out of space.
+
        // Set the stack bounds to match the current stack. If we don't
        // actually know how big the stack is, like we don't know how big any
        // scheduling stack is, but we assume there's at least 32 kB. If we