[PATCH 00/12] Misc patches

[PATCH 00/12] Misc patches

[Simon Ruderich]

Hello,

In addition to the TUN patches I noticed a few other things while
reading the code. Some of these patches might be biased so just
drop anything you don't like (or tell me how to improve it)

Regards
Simon

Simon Ruderich (12):
  Fix typos in comments
  tun_linux: use getDummySock()
  receive, send: use AtomicBool for dropped in QueueInboundElement,
    QueueOutboundElement
  device: move removePeerUnsafe() into Device
  index: NewIndex(): don't use separate read-lock to check if index is
    present
  device: use UnderLoadAfterTime constant
  peer: NewPeer(): add missing device.mutex.Unlock() in error paths
  conn_linux: move comment to make its meaning more obvious
  ratelimiter: Allow(): don't use separate read-lock to check if ip is
    present
  receive, send: specialize addTo*Queue() functions
  noise_protocol: mixHash(): remove unnecessary Reset()
  timers: log error if handshake sending fails

 src/conn_linux.go     |  9 ++++-----
 src/device.go         | 12 ++++++------
 src/index.go          | 14 ++++----------
 src/keypair.go        |  2 +-
 src/main.go           |  2 +-
 src/noise_protocol.go | 11 +++++------
 src/peer.go           |  4 +++-
 src/ratelimiter.go    | 10 ++--------
 src/receive.go        | 30 +++++++++++-------------------
 src/send.go           | 17 +++++++----------
 src/timers.go         |  4 ++--
 src/tun_linux.go      | 23 ++---------------------
 12 files changed, 48 insertions(+), 90 deletions(-)

-- 
2.15.1

[PATCH 01/12] Fix typos in comments

[Simon Ruderich]

---
 src/conn_linux.go     |  6 +++---
 src/index.go          |  2 +-
 src/keypair.go        |  2 +-
 src/main.go           |  2 +-
 src/noise_protocol.go | 10 +++++-----
 src/peer.go           |  2 +-
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/conn_linux.go b/src/conn_linux.go
index cdba74f..63ad2ec 100644
--- a/src/conn_linux.go
+++ b/src/conn_linux.go
@@ -19,7 +19,7 @@ import (
  *
  * Currently there is no way to achieve this within the net package:
  * See e.g. https://github.com/golang/go/issues/17930
- * So this code is remains platform dependent.
+ * So this code remains platform dependent.
  */
 type NativeEndpoint struct {
 	src unix.RawSockaddrInet6
@@ -488,7 +488,7 @@ func send4(sock int, end *NativeEndpoint, buff []byte) error {
 
 func receive4(sock int, buff []byte, end *NativeEndpoint) (int, error) {
 
-	// contruct message header
+	// construct message header
 
 	var iovec unix.Iovec
 	iovec.Base = (*byte)(unsafe.Pointer(&buff[0]))
@@ -536,7 +536,7 @@ func receive4(sock int, buff []byte, end *NativeEndpoint) (int, error) {
 
 func receive6(sock int, buff []byte, end *NativeEndpoint) (int, error) {
 
-	// contruct message header
+	// construct message header
 
 	var iovec unix.Iovec
 	iovec.Base = (*byte)(unsafe.Pointer(&buff[0]))
diff --git a/src/index.go b/src/index.go
index 1ba040e..13e8693 100644
--- a/src/index.go
+++ b/src/index.go
@@ -6,7 +6,7 @@ import (
 	"sync"
 )
 
-/* Index=0 is reserved for unset indecies
+/* Index=0 is reserved for unset indices
  *
  */
 
diff --git a/src/keypair.go b/src/keypair.go
index 7e5297b..92c03c7 100644
--- a/src/keypair.go
+++ b/src/keypair.go
@@ -7,7 +7,7 @@ import (
 )
 
 /* Due to limitations in Go and /x/crypto there is currently
- * no way to ensure that key material is securely ereased in memory.
+ * no way to ensure that key material is securely erased in memory.
  *
  * Since this may harm the forward secrecy property,
  * we plan to resolve this issue; whenever Go allows us to do so.
diff --git a/src/main.go b/src/main.go
index b12bb09..8d1b364 100644
--- a/src/main.go
+++ b/src/main.go
@@ -156,7 +156,7 @@ func main() {
 
 	logger.Info.Println("Device started")
 
-	// start uapi listener
+	// start UAPI listener
 
 	errs := make(chan error)
 	term := make(chan os.Signal)
diff --git a/src/noise_protocol.go b/src/noise_protocol.go
index 2f9e1d5..2bf38fe 100644
--- a/src/noise_protocol.go
+++ b/src/noise_protocol.go
@@ -32,13 +32,13 @@ const (
 )
 
 const (
-	MessageInitiationSize      = 148                                           // size of handshake initation message
+	MessageInitiationSize      = 148                                           // size of handshake initiation message
 	MessageResponseSize        = 92                                            // size of response message
 	MessageCookieReplySize     = 64                                            // size of cookie reply message
-	MessageTransportHeaderSize = 16                                            // size of data preceeding content in transport message
+	MessageTransportHeaderSize = 16                                            // size of data preceding content in transport message
 	MessageTransportSize       = MessageTransportHeaderSize + poly1305.TagSize // size of empty transport
-	MessageKeepaliveSize       = MessageTransportSize                          // size of keepalive
-	MessageHandshakeSize       = MessageInitiationSize                         // size of largest handshake releated message
+	MessageKeepaliveSize       = MessageTransportSize                          // size of keep-alive
+	MessageHandshakeSize       = MessageInitiationSize                         // size of largest handshake related message
 )
 
 const (
@@ -371,7 +371,7 @@ func (device *Device) ConsumeMessageResponse(msg *MessageResponse) *Peer {
 		return nil
 	}
 
-	// lookup handshake by reciever
+	// lookup handshake by receiver
 
 	lookup := device.indices.Lookup(msg.Receiver)
 	handshake := lookup.handshake
diff --git a/src/peer.go b/src/peer.go
index 7c6ad47..228fe73 100644
--- a/src/peer.go
+++ b/src/peer.go
@@ -40,7 +40,7 @@ type Peer struct {
 		// state related to WireGuard timers
 
 		keepalivePersistent Timer // set for persistent keepalives
-		keepalivePassive    Timer // set upon recieving messages
+		keepalivePassive    Timer // set upon receiving messages
 		zeroAllKeys         Timer // zero all key material
 		handshakeNew        Timer // begin a new handshake (stale)
 		handshakeDeadline   Timer // complete handshake timeout
-- 
2.15.1

[PATCH 02/12] tun_linux: use getDummySock()

[Simon Ruderich]

Using this helper reduces code duplication.
---
 src/tun_linux.go | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

diff --git a/src/tun_linux.go b/src/tun_linux.go
index daa2462..b88426b 100644
--- a/src/tun_linux.go
+++ b/src/tun_linux.go
@@ -156,7 +156,6 @@ func getIFIndex(name string) (int32, error) {
 	if err != nil {
 		return 0, err
 	}
-
 	defer unix.Close(fd)
 
 	var ifr [IFReqSize]byte
@@ -177,19 +176,10 @@ func getIFIndex(name string) (int32, error) {
 }
 
 func (tun *NativeTun) setMTU(n int) error {
-
-	// open datagram socket
-
-	fd, err := unix.Socket(
-		unix.AF_INET,
-		unix.SOCK_DGRAM,
-		0,
-	)
-
+	fd, err := getDummySock()
 	if err != nil {
 		return err
 	}
-
 	defer unix.Close(fd)
 
 	// do ioctl call
@@ -212,19 +202,10 @@ func (tun *NativeTun) setMTU(n int) error {
 }
 
 func (tun *NativeTun) MTU() (int, error) {
-
-	// open datagram socket
-
-	fd, err := unix.Socket(
-		unix.AF_INET,
-		unix.SOCK_DGRAM,
-		0,
-	)
-
+	fd, err := getDummySock()
 	if err != nil {
 		return 0, err
 	}
-
 	defer unix.Close(fd)
 
 	// do ioctl call
-- 
2.15.1

[PATCH 03/12] receive, send: use AtomicBool for dropped in QueueInboundElement, QueueOutboundElement

[Simon Ruderich]

Also remove explicit initialization which defaults to false anyway.

This removes code duplication with AtomicBool.
---
 src/receive.go | 7 +++----
 src/send.go    | 9 ++++-----
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/receive.go b/src/receive.go
index dbd2813..2c4f191 100644
--- a/src/receive.go
+++ b/src/receive.go
@@ -20,7 +20,7 @@ type QueueHandshakeElement struct {
 }
 
 type QueueInboundElement struct {
-	dropped  int32
+	dropped  AtomicBool
 	mutex    sync.Mutex
 	buffer   *[MaxMessageSize]byte
 	packet   []byte
@@ -30,11 +30,11 @@ type QueueInboundElement struct {
 }
 
 func (elem *QueueInboundElement) Drop() {
-	atomic.StoreInt32(&elem.dropped, AtomicTrue)
+	elem.dropped.Set(true)
 }
 
 func (elem *QueueInboundElement) IsDropped() bool {
-	return atomic.LoadInt32(&elem.dropped) == AtomicTrue
+	return elem.dropped.Get()
 }
 
 func (device *Device) addToInboundQueue(
@@ -177,7 +177,6 @@ func (device *Device) RoutineReceiveIncoming(IP int, bind Bind) {
 				packet:   packet,
 				buffer:   buffer,
 				keyPair:  keyPair,
-				dropped:  AtomicFalse,
 				endpoint: endpoint,
 			}
 			elem.mutex.Lock()
diff --git a/src/send.go b/src/send.go
index 9537f5e..163b75f 100644
--- a/src/send.go
+++ b/src/send.go
@@ -36,7 +36,7 @@ import (
  */
 
 type QueueOutboundElement struct {
-	dropped int32
+	dropped AtomicBool
 	mutex   sync.Mutex
 	buffer  *[MaxMessageSize]byte // slice holding the packet data
 	packet  []byte                // slice of "buffer" (always!)
@@ -58,17 +58,16 @@ func (peer *Peer) FlushNonceQueue() {
 
 func (device *Device) NewOutboundElement() *QueueOutboundElement {
 	return &QueueOutboundElement{
-		dropped: AtomicFalse,
 		buffer:  device.pool.messageBuffers.Get().(*[MaxMessageSize]byte),
 	}
 }
 
 func (elem *QueueOutboundElement) Drop() {
-	atomic.StoreInt32(&elem.dropped, AtomicTrue)
+	elem.dropped.Set(true)
 }
 
 func (elem *QueueOutboundElement) IsDropped() bool {
-	return atomic.LoadInt32(&elem.dropped) == AtomicTrue
+	return elem.dropped.Get()
 }
 
 func addToOutboundQueue(
@@ -227,7 +226,7 @@ func (peer *Peer) RoutineNonce() {
 			elem.peer = peer
 			elem.nonce = atomic.AddUint64(&keyPair.sendNonce, 1) - 1
 			elem.keyPair = keyPair
-			elem.dropped = AtomicFalse
+			elem.dropped.Set(false)
 			elem.mutex.Lock()
 
 			// add to parallel and sequential queue
-- 
2.15.1

[PATCH 04/12] device: move removePeerUnsafe() into Device

[Simon Ruderich]

Feels more natural as it operates on a Device's state and is just one
more private helper function.
---
 src/device.go | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/device.go b/src/device.go
index f4a087c..537af67 100644
--- a/src/device.go
+++ b/src/device.go
@@ -70,7 +70,7 @@ func (device *Device) Down() {
 /* Warning:
  * The caller must hold the device mutex (write lock)
  */
-func removePeerUnsafe(device *Device, key NoisePublicKey) {
+func (device *Device) removePeerUnsafe(key NoisePublicKey) {
 	peer, ok := device.peers[key]
 	if !ok {
 		return
@@ -109,7 +109,7 @@ func (device *Device) SetPrivateKey(sk NoisePrivateKey) error {
 		h := &peer.handshake
 		h.mutex.RLock()
 		if h.remoteStatic.Equals(publicKey) {
-			removePeerUnsafe(device, key)
+			device.removePeerUnsafe(key)
 		}
 		h.mutex.RUnlock()
 	}
@@ -132,7 +132,7 @@ func (device *Device) SetPrivateKey(sk NoisePrivateKey) error {
 		} else {
 			h.precomputedStaticStatic = device.privateKey.sharedSecret(h.remoteStatic)
 			if isZero(h.precomputedStaticStatic[:]) {
-				removePeerUnsafe(device, key)
+				device.removePeerUnsafe(key)
 			}
 		}
 		h.mutex.Unlock()
@@ -214,14 +214,14 @@ func (device *Device) LookupPeer(pk NoisePublicKey) *Peer {
 func (device *Device) RemovePeer(key NoisePublicKey) {
 	device.mutex.Lock()
 	defer device.mutex.Unlock()
-	removePeerUnsafe(device, key)
+	device.removePeerUnsafe(key)
 }
 
 func (device *Device) RemoveAllPeers() {
 	device.mutex.Lock()
 	defer device.mutex.Unlock()
 	for key := range device.peers {
-		removePeerUnsafe(device, key)
+		device.removePeerUnsafe(key)
 	}
 }
 
-- 
2.15.1

[PATCH 05/12] index: NewIndex(): don't use separate read-lock to check if index is present

[Simon Ruderich]

The new index is random making collisions very unlikely, therefore
taking a separate read-only lock before taking the full lock in most
cases anyway will not improve performance and only makes the code more
complicated. However I didn't benchmark this change.
---
 src/index.go | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/src/index.go b/src/index.go
index 13e8693..bf8f0a0 100644
--- a/src/index.go
+++ b/src/index.go
@@ -63,21 +63,15 @@ func (table *IndexTable) NewIndex(peer *Peer) (uint32, error) {
 
 		// check if index used
 
-		table.mutex.RLock()
-		_, ok := table.table[index]
-		table.mutex.RUnlock()
-		if ok {
-			continue
-		}
-
-		// map index to handshake
-
 		table.mutex.Lock()
 		_, found := table.table[index]
 		if found {
 			table.mutex.Unlock()
 			continue
 		}
+
+		// map index to handshake
+
 		table.table[index] = IndexTableEntry{
 			peer:      peer,
 			handshake: &peer.handshake,
-- 
2.15.1

[PATCH 06/12] device: use UnderLoadAfterTime constant

[Simon Ruderich]

---
 src/device.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/device.go b/src/device.go
index 537af67..85d65b2 100644
--- a/src/device.go
+++ b/src/device.go
@@ -88,7 +88,7 @@ func (device *Device) IsUnderLoad() bool {
 	now := time.Now()
 	underLoad := len(device.queue.handshake) >= UnderLoadQueueSize
 	if underLoad {
-		device.underLoadUntil.Store(now.Add(time.Second))
+		device.underLoadUntil.Store(now.Add(UnderLoadAfterTime))
 		return true
 	}
 
-- 
2.15.1

[PATCH 07/12] peer: NewPeer(): add missing device.mutex.Unlock() in error paths

[Simon Ruderich]

---
 src/peer.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/peer.go b/src/peer.go
index 228fe73..244bae0 100644
--- a/src/peer.go
+++ b/src/peer.go
@@ -83,6 +83,7 @@ func (device *Device) NewPeer(pk NoisePublicKey) (*Peer, error) {
 	// check if over limit
 
 	if len(device.peers) >= MaxPeers {
+		device.mutex.Unlock()
 		return nil, errors.New("Too many peers")
 	}
 
@@ -90,6 +91,7 @@ func (device *Device) NewPeer(pk NoisePublicKey) (*Peer, error) {
 
 	_, ok := device.peers[pk]
 	if ok {
+		device.mutex.Unlock()
 		return nil, errors.New("Adding existing peer")
 	}
 	device.peers[pk] = peer
-- 
2.15.1

[PATCH 08/12] conn_linux: move comment to make its meaning more obvious

[Simon Ruderich]

At first I didn't understand the connection between errno == 0 and
"error instance", putting it immediately before the nil should help.
---
 src/conn_linux.go | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/conn_linux.go b/src/conn_linux.go
index 63ad2ec..571c882 100644
--- a/src/conn_linux.go
+++ b/src/conn_linux.go
@@ -477,9 +477,8 @@ func send4(sock int, end *NativeEndpoint, buff []byte) error {
 		)
 	}
 
-	// errno = 0 is still an error instance
-
 	if errno == 0 {
+		// errno = 0 is still an error instance
 		return nil
 	}
 
-- 
2.15.1

[PATCH 09/12] ratelimiter: Allow(): don't use separate read-lock to check if ip is present

[Simon Ruderich]

We have to take the full write-lock immediately afterwards anyway and
there's no costly operation between taking the read-lock and the
write-lock. Taking only one lock should improve the performance and
makes the code easier to read. However I haven't benchmarked this.
---
 src/ratelimiter.go | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/ratelimiter.go b/src/ratelimiter.go
index 6e5f005..5b30926 100644
--- a/src/ratelimiter.go
+++ b/src/ratelimiter.go
@@ -89,7 +89,8 @@ func (rate *Ratelimiter) Allow(ip net.IP) bool {
 	IPv4 := ip.To4()
 	IPv6 := ip.To16()
 
-	rate.mutex.RLock()
+	rate.mutex.Lock()
+	defer rate.mutex.Unlock()
 
 	if IPv4 != nil {
 		copy(KeyIPv4[:], IPv4)
@@ -99,12 +100,9 @@ func (rate *Ratelimiter) Allow(ip net.IP) bool {
 		entry = rate.tableIPv6[KeyIPv6]
 	}
 
-	rate.mutex.RUnlock()
-
 	// make new entry if not found
 
 	if entry == nil {
-		rate.mutex.Lock()
 		entry = new(RatelimiterEntry)
 		entry.tokens = RatelimiterMaxTokens - RatelimiterPacketCost
 		entry.lastTime = time.Now()
@@ -113,13 +111,11 @@ func (rate *Ratelimiter) Allow(ip net.IP) bool {
 		} else {
 			rate.tableIPv6[KeyIPv6] = entry
 		}
-		rate.mutex.Unlock()
 		return true
 	}
 
 	// add tokens to entry
 
-	entry.mutex.Lock()
 	now := time.Now()
 	entry.tokens += now.Sub(entry.lastTime).Nanoseconds()
 	entry.lastTime = now
@@ -131,9 +127,7 @@ func (rate *Ratelimiter) Allow(ip net.IP) bool {
 
 	if entry.tokens > RatelimiterPacketCost {
 		entry.tokens -= RatelimiterPacketCost
-		entry.mutex.Unlock()
 		return true
 	}
-	entry.mutex.Unlock()
 	return false
 }
-- 
2.15.1

[PATCH 10/12] receive, send: specialize addTo*Queue() functions

[Simon Ruderich]

As each function is only used with a single queue, this removes an
unnecessary argument and prevents passing the wrong queue.
---
 src/receive.go | 23 ++++++++---------------
 src/send.go    |  8 +++-----
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/src/receive.go b/src/receive.go
index 2c4f191..f5cbe74 100644
--- a/src/receive.go
+++ b/src/receive.go
@@ -37,10 +37,8 @@ func (elem *QueueInboundElement) IsDropped() bool {
 	return elem.dropped.Get()
 }
 
-func (device *Device) addToInboundQueue(
-	queue chan *QueueInboundElement,
-	element *QueueInboundElement,
-) {
+func (peer *Peer) addToInboundQueue(element *QueueInboundElement) {
+	queue := peer.queue.inbound
 	for {
 		select {
 		case queue <- element:
@@ -55,10 +53,8 @@ func (device *Device) addToInboundQueue(
 	}
 }
 
-func (device *Device) addToDecryptionQueue(
-	queue chan *QueueInboundElement,
-	element *QueueInboundElement,
-) {
+func (device *Device) addToDecryptionQueue(element *QueueInboundElement) {
+	queue := device.queue.decryption
 	for {
 		select {
 		case queue <- element:
@@ -75,10 +71,8 @@ func (device *Device) addToDecryptionQueue(
 	}
 }
 
-func (device *Device) addToHandshakeQueue(
-	queue chan QueueHandshakeElement,
-	element QueueHandshakeElement,
-) {
+func (device *Device) addToHandshakeQueue(element QueueHandshakeElement) {
+	queue := device.queue.handshake
 	for {
 		select {
 		case queue <- element:
@@ -183,8 +177,8 @@ func (device *Device) RoutineReceiveIncoming(IP int, bind Bind) {
 
 			// add to decryption queues
 
-			device.addToDecryptionQueue(device.queue.decryption, elem)
-			device.addToInboundQueue(peer.queue.inbound, elem)
+			device.addToDecryptionQueue(elem)
+			peer.addToInboundQueue(elem)
 			buffer = device.GetMessageBuffer()
 
 			continue
@@ -203,7 +197,6 @@ func (device *Device) RoutineReceiveIncoming(IP int, bind Bind) {
 
 		if okay {
 			device.addToHandshakeQueue(
-				device.queue.handshake,
 				QueueHandshakeElement{
 					msgType:  msgType,
 					buffer:   buffer,
diff --git a/src/send.go b/src/send.go
index 163b75f..7f1f25c 100644
--- a/src/send.go
+++ b/src/send.go
@@ -88,10 +88,8 @@ func addToOutboundQueue(
 	}
 }
 
-func addToEncryptionQueue(
-	queue chan *QueueOutboundElement,
-	element *QueueOutboundElement,
-) {
+func (device *Device) addToEncryptionQueue(element *QueueOutboundElement) {
+	queue := device.queue.encryption
 	for {
 		select {
 		case queue <- element:
@@ -231,7 +229,7 @@ func (peer *Peer) RoutineNonce() {
 
 			// add to parallel and sequential queue
 
-			addToEncryptionQueue(device.queue.encryption, elem)
+			device.addToEncryptionQueue(elem)
 			addToOutboundQueue(peer.queue.outbound, elem)
 		}
 	}
-- 
2.15.1

[PATCH 11/12] noise_protocol: mixHash(): remove unnecessary Reset()

[Simon Ruderich]

Resetting the hash is only necessary when reusing it which is not the
case as the function immediately returns.

Also prevents confusion by the reader who might expect a reason why the
hash is reset here.
---
 src/noise_protocol.go | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/noise_protocol.go b/src/noise_protocol.go
index 2bf38fe..c19f836 100644
--- a/src/noise_protocol.go
+++ b/src/noise_protocol.go
@@ -118,7 +118,6 @@ func mixHash(dst *[blake2s.Size]byte, h *[blake2s.Size]byte, data []byte) {
 	hsh.Write(h[:])
 	hsh.Write(data)
 	hsh.Sum(dst[:0])
-	hsh.Reset()
 }
 
 func (h *Handshake) mixHash(data []byte) {
-- 
2.15.1

[PATCH 12/12] timers: log error if handshake sending fails

[Simon Ruderich]

---
 src/timers.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/timers.go b/src/timers.go
index f2fed30..8e8cf22 100644
--- a/src/timers.go
+++ b/src/timers.go
@@ -296,7 +296,7 @@ func (peer *Peer) RoutineTimerHandler(ready *sync.WaitGroup) {
 			err := peer.sendNewHandshake()
 			if err != nil {
 				logInfo.Println(
-					"Failed to send handshake to peer:", peer.String())
+					"Failed to send handshake to peer:", peer.String(), err)
 			}
 
 		case <-peer.timer.handshakeDeadline.Wait():
@@ -322,7 +322,7 @@ func (peer *Peer) RoutineTimerHandler(ready *sync.WaitGroup) {
 			err := peer.sendNewHandshake()
 			if err != nil {
 				logInfo.Println(
-					"Failed to send handshake to peer:", peer.String())
+					"Failed to send handshake to peer:", peer.String(), err)
 			}
 
 			peer.timer.handshakeDeadline.Reset(RekeyAttemptTime)
-- 
2.15.1