From 49e060bbcb314739a1016ef0b1990539c84f8919 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 23 Jun 2026 18:52:37 +0000 Subject: [PATCH] wgengine: add Engine.ProbeLocks, drop PeerForIP lock-probe overload The watchdog (ipn/ipnlocal/watchdog.go) was abusing PeerForIP with an invalid netip.Addr as a way to acquire and release the engine's internal locks for deadlock detection. This does the TODO to break it out into its own method like all the other similarly named methods. Splitting this out as a prerequisite for a follow-up rewrite of PeerForIP itself; not having to preserve the lock-probe overload in the new implementation keeps that follow-up smaller. Updates #12542 Updates #cleanup Signed-off-by: Brad Fitzpatrick Change-Id: I25cbffd11aeb65600d9128845404c4918ef88ead --- ipn/ipnlocal/state_test.go | 3 ++- ipn/ipnlocal/watchdog.go | 3 +-- wgengine/userspace.go | 19 +++++++++---------- wgengine/wgengine.go | 5 +++++ 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index f55542305..08d95f0d7 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -1985,8 +1985,9 @@ func (e *mockEngine) InstallCaptureHook(packet.CaptureCallback) {} func (e *mockEngine) SetPeerByIPPacketFunc(func(netip.Addr) (_ key.NodePublic, ok bool)) {} func (e *mockEngine) SetPeerSessionStateFunc(func(key.NodePublic, wgengine.PeerWireGuardState)) { } -func (e *mockEngine) SetNetLogNodeSource(netlog.NodeSource) {} +func (e *mockEngine) SetNetLogNodeSource(netlog.NodeSource) {} func (e *mockEngine) SetWGPeerLookup(func(wgString string) (tsString string, ok bool)) {} +func (e *mockEngine) ProbeLocks() {} func (e *mockEngine) Close() { e.mu.Lock() diff --git a/ipn/ipnlocal/watchdog.go b/ipn/ipnlocal/watchdog.go index 676852cd4..2dd7b41dc 100644 --- a/ipn/ipnlocal/watchdog.go +++ b/ipn/ipnlocal/watchdog.go @@ -5,7 +5,6 @@ package ipnlocal import ( "log" - "net/netip" "runtime" "time" @@ -117,7 +116,7 @@ func (b *LocalBackend) probeLocks() { dm.ProbeLocks() } if e, ok := sys.Engine.GetOK(); ok && e != nil { - e.PeerForIP(netip.Addr{}) // acquires e.mu and e.wgLock + e.ProbeLocks() } if nm, ok := sys.NetMon.GetOK(); ok && nm != nil { nm.ProbeLocks() diff --git a/wgengine/userspace.go b/wgengine/userspace.go index e9ba8383a..6175bfc90 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -1557,6 +1557,15 @@ func (e *userspaceEngine) setICMPEchoResponseCallback(idSeq uint32, cb func()) { } } +// ProbeLocks implements [Engine.ProbeLocks]. +func (e *userspaceEngine) ProbeLocks() { + e.mu.Lock() + e.mu.Unlock() + + e.wgLock.Lock() + e.wgLock.Unlock() +} + // PeerForIP returns the Node in the wireguard config // that's responsible for handling the given IP address. // @@ -1570,16 +1579,6 @@ func (e *userspaceEngine) PeerForIP(ip netip.Addr) (ret PeerForIP, ok bool) { nm := e.netMap e.mu.Unlock() - if !ip.IsValid() { - // Treat invalid IPs as just a mutex probe to detect deadlocks. - // TODO(bradfitz): extend the Engine interface to have an explicit method for - // this purpose, instead of overloading PeerForIP with this special case. - // But I'd rather do that at the beginning of a dev cycle. - e.wgLock.Lock() - defer e.wgLock.Unlock() - return ret, false - } - if nm == nil { return ret, false } diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go index 73023b5a3..4c02d60b1 100644 --- a/wgengine/wgengine.go +++ b/wgengine/wgengine.go @@ -197,4 +197,9 @@ type Engine interface { // ipnlocal.LocalBackend.onPeerWireGuardState, installed early in // LocalBackend construction. SetPeerSessionStateFunc(func(key.NodePublic, PeerWireGuardState)) + + // ProbeLocks acquires and releases the engine's internal locks so + // that [ipnlocal.LocalBackend]'s watchdog can detect deadlocks in + // the engine. It is otherwise a no-op. + ProbeLocks() }