Browse Source

device: make Peer fields safe for atomic access on 32-bit.

All atomic access must be aligned to 64 bits, even on 32-bit
platforms. Go promises that the start of allocated structs is
aligned to 64 bits. So, place the atomically-accessed things
first in the struct so that they benefit from that alignment.

As a side bonus, it cleanly separates fields that are accessed
by atomic ops, and those that should be accessed under mu.

Also adds a test that will fail consistently on 32-bit platforms
if the struct ever changes again to violate the rules. This is
likely not needed because unaligned access crashes reliably,
but this will reliably fail even if tests accidentally pass due
to lucky alignment.

Signed-Off-By: David Anderson <danderson@tailscale.com>
David Anderson 3 months ago
parent
commit
d49f4e9fe3
2 changed files with 45 additions and 9 deletions
  1. 16
    9
      device/peer.go
  2. 29
    0
      device/peer_test.go

+ 16
- 9
device/peer.go View File

@@ -19,20 +19,27 @@ const (
19 19
 )
20 20
 
21 21
 type Peer struct {
22
-	isRunning                   AtomicBool
23
-	sync.RWMutex                // Mostly protects endpoint, but is generally taken whenever we modify peer
24
-	keypairs                    Keypairs
25
-	handshake                   Handshake
26
-	device                      *Device
27
-	endpoint                    Endpoint
28
-	persistentKeepaliveInterval uint16
29
-
30
-	// This must be 64-bit aligned, so make sure the above members come out to even alignment and pad accordingly
22
+	// These fields are accessed with atomic operations, which must be
23
+	// 64-bit aligned even on 32-bit platforms. Go guarantees that an
24
+	// allocated struct will be 64-bit aligned. So we place
25
+	// atomically-accessed fields up front, so that they can share in
26
+	// this alignment before smaller fields throw it off.
31 27
 	stats struct {
32 28
 		txBytes           uint64 // bytes send to peer (endpoint)
33 29
 		rxBytes           uint64 // bytes received from peer
34 30
 		lastHandshakeNano int64  // nano seconds since epoch
35 31
 	}
32
+	// This field is only 32 bits wide, but is still aligned to 64
33
+	// bits. Don't place other atomic fields after this one.
34
+	isRunning AtomicBool
35
+
36
+	// Mostly protects endpoint, but is generally taken whenever we modify peer
37
+	sync.RWMutex
38
+	keypairs                    Keypairs
39
+	handshake                   Handshake
40
+	device                      *Device
41
+	endpoint                    Endpoint
42
+	persistentKeepaliveInterval uint16
36 43
 
37 44
 	timers struct {
38 45
 		retransmitHandshake     *Timer

+ 29
- 0
device/peer_test.go View File

@@ -0,0 +1,29 @@
1
+/* SPDX-License-Identifier: MIT
2
+ *
3
+ * Copyright (C) 2017-2019 WireGuard LLC. All Rights Reserved.
4
+ */
5
+
6
+package device
7
+
8
+import (
9
+	"testing"
10
+	"unsafe"
11
+)
12
+
13
+func checkAlignment(t *testing.T, name string, offset uintptr) {
14
+	t.Helper()
15
+	if offset%8 != 0 {
16
+		t.Errorf("offset of %q within struct is %d bytes, which does not align to 64-bit word boundaries (missing %d bytes). Atomic operations will crash on 32-bit systems.", name, offset, 8-(offset%8))
17
+	}
18
+}
19
+
20
+// TestPeerAlignment checks that atomically-accessed fields are
21
+// aligned to 64-bit boundaries, as required by the atomic package.
22
+//
23
+// Unfortunately, violating this rule on 32-bit platforms results in a
24
+// hard segfault at runtime.
25
+func TestPeerAlignment(t *testing.T) {
26
+	var p Peer
27
+	checkAlignment(t, "Peer.stats", unsafe.Offsetof(p.stats))
28
+	checkAlignment(t, "Peer.isRunning", unsafe.Offsetof(p.isRunning))
29
+}

Loading…
Cancel
Save