From 6f27f4696536679de34285c75aa7efef1e90c1d8 Mon Sep 17 00:00:00 2001 From: Wade Simmons Date: Tue, 19 Dec 2023 09:10:00 -0500 Subject: [PATCH] simplify --- connection_manager.go | 6 ++-- connection_state.go | 2 +- firewall.go | 2 +- go.mod | 2 +- go.sum | 1 + handshake_ix.go | 4 +-- handshake_manager.go | 8 +++--- hostmap.go | 2 +- lighthouse.go | 2 +- mutex.go | 64 ------------------------------------------- mutex_debug.go | 53 +++++++++++++++++++++++++++-------- mutex_nodebug.go | 1 + remote_list.go | 2 +- 13 files changed, 59 insertions(+), 90 deletions(-) delete mode 100644 mutex.go diff --git a/connection_manager.go b/connection_manager.go index 924d19860..2bc94455f 100644 --- a/connection_manager.go +++ b/connection_manager.go @@ -58,11 +58,11 @@ func newConnectionManager(ctx context.Context, l *logrus.Logger, intf *Interface nc := &connectionManager{ hostMap: intf.hostMap, in: make(map[uint32]struct{}), - inLock: newSyncRWMutex(mutexKey{Type: mutexKeyTypeConnectionManagerIn}), + inLock: newSyncRWMutex("connection-manager-in"), out: make(map[uint32]struct{}), - outLock: newSyncRWMutex(mutexKey{Type: mutexKeyTypeConnectionManagerOut}), + outLock: newSyncRWMutex("connection-manager-out"), relayUsed: make(map[uint32]struct{}), - relayUsedLock: newSyncRWMutex(mutexKey{Type: mutexKeyTypeConnectionManagerRelayUsed}), + relayUsedLock: newSyncRWMutex("connection-manager-relay-used"), trafficTimer: NewLockingTimerWheel[uint32](time.Millisecond*500, max), intf: intf, pendingDeletion: make(map[uint32]struct{}), diff --git a/connection_state.go b/connection_state.go index 31d21028e..5373f967c 100644 --- a/connection_state.go +++ b/connection_state.go @@ -70,7 +70,7 @@ func NewConnectionState(l *logrus.Logger, cipher string, certState *CertState, i initiator: initiator, window: b, myCert: certState.Certificate, - writeLock: newSyncMutex(mutexKey{Type: mutexKeyTypeConnectionStateWrite}), + writeLock: newSyncMutex("connection-state-write"), } return ci diff --git a/firewall.go b/firewall.go index 513aaf50a..53d7b68d0 100644 --- a/firewall.go +++ b/firewall.go @@ -148,7 +148,7 @@ func NewFirewall(l *logrus.Logger, tcpTimeout, UDPTimeout, defaultTimeout time.D return &Firewall{ Conntrack: &FirewallConntrack{ - syncMutex: newSyncMutex(mutexKey{Type: mutexKeyTypeFirewallConntrack}), + syncMutex: newSyncMutex("firewall-conntrack"), Conns: make(map[firewall.Packet]*conn), TimerWheel: NewTimerWheel[firewall.Packet](min, max), }, diff --git a/go.mod b/go.mod index da13f10a4..d502622dc 100644 --- a/go.mod +++ b/go.mod @@ -10,6 +10,7 @@ require ( github.com/flynn/noise v1.0.0 github.com/gogo/protobuf v1.3.2 github.com/google/gopacket v1.1.19 + github.com/heimdalr/dag v1.4.0 github.com/kardianos/service v1.2.2 github.com/miekg/dns v1.1.56 github.com/nbrownus/go-metrics-prometheus v0.0.0-20210712211119-974a6260965f @@ -43,7 +44,6 @@ require ( github.com/golang/protobuf v1.5.3 // indirect github.com/google/btree v1.0.1 // indirect github.com/google/uuid v1.3.0 // indirect - github.com/heimdalr/dag v1.4.0 // indirect github.com/matttproud/golang_protobuf_extensions v1.0.4 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/prometheus/client_model v0.4.1-0.20230718164431-9a2bf3000d16 // indirect diff --git a/go.sum b/go.sum index 876e35462..4421a3d5b 100644 --- a/go.sum +++ b/go.sum @@ -33,6 +33,7 @@ github.com/go-logfmt/logfmt v0.3.0/go.mod h1:Qt1PoO58o5twSAckw1HlFXLmHsOX5/0LbT9 github.com/go-logfmt/logfmt v0.4.0/go.mod h1:3RMwSq7FuexP4Kalkev3ejPJsZTpXXBr9+V4qmtdjCk= github.com/go-logfmt/logfmt v0.5.0/go.mod h1:wCYkCAKZfumFQihp8CzCvQ3paCTfi41vtzG1KdI/P7A= github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY= +github.com/go-test/deep v1.1.0 h1:WOcxcdHcvdgThNXjw0t76K42FXTU7HpNQWHpA2HHNlg= github.com/go-test/deep v1.1.0/go.mod h1:5C2ZWiW0ErCdrYzpqxLbTX7MG14M9iiw8DgHncVwcsE= github.com/gogo/protobuf v1.1.1/go.mod h1:r8qH/GZQm5c6nD/R0oafs1akxWv10x8SbQlK7atdtwQ= github.com/gogo/protobuf v1.3.2 h1:Ov1cvc58UF3b5XjBnZv7+opcTcQFZebYjWzi34vdm4Q= diff --git a/handshake_ix.go b/handshake_ix.go index 1c7ff6c90..1f930407f 100644 --- a/handshake_ix.go +++ b/handshake_ix.go @@ -127,7 +127,7 @@ func ixHandshakeStage1(f *Interface, addr *udp.Addr, via *ViaSender, packet []by } hostinfo := &HostInfo{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeHostInfo, ID: uint32(vpnIp)}), + syncRWMutex: newSyncRWMutex("hostinfo"), ConnectionState: ci, localIndexId: myIndex, remoteIndexId: hs.Details.InitiatorIndex, @@ -135,7 +135,7 @@ func ixHandshakeStage1(f *Interface, addr *udp.Addr, via *ViaSender, packet []by HandshakePacket: make(map[uint8][]byte, 0), lastHandshakeTime: hs.Details.Time, relayState: RelayState{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeRelayState, ID: uint32(vpnIp)}), + syncRWMutex: newSyncRWMutex("relay-state"), relays: map[iputil.VpnIp]struct{}{}, relayForByIp: map[iputil.VpnIp]*Relay{}, relayForByIdx: map[uint32]*Relay{}, diff --git a/handshake_manager.go b/handshake_manager.go index db7e2a564..5834938ea 100644 --- a/handshake_manager.go +++ b/handshake_manager.go @@ -102,7 +102,7 @@ func (hh *HandshakeHostInfo) cachePacket(l *logrus.Logger, t header.MessageType, func NewHandshakeManager(l *logrus.Logger, mainHostMap *HostMap, lightHouse *LightHouse, outside udp.Conn, config HandshakeConfig) *HandshakeManager { return &HandshakeManager{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeHandshakeManager}), + syncRWMutex: newSyncRWMutex("handshake-manager"), vpnIps: map[iputil.VpnIp]*HandshakeHostInfo{}, indexes: map[uint32]*HandshakeHostInfo{}, mainHostMap: mainHostMap, @@ -385,11 +385,11 @@ func (hm *HandshakeManager) StartHandshake(vpnIp iputil.VpnIp, cacheCb func(*Han } hostinfo := &HostInfo{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeHostInfo, ID: uint32(vpnIp)}), + syncRWMutex: newSyncRWMutex("hostinfo"), vpnIp: vpnIp, HandshakePacket: make(map[uint8][]byte, 0), relayState: RelayState{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeRelayState, ID: uint32(vpnIp)}), + syncRWMutex: newSyncRWMutex("relay-state"), relays: map[iputil.VpnIp]struct{}{}, relayForByIp: map[iputil.VpnIp]*Relay{}, relayForByIdx: map[uint32]*Relay{}, @@ -397,7 +397,7 @@ func (hm *HandshakeManager) StartHandshake(vpnIp iputil.VpnIp, cacheCb func(*Han } hh := &HandshakeHostInfo{ - syncMutex: newSyncMutex(mutexKey{Type: mutexKeyTypeHandshakeHostInfo, ID: uint32(vpnIp)}), + syncMutex: newSyncMutex("handshake-hostinfo"), hostinfo: hostinfo, startTime: time.Now(), } diff --git a/hostmap.go b/hostmap.go index c5cbc5d28..3cf316e3a 100644 --- a/hostmap.go +++ b/hostmap.go @@ -260,7 +260,7 @@ func NewHostMap(l *logrus.Logger, vpnCIDR *net.IPNet, preferredRanges []*net.IPN r := map[uint32]*HostInfo{} relays := map[uint32]*HostInfo{} m := HostMap{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeHostMap}), + syncRWMutex: newSyncRWMutex("hostmap"), Indexes: i, Relays: relays, RemoteIndexes: r, diff --git a/lighthouse.go b/lighthouse.go index 7fc573aad..29970e708 100644 --- a/lighthouse.go +++ b/lighthouse.go @@ -100,7 +100,7 @@ func NewLightHouseFromConfig(ctx context.Context, l *logrus.Logger, c *config.C, ones, _ := myVpnNet.Mask.Size() h := LightHouse{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeLightHouse}), + syncRWMutex: newSyncRWMutex("lighthouse"), ctx: ctx, amLighthouse: amLighthouse, myVpnIp: iputil.Ip2VpnIp(myVpnNet.IP), diff --git a/mutex.go b/mutex.go deleted file mode 100644 index 21a47ae3b..000000000 --- a/mutex.go +++ /dev/null @@ -1,64 +0,0 @@ -package nebula - -import "fmt" - -type mutexKeyType string - -const ( - mutexKeyTypeHostMap mutexKeyType = "hostmap" - - mutexKeyTypeLightHouse = "lighthouse" - mutexKeyTypeRemoteList = "remote-list" - mutexKeyTypeFirewallConntrack = "firewall-conntrack" - mutexKeyTypeHostInfo = "hostinfo" - mutexKeyTypeRelayState = "relay-state" - mutexKeyTypeHandshakeHostInfo = "handshake-hostinfo" - mutexKeyTypeHandshakeManager = "handshake-manager" - mutexKeyTypeConnectionStateWrite = "connection-state-write-lock" - - mutexKeyTypeConnectionManagerIn = "connection-manager-in-lock" - mutexKeyTypeConnectionManagerOut = "connection-manager-out-lock" - mutexKeyTypeConnectionManagerRelayUsed = "connection-manager-relay-used-lock" -) - -// For each Key in this map, the Value is a list of lock types you can already have -// when you want to grab that Key. This ensures that locks are always fetched -// in the same order, to prevent deadlocks. -var allowedConcurrentLocks = map[mutexKeyType][]mutexKeyType{ - mutexKeyTypeHostMap: {mutexKeyTypeHandshakeHostInfo}, - mutexKeyTypeFirewallConntrack: {mutexKeyTypeHandshakeHostInfo}, - - mutexKeyTypeHandshakeManager: {mutexKeyTypeHostMap}, - mutexKeyTypeConnectionStateWrite: {mutexKeyTypeHostMap}, - - mutexKeyTypeLightHouse: {mutexKeyTypeHandshakeManager}, - mutexKeyTypeRemoteList: {mutexKeyTypeLightHouse}, - - mutexKeyTypeConnectionManagerIn: {mutexKeyTypeHostMap}, - mutexKeyTypeConnectionManagerOut: {mutexKeyTypeConnectionStateWrite, mutexKeyTypeConnectionManagerIn}, - mutexKeyTypeConnectionManagerRelayUsed: {mutexKeyTypeHandshakeHostInfo}, - - mutexKeyTypeRelayState: {mutexKeyTypeHostMap, mutexKeyTypeConnectionManagerRelayUsed}, -} - -type mutexKey struct { - Type mutexKeyType - ID uint32 -} - -type mutexValue struct { - file string - line int -} - -func (m mutexKey) String() string { - if m.ID == 0 { - return fmt.Sprintf("%s", m.Type) - } else { - return fmt.Sprintf("%s(%d)", m.Type, m.ID) - } -} - -func (m mutexValue) String() string { - return fmt.Sprintf("%s:%d", m.file, m.line) -} diff --git a/mutex_debug.go b/mutex_debug.go index ce52590d5..b015c5dcd 100644 --- a/mutex_debug.go +++ b/mutex_debug.go @@ -12,35 +12,66 @@ import ( "github.com/timandy/routine" ) +type mutexKey = string + +// For each Key in this map, the Value is a list of lock types you can already have +// when you want to grab that Key. This ensures that locks are always fetched +// in the same order, to prevent deadlocks. +var allowedConcurrentLocks = map[mutexKey][]mutexKey{ + "connection-manager-in": {"hostmap"}, + "connection-manager-out": {"connection-state-write", "connection-manager-in"}, + "connection-manager-relay-used": {"handshake-hostinfo"}, + "connection-state-write": {"hostmap"}, + "firewall-conntrack": {"handshake-hostinfo"}, + "handshake-manager": {"hostmap"}, + "hostmap": {"handshake-hostinfo"}, + "lighthouse": {"handshake-manager"}, + "relay-state": {"hostmap", "connection-manager-relay-used"}, + "remote-list": {"lighthouse"}, +} + +type mutexValue struct { + file string + line int +} + +func (m mutexValue) String() string { + return fmt.Sprintf("%s:%d", m.file, m.line) +} + var threadLocal routine.ThreadLocal = routine.NewThreadLocalWithInitial(func() any { return map[mutexKey]mutexValue{} }) var allowedDAG *dag.DAG +// We build a directed acyclic graph to assert that the locks can only be +// acquired in a determined order, If there are cycles in the DAG, then we +// know that the locking order is not guaranteed. func init() { allowedDAG = dag.NewDAG() for k, v := range allowedConcurrentLocks { - allowedDAG.AddVertexByID(string(k), k) + _ = allowedDAG.AddVertexByID(k, k) for _, t := range v { - if _, err := allowedDAG.GetVertex(string(t)); err != nil { - allowedDAG.AddVertexByID(string(t), t) - } + _ = allowedDAG.AddVertexByID(t, t) } } for k, v := range allowedConcurrentLocks { for _, t := range v { - allowedDAG.AddEdge(string(t), string(k)) + if err := allowedDAG.AddEdge(t, k); err != nil { + panic(fmt.Errorf("Failed to assembled DAG for allowedConcurrentLocks: %w", err)) + } } } + // Rebuild allowedConcurrentLocks as a flattened list of all possibilities for k := range allowedConcurrentLocks { - anc, err := allowedDAG.GetAncestors(string(k)) + anc, err := allowedDAG.GetAncestors(k) if err != nil { panic(err) } - var allowed []mutexKeyType + var allowed []mutexKey for t := range anc { - allowed = append(allowed, mutexKeyType(t)) + allowed = append(allowed, mutexKey(t)) } allowedConcurrentLocks[k] = allowed } @@ -76,7 +107,7 @@ func alertMutex(err error) { } func checkMutex(state map[mutexKey]mutexValue, add mutexKey) { - allowedConcurrent := allowedConcurrentLocks[add.Type] + allowedConcurrent := allowedConcurrentLocks[add] for k, v := range state { if add == k { @@ -86,13 +117,13 @@ func checkMutex(state map[mutexKey]mutexValue, add mutexKey) { // TODO use slices.Contains, but requires go1.21 var found bool for _, a := range allowedConcurrent { - if a == k.Type { + if a == k { found = true break } } if !found { - alertMutex(fmt.Errorf("grabbing %s lock and already have these locks: %s", add.Type, state)) + alertMutex(fmt.Errorf("grabbing %s lock and already have these locks: %s", add, state)) } } } diff --git a/mutex_nodebug.go b/mutex_nodebug.go index 823c4a7cd..76086ff54 100644 --- a/mutex_nodebug.go +++ b/mutex_nodebug.go @@ -7,6 +7,7 @@ import ( "sync" ) +type mutexKey = string type syncRWMutex = sync.RWMutex type syncMutex = sync.Mutex diff --git a/remote_list.go b/remote_list.go index a573e6531..b07d15cc1 100644 --- a/remote_list.go +++ b/remote_list.go @@ -216,7 +216,7 @@ type RemoteList struct { // NewRemoteList creates a new empty RemoteList func NewRemoteList(shouldAdd func(netip.Addr) bool) *RemoteList { return &RemoteList{ - syncRWMutex: newSyncRWMutex(mutexKey{Type: mutexKeyTypeRemoteList}), + syncRWMutex: newSyncRWMutex("remote-list"), addrs: make([]*udp.Addr, 0), relays: make([]*iputil.VpnIp, 0), cache: make(map[iputil.VpnIp]*cache),