From 40ed210521d880b46504a8d053f3112a7511fec0 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 10 Jun 2026 13:42:15 +0000 Subject: [PATCH] policy: read pm.pol under the mutex NodeCanHaveTag, TagExists, ViaRoutesForPeer checked pm.pol before taking pm.mu, racing SetPolicy's write (caught by -race in TestRaceConcurrentServerMutations). DebugString read pol, filter, and the derived maps with no lock at all. --- hscontrol/policy/v2/policy.go | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/hscontrol/policy/v2/policy.go b/hscontrol/policy/v2/policy.go index 57beae59..a7b55a2a 100644 --- a/hscontrol/policy/v2/policy.go +++ b/hscontrol/policy/v2/policy.go @@ -917,13 +917,19 @@ func (pm *PolicyManager) nodesHavePolicyAffectingChanges(newNodes views.Slice[ty // set any existing tag on any node by calling [state.State.SetNodeTags] directly, // which bypasses this authorization check. func (pm *PolicyManager) NodeCanHaveTag(node types.NodeView, tag string) bool { - if pm == nil || pm.pol == nil { + if pm == nil { return false } pm.mu.Lock() defer pm.mu.Unlock() + // pm.pol is written by SetPolicy under pm.mu; reading it before the + // lock races with concurrent policy reloads. + if pm.pol == nil { + return false + } + // Check if tag exists in policy owners, exists := pm.pol.TagOwners[Tag(tag)] if !exists { @@ -1000,13 +1006,19 @@ func (pm *PolicyManager) userMatchesOwner(user types.UserView, owner Owner) bool // TagExists reports whether the given tag is defined in the policy. func (pm *PolicyManager) TagExists(tag string) bool { - if pm == nil || pm.pol == nil { + if pm == nil { return false } pm.mu.Lock() defer pm.mu.Unlock() + // pm.pol is written by SetPolicy under pm.mu; reading it before the + // lock races with concurrent policy reloads. + if pm.pol == nil { + return false + } + _, exists := pm.pol.TagOwners[Tag(tag)] return exists @@ -1076,13 +1088,19 @@ func (pm *PolicyManager) NodeCanApproveRoute(node types.NodeView, route netip.Pr func (pm *PolicyManager) ViaRoutesForPeer(viewer, peer types.NodeView) types.ViaRouteResult { var result types.ViaRouteResult - if pm == nil || pm.pol == nil { + if pm == nil { return result } pm.mu.Lock() defer pm.mu.Unlock() + // pm.pol is written by SetPolicy under pm.mu; reading it before the + // lock races with concurrent policy reloads. + if pm.pol == nil { + return result + } + // Self-steering doesn't apply. if viewer.ID() == peer.ID() { return result @@ -1291,6 +1309,11 @@ func (pm *PolicyManager) DebugString() string { return "PolicyManager is not setup" } + // pm.pol, filter, matchers, and the derived maps are all written + // under pm.mu by SetPolicy/SetUsers/SetNodes. + pm.mu.Lock() + defer pm.mu.Unlock() + var sb strings.Builder fmt.Fprintf(&sb, "PolicyManager (v%d):\n\n", pm.Version())