[release-branch.go1.21] 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.

For #62440.
Fixes #67297.

Cq-Include-Trybots: luci.golang.try:go1.21-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>
(cherry picked from commit 1ffc296717ba27d5ca5dd9a81439a49367363c89)
Reviewed-on: https://go-review.googlesource.com/c/go/+/585337

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 f3fead9c9eca6a2f9845e4decf875cd926d52880..7b2ac61fe71d468393bf741024e15cf2946ccbdd 100644 (file)
--- a/src/runtime/cgo/gcc_stack_unix.c
+++ b/src/runtime/cgo/gcc_stack_unix.c
@@ -35,6 +35,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 d226c2e907fd687ddab0cd4e96fef5eabebff3fe..6e96b2c2e243f4530bd1bc35914d7ecc5c786ef5 100644 (file)
--- a/src/runtime/cgocall.go
+++ b/src/runtime/cgocall.go
@@ -214,15 +214,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.
@@ -239,12 +242,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