From 4ec83fbad68b861c3660bb3f778187ea0b1a1215 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Sun, 17 Apr 2022 14:49:16 -0700 Subject: [PATCH] ipn/ipnlocal: only call updateFilter with mutex held And rename to updateFilterLocked to prevent future mistakes. Fixes #4427 Change-Id: I4d37b90027d5ff872a339ce8180f5723704848dc Signed-off-by: Brad Fitzpatrick --- ipn/ipnlocal/local.go | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index cc7eea7a9..fbd687319 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -119,13 +119,12 @@ type LocalBackend struct { sshAtomicBool syncs.AtomicBool sshServer SSHServer // or nil - filterHash deephash.Sum - filterAtomic atomic.Value // of *filter.Filter containsViaIPFuncAtomic atomic.Value // of func(netaddr.IP) bool // The mutex protects the following elements. mu sync.Mutex + filterHash deephash.Sum httpTestClient *http.Client // for controlclient. nil by default, used by tests. ccGen clientGen // function for producing controlclient; lazily populated notify func(ipn.Notify) @@ -316,7 +315,7 @@ func (b *LocalBackend) linkChange(major bool, ifst *interfaces.State) { // If the local network configuration has changed, our filter may // need updating to tweak default routes. - b.updateFilter(b.netMap, b.prefs) + b.updateFilterLocked(b.netMap, b.prefs) if peerAPIListenAsync && b.netMap != nil && b.state == ipn.Running { want := len(b.netMap.Addresses) @@ -661,7 +660,9 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) { if prefsChanged { prefs = b.prefs.Clone() } - + if st.NetMap != nil { + b.updateFilterLocked(st.NetMap, prefs) + } b.mu.Unlock() // Now complete the lock-free parts of what we started while locked. @@ -683,7 +684,6 @@ func (b *LocalBackend) setClientStatus(st controlclient.Status) { } } - b.updateFilter(st.NetMap, prefs) b.e.SetNetworkMap(st.NetMap) b.e.SetDERPMap(st.NetMap.DERPMap) @@ -968,10 +968,9 @@ func (b *LocalBackend) Start(opts ipn.Options) error { b.setNetMapLocked(nil) persistv := b.prefs.Persist + b.updateFilterLocked(nil, nil) b.mu.Unlock() - b.updateFilter(nil, nil) - if b.portpoll != nil { b.portpollOnce.Do(func() { go b.portpoll.Run(b.ctx) @@ -1069,9 +1068,11 @@ func (b *LocalBackend) Start(opts ipn.Options) error { return nil } -// updateFilter updates the packet filter in wgengine based on the +// updateFilterLocked updates the packet filter in wgengine based on the // given netMap and user preferences. -func (b *LocalBackend) updateFilter(netMap *netmap.NetworkMap, prefs *ipn.Prefs) { +// +// b.mu must be held. +func (b *LocalBackend) updateFilterLocked(netMap *netmap.NetworkMap, prefs *ipn.Prefs) { // NOTE(danderson): keep change detection as the first thing in // this function. Don't try to optimize by returning early, more // likely than not you'll just end up breaking the change @@ -1825,6 +1826,12 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) { userID := b.userID cc := b.cc + // [GRINDER STATS LINE] - please don't remove (used for log parsing) + if caller == "SetPrefs" { + b.logf("SetPrefs: %v", newp.Pretty()) + } + b.updateFilterLocked(netMap, newp) + b.mu.Unlock() if stateKey != "" { @@ -1834,10 +1841,6 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) { } b.writeServerModeStartState(userID, newp) - // [GRINDER STATS LINE] - please don't remove (used for log parsing) - if caller == "SetPrefs" { - b.logf("SetPrefs: %v", newp.Pretty()) - } if netMap != nil { if login := netMap.UserProfiles[netMap.User].LoginName; login != "" { if newp.Persist == nil { @@ -1857,8 +1860,6 @@ func (b *LocalBackend) setPrefsLockedOnEntry(caller string, newp *ipn.Prefs) { b.doSetHostinfoFilterServices(newHi) } - b.updateFilter(netMap, newp) - if netMap != nil { b.e.SetDERPMap(netMap.DERPMap) }