TUN fixes

PATCH In review Simon Ruderich 2 participants 11 messages Dec 31, 2017 – Jan 1, 2018

lore

Share & tools

lore

Simon Ruderich

author

2017-12-31 16:16 UTC #0 lore raw

cover TUN fixes

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

Signature · 1 line

2.15.1

Simon Ruderich

author

2017-12-31 16:16 UTC #1 lore raw

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

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

src/tun.go +2 -2

index 024f0f0..cb50cd0 100644
@@ -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

Signature · 1 line

2.15.1

Simon Ruderich

author

2017-12-31 16:16 UTC #2 lore raw

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

In reply to Simon Ruderich

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

src/tun_linux.go +5 -4

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

Signature · 1 line

2.15.1

Simon Ruderich

author

2017-12-31 16:16 UTC #3 lore raw

PATCH 3/7 tun_linux: document packet information header values

In reply to Simon Ruderich

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

src/tun_linux.go +2 -0

index 7fcbe00..dfa74a6 100644
@@ -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

Signature · 1 line

2.15.1

Simon Ruderich

author

2017-12-31 16:16 UTC #4 lore raw

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

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

src/helper_test.go +12 ~~-12~~

index 8548121..46bb782 100644
@@ -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) {

Signature · 1 line

2.15.1

Simon Ruderich

author

2017-12-31 16:16 UTC #5 lore raw

PATCH 5/7 helper_test: adapt to TUNDevice interface change

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

src/helper_test.go +4 -4

index 46bb782..99eb5d6 100644
@@ -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
 }

Signature · 1 line

2.15.1

Simon Ruderich

author

2017-12-31 16:16 UTC #6 lore raw

PATCH 6/7 tun_darwin: adapt to TUNDevice interface change

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

src/tun_darwin.go +10 -3

index 87f6af6..a984e94 100644
@@ -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)
 	}

Signature · 1 line

2.15.1

Simon Ruderich

author

2017-12-31 16:16 UTC #7 lore raw

PATCH 7/7 tun_darwin: adapt to TUNDevice interface change

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

src/tun_windows.go +6 -2

index 0711032..8bb8a66 100644
@@ -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()

Signature · 1 line

2.15.1

Mathias Hall-Andersen

2017-12-31 19:16 UTC #8 lore raw

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

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

Simon Ruderich

author

2018-01-01 11:13 UTC #9 lore raw

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

Quoted 3 lines · Mathias Hall-Andersen

Hi Simon
Thanks for your patches!

Hi Mathias,

Thanks for your quick response.

Quoted 4 lines

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

Signature · 3 lines

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

Simon Ruderich

author

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

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

In reply to Mathias Hall-Andersen

Quoted 2 lines · Mathias Hall-Andersen

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

Signature · 3 lines

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