From 11b0a82c4a6ad71fce12a7e227dc2356025e62da Mon Sep 17 00:00:00 2001 From: cathugger Date: Sun, 29 Jul 2018 22:09:16 +0000 Subject: [PATCH 1/8] Simpler flowlabel parsing; avoid using 0 flowlabel. --- src/yggdrasil/session.go | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 417760b7..647bb0ce 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -425,28 +425,28 @@ func (sinfo *sessionInfo) doSend(bs []byte) { if !sinfo.init { return } // To prevent using empty session keys - // Now we append something to the coords - // Specifically, we append a 0, and then arbitrary data - // The 0 ensures that the destination node switch forwards to the self peer (router) - // The rest is ignored, but it's still part as the coords, so it affects switch queues - // This helps separate traffic streams (coords, flowlabel) to be queued independently + var coords []byte - addUint64 := func(bs []byte) { - // Converts bytes to a uint64 - // Converts that back to variable length bytes - // Appends it to coords - var u uint64 - for _, b := range bs { - u <<= 8 - u |= uint64(b) - } - coords = append(coords, wire_encode_uint64(u)...) + // Read IPv6 flowlabel field (20 bits). + // XXX(cathugger): is len(bs) validated there? + flowlabel := uint(bs[1]&0x0f)<<16 | uint(bs[2])<<8 | uint(bs[3]) + if flowlabel != 0 { + // Now we append something to the coords + // Specifically, we append a 0, and then arbitrary data + // The 0 ensures that the destination node switch forwards to the self peer (router) + // The rest is ignored, but it's still part as the coords, so it affects switch queues + // This helps separate traffic streams (coords, flowlabel) to be queued independently + + coords = append(coords, sinfo.coords...) // Start with the real coords + coords = append(coords, 0) // Then target the local switchport + coords = append(coords, wire_encode_uint64(uint64(flowlabel))...) // Then variable-length encoded flowlabel + } else { + // 0 value means that flowlabels aren't being generated by OS. + // To save bytes, we're not including it, therefore we won't need self-port override either. + // So just use sinfo.coords directly to avoid golang GC allocations. + // XXX: investigate where flowlabels aren't included, and attempt to look into TCP/UDP/SCTP/DCCP headers' sport/dport fields? + coords = sinfo.coords } - coords = append(coords, sinfo.coords...) // Start with the real coords - coords = append(coords, 0) // Then target the local switchport - flowlabel := append([]byte(nil), bs[1:4]...) - flowlabel[0] &= 0x0f - addUint64(flowlabel) payload, nonce := boxSeal(&sinfo.sharedSesKey, bs, &sinfo.myNonce) defer util_putBytes(payload) p := wire_trafficPacket{ From fec71008984994d4849287608f41164f5c953de2 Mon Sep 17 00:00:00 2001 From: cathugger Date: Mon, 30 Jul 2018 00:01:37 +0000 Subject: [PATCH 2/8] Clean up / clarify coords sending code. --- src/yggdrasil/session.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 647bb0ce..4a448871 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -428,7 +428,7 @@ func (sinfo *sessionInfo) doSend(bs []byte) { var coords []byte // Read IPv6 flowlabel field (20 bits). - // XXX(cathugger): is len(bs) validated there? + // Assumes packet at least contains IPv6 header. flowlabel := uint(bs[1]&0x0f)<<16 | uint(bs[2])<<8 | uint(bs[3]) if flowlabel != 0 { // Now we append something to the coords @@ -437,14 +437,15 @@ func (sinfo *sessionInfo) doSend(bs []byte) { // The rest is ignored, but it's still part as the coords, so it affects switch queues // This helps separate traffic streams (coords, flowlabel) to be queued independently - coords = append(coords, sinfo.coords...) // Start with the real coords - coords = append(coords, 0) // Then target the local switchport - coords = append(coords, wire_encode_uint64(uint64(flowlabel))...) // Then variable-length encoded flowlabel + coords = append(coords, sinfo.coords...) // Start with the real coords + coords = append(coords, 0) // Then target the local switchport + coords = wire_put_uint64(uint64(flowlabel), coords) // Then variable-length encoded flowlabel } else { // 0 value means that flowlabels aren't being generated by OS. // To save bytes, we're not including it, therefore we won't need self-port override either. // So just use sinfo.coords directly to avoid golang GC allocations. - // XXX: investigate where flowlabels aren't included, and attempt to look into TCP/UDP/SCTP/DCCP headers' sport/dport fields? + // Recent enough Linux kernel supports flowlabels out of the box so this will be rare. + // XXX Attempt to look into TCP/UDP/SCTP/DCCP headers' sport/dport fields there? coords = sinfo.coords } payload, nonce := boxSeal(&sinfo.sharedSesKey, bs, &sinfo.myNonce) From 36dcab9300bbf8b0cf1aa53e1de6561344a3dc3c Mon Sep 17 00:00:00 2001 From: cathugger Date: Mon, 30 Jul 2018 01:58:52 +0000 Subject: [PATCH 3/8] optimize wire_put_uint64; use protokey for flowlabel fallback. --- src/yggdrasil/session.go | 15 ++++++++++++--- src/yggdrasil/wire.go | 20 ++++++++------------ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 4a448871..7a1f19db 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -440,12 +440,21 @@ func (sinfo *sessionInfo) doSend(bs []byte) { coords = append(coords, sinfo.coords...) // Start with the real coords coords = append(coords, 0) // Then target the local switchport coords = wire_put_uint64(uint64(flowlabel), coords) // Then variable-length encoded flowlabel + } else if len(bs) >= 48 /* min UDP len, others are bigger */ && + (bs[6] == 0x06 || bs[6] == 0x11 || bs[6] == 0x84) /* TCP UDP SCTP */ { + // if flowlabel was unspecified (0), try to use known protocols' ports + // protokey: proto | sport | dport + pkey := uint64(bs[6])<<32 /* proto */ | + uint64(bs[40])<<24 | uint64(bs[41])<<16 /* sport */ | + uint64(bs[42])<<8 | uint64(bs[43]) /* dport */ + coords = append(coords, sinfo.coords...) // Start with the real coords + coords = append(coords, 0) // Then target the local switchport + coords = wire_put_uint64(pkey, coords) // Then variable-length encoded protokey } else { - // 0 value means that flowlabels aren't being generated by OS. + // flowlabel was unspecified (0) and protocol unrecognised. // To save bytes, we're not including it, therefore we won't need self-port override either. // So just use sinfo.coords directly to avoid golang GC allocations. - // Recent enough Linux kernel supports flowlabels out of the box so this will be rare. - // XXX Attempt to look into TCP/UDP/SCTP/DCCP headers' sport/dport fields there? + // Recent enough Linux and BSDs support flowlabels (auto_flowlabel) out of the box so this will be rare. coords = sinfo.coords } payload, nonce := boxSeal(&sinfo.sharedSesKey, bs, &sinfo.myNonce) diff --git a/src/yggdrasil/wire.go b/src/yggdrasil/wire.go index e92b4fcf..d05624e2 100644 --- a/src/yggdrasil/wire.go +++ b/src/yggdrasil/wire.go @@ -25,19 +25,15 @@ func wire_encode_uint64(elem uint64) []byte { // Encode uint64 using a variable length scheme. // Similar to binary.Uvarint, but big-endian. -func wire_put_uint64(elem uint64, out []byte) []byte { - bs := make([]byte, 0, 10) - bs = append(bs, byte(elem&0x7f)) - for e := elem >> 7; e > 0; e >>= 7 { - bs = append(bs, byte(e|0x80)) +func wire_put_uint64(e uint64, out []byte) []byte { + var b [10]byte + i := len(b) - 1 + b[i] = byte(e & 0x7f) + for e >>= 7; e != 0; e >>= 7 { + i-- + b[i] = byte(e | 0x80) } - // Now reverse bytes, because we set them in the wrong order - // TODO just put them in the right place the first time... - last := len(bs) - 1 - for idx := 0; idx < len(bs)/2; idx++ { - bs[idx], bs[last-idx] = bs[last-idx], bs[idx] - } - return append(out, bs...) + return append(out, b[i:]...) } // Returns the length of a wire encoded uint64 of this value. From 68a482ed92fa052ba67b8defdf1812ca2f1aac18 Mon Sep 17 00:00:00 2001 From: cathugger Date: Mon, 30 Jul 2018 02:15:57 +0000 Subject: [PATCH 4/8] Simplify flowkey stuff further. --- src/yggdrasil/session.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 7a1f19db..9a4dff31 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -426,30 +426,30 @@ func (sinfo *sessionInfo) doSend(bs []byte) { return } // To prevent using empty session keys - var coords []byte // Read IPv6 flowlabel field (20 bits). // Assumes packet at least contains IPv6 header. - flowlabel := uint(bs[1]&0x0f)<<16 | uint(bs[2])<<8 | uint(bs[3]) - if flowlabel != 0 { + flowkey := uint64(bs[1]&0x0f)<<16 | uint64(bs[2])<<8 | uint64(bs[3]) + if flowkey == 0 /* not specified */ && + len(bs) >= 48 /* min UDP len, others are bigger */ && + (bs[6] == 0x06 || bs[6] == 0x11 || bs[6] == 0x84) /* TCP UDP SCTP */ { + // if flowlabel was unspecified (0), try to use known protocols' ports + // protokey: proto | sport | dport + flowkey = uint64(bs[6])<<32 /* proto */ | + uint64(bs[40])<<24 | uint64(bs[41])<<16 /* sport */ | + uint64(bs[42])<<8 | uint64(bs[43]) /* dport */ + } + var coords []byte + if flowkey != 0 { // Now we append something to the coords // Specifically, we append a 0, and then arbitrary data // The 0 ensures that the destination node switch forwards to the self peer (router) // The rest is ignored, but it's still part as the coords, so it affects switch queues // This helps separate traffic streams (coords, flowlabel) to be queued independently - coords = append(coords, sinfo.coords...) // Start with the real coords - coords = append(coords, 0) // Then target the local switchport - coords = wire_put_uint64(uint64(flowlabel), coords) // Then variable-length encoded flowlabel - } else if len(bs) >= 48 /* min UDP len, others are bigger */ && - (bs[6] == 0x06 || bs[6] == 0x11 || bs[6] == 0x84) /* TCP UDP SCTP */ { - // if flowlabel was unspecified (0), try to use known protocols' ports - // protokey: proto | sport | dport - pkey := uint64(bs[6])<<32 /* proto */ | - uint64(bs[40])<<24 | uint64(bs[41])<<16 /* sport */ | - uint64(bs[42])<<8 | uint64(bs[43]) /* dport */ - coords = append(coords, sinfo.coords...) // Start with the real coords - coords = append(coords, 0) // Then target the local switchport - coords = wire_put_uint64(pkey, coords) // Then variable-length encoded protokey + // TODO could we avoid allocations there and put this work into wire_trafficPacket.encode()? + coords = append(coords, sinfo.coords...) // Start with the real coords + coords = append(coords, 0) // Then target the local switchport + coords = wire_put_uint64(flowkey, coords) // Then variable-length encoded flowkey } else { // flowlabel was unspecified (0) and protocol unrecognised. // To save bytes, we're not including it, therefore we won't need self-port override either. From ebb4ec7c33f55214a7c0a4d2d80a0e940ce04e7a Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Mon, 30 Jul 2018 11:46:44 +0100 Subject: [PATCH 5/8] Clean up the flow a bit (partly because I am allergic to huge compounded if statements) --- src/yggdrasil/session.go | 49 +++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 9a4dff31..ac1e3930 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -423,40 +423,37 @@ func (sinfo *sessionInfo) doWorker() { func (sinfo *sessionInfo) doSend(bs []byte) { defer util_putBytes(bs) if !sinfo.init { + // To prevent using empty session keys return - } // To prevent using empty session keys - + } + coords := sinfo.coords // Read IPv6 flowlabel field (20 bits). // Assumes packet at least contains IPv6 header. flowkey := uint64(bs[1]&0x0f)<<16 | uint64(bs[2])<<8 | uint64(bs[3]) - if flowkey == 0 /* not specified */ && - len(bs) >= 48 /* min UDP len, others are bigger */ && - (bs[6] == 0x06 || bs[6] == 0x11 || bs[6] == 0x84) /* TCP UDP SCTP */ { - // if flowlabel was unspecified (0), try to use known protocols' ports - // protokey: proto | sport | dport - flowkey = uint64(bs[6])<<32 /* proto */ | - uint64(bs[40])<<24 | uint64(bs[41])<<16 /* sport */ | - uint64(bs[42])<<8 | uint64(bs[43]) /* dport */ + // Check if the flowlabel was specified + if flowkey == 0 { + // Does the packet meet the minimum UDP packet size? (others are bigger) + if len(bs) >= 48 { + // Is the protocol TCP, UDP, SCTP? + if bs[6] == 0x06 || bs[6] == 0x11 || bs[6] == 0x84 { + // if flowlabel was unspecified (0), try to use known protocols' ports + // protokey: proto | sport | dport + flowkey = uint64(bs[6])<<32 /* proto */ | + uint64(bs[40])<<24 | uint64(bs[41])<<16 /* sport */ | + uint64(bs[42])<<8 | uint64(bs[43]) /* dport */ + } + } } - var coords []byte + // If we have a flowkey, either through the IPv6 flowlabel field or through + // known TCP/UDP/SCTP proto-sport-dport triplet, then append it to the coords. + // Appending extra coords after a 0 ensures that we still target the local router + // but lets us send extra data (which is otherwise ignored) to help separate + // traffic streams into independent queues if flowkey != 0 { - // Now we append something to the coords - // Specifically, we append a 0, and then arbitrary data - // The 0 ensures that the destination node switch forwards to the self peer (router) - // The rest is ignored, but it's still part as the coords, so it affects switch queues - // This helps separate traffic streams (coords, flowlabel) to be queued independently - - // TODO could we avoid allocations there and put this work into wire_trafficPacket.encode()? - coords = append(coords, sinfo.coords...) // Start with the real coords - coords = append(coords, 0) // Then target the local switchport + coords = append(coords, 0) // First target the local switchport coords = wire_put_uint64(flowkey, coords) // Then variable-length encoded flowkey - } else { - // flowlabel was unspecified (0) and protocol unrecognised. - // To save bytes, we're not including it, therefore we won't need self-port override either. - // So just use sinfo.coords directly to avoid golang GC allocations. - // Recent enough Linux and BSDs support flowlabels (auto_flowlabel) out of the box so this will be rare. - coords = sinfo.coords } + // Prepare the payload payload, nonce := boxSeal(&sinfo.sharedSesKey, bs, &sinfo.myNonce) defer util_putBytes(payload) p := wire_trafficPacket{ From c4e6894d6af8c6b21598524be0ae89245621d334 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Mon, 30 Jul 2018 13:34:32 +0100 Subject: [PATCH 6/8] Copy sinfo.coords for safety --- src/yggdrasil/session.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index ac1e3930..bc9d21f0 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -426,7 +426,8 @@ func (sinfo *sessionInfo) doSend(bs []byte) { // To prevent using empty session keys return } - coords := sinfo.coords + var coords []byte + coords = append(coords, sinfo.coords...) // Read IPv6 flowlabel field (20 bits). // Assumes packet at least contains IPv6 header. flowkey := uint64(bs[1]&0x0f)<<16 | uint64(bs[2])<<8 | uint64(bs[3]) From 67b8a7a53d87dddbd182b8d812adf79846f1b7a3 Mon Sep 17 00:00:00 2001 From: cathugger Date: Mon, 30 Jul 2018 12:43:34 +0000 Subject: [PATCH 7/8] Ensure no memory allocations happen at hot path --- src/yggdrasil/session.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index bc9d21f0..0abf524c 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -72,7 +72,8 @@ func (s *sessionInfo) update(p *sessionPing) bool { if p.MTU >= 1280 || p.MTU == 0 { s.theirMTU = p.MTU } - s.coords = append([]byte{}, p.Coords...) + // allocate enough space for additional coords + s.coords = append(make([]byte, 0, len(p.Coords)+11), p.Coords...) now := time.Now() s.time = now s.tstamp = p.Tstamp @@ -426,8 +427,8 @@ func (sinfo *sessionInfo) doSend(bs []byte) { // To prevent using empty session keys return } - var coords []byte - coords = append(coords, sinfo.coords...) + // code isn't multithreaded so appending to this is safe + coords := sinfo.coords // Read IPv6 flowlabel field (20 bits). // Assumes packet at least contains IPv6 header. flowkey := uint64(bs[1]&0x0f)<<16 | uint64(bs[2])<<8 | uint64(bs[3]) From b4db89ea9def1e851c67d90f493ce57f55c322ba Mon Sep 17 00:00:00 2001 From: cathugger Date: Mon, 30 Jul 2018 13:44:46 +0000 Subject: [PATCH 8/8] Avoid unnecessarily allocating coords slice if it's unchanged. --- src/yggdrasil/session.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index 0abf524c..7a332265 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -4,7 +4,10 @@ package yggdrasil // It's responsible for keeping track of open sessions to other nodes // The session information consists of crypto keys and coords -import "time" +import ( + "bytes" + "time" +) // All the information we know about an active session. // This includes coords, permanent and ephemeral keys, handles and nonces, various sorts of timing information for timeout and maintenance, and some metadata for the admin API. @@ -72,8 +75,10 @@ func (s *sessionInfo) update(p *sessionPing) bool { if p.MTU >= 1280 || p.MTU == 0 { s.theirMTU = p.MTU } - // allocate enough space for additional coords - s.coords = append(make([]byte, 0, len(p.Coords)+11), p.Coords...) + if !bytes.Equal(s.coords, p.Coords) { + // allocate enough space for additional coords + s.coords = append(make([]byte, 0, len(p.Coords)+11), p.Coords...) + } now := time.Now() s.time = now s.tstamp = p.Tstamp