From 74ed589042c4fc255d148fc5356dc7e3aa1693be Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 19 Nov 2025 10:54:42 -0800 Subject: [PATCH] syncs: add means of declare locking assumptions for debug mode validation Updates #17852 Change-Id: I42a64a990dcc8f708fa23a516a40731a19967aba Signed-off-by: Brad Fitzpatrick --- ipn/ipnlocal/local.go | 39 +++++++++++++++++++++++++++++++++++++++ syncs/mutex.go | 5 +++++ syncs/mutex_debug.go | 4 ++++ 3 files changed, 48 insertions(+) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 3e7054896..fbf34aa42 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -876,6 +876,7 @@ func (b *LocalBackend) initPrefsFromConfig(conf *conffile.Config) error { } func (b *LocalBackend) setStaticEndpointsFromConfigLocked(conf *conffile.Config) { + syncs.RequiresMutex(&b.mu) if conf.Parsed.StaticEndpoints == nil && (b.conf == nil || b.conf.Parsed.StaticEndpoints == nil) { return } @@ -894,6 +895,7 @@ func (b *LocalBackend) setStaticEndpointsFromConfigLocked(conf *conffile.Config) } func (b *LocalBackend) setStateLocked(state ipn.State) { + syncs.RequiresMutex(&b.mu) if b.state == state { return } @@ -906,6 +908,7 @@ func (b *LocalBackend) setStateLocked(state ipn.State) { // setConfigLocked uses the provided config to update the backend's prefs // and other state. func (b *LocalBackend) setConfigLocked(conf *conffile.Config) error { + syncs.RequiresMutex(&b.mu) p := b.pm.CurrentPrefs().AsStruct() mp, err := conf.Parsed.ToPrefs() if err != nil { @@ -927,6 +930,7 @@ var assumeNetworkUpdateForTest = envknob.RegisterBool("TS_ASSUME_NETWORK_UP_FOR_ // // b.mu must be held. func (b *LocalBackend) pauseOrResumeControlClientLocked() { + syncs.RequiresMutex(&b.mu) if b.cc == nil { return } @@ -1204,6 +1208,7 @@ func (b *LocalBackend) Prefs() ipn.PrefsView { } func (b *LocalBackend) sanitizedPrefsLocked() ipn.PrefsView { + syncs.RequiresMutex(&b.mu) return stripKeysFromPrefs(b.pm.CurrentPrefs()) } @@ -1335,6 +1340,7 @@ func (b *LocalBackend) UpdateStatus(sb *ipnstate.StatusBuilder) { } func (b *LocalBackend) populatePeerStatusLocked(sb *ipnstate.StatusBuilder) { + syncs.RequiresMutex(&b.mu) cn := b.currentNode() nm := cn.NetMap() if nm == nil { @@ -1873,6 +1879,8 @@ func (b *LocalBackend) applySysPolicyLocked(prefs *ipn.Prefs) (anyChange bool) { if !buildfeatures.HasSystemPolicy { return false } + syncs.RequiresMutex(&b.mu) + if controlURL, err := b.polc.GetString(pkey.ControlURL, prefs.ControlURL); err == nil && prefs.ControlURL != controlURL { prefs.ControlURL = controlURL anyChange = true @@ -1941,6 +1949,8 @@ func (b *LocalBackend) applyExitNodeSysPolicyLocked(prefs *ipn.Prefs) (anyChange if !buildfeatures.HasUseExitNode { return false } + syncs.RequiresMutex(&b.mu) + if exitNodeIDStr, _ := b.polc.GetString(pkey.ExitNodeID, ""); exitNodeIDStr != "" { exitNodeID := tailcfg.StableNodeID(exitNodeIDStr) @@ -2182,6 +2192,8 @@ func (b *LocalBackend) resolveAutoExitNodeLocked(prefs *ipn.Prefs) (prefsChanged if !buildfeatures.HasUseExitNode { return false } + syncs.RequiresMutex(&b.mu) + // As of 2025-07-08, the only supported auto exit node expression is [ipn.AnyExitNode]. // // However, to maintain forward compatibility with future auto exit node expressions, @@ -2295,6 +2307,8 @@ func (b *LocalBackend) setWgengineStatus(s *wgengine.Status, err error) { // // b.mu must be held. func (b *LocalBackend) setWgengineStatusLocked(s *wgengine.Status) { + syncs.RequiresMutex(&b.mu) + es := b.parseWgStatusLocked(s) cc := b.cc @@ -4312,6 +4326,7 @@ func (b *LocalBackend) EditPrefsAs(mp *ipn.MaskedPrefs, actor ipnauth.Actor) (ip // // b.mu must be held. func (b *LocalBackend) checkEditPrefsAccessLocked(actor ipnauth.Actor, prefs ipn.PrefsView, mp *ipn.MaskedPrefs) error { + syncs.RequiresMutex(&b.mu) var errs []error if mp.RunSSHSet && mp.RunSSH && !envknob.CanSSHD() { @@ -4362,6 +4377,7 @@ func (b *LocalBackend) checkEditPrefsAccessLocked(actor ipnauth.Actor, prefs ipn // // b.mu must be held. func (b *LocalBackend) changeDisablesExitNodeLocked(prefs ipn.PrefsView, change *ipn.MaskedPrefs) bool { + syncs.RequiresMutex(&b.mu) if !buildfeatures.HasUseExitNode { return false } @@ -4403,6 +4419,7 @@ func (b *LocalBackend) changeDisablesExitNodeLocked(prefs ipn.PrefsView, change // // b.mu must be held. func (b *LocalBackend) adjustEditPrefsLocked(prefs ipn.PrefsView, mp *ipn.MaskedPrefs) { + syncs.RequiresMutex(&b.mu) // Zeroing the ExitNodeID via localAPI must also zero the prior exit node. if mp.ExitNodeIDSet && mp.ExitNodeID == "" && !mp.InternalExitNodePriorSet { mp.InternalExitNodePrior = "" @@ -4480,6 +4497,7 @@ func (b *LocalBackend) onEditPrefsLocked(_ ipnauth.Actor, mp *ipn.MaskedPrefs, o // startReconnectTimerLocked sets a timer to automatically set WantRunning to true // after the specified duration. func (b *LocalBackend) startReconnectTimerLocked(d time.Duration) { + syncs.RequiresMutex(&b.mu) if b.reconnectTimer != nil { // Stop may return false if the timer has already fired, // and the function has been called in its own goroutine, @@ -4522,11 +4540,13 @@ func (b *LocalBackend) startReconnectTimerLocked(d time.Duration) { } func (b *LocalBackend) resetAlwaysOnOverrideLocked() { + syncs.RequiresMutex(&b.mu) b.overrideAlwaysOn = false b.stopReconnectTimerLocked() } func (b *LocalBackend) stopReconnectTimerLocked() { + syncs.RequiresMutex(&b.mu) if b.reconnectTimer != nil { // Stop may return false if the timer has already fired, // and the function has been called in its own goroutine, @@ -4542,6 +4562,7 @@ func (b *LocalBackend) stopReconnectTimerLocked() { // b.mu must be held. func (b *LocalBackend) editPrefsLocked(actor ipnauth.Actor, mp *ipn.MaskedPrefs) (ipn.PrefsView, error) { + syncs.RequiresMutex(&b.mu) p0 := b.pm.CurrentPrefs() // Check if the changes in mp are allowed. @@ -5660,6 +5681,7 @@ func (b *LocalBackend) enterStateLocked(newState ipn.State) { } func (b *LocalBackend) hasNodeKeyLocked() bool { + syncs.RequiresMutex(&b.mu) // we can't use b.Prefs(), because it strips the keys, oops! p := b.pm.CurrentPrefs() return p.Valid() && p.Persist().Valid() && !p.Persist().PrivateNodeKey().IsZero() @@ -5680,9 +5702,11 @@ func (b *LocalBackend) NodeKey() key.NodePublic { // // b.mu must be held func (b *LocalBackend) nextStateLocked() ipn.State { + syncs.RequiresMutex(&b.mu) if b.health.IsUnhealthy(ipn.StateStoreHealth) { return ipn.NoState } + var ( cc = b.cc cn = b.currentNode() @@ -5758,6 +5782,8 @@ func (b *LocalBackend) nextStateLocked() ipn.State { // // requires b.mu to be held. func (b *LocalBackend) stateMachineLocked() { + syncs.RequiresMutex(&b.mu) + b.enterStateLocked(b.nextStateLocked()) } @@ -5767,6 +5793,7 @@ func (b *LocalBackend) stateMachineLocked() { // // b.mu must be held. func (b *LocalBackend) stopEngineAndWaitLocked() { + syncs.RequiresMutex(&b.mu) b.logf("stopEngineAndWait...") st, _ := b.e.ResetAndStop() // TODO: what should we do if this returns an error? b.setWgengineStatusLocked(st) @@ -5787,6 +5814,7 @@ func (b *LocalBackend) setControlClientLocked(cc controlclient.Client) { // returned value is non-nil, the caller must call Shutdown on it after // releasing b.mu. func (b *LocalBackend) resetControlClientLocked() controlclient.Client { + syncs.RequiresMutex(&b.mu) if b.cc == nil { return nil } @@ -5813,6 +5841,8 @@ func (b *LocalBackend) resetControlClientLocked() controlclient.Client { // resetAuthURLLocked resets authURL, canceling any pending interactive login. func (b *LocalBackend) resetAuthURLLocked() { + syncs.RequiresMutex(&b.mu) + b.authURL = "" b.authURLTime = time.Time{} b.authActor = nil @@ -5842,6 +5872,8 @@ func (b *LocalBackend) ShouldExposeRemoteWebClient() bool { // // b.mu must be held. func (b *LocalBackend) setWebClientAtomicBoolLocked(nm *netmap.NetworkMap) { + syncs.RequiresMutex(&b.mu) + shouldRun := !nm.HasCap(tailcfg.NodeAttrDisableWebClient) wasRunning := b.webClientAtomicBool.Swap(shouldRun) if wasRunning && !shouldRun { @@ -5854,6 +5886,8 @@ func (b *LocalBackend) setWebClientAtomicBoolLocked(nm *netmap.NetworkMap) { // // b.mu must be held. func (b *LocalBackend) setExposeRemoteWebClientAtomicBoolLocked(prefs ipn.PrefsView) { + syncs.RequiresMutex(&b.mu) + if !buildfeatures.HasWebClient { return } @@ -5982,6 +6016,8 @@ func (b *LocalBackend) RefreshExitNode() { // refreshExitNodeLocked is like RefreshExitNode but requires b.mu be held. func (b *LocalBackend) refreshExitNodeLocked() { + syncs.RequiresMutex(&b.mu) + if b.resolveExitNodeLocked() { b.authReconfigLocked() } @@ -5997,6 +6033,8 @@ func (b *LocalBackend) refreshExitNodeLocked() { // // b.mu must be held. func (b *LocalBackend) resolveExitNodeLocked() (changed bool) { + syncs.RequiresMutex(&b.mu) + if !buildfeatures.HasUseExitNode { return false } @@ -6058,6 +6096,7 @@ func (b *LocalBackend) reconcilePrefsLocked(prefs *ipn.Prefs) (changed bool) { // // b.mu must be held. func (b *LocalBackend) resolveExitNodeInPrefsLocked(prefs *ipn.Prefs) (changed bool) { + syncs.RequiresMutex(&b.mu) if !buildfeatures.HasUseExitNode { return false } diff --git a/syncs/mutex.go b/syncs/mutex.go index e61d1d1ab..8034e1712 100644 --- a/syncs/mutex.go +++ b/syncs/mutex.go @@ -16,3 +16,8 @@ type Mutex = sync.Mutex // // It's only not a sync.RWMutex when built with the ts_mutex_debug build tag. type RWMutex = sync.RWMutex + +// RequiresMutex declares the caller assumes it has the given +// mutex held. In non-debug builds, it's a no-op and compiles to +// nothing. +func RequiresMutex(mu *sync.Mutex) {} diff --git a/syncs/mutex_debug.go b/syncs/mutex_debug.go index 14b52ffe3..55a9b1231 100644 --- a/syncs/mutex_debug.go +++ b/syncs/mutex_debug.go @@ -15,4 +15,8 @@ type RWMutex struct { sync.RWMutex } +func RequiresMutex(mu *sync.Mutex) { + // TODO: check +} + // TODO(bradfitz): actually track stuff when in debug mode.