From 10a828af2c42b42fdd9f35563b0badfe1039f6af Mon Sep 17 00:00:00 2001 From: Arceliar Date: Mon, 9 Sep 2019 19:20:46 -0500 Subject: [PATCH 1/5] when forwarding traffic, break distance ties by favoring the link that sent the most recent switch update the fastest --- src/yggdrasil/switch.go | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/yggdrasil/switch.go b/src/yggdrasil/switch.go index 163c85c..5ed7032 100644 --- a/src/yggdrasil/switch.go +++ b/src/yggdrasil/switch.go @@ -143,6 +143,7 @@ type switchPort uint64 type tableElem struct { port switchPort locator switchLocator + time time.Time } // This is the subset of the information about all peers needed to make routing decisions, and it stored separately in an atomically accessed table, which gets hammered in the "hot loop" of the routing logic (see: peer.handleTraffic in peers.go). @@ -562,6 +563,7 @@ func (t *switchTable) updateTable() { newTable.elems[pinfo.port] = tableElem{ locator: loc, port: pinfo.port, + time: pinfo.time, } } t.table.Store(newTable) @@ -581,7 +583,7 @@ func (t *switchTable) start() error { } type closerInfo struct { - port switchPort + elem tableElem dist int } @@ -598,7 +600,7 @@ func (t *switchTable) getCloser(dest []byte) []closerInfo { for _, info := range table.elems { dist := info.locator.dist(dest) if dist < myDist { - t.queues.closer = append(t.queues.closer, closerInfo{info.port, dist}) + t.queues.closer = append(t.queues.closer, closerInfo{info, dist}) } } return t.queues.closer @@ -671,13 +673,12 @@ func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]time.Time) bo self.sendPacketsFrom(t, [][]byte{packet}) return true } - var best *peer - var bestDist int + var best *closerInfo var bestTime time.Time ports := t.core.peers.getPorts() for _, cinfo := range closer { - to := ports[cinfo.port] - thisTime, isIdle := idle[cinfo.port] + to := ports[cinfo.elem.port] + thisTime, isIdle := idle[cinfo.elem.port] var update bool switch { case to == nil: @@ -688,13 +689,24 @@ func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]time.Time) bo // this is the first idle port we've found, so select it until we find a // better candidate port to use instead update = true - case cinfo.dist < bestDist: + case cinfo.dist < best.dist: // the port takes a shorter path/is more direct than our current // candidate, so select that instead update = true - case cinfo.dist > bestDist: + case cinfo.dist > best.dist: // the port takes a longer path/is less direct than our current candidate, // ignore it + case cinfo.elem.locator.tstamp > best.elem.locator.tstamp: + // has a newer tstamp from the root, so presumably a better path + update = true + case cinfo.elem.locator.tstamp < best.elem.locator.tstamp: + // has a n older tstamp, so presumably a worse path + case cinfo.elem.time.Before(best.elem.time): + // same tstamp, but got it earlier, so presumably a better path + update = true + case cinfo.elem.time.After(best.elem.time): + // same tstamp, but got it later, so presumably a worse path + // I do not expect the remaining cases to ever be reached... TODO cleanup case thisTime.After(bestTime): // all else equal, this port was used more recently than our current // candidate, so choose that instead. this should mean that, in low @@ -705,15 +717,15 @@ func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]time.Time) bo // the search for a port has finished } if update { - best = to - bestDist = cinfo.dist + b := cinfo // because cinfo gets mutated by the iteration + best = &b bestTime = thisTime } } if best != nil { // Send to the best idle next hop - delete(idle, best.port) - best.sendPacketsFrom(t, [][]byte{packet}) + delete(idle, best.elem.port) + ports[best.elem.port].sendPacketsFrom(t, [][]byte{packet}) return true } // Didn't find anyone idle to send it to From 0141180279cc896f70f8f8b1b80d3e895c24b16a Mon Sep 17 00:00:00 2001 From: Arceliar Date: Mon, 9 Sep 2019 19:25:10 -0500 Subject: [PATCH 2/5] cleanup --- src/yggdrasil/switch.go | 21 +++++---------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/src/yggdrasil/switch.go b/src/yggdrasil/switch.go index 5ed7032..f4df60d 100644 --- a/src/yggdrasil/switch.go +++ b/src/yggdrasil/switch.go @@ -176,7 +176,7 @@ type switchTable struct { table atomic.Value // lookupTable phony.Inbox // Owns the below queues switch_buffers // Queues - not atomic so ONLY use through the actor - idle map[switchPort]time.Time // idle peers - not atomic so ONLY use through the actor + idle map[switchPort]struct{} // idle peers - not atomic so ONLY use through the actor } // Minimum allowed total size of switch queues. @@ -202,7 +202,7 @@ func (t *switchTable) init(core *Core) { } core.config.Mutex.RUnlock() t.queues.bufs = make(map[string]switch_buffer) - t.idle = make(map[switchPort]time.Time) + t.idle = make(map[switchPort]struct{}) }) } @@ -664,7 +664,7 @@ func (t *switchTable) bestPortForCoords(coords []byte) switchPort { // Handle an incoming packet // Either send it to ourself, or to the first idle peer that's free // Returns true if the packet has been handled somehow, false if it should be queued -func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]time.Time) bool { +func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]struct{}) bool { coords := switch_getPacketCoords(packet) closer := t.getCloser(coords) if len(closer) == 0 { @@ -674,11 +674,10 @@ func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]time.Time) bo return true } var best *closerInfo - var bestTime time.Time ports := t.core.peers.getPorts() for _, cinfo := range closer { to := ports[cinfo.elem.port] - thisTime, isIdle := idle[cinfo.elem.port] + _, isIdle := idle[cinfo.elem.port] var update bool switch { case to == nil: @@ -704,22 +703,12 @@ func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]time.Time) bo case cinfo.elem.time.Before(best.elem.time): // same tstamp, but got it earlier, so presumably a better path update = true - case cinfo.elem.time.After(best.elem.time): - // same tstamp, but got it later, so presumably a worse path - // I do not expect the remaining cases to ever be reached... TODO cleanup - case thisTime.After(bestTime): - // all else equal, this port was used more recently than our current - // candidate, so choose that instead. this should mean that, in low - // traffic scenarios, we consistently pick the same link which helps with - // packet ordering - update = true default: // the search for a port has finished } if update { b := cinfo // because cinfo gets mutated by the iteration best = &b - bestTime = thisTime } } if best != nil { @@ -882,6 +871,6 @@ func (t *switchTable) _idleIn(port switchPort) { // Try to find something to send to this peer if !t._handleIdle(port) { // Didn't find anything ready to send yet, so stay idle - t.idle[port] = time.Now() + t.idle[port] = struct{}{} } } From 80ba24d51255c3751e2b25aceee52b20d59ff746 Mon Sep 17 00:00:00 2001 From: Arceliar Date: Tue, 17 Sep 2019 19:42:07 -0500 Subject: [PATCH 3/5] force things to buffer in the switch if the best link is currently busy. note that other links can end up sending if they become non-idle for other reasons. this is a temporary workaround to packet reordering, until we can figure out a better solution --- src/yggdrasil/switch.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/yggdrasil/switch.go b/src/yggdrasil/switch.go index f4df60d..4a1999a 100644 --- a/src/yggdrasil/switch.go +++ b/src/yggdrasil/switch.go @@ -677,13 +677,10 @@ func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]struct{}) boo ports := t.core.peers.getPorts() for _, cinfo := range closer { to := ports[cinfo.elem.port] - _, isIdle := idle[cinfo.elem.port] var update bool switch { case to == nil: // no port was found, ignore it - case !isIdle: - // the port is busy, ignore it case best == nil: // this is the first idle port we've found, so select it until we find a // better candidate port to use instead @@ -713,6 +710,9 @@ func (t *switchTable) _handleIn(packet []byte, idle map[switchPort]struct{}) boo } if best != nil { // Send to the best idle next hop + if _, isIdle := idle[best.elem.port]; !isIdle { + return false + } delete(idle, best.elem.port) ports[best.elem.port].sendPacketsFrom(t, [][]byte{packet}) return true From be35675d0f01aa5e22571f9c71ed36ba4a87b8ba Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Wed, 18 Sep 2019 13:37:01 +0100 Subject: [PATCH 4/5] Catch a nil pointer when sending a session packet to a conn, this shouldn't happen but it's caused multiple crashes in conn.recvMsg --- src/yggdrasil/session.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 8a6d16f..4980862 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -470,7 +470,14 @@ func (sinfo *sessionInfo) _recvPacket(p *wire_trafficPacket) { callback := func() { util.PutBytes(p.Payload) if !isOK || k != sinfo.sharedSesKey || !sinfo._nonceIsOK(&p.Nonce) { - // Either we failed to decrypt, or the session was updated, or we received this packet in the mean time + // Either we failed to decrypt, or the session was updated, or we + // received this packet in the mean time + util.PutBytes(bs) + return + } + if sinfo.conn == nil { + // There's no connection associated with this session for some reason + // TODO: Figure out why this happens sometimes, it shouldn't util.PutBytes(bs) return } From 0a12e4b1c1d50c910defbf0607ccb86165c16791 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Wed, 18 Sep 2019 20:26:06 +0100 Subject: [PATCH 5/5] Revert "Catch a nil pointer when sending a session packet to a conn, this shouldn't happen but it's caused multiple crashes in conn.recvMsg" This reverts commit be35675d0f01aa5e22571f9c71ed36ba4a87b8ba. --- src/yggdrasil/session.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 4980862..8a6d16f 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -470,14 +470,7 @@ func (sinfo *sessionInfo) _recvPacket(p *wire_trafficPacket) { callback := func() { util.PutBytes(p.Payload) if !isOK || k != sinfo.sharedSesKey || !sinfo._nonceIsOK(&p.Nonce) { - // Either we failed to decrypt, or the session was updated, or we - // received this packet in the mean time - util.PutBytes(bs) - return - } - if sinfo.conn == nil { - // There's no connection associated with this session for some reason - // TODO: Figure out why this happens sometimes, it shouldn't + // Either we failed to decrypt, or the session was updated, or we received this packet in the mean time util.PutBytes(bs) return }