[PATCH 0/7] TUN fixes

[PATCH 0/7] TUN fixes

[Simon Ruderich]

Hello,

While looking through the latest commits I noticed that 996c7c4
("Removed IFF_NO_PI from TUN linux", 2017-12-04) broke the test
suite and darwin and windows builds by changing the TUNDevice
interface. This patch series adds documentation and tries to fix
the tests and the darwin/windows tun (however that part is
untested as I don't have access to these systems and it looks
like the build is already broken for a while).

Note that d73f960 ("Peer timer teardown", 2017-12-29) broke the
tests in a separate way so even with these patches the test suite
will not pass.

Regards
Simon

Simon Ruderich (7):
  tun: TUNDevice: document behavior of offset parameter
  tun_linux: add PIHeaderSize constant instead of magic value
  tun_linux: document packet information header values
  helper_test: reorder DummyTUN functions to follow interface order
  helper_test: adapt to TUNDevice interface change
  tun_darwin: adapt to TUNDevice interface change
  tun_darwin: adapt to TUNDevice interface change

 src/helper_test.go | 26 +++++++++++++-------------
 src/tun.go         |  4 ++--
 src/tun_darwin.go  | 13 ++++++++++---
 src/tun_linux.go   | 11 +++++++----
 src/tun_windows.go |  8 ++++++--
 5 files changed, 38 insertions(+), 24 deletions(-)

-- 
2.15.1

[PATCH 1/7] tun: TUNDevice: document behavior of offset parameter

[Simon Ruderich]

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

diff --git a/src/tun.go b/src/tun.go
index 024f0f0..cb50cd0 100644
--- a/src/tun.go
+++ b/src/tun.go
@@ -17,8 +17,8 @@ const (
 
 type TUNDevice interface {
 	File() *os.File                 // returns the file descriptor of the device
-	Read([]byte, int) (int, error)  // read a packet from the device (without any additional headers)
-	Write([]byte, int) (int, error) // writes a packet to the device (without any additional headers)
+	Read([]byte, int) (int, error)  // read a packet from the device (without any additional headers); packet starts at the given offset (however data preceding offset may be overwritten!)
+	Write([]byte, int) (int, error) // writes a packet to the device (without any additional headers); packet starts at the given offset (however data preceding offset may be overwritten!)
 	MTU() (int, error)              // returns the MTU of the device
 	Name() string                   // returns the current name
 	Events() chan TUNEvent          // returns a constant channel of events related to the device
-- 
2.15.1

[PATCH 2/7] tun_linux: add PIHeaderSize constant instead of magic value

[Simon Ruderich]

---
 src/tun_linux.go | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/tun_linux.go b/src/tun_linux.go
index daa2462..7fcbe00 100644
--- a/src/tun_linux.go
+++ b/src/tun_linux.go
@@ -48,6 +48,7 @@ import "C"
 const (
 	CloneDevicePath = "/dev/net/tun"
 	IFReqSize       = unix.IFNAMSIZ + 64
+	PIHeaderSize    = 4 // packet information of each frame (if IFF_NO_PI unset)
 )
 
 type NativeTun struct {
@@ -254,7 +255,7 @@ func (tun *NativeTun) Write(buff []byte, offset int) (int, error) {
 
 	// reserve space for header
 
-	buff = buff[offset-4:]
+	buff = buff[offset-PIHeaderSize:]
 
 	// add packet information header
 
@@ -279,12 +280,12 @@ func (tun *NativeTun) Read(buff []byte, offset int) (int, error) {
 	case err := <-tun.errors:
 		return 0, err
 	default:
-		buff := buff[offset-4:]
+		buff := buff[offset-PIHeaderSize:]
 		n, err := tun.fd.Read(buff[:])
-		if n < 4 {
+		if n < PIHeaderSize {
 			return 0, err
 		}
-		return n - 4, err
+		return n - PIHeaderSize, err
 	}
 }
 
-- 
2.15.1

[PATCH 3/7] tun_linux: document packet information header values

[Simon Ruderich]

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

diff --git a/src/tun_linux.go b/src/tun_linux.go
index 7fcbe00..dfa74a6 100644
--- a/src/tun_linux.go
+++ b/src/tun_linux.go
@@ -258,6 +258,8 @@ func (tun *NativeTun) Write(buff []byte, offset int) (int, error) {
 	buff = buff[offset-PIHeaderSize:]
 
 	// add packet information header
+	// bytes 0-1: flags
+	// bytes 2-3: protocol
 
 	buff[0] = 0x00
 	buff[1] = 0x00
-- 
2.15.1

[PATCH 4/7] helper_test: reorder DummyTUN functions to follow interface order

[Simon Ruderich]

No code change. Makes it easier to compare the implementation with the
interface.

---
 src/helper_test.go | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/helper_test.go b/src/helper_test.go
index 8548121..46bb782 100644
--- a/src/helper_test.go
+++ b/src/helper_test.go
@@ -20,12 +20,10 @@ func (tun *DummyTUN) File() *os.File {
 	return nil
 }
 
-func (tun *DummyTUN) Name() string {
-	return tun.name
-}
-
-func (tun *DummyTUN) MTU() (int, error) {
-	return tun.mtu, nil
+func (tun *DummyTUN) Read(d []byte) (int, error) {
+	t := <-tun.packets
+	copy(d, t)
+	return len(t), nil
 }
 
 func (tun *DummyTUN) Write(d []byte) (int, error) {
@@ -33,18 +31,20 @@ func (tun *DummyTUN) Write(d []byte) (int, error) {
 	return len(d), nil
 }
 
-func (tun *DummyTUN) Close() error {
-	return nil
+func (tun *DummyTUN) MTU() (int, error) {
+	return tun.mtu, nil
+}
+
+func (tun *DummyTUN) Name() string {
+	return tun.name
 }
 
 func (tun *DummyTUN) Events() chan TUNEvent {
 	return tun.events
 }
 
-func (tun *DummyTUN) Read(d []byte) (int, error) {
-	t := <-tun.packets
-	copy(d, t)
-	return len(t), nil
+func (tun *DummyTUN) Close() error {
+	return nil
 }
 
 func CreateDummyTUN(name string) (TUNDevice, error) {
-- 
2.15.1

[PATCH 5/7] helper_test: adapt to TUNDevice interface change

[Simon Ruderich]

Broken in 996c7c4 ("Removed IFF_NO_PI from TUN linux", 2017-12-04).

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

diff --git a/src/helper_test.go b/src/helper_test.go
index 46bb782..99eb5d6 100644
--- a/src/helper_test.go
+++ b/src/helper_test.go
@@ -20,14 +20,14 @@ func (tun *DummyTUN) File() *os.File {
 	return nil
 }
 
-func (tun *DummyTUN) Read(d []byte) (int, error) {
+func (tun *DummyTUN) Read(d []byte, offset int) (int, error) {
 	t := <-tun.packets
-	copy(d, t)
+	copy(d[offset:], t)
 	return len(t), nil
 }
 
-func (tun *DummyTUN) Write(d []byte) (int, error) {
-	tun.packets <- d
+func (tun *DummyTUN) Write(d []byte, offset int) (int, error) {
+	tun.packets <- d[offset:]
 	return len(d), nil
 }
 
-- 
2.15.1

[PATCH 6/7] tun_darwin: adapt to TUNDevice interface change

[Simon Ruderich]

Broken in 996c7c4 ("Removed IFF_NO_PI from TUN linux", 2017-12-04).

Untested!
---
 src/tun_darwin.go | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/tun_darwin.go b/src/tun_darwin.go
index 87f6af6..a984e94 100644
--- a/src/tun_darwin.go
+++ b/src/tun_darwin.go
@@ -184,10 +184,14 @@ func (t *NativeTUN) Events() chan TUNEvent {
 	return t.events
 }
 
-func (t *NativeTUN) Read(to []byte) (int, error) {
+func (t *NativeTUN) Read(to []byte, offset int) (int, error) {
+	to = to[offset:]
+
 	t.rMu.Lock()
 	defer t.rMu.Unlock()
 
+	// TODO: implement in-place read
+
 	if cap(t.rBuf) < len(to)+4 {
 		t.rBuf = make([]byte, len(to)+4)
 	}
@@ -198,15 +202,18 @@ func (t *NativeTUN) Read(to []byte) (int, error) {
 	return n - 4, err
 }
 
-func (t *NativeTUN) Write(from []byte) (int, error) {
+func (t *NativeTUN) Write(from []byte, offset int) (int, error) {
 
-	if len(from) == 0 {
+	if len(from) < offset {
 		return 0, unix.EIO
 	}
+	from := from[offset:]
 
 	t.wMu.Lock()
 	defer t.wMu.Unlock()
 
+	// TODO: implement in-place write
+
 	if cap(t.wBuf) < len(from)+4 {
 		t.wBuf = make([]byte, len(from)+4)
 	}
-- 
2.15.1

[PATCH 7/7] tun_darwin: adapt to TUNDevice interface change

[Simon Ruderich]

Broken in 996c7c4 ("Removed IFF_NO_PI from TUN linux", 2017-12-04).

Untested!
---
 src/tun_windows.go | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/tun_windows.go b/src/tun_windows.go
index 0711032..8bb8a66 100644
--- a/src/tun_windows.go
+++ b/src/tun_windows.go
@@ -123,7 +123,9 @@ func (f *NativeTUN) Close() error {
 	return windows.Close(f.fd)
 }
 
-func (f *NativeTUN) Write(b []byte) (int, error) {
+func (f *NativeTUN) Write(b []byte, offset int) (int, error) {
+	b := b[offset:]
+
 	f.wl.Lock()
 	defer f.wl.Unlock()
 
@@ -138,7 +140,9 @@ func (f *NativeTUN) Write(b []byte) (int, error) {
 	return getOverlappedResult(f.fd, f.wo)
 }
 
-func (f *NativeTUN) Read(b []byte) (int, error) {
+func (f *NativeTUN) Read(b []byte, offset int) (int, error) {
+	b := b[offset:]
+
 	f.rl.Lock()
 	defer f.rl.Unlock()
 
-- 
2.15.1

Re: [PATCH 7/7] tun_darwin: adapt to TUNDevice interface change

[Mathias Hall-Andersen]

Hi Simon

Thanks for your patches!

I am still fixing the interface up / down semantics (stopping / starting
peer timers and routines not needed when the interface is down).
I hope to fix this and merge your linux code in the next couple of days.
This should bring us very close to a usable linux client.

I will look at your OSX code when I have access to an OSX machine.

On Windows, we preferably want to move away from the OpenVPN driver
horrer show altogether.

Once again, thanks for the patch set.

Best Regards & A happy new year
Mathias Hall-Andersen

Re: [PATCH 7/7] tun_darwin: adapt to TUNDevice interface change

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 1618 bytes --]

On Sun, Dec 31, 2017 at 08:17:58PM +0100, Mathias Hall-Andersen wrote:
> Hi Simon
>
> Thanks for your patches!

Hi Mathias,

Thanks for your quick response.

> I am still fixing the interface up / down semantics (stopping / starting
> peer timers and routines not needed when the interface is down).
> I hope to fix this and merge your linux code in the next couple of days.
> This should bring us very close to a usable linux client.

Awesome!

I just sent out a second set of patches with stuff I noticed
while reading the code (I only stumbled over the TUN code when I
tried to run the test suite).

While I'm at I have a few questions about the code (zx2c4 said I
should ask here):

conn_linux.go: DstToBytes() uses end.src, however DstIP() uses
end.dst. Is this a typo?

device.go: removePeerUnsafe() doesn't unlock the peer mutex. Is
this intended? If so a comment would be nice.

ratelimiter.go: RoutineGarbageCollector() uses time.Second as
magic value, maybe use RatelimiterGarbageCollectTime or a new
constant instead?

receive.go: RoutineHandshake() returns on some errors (e.g.
"Failed to decode cookie reply" or mac failure), shouldn't it
continue instead?

timers.go: TimerEphemeralKeyCreated() uses 3 as magic value
(multiplied by RejectAfterTime), why 3?

daemon_darwin.go, daemon_windows.go: the Daemonize() function
uses a different signature so the build will fail on those
systems (but I think there are more compile errors on non linux).

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 7/7] tun_darwin: adapt to TUNDevice interface change

[Simon Ruderich]

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

On Sun, Dec 31, 2017 at 08:17:58PM +0100, Mathias Hall-Andersen wrote:
> I am still fixing the interface up / down semantics (stopping / starting
> peer timers and routines not needed when the interface is down).

Regarding this, with d73f960 ("Peer timer teardown", 2017-12-29)
I can't configure wireguard-go via wg anymore. wg just hangs on
any command, everything works fine with 996c7c4 ("Removed
IFF_NO_PI from TUN linux", 2017-12-04).

Any idea what's wrong? Not sure if this is related to the test
suite failure which also occurs since d73f960 (only visible with
my TUN patches or the test suite fails to build).

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]