Misc patches

PATCH New Simon Ruderich 1 participant 13 messages Jan 1, 2018

lore

Share & tools

lore

Simon Ruderich

author

2018-01-01 10:51 UTC #0 lore raw

cover Misc patches

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(-)

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #1 lore raw

PATCH 1/12 Fix typos in comments

---
 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(-)

src/conn_linux.go +3 -3

index cdba74f..63ad2ec 100644
@@ -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]))

src/index.go +1 -1

index 1ba040e..13e8693 100644
@@ -6,7 +6,7 @@ import (
 	"sync"
 )
 
-/* Index=0 is reserved for unset indecies
+/* Index=0 is reserved for unset indices
  *
  */

src/keypair.go +1 -1

index 7e5297b..92c03c7 100644
@@ -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.

src/main.go +1 -1

index b12bb09..8d1b364 100644
@@ -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)

src/noise_protocol.go +5 -5

index 2f9e1d5..2bf38fe 100644
@@ -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

src/peer.go +1 -1

index 7c6ad47..228fe73 100644
@@ -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

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #2 lore raw

PATCH 2/12 tun_linux: use getDummySock()

In reply to Simon Ruderich

Using this helper reduces code duplication.

---
 src/tun_linux.go | 23 ++---------------------
 1 file changed, 2 insertions(+), 21 deletions(-)

src/tun_linux.go +2 ~~-21~~

index daa2462..b88426b 100644
@@ -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

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #3 lore raw

PATCH 3/12 receive, send: use AtomicBool for dropped in QueueInboundElement, QueueOutboundElement

In reply to 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(-)

src/receive.go +3 -4

index dbd2813..2c4f191 100644
@@ -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()

src/send.go +4 -5

index 9537f5e..163b75f 100644
@@ -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

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #4 lore raw

PATCH 4/12 device: move removePeerUnsafe() into Device

In reply to 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(-)

src/device.go +5 -5

index f4a087c..537af67 100644
@@ -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)
 	}
 }

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #5 lore raw

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

In reply to 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(-)

src/index.go +3 -9

index 13e8693..bf8f0a0 100644
@@ -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,

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #6 lore raw

PATCH 6/12 device: use UnderLoadAfterTime constant

In reply to Simon Ruderich

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

src/device.go +1 -1

index 537af67..85d65b2 100644
@@ -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
 	}

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #7 lore raw

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

In reply to Simon Ruderich

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

src/peer.go +2 -0

index 228fe73..244bae0 100644
@@ -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

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #8 lore raw

PATCH 8/12 conn_linux: move comment to make its meaning more obvious

In reply to 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(-)

src/conn_linux.go +1 -2

index 63ad2ec..571c882 100644
@@ -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
 	}

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #9 lore raw

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

In reply to 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(-)

src/ratelimiter.go +2 -8

index 6e5f005..5b30926 100644
@@ -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
 }

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #10 lore raw

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

In reply to 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(-)

src/receive.go +8 ~~-15~~

index 2c4f191..f5cbe74 100644
@@ -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,

src/send.go +3 -5

index 163b75f..7f1f25c 100644
@@ -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)
 		}
 	}

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #11 lore raw

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

In reply to 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(-)

src/noise_protocol.go +0 -1

index 2bf38fe..c19f836 100644
@@ -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) {

Signature · 1 line

2.15.1

Simon Ruderich

author

2018-01-01 10:52 UTC #12 lore raw

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

In reply to Simon Ruderich

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

src/timers.go +2 -2

index f2fed30..8e8cf22 100644
@@ -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)

Signature · 1 line

2.15.1