From: Michael Pratt Date: Fri, 11 Sep 2020 16:14:06 +0000 (-0400) Subject: runtime: drop nosplit from primary lockrank functions X-Git-Tag: go1.16beta1~929 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=989ab8a7d67c4111d71bd3a8bb2acbe38e16ff5b;p=gostls13.git runtime: drop nosplit from primary lockrank functions acquireLockRank and releaseLockRank are called from nosplit context, and thus must be nosplit. lockWithRank, unlockWithRank, and lockWithRankMayAcquire are called from spittable context, and thus don't strictly need to be nosplit. The stated reasoning for making these functions nosplit is to avoid re-entrant calls due to a stack split on function entry taking a lock. There are two potential issues at play here: 1. A stack split on function entry adds a new lock ordering edge before we (a) take lock l, or (b) release lock l. 2. A stack split in a child call (such as to lock2) introduces a new lock ordering edge _in the wrong order_ because e.g., in the case of lockWithRank, we've noted that l is taken, but the stack split in lock2 actually takes stack split locks _before_ l is actually locked. (1) is indeed avoided by marking these functions nosplit, but this is really just a bit of duct tape that generally has no effect overall. Any earlier call can have a stack split and introduce the same new edge. This includes lock/unlock which are not nosplit! I began this CL as a change to extend nosplit to lock and unlock to try to make this mitigation more effective, but I've realized that as long as there is a _single_ nosplit call between a lock and unlock, we can end up with the edge. There seems to be few enough cases without any calls that is does not seem worth the extra cognitive load to extend nosplit throughout all of the locking functions. (2) is a real issue which would cause incorrect ordering, but it is already handled by switching to the system stack before recording the lock ordering. Adding / removing nosplit has no effect on this issue. Change-Id: I94fbd21b2bf928dbf1bf71aabb6788fc0a012829 Reviewed-on: https://go-review.googlesource.com/c/go/+/254367 Run-TryBot: Michael Pratt TryBot-Result: Go Bot Reviewed-by: Dan Scales Trust: Michael Pratt --- diff --git a/src/runtime/lockrank_off.go b/src/runtime/lockrank_off.go index c04b61edc7..40edf882ee 100644 --- a/src/runtime/lockrank_off.go +++ b/src/runtime/lockrank_off.go @@ -18,30 +18,24 @@ func getLockRank(l *mutex) lockRank { return 0 } -// The following functions may be called in nosplit context. -// Nosplit is not strictly required for lockWithRank, unlockWithRank -// and lockWithRankMayAcquire, but these nosplit annotations must -// be kept consistent with the equivalent functions in lockrank_on.go. - -//go:nosplit func lockWithRank(l *mutex, rank lockRank) { lock2(l) } +// This function may be called in nosplit context and thus must be nosplit. //go:nosplit func acquireLockRank(rank lockRank) { } -//go:nosplit func unlockWithRank(l *mutex) { unlock2(l) } +// This function may be called in nosplit context and thus must be nosplit. //go:nosplit func releaseLockRank(rank lockRank) { } -//go:nosplit func lockWithRankMayAcquire(l *mutex, rank lockRank) { } diff --git a/src/runtime/lockrank_on.go b/src/runtime/lockrank_on.go index 850f7cdd38..db7ff23a58 100644 --- a/src/runtime/lockrank_on.go +++ b/src/runtime/lockrank_on.go @@ -40,15 +40,19 @@ func getLockRank(l *mutex) lockRank { return l.rank } -// The following functions are the entry-points to record lock -// operations. -// All of these are nosplit and switch to the system stack immediately -// to avoid stack growths. Since a stack growth could itself have lock -// operations, this prevents re-entrant calls. - // lockWithRank is like lock(l), but allows the caller to specify a lock rank // when acquiring a non-static lock. -//go:nosplit +// +// Note that we need to be careful about stack splits: +// +// This function is not nosplit, thus it may split at function entry. This may +// introduce a new edge in the lock order, but it is no different from any +// other (nosplit) call before this call (including the call to lock() itself). +// +// However, we switch to the systemstack to record the lock held to ensure that +// we record an accurate lock ordering. e.g., without systemstack, a stack +// split on entry to lock2() would record stack split locks as taken after l, +// even though l is not actually locked yet. func lockWithRank(l *mutex, rank lockRank) { if l == &debuglock || l == &paniclk { // debuglock is only used for println/printlock(). Don't do lock @@ -99,6 +103,8 @@ func printHeldLocks(gp *g) { } // acquireLockRank acquires a rank which is not associated with a mutex lock +// +// This function may be called in nosplit context and thus must be nosplit. //go:nosplit func acquireLockRank(rank lockRank) { gp := getg() @@ -154,7 +160,7 @@ func checkRanks(gp *g, prevRank, rank lockRank) { } } -//go:nosplit +// See comment on lockWithRank regarding stack splitting. func unlockWithRank(l *mutex) { if l == &debuglock || l == &paniclk { // See comment at beginning of lockWithRank. @@ -181,6 +187,8 @@ func unlockWithRank(l *mutex) { } // releaseLockRank releases a rank which is not associated with a mutex lock +// +// This function may be called in nosplit context and thus must be nosplit. //go:nosplit func releaseLockRank(rank lockRank) { gp := getg() @@ -201,7 +209,7 @@ func releaseLockRank(rank lockRank) { }) } -//go:nosplit +// See comment on lockWithRank regarding stack splitting. func lockWithRankMayAcquire(l *mutex, rank lockRank) { gp := getg() if gp.m.locksHeldLen == 0 {