archive
/
tailscale
mirror of https://github.com/tailscale/tailscale/
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

ipn/ipnlocal: replace the LockedOnEntry pattern with conventional lock/unlock discipline (#16925)

Browse Source 
There are several methods within the LocalBackend that used an unusual and
error-prone lock discipline whereby they require the caller to hold the backend
mutex on entry, but release it on the way out.

In #11650 we added some support code to make this pattern more visible.
Now it is time to eliminate the pattern (at least within this package).
This is intended to produce no semantic changes, though I am relying on
integration tests and careful inspection to achieve that.

To the extent possible I preserved the existing control flow. In a few places,
however, I replaced this with an unlock/lock closure. This means we will
sometimes reacquire a lock only to release it again one frame up the stack, but
these operations are not performance sensitive and the legibility gain seems
worthwhile.

We can probably also pull some of these out into separate methods, but I did
not do that here so as to avoid other variable scope changes that might be hard
to see. I would like to do some more cleanup separately.

As a follow-up, we could also remove the unlockOnce helper, but I did not do
that here either.

Updates #11649

Change-Id: I4c92d4536eca629cfcd6187528381c33f4d64e20
Signed-off-by: M. J. Fromberger <fromberger@tailscale.com>
pull/16931/head
M. J. Fromberger 3 months ago committed by GitHub
parent fa0e83ab4f
commit 6c8fef961e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 173 additions and 160 deletions
Show Stats Download Patch File Download Diff File

316
ipn/ipnlocal/local.go
Unescape Escape View File

@ -806,7 +806,7 @@ func (b *LocalBackend) ReloadConfig() (ok bool, err error) {
if err != nil { if err != nil {
return false, err return false, err
} }
if err := b.setConfigLockedOnEntry(conf, unlock); err != nil { if err := b.setConfigLocked(conf); err != nil {
return false, fmt.Errorf("error setting config: %w", err) return false, fmt.Errorf("error setting config: %w", err)
} }
@ -863,10 +863,9 @@ func (b *LocalBackend) setStateLocked(state ipn.State) {
} }
} }
// setConfigLockedOnEntry uses the provided config to update the backend's prefs // setConfigLocked uses the provided config to update the backend's prefs
// and other state. // and other state.
func (b *LocalBackend) setConfigLockedOnEntry(conf *conffile.Config, unlock unlockOnce) error { func (b *LocalBackend) setConfigLocked(conf *conffile.Config) error {
defer unlock()
p := b.pm.CurrentPrefs().AsStruct() p := b.pm.CurrentPrefs().AsStruct()
mp, err := conf.Parsed.ToPrefs() mp, err := conf.Parsed.ToPrefs()
if err != nil { if err != nil {
@ -874,8 +873,7 @@ func (b *LocalBackend) setConfigLockedOnEntry(conf *conffile.Config, unlock unlo
} }
p.ApplyEdits(&mp) p.ApplyEdits(&mp)
b.setStaticEndpointsFromConfigLocked(conf) b.setStaticEndpointsFromConfigLocked(conf)
b.setPrefsLockedOnEntry(p, unlock) b.setPrefsLocked(p)
b.conf = conf b.conf = conf
return nil return nil
} }
@ -1959,12 +1957,12 @@ func (b *LocalBackend) registerSysPolicyWatch() (unregister func(), err error) {
// b.mu must not be held. // b.mu must not be held.
func (b *LocalBackend) reconcilePrefs() (_ ipn.PrefsView, anyChange bool) { func (b *LocalBackend) reconcilePrefs() (_ ipn.PrefsView, anyChange bool) {
unlock := b.lockAndGetUnlock() unlock := b.lockAndGetUnlock()
defer unlock()
prefs := b.pm.CurrentPrefs().AsStruct() prefs := b.pm.CurrentPrefs().AsStruct()
if !b.reconcilePrefsLocked(prefs) { if !b.reconcilePrefsLocked(prefs) {
unlock.UnlockEarly()
return prefs.View(), false return prefs.View(), false
} }
return b.setPrefsLockedOnEntry(prefs, unlock), true return b.setPrefsLocked(prefs), true
} }
// sysPolicyChanged is a callback triggered by syspolicy when it detects // sysPolicyChanged is a callback triggered by syspolicy when it detects
@ -2492,8 +2490,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
// regress tsnet.Server restarts. // regress tsnet.Server restarts.
cc.Login(controlclient.LoginDefault) cc.Login(controlclient.LoginDefault)
} }
b.stateMachineLockedOnEntry(unlock) b.stateMachineLocked()
return nil return nil
} }
@ -3512,14 +3509,14 @@ func (b *LocalBackend) onTailnetDefaultAutoUpdate(au bool) {
b.logf("using tailnet default auto-update setting: %v", au) b.logf("using tailnet default auto-update setting: %v", au)
prefsClone := prefs.AsStruct() prefsClone := prefs.AsStruct()
prefsClone.AutoUpdate.Apply = opt.NewBool(au) prefsClone.AutoUpdate.Apply = opt.NewBool(au)
_, err := b.editPrefsLockedOnEntry( _, err := b.editPrefsLocked(
ipnauth.Self, ipnauth.Self,
&ipn.MaskedPrefs{ &ipn.MaskedPrefs{
Prefs: *prefsClone, Prefs: *prefsClone,
AutoUpdateSet: ipn.AutoUpdatePrefsMask{ AutoUpdateSet: ipn.AutoUpdatePrefsMask{
ApplySet: true, ApplySet: true,
}, },
}, unlock) })
if err != nil { if err != nil {
b.logf("failed to apply tailnet-wide default for auto-updates (%v): %v", au, err) b.logf("failed to apply tailnet-wide default for auto-updates (%v): %v", au, err)
return return
@ -3979,7 +3976,7 @@ func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) {
action = "connected" action = "connected"
} }
reason := fmt.Sprintf("client %s (%s)", action, userIdentifier) reason := fmt.Sprintf("client %s (%s)", action, userIdentifier)
b.switchToBestProfileLockedOnEntry(reason, unlock) b.switchToBestProfileLocked(reason)
} }
// SwitchToBestProfile selects the best profile to use, // SwitchToBestProfile selects the best profile to use,
@ -3989,13 +3986,14 @@ func (b *LocalBackend) SetCurrentUser(actor ipnauth.Actor) {
// or disconnecting, or a change in the desktop session state, and is used // or disconnecting, or a change in the desktop session state, and is used
// for logging. // for logging.
func (b *LocalBackend) SwitchToBestProfile(reason string) { func (b *LocalBackend) SwitchToBestProfile(reason string) {
b.switchToBestProfileLockedOnEntry(reason, b.lockAndGetUnlock()) unlock := b.lockAndGetUnlock()
defer unlock()
b.switchToBestProfileLocked(reason)
} }
// switchToBestProfileLockedOnEntry is like [LocalBackend.SwitchToBestProfile], // switchToBestProfileLocked is like [LocalBackend.SwitchToBestProfile], but
// but b.mu must held on entry. It is released on exit. // the caller must hold b.mu.
func (b *LocalBackend) switchToBestProfileLockedOnEntry(reason string, unlock unlockOnce) { func (b *LocalBackend) switchToBestProfileLocked(reason string) {
defer unlock()
oldControlURL := b.pm.CurrentPrefs().ControlURLOrDefault() oldControlURL := b.pm.CurrentPrefs().ControlURLOrDefault()
profile, background := b.resolveBestProfileLocked() profile, background := b.resolveBestProfileLocked()
cp, switched, err := b.pm.SwitchToProfile(profile) cp, switched, err := b.pm.SwitchToProfile(profile)
@ -4026,7 +4024,7 @@ func (b *LocalBackend) switchToBestProfileLockedOnEntry(reason string, unlock un
if newControlURL := b.pm.CurrentPrefs().ControlURLOrDefault(); oldControlURL != newControlURL { if newControlURL := b.pm.CurrentPrefs().ControlURLOrDefault(); oldControlURL != newControlURL {
b.resetDialPlan() b.resetDialPlan()
} }
if err := b.resetForProfileChangeLockedOnEntry(unlock); err != nil { if err := b.resetForProfileChangeLocked(); err != nil {
// TODO(nickkhyl): The actual reset cannot fail. However, // TODO(nickkhyl): The actual reset cannot fail. However,
// the TKA initialization or [LocalBackend.Start] can fail. // the TKA initialization or [LocalBackend.Start] can fail.
// These errors are not critical as far as we're concerned. // These errors are not critical as far as we're concerned.
@ -4304,7 +4302,7 @@ func (b *LocalBackend) SetUseExitNodeEnabled(actor ipnauth.Actor, v bool) (ipn.P
mp.InternalExitNodePrior = p0.ExitNodeID() mp.InternalExitNodePrior = p0.ExitNodeID()
} }
} }
return b.editPrefsLockedOnEntry(actor, mp, unlock) return b.editPrefsLocked(actor, mp)
} }
// MaybeClearAppConnector clears the routes from any AppConnector if // MaybeClearAppConnector clears the routes from any AppConnector if
@ -4333,7 +4331,9 @@ func (b *LocalBackend) EditPrefsAs(mp *ipn.MaskedPrefs, actor ipnauth.Actor) (ip
return ipn.PrefsView{}, errors.New("can't set Internal fields") return ipn.PrefsView{}, errors.New("can't set Internal fields")
} }
return b.editPrefsLockedOnEntry(actor, mp, b.lockAndGetUnlock()) unlock := b.lockAndGetUnlock()
defer unlock()
return b.editPrefsLocked(actor, mp)
} }
// checkEditPrefsAccessLocked checks whether the current user has access // checkEditPrefsAccessLocked checks whether the current user has access
@ -4540,7 +4540,7 @@ func (b *LocalBackend) startReconnectTimerLocked(d time.Duration) {
} }
mp := &ipn.MaskedPrefs{WantRunningSet: true, Prefs: ipn.Prefs{WantRunning: true}} mp := &ipn.MaskedPrefs{WantRunningSet: true, Prefs: ipn.Prefs{WantRunning: true}}
if _, err := b.editPrefsLockedOnEntry(ipnauth.Self, mp, unlock); err != nil { if _, err := b.editPrefsLocked(ipnauth.Self, mp); err != nil {
b.logf("failed to automatically reconnect as %q after %v: %v", cp.Name(), d, err) b.logf("failed to automatically reconnect as %q after %v: %v", cp.Name(), d, err)
} else { } else {
b.logf("automatically reconnected as %q after %v", cp.Name(), d) b.logf("automatically reconnected as %q after %v", cp.Name(), d)
@ -4569,11 +4569,8 @@ func (b *LocalBackend) stopReconnectTimerLocked() {
} }
} }
// Warning: b.mu must be held on entry, but it unlocks it on the way out. // Warning: b.mu must be held on entry.
// TODO(bradfitz): redo the locking on all these weird methods like this. func (b *LocalBackend) editPrefsLocked(actor ipnauth.Actor, mp *ipn.MaskedPrefs) (ipn.PrefsView, error) {
func (b *LocalBackend) editPrefsLockedOnEntry(actor ipnauth.Actor, mp *ipn.MaskedPrefs, unlock unlockOnce) (ipn.PrefsView, error) {
defer unlock() // for error paths
p0 := b.pm.CurrentPrefs() p0 := b.pm.CurrentPrefs()
// Check if the changes in mp are allowed. // Check if the changes in mp are allowed.
@ -4610,12 +4607,10 @@ func (b *LocalBackend) editPrefsLockedOnEntry(actor ipnauth.Actor, mp *ipn.Maske
// before the modified prefs are actually set for the current profile. // before the modified prefs are actually set for the current profile.
b.onEditPrefsLocked(actor, mp, p0, p1.View()) b.onEditPrefsLocked(actor, mp, p0, p1.View())
newPrefs := b.setPrefsLockedOnEntry(p1, unlock) newPrefs := b.setPrefsLocked(p1)
// Note: don't perform any actions for the new prefs here. Not
// every prefs change goes through EditPrefs. Put your actions
// in setPrefsLocksOnEntry instead.
// Note: don't perform any actions for the new prefs here. Not every prefs
// change goes through EditPrefs. Put your actions in setPrefsLocked instead.
// This should return the public prefs, not the private ones. // This should return the public prefs, not the private ones.
return stripKeysFromPrefs(newPrefs), nil return stripKeysFromPrefs(newPrefs), nil
} }
@ -4663,12 +4658,9 @@ func (b *LocalBackend) shouldWireInactiveIngressLocked() bool {
return b.serveConfig.Valid() && !b.hasIngressEnabledLocked() && b.wantIngressLocked() return b.serveConfig.Valid() && !b.hasIngressEnabledLocked() && b.wantIngressLocked()
} }
// setPrefsLockedOnEntry requires b.mu be held to call it, but it // setPrefsLocked requires b.mu be held to call it. It returns a read-only
// unlocks b.mu when done. newp ownership passes to this function. // copy of the new prefs.
// It returns a read-only copy of the new prefs. func (b *LocalBackend) setPrefsLocked(newp *ipn.Prefs) ipn.PrefsView {
func (b *LocalBackend) setPrefsLockedOnEntry(newp *ipn.Prefs, unlock unlockOnce) ipn.PrefsView {
defer unlock()
cn := b.currentNode() cn := b.currentNode()
netMap := cn.NetMap() netMap := cn.NetMap()
b.setAtomicValuesFromPrefsLocked(newp.View()) b.setAtomicValuesFromPrefsLocked(newp.View())
@ -4737,28 +4729,33 @@ func (b *LocalBackend) setPrefsLockedOnEntry(newp *ipn.Prefs, unlock unlockOnce)
b.stopOfflineAutoUpdate() b.stopOfflineAutoUpdate()
} }
unlock.UnlockEarly() // Update status that needs to happen outside the lock, but reacquire it
// before returning (including in case of panics).
func() {
b.mu.Unlock()
defer b.mu.Lock()
if oldp.ShieldsUp() != newp.ShieldsUp || hostInfoChanged { if oldp.ShieldsUp() != newp.ShieldsUp || hostInfoChanged {
b.doSetHostinfoFilterServices() b.doSetHostinfoFilterServices()
} }
if netMap != nil { if netMap != nil {
b.MagicConn().SetDERPMap(netMap.DERPMap) b.MagicConn().SetDERPMap(netMap.DERPMap)
} }
if !oldp.WantRunning() && newp.WantRunning && cc != nil { if !oldp.WantRunning() && newp.WantRunning && cc != nil {
b.logf("transitioning to running; doing Login...") b.logf("transitioning to running; doing Login...")
cc.Login(controlclient.LoginDefault) cc.Login(controlclient.LoginDefault)
} }
if oldp.WantRunning() != newp.WantRunning { if oldp.WantRunning() != newp.WantRunning {
b.stateMachine() b.stateMachine()
} else { } else {
b.authReconfig() b.authReconfig()
} }
b.send(ipn.Notify{Prefs: &prefs}) b.send(ipn.Notify{Prefs: &prefs})
}()
return prefs return prefs
} }
@ -5620,12 +5617,12 @@ func (b *LocalBackend) applyPrefsToHostinfoLocked(hi *tailcfg.Hostinfo, prefs ip
// happen". // happen".
func (b *LocalBackend) enterState(newState ipn.State) { func (b *LocalBackend) enterState(newState ipn.State) {
unlock := b.lockAndGetUnlock() unlock := b.lockAndGetUnlock()
b.enterStateLockedOnEntry(newState, unlock) defer unlock()
b.enterStateLocked(newState)
} }
// enterStateLockedOnEntry is like enterState but requires b.mu be held to call // enterStateLocked is like enterState but requires the caller to hold b.mu.
// it, but it unlocks b.mu when done (via unlock, a once func). func (b *LocalBackend) enterStateLocked(newState ipn.State) {
func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State, unlock unlockOnce) {
cn := b.currentNode() cn := b.currentNode()
oldState := b.state oldState := b.state
b.setStateLocked(newState) b.setStateLocked(newState)
@ -5674,51 +5671,56 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State, unlock unlock
b.maybeStartOfflineAutoUpdate(prefs) b.maybeStartOfflineAutoUpdate(prefs)
} }
unlock.UnlockEarly() // Resolve the state transition outside the lock, but reacquire it before
// returning (including in case of panics).
// prefs may change irrespective of state; WantRunning should be explicitly func() {
// set before potential early return even if the state is unchanged. b.mu.Unlock()
b.health.SetIPNState(newState.String(), prefs.Valid() && prefs.WantRunning()) defer b.mu.Lock()
if oldState == newState {
return
}
b.logf("Switching ipn state %v -> %v (WantRunning=%v, nm=%v)",
oldState, newState, prefs.WantRunning(), netMap != nil)
b.send(ipn.Notify{State: &newState})
switch newState { // prefs may change irrespective of state; WantRunning should be explicitly
case ipn.NeedsLogin: // set before potential early return even if the state is unchanged.
systemd.Status("Needs login: %s", authURL) b.health.SetIPNState(newState.String(), prefs.Valid() && prefs.WantRunning())
if b.seamlessRenewalEnabled() { if oldState == newState {
break return
}
b.blockEngineUpdates(true)
fallthrough
case ipn.Stopped, ipn.NoState:
// Unconfigure the engine if it has stopped (WantRunning is set to false)
// or if we've switched to a different profile and the state is unknown.
err := b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{})
if err != nil {
b.logf("Reconfig(down): %v", err)
} }
b.logf("Switching ipn state %v -> %v (WantRunning=%v, nm=%v)",
oldState, newState, prefs.WantRunning(), netMap != nil)
b.send(ipn.Notify{State: &newState})
switch newState {
case ipn.NeedsLogin:
systemd.Status("Needs login: %s", authURL)
if b.seamlessRenewalEnabled() {
break
}
b.blockEngineUpdates(true)
fallthrough
case ipn.Stopped, ipn.NoState:
// Unconfigure the engine if it has stopped (WantRunning is set to false)
// or if we've switched to a different profile and the state is unknown.
err := b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{})
if err != nil {
b.logf("Reconfig(down): %v", err)
}
if newState == ipn.Stopped && authURL == "" { if newState == ipn.Stopped && authURL == "" {
systemd.Status("Stopped; run 'tailscale up' to log in") systemd.Status("Stopped; run 'tailscale up' to log in")
}
case ipn.Starting, ipn.NeedsMachineAuth:
b.authReconfig()
// Needed so that UpdateEndpoints can run
b.e.RequestStatus()
case ipn.Running:
var addrStrs []string
addrs := netMap.GetAddresses()
for _, p := range addrs.All() {
addrStrs = append(addrStrs, p.Addr().String())
}
systemd.Status("Connected; %s; %s", activeLogin, strings.Join(addrStrs, " "))
default:
b.logf("[unexpected] unknown newState %#v", newState)
} }
case ipn.Starting, ipn.NeedsMachineAuth: }()
b.authReconfig()
// Needed so that UpdateEndpoints can run
b.e.RequestStatus()
case ipn.Running:
var addrStrs []string
addrs := netMap.GetAddresses()
for _, p := range addrs.All() {
addrStrs = append(addrStrs, p.Addr().String())
}
systemd.Status("Connected; %s; %s", activeLogin, strings.Join(addrStrs, " "))
default:
b.logf("[unexpected] unknown newState %#v", newState)
}
} }
func (b *LocalBackend) hasNodeKeyLocked() bool { func (b *LocalBackend) hasNodeKeyLocked() bool {
@ -5819,26 +5821,28 @@ func (b *LocalBackend) nextStateLocked() ipn.State {
// Or maybe just call the state machine from fewer places. // Or maybe just call the state machine from fewer places.
func (b *LocalBackend) stateMachine() { func (b *LocalBackend) stateMachine() {
unlock := b.lockAndGetUnlock() unlock := b.lockAndGetUnlock()
b.stateMachineLockedOnEntry(unlock) defer unlock()
b.stateMachineLocked()
} }
// stateMachineLockedOnEntry is like stateMachine but requires b.mu be held to // stateMachineLocked is like stateMachine but requires b.mu be held.
// call it, but it unlocks b.mu when done (via unlock, a once func). func (b *LocalBackend) stateMachineLocked() {
func (b *LocalBackend) stateMachineLockedOnEntry(unlock unlockOnce) { b.enterStateLocked(b.nextStateLocked())
b.enterStateLockedOnEntry(b.nextStateLocked(), unlock)
} }
// lockAndGetUnlock locks b.mu and returns a sync.OnceFunc function that will // lockAndGetUnlock locks b.mu and returns a function that will unlock it at
// unlock it at most once. // most once.
//
// TODO(creachadair): This was added as a guardrail against the unfortunate
// "LockedOnEntry" methods that were originally used in this package (primarily
// enterStateLockedOnEntry) that required b.mu held to be locked on entry to
// the function but unlocked the mutex on their way out.
// //
// This is all very unfortunate but exists as a guardrail against the // Now that these have all been updated, we could remove this type and acquire
// unfortunate "lockedOnEntry" methods in this package (primarily // and release locks directly. For now, however, I've left it alone to reduce
// enterStateLockedOnEntry) that require b.mu held to be locked on entry to the // the scope of lock-related changes.
// function but unlock the mutex on their way out. As a stepping stone to //
// cleaning things up (as of 2024-04-06), we at least pass the unlock func // See: https://github.com/tailscale/tailscale/issues/11649
// around now and defer unlock in the caller to avoid missing unlocks and double
// unlocks. TODO(bradfitz,maisem): make the locking in this package more
// traditional (simple). See https://github.com/tailscale/tailscale/issues/11649
func (b *LocalBackend) lockAndGetUnlock() (unlock unlockOnce) { func (b *LocalBackend) lockAndGetUnlock() (unlock unlockOnce) {
b.mu.Lock() b.mu.Lock()
var unlocked atomic.Bool var unlocked atomic.Bool
@ -6006,30 +6010,35 @@ func (b *LocalBackend) ShouldHandleViaIP(ip netip.Addr) bool {
// Logout logs out the current profile, if any, and waits for the logout to // Logout logs out the current profile, if any, and waits for the logout to
// complete. // complete.
func (b *LocalBackend) Logout(ctx context.Context, actor ipnauth.Actor) error { func (b *LocalBackend) Logout(ctx context.Context, actor ipnauth.Actor) error {
unlock := b.lockAndGetUnlock() // These values are initialized inside the lock on success.
defer unlock() var cc controlclient.Client
var profile ipn.LoginProfileView
if !b.hasNodeKeyLocked() { if err := func() error {
// Already logged out. unlock := b.lockAndGetUnlock()
return nil defer unlock()
}
cc := b.cc
// Grab the current profile before we unlock the mutex, so that we can if !b.hasNodeKeyLocked() {
// delete it later. // Already logged out.
profile := b.pm.CurrentProfile() return nil
}
_, err := b.editPrefsLockedOnEntry( cc = b.cc
actor,
&ipn.MaskedPrefs{ // Grab the current profile before we unlock the mutex, so that we can
WantRunningSet: true, // delete it later.
LoggedOutSet: true, profile = b.pm.CurrentProfile()
Prefs: ipn.Prefs{WantRunning: false, LoggedOut: true},
}, unlock) _, err := b.editPrefsLocked(
if err != nil { actor,
&ipn.MaskedPrefs{
WantRunningSet: true,
LoggedOutSet: true,
Prefs: ipn.Prefs{WantRunning: false, LoggedOut: true},
})
return err
}(); err != nil {
return err return err
} }
// b.mu is now unlocked, after editPrefsLockedOnEntry.
// Clear any previous dial plan(s), if set. // Clear any previous dial plan(s), if set.
b.resetDialPlan() b.resetDialPlan()
@ -6049,14 +6058,14 @@ func (b *LocalBackend) Logout(ctx context.Context, actor ipnauth.Actor) error {
return err return err
} }
unlock = b.lockAndGetUnlock() unlock := b.lockAndGetUnlock()
defer unlock() defer unlock()
if err := b.pm.DeleteProfile(profile.ID()); err != nil { if err := b.pm.DeleteProfile(profile.ID()); err != nil {
b.logf("error deleting profile: %v", err) b.logf("error deleting profile: %v", err)
return err return err
} }
return b.resetForProfileChangeLockedOnEntry(unlock) return b.resetForProfileChangeLocked()
} }
// setNetInfo sets b.hostinfo.NetInfo to ni, and passes ni along to the // setNetInfo sets b.hostinfo.NetInfo to ni, and passes ni along to the
@ -7245,7 +7254,7 @@ func (b *LocalBackend) SwitchProfile(profile ipn.ProfileID) error {
b.resetDialPlan() b.resetDialPlan()
} }
return b.resetForProfileChangeLockedOnEntry(unlock) return b.resetForProfileChangeLocked()
} }
func (b *LocalBackend) initTKALocked() error { func (b *LocalBackend) initTKALocked() error {
@ -7325,12 +7334,10 @@ func (b *LocalBackend) getHardwareAddrs() ([]string, error) {
return addrs, nil return addrs, nil
} }
// resetForProfileChangeLockedOnEntry resets the backend for a profile change. // resetForProfileChangeLocked resets the backend for a profile change.
// //
// b.mu must held on entry. It is released on exit. // b.mu must held on entry. It is released on exit.
func (b *LocalBackend) resetForProfileChangeLockedOnEntry(unlock unlockOnce) error { func (b *LocalBackend) resetForProfileChangeLocked() error {
defer unlock()
if b.shutdownCalled { if b.shutdownCalled {
// Prevent a call back to Start during Shutdown, which calls Logout for // Prevent a call back to Start during Shutdown, which calls Logout for
// ephemeral nodes, which can then call back here. But we're shutting // ephemeral nodes, which can then call back here. But we're shutting
@ -7361,12 +7368,19 @@ func (b *LocalBackend) resetForProfileChangeLockedOnEntry(unlock unlockOnce) err
b.resetAlwaysOnOverrideLocked() b.resetAlwaysOnOverrideLocked()
b.extHost.NotifyProfileChange(b.pm.CurrentProfile(), b.pm.CurrentPrefs(), false) b.extHost.NotifyProfileChange(b.pm.CurrentProfile(), b.pm.CurrentPrefs(), false)
b.setAtomicValuesFromPrefsLocked(b.pm.CurrentPrefs()) b.setAtomicValuesFromPrefsLocked(b.pm.CurrentPrefs())
b.enterStateLockedOnEntry(ipn.NoState, unlock) // Reset state; releases b.mu b.enterStateLocked(ipn.NoState)
b.health.SetLocalLogConfigHealth(nil)
if tkaErr != nil { // Update health status and start outside the lock.
return tkaErr return func() error {
} b.mu.Unlock()
return b.Start(ipn.Options{}) defer b.mu.Lock()
b.health.SetLocalLogConfigHealth(nil)
if tkaErr != nil {
return tkaErr
}
return b.Start(ipn.Options{})
}()
} }
// DeleteProfile deletes a profile with the given ID. // DeleteProfile deletes a profile with the given ID.
@ -7385,7 +7399,7 @@ func (b *LocalBackend) DeleteProfile(p ipn.ProfileID) error {
if !needToRestart { if !needToRestart {
return nil return nil
} }
return b.resetForProfileChangeLockedOnEntry(unlock) return b.resetForProfileChangeLocked()
} }
// CurrentProfile returns the current LoginProfile. // CurrentProfile returns the current LoginProfile.
@ -7407,7 +7421,7 @@ func (b *LocalBackend) NewProfile() error {
// set. Conservatively reset the dialPlan. // set. Conservatively reset the dialPlan.
b.resetDialPlan() b.resetDialPlan()
return b.resetForProfileChangeLockedOnEntry(unlock) return b.resetForProfileChangeLocked()
} }
// ListProfiles returns a list of all LoginProfiles. // ListProfiles returns a list of all LoginProfiles.
@ -7436,7 +7450,7 @@ func (b *LocalBackend) ResetAuth() error {
return err return err
} }
b.resetDialPlan() // always reset if we're removing everything b.resetDialPlan() // always reset if we're removing everything
return b.resetForProfileChangeLockedOnEntry(unlock) return b.resetForProfileChangeLocked()
} }
func (b *LocalBackend) GetPeerEndpointChanges(ctx context.Context, ip netip.Addr) ([]magicsock.EndpointChange, error) { func (b *LocalBackend) GetPeerEndpointChanges(ctx context.Context, ip netip.Addr) ([]magicsock.EndpointChange, error) {

2
ipn/ipnlocal/local_test.go
Unescape Escape View File

@ -4300,7 +4300,7 @@ func (b *LocalBackend) SetPrefsForTest(newp *ipn.Prefs) {
} }
unlock := b.lockAndGetUnlock() unlock := b.lockAndGetUnlock()
defer unlock() defer unlock()
b.setPrefsLockedOnEntry(newp, unlock) b.setPrefsLocked(newp)
} }
type peerOptFunc func(*tailcfg.Node) type peerOptFunc func(*tailcfg.Node)

15
ipn/ipnlocal/profiles.go
Unescape Escape View File

@ -180,7 +180,7 @@ func (pm *profileManager) SwitchToProfile(profile ipn.LoginProfileView) (cp ipn.
f(pm.currentProfile, pm.prefs, false) f(pm.currentProfile, pm.prefs, false)
} }
// Do not call pm.extHost.NotifyProfileChange here; it is invoked in // Do not call pm.extHost.NotifyProfileChange here; it is invoked in
// [LocalBackend.resetForProfileChangeLockedOnEntry] after the netmap reset. // [LocalBackend.resetForProfileChangeLocked] after the netmap reset.
// TODO(nickkhyl): Consider moving it here (or into the stateChangeCb handler // TODO(nickkhyl): Consider moving it here (or into the stateChangeCb handler
// in [LocalBackend]) once the profile/node state, including the netmap, // in [LocalBackend]) once the profile/node state, including the netmap,
// is actually tied to the current profile. // is actually tied to the current profile.
@ -359,9 +359,9 @@ func (pm *profileManager) SetPrefs(prefsIn ipn.PrefsView, np ipn.NetworkProfile)
// where prefsIn is the previous profile's prefs with an updated Persist, LoggedOut, // where prefsIn is the previous profile's prefs with an updated Persist, LoggedOut,
// WantRunning and possibly other fields. This may not be the desired behavior. // WantRunning and possibly other fields. This may not be the desired behavior.
// //
// Additionally, LocalBackend doesn't treat it as a proper profile switch, meaning that // Additionally, LocalBackend doesn't treat it as a proper profile switch,
// [LocalBackend.resetForProfileChangeLockedOnEntry] is not called and certain // meaning that [LocalBackend.resetForProfileChangeLocked] is not called and
// node/profile-specific state may not be reset as expected. // certain node/profile-specific state may not be reset as expected.
// //
// However, [profileManager] notifies [ipnext.Extension]s about the profile change, // However, [profileManager] notifies [ipnext.Extension]s about the profile change,
// so features migrated from LocalBackend to external packages should not be affected. // so features migrated from LocalBackend to external packages should not be affected.
@ -494,10 +494,9 @@ func (pm *profileManager) setProfilePrefsNoPermCheck(profile ipn.LoginProfileVie
oldPrefs := pm.prefs oldPrefs := pm.prefs
pm.prefs = clonedPrefs pm.prefs = clonedPrefs
// Sadly, profile prefs can be changed in multiple ways. // Sadly, profile prefs can be changed in multiple ways. It's pretty
// It's pretty chaotic, and in many cases callers use // chaotic, and in many cases callers use unexported methods of the
// unexported methods of the profile manager instead of // profile manager instead of going through [LocalBackend.setPrefsLocked]
// going through [LocalBackend.setPrefsLockedOnEntry]
// or at least using [profileManager.SetPrefs]. // or at least using [profileManager.SetPrefs].
// //
// While we should definitely clean this up to improve // While we should definitely clean this up to improve

Write Preview
Loading…
Cancel
Save