logpolicy: consider /var/lib/tailscale when no STATE_DIRECTORY

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
(cherry picked from commit 5a0c37aafd)

control/controlclient/direct.go

@@ -596,17 +596,19 @@ func (c *Direct) PollNetMap(ctx context.Context, maxPolls int, cb func(*NetworkM
 		}
 		if resp.KeepAlive {
 			vlogf("netmap: got keep-alive")
-			select {
-			case timeoutReset <- struct{}{}:
-				vlogf("netmap: sent keep-alive timer reset")
-			case <-ctx.Done():
-				c.logf("netmap: not resetting timer for keep-alive due to: %v", ctx.Err())
-				return ctx.Err()
-			}
+		} else {
+			vlogf("netmap: got new map")
+		}
+		select {
+		case timeoutReset <- struct{}{}:
+			vlogf("netmap: sent timer reset")
+		case <-ctx.Done():
+			c.logf("netmap: not resetting timer; context done: %v", ctx.Err())
+			return ctx.Err()
+		}
+		if resp.KeepAlive {
 			continue
 		}
-		vlogf("netmap: got new map")
-
 		if resp.DERPMap != nil {
 			vlogf("netmap: new map contains DERP map")
 			lastDERPMap = resp.DERPMap

control/controlclient/netmap.go

@@ -7,7 +7,6 @@ package controlclient
 import (
 	"encoding/json"
 	"fmt"
-	"log"
 	"net"
 	"reflect"
 	"strconv"
@@ -132,12 +131,18 @@ func printPeerConcise(buf *strings.Builder, p *tailcfg.Node) {
 	if strings.HasPrefix(derp, derpPrefix) {
 		derp = "D" + derp[len(derpPrefix):]
 	}
+	var discoShort string
+	if !p.DiscoKey.IsZero() {
+		discoShort = p.DiscoKey.ShortString() + " "
+	}
 
 	// Most of the time, aip is just one element, so format the
 	// table to look good in that case. This will also make multi-
 	// subnet nodes stand out visually.
-	fmt.Fprintf(buf, " %v %-2v %-15v : %v\n",
-		p.Key.ShortString(), derp,
+	fmt.Fprintf(buf, " %v %s%-2v %-15v : %v\n",
+		p.Key.ShortString(),
+		discoShort,
+		derp,
 		strings.Join(aip, " "),
 		strings.Join(ep, " "))
 }
@@ -146,6 +151,7 @@ func printPeerConcise(buf *strings.Builder, p *tailcfg.Node) {
 func nodeConciseEqual(a, b *tailcfg.Node) bool {
 	return a.Key == b.Key &&
 		a.DERP == b.DERP &&
+		a.DiscoKey == b.DiscoKey &&
 		eqCIDRsIgnoreNil(a.AllowedIPs, b.AllowedIPs) &&
 		eqStringsIgnoreNil(a.Endpoints, b.Endpoints)
 }
@@ -216,20 +222,6 @@ const (
 	HackDefaultRoute
 )
 
-// TODO(bradfitz): UAPI seems to only be used by the old confnode and
-// pingnode; delete this when those are deleted/rewritten?
-func (nm *NetworkMap) UAPI(flags WGConfigFlags) string {
-	wgcfg, err := nm.WGCfg(log.Printf, flags)
-	if err != nil {
-		log.Fatalf("WGCfg() failed unexpectedly: %v", err)
-	}
-	s, err := wgcfg.ToUAPI()
-	if err != nil {
-		log.Fatalf("ToUAPI() failed unexpectedly: %v", err)
-	}
-	return s
-}
-
 // EndpointDiscoSuffix is appended to the hex representation of a peer's discovery key
 // and is then the sole wireguard endpoint for peers with a non-zero discovery key.
 // This form is then recognize by magicsock's CreateEndpoint.

control/controlclient/netmap_test.go

@@ -5,8 +5,10 @@
 package controlclient
 
 import (
+	"encoding/hex"
 	"testing"
 
+	"github.com/tailscale/wireguard-go/wgcfg"
 	"tailscale.com/tailcfg"
 )
 
@@ -17,6 +19,15 @@ func testNodeKey(b byte) (ret tailcfg.NodeKey) {
 	return
 }
 
+func testDiscoKey(hexPrefix string) (ret tailcfg.DiscoKey) {
+	b, err := hex.DecodeString(hexPrefix)
+	if err != nil {
+		panic(err)
+	}
+	copy(ret[:], b)
+	return
+}
+
 func TestNetworkMapConcise(t *testing.T) {
 	for _, tt := range []struct {
 		name string
@@ -202,6 +213,62 @@ func TestConciseDiffFrom(t *testing.T) {
 			},
 			want: "- [AQEBA] D1                 :    192.168.0.100:12     192.168.0.100:12354\n- [AwMDA] D3                 :    192.168.0.100:12     192.168.0.100:12354\n",
 		},
+		{
+			name: "peer_port_change",
+			a: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:        2,
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "1.1.1.1:1"},
+					},
+				},
+			},
+			b: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:        2,
+						Key:       testNodeKey(2),
+						DERP:      "127.3.3.40:2",
+						Endpoints: []string{"192.168.0.100:12", "1.1.1.1:2"},
+					},
+				},
+			},
+			want: "- [AgICA] D2                 :    192.168.0.100:12             1.1.1.1:1  \n+ [AgICA] D2                 :    192.168.0.100:12             1.1.1.1:2  \n",
+		},
+		{
+			name: "disco_key_only_change",
+			a: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:         2,
+						Key:        testNodeKey(2),
+						DERP:       "127.3.3.40:2",
+						Endpoints:  []string{"192.168.0.100:41641", "1.1.1.1:41641"},
+						DiscoKey:   testDiscoKey("f00f00f00f"),
+						AllowedIPs: []wgcfg.CIDR{{IP: wgcfg.IPv4(100, 102, 103, 104), Mask: 32}},
+					},
+				},
+			},
+			b: &NetworkMap{
+				NodeKey: testNodeKey(1),
+				Peers: []*tailcfg.Node{
+					{
+						ID:         2,
+						Key:        testNodeKey(2),
+						DERP:       "127.3.3.40:2",
+						Endpoints:  []string{"192.168.0.100:41641", "1.1.1.1:41641"},
+						DiscoKey:   testDiscoKey("ba4ba4ba4b"),
+						AllowedIPs: []wgcfg.CIDR{{IP: wgcfg.IPv4(100, 102, 103, 104), Mask: 32}},
+					},
+				},
+			},
+			want: "- [AgICA] d:f00f00f00f000000 D2 100.102.103.104 :   192.168.0.100:41641         1.1.1.1:41641\n+ [AgICA] d:ba4ba4ba4b000000 D2 100.102.103.104 :   192.168.0.100:41641         1.1.1.1:41641\n",
+		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			var got string

logpolicy/logpolicy.go

@@ -31,7 +31,9 @@ import (
 	"tailscale.com/logtail/filch"
 	"tailscale.com/net/netns"
 	"tailscale.com/net/tlsdial"
+	"tailscale.com/paths"
 	"tailscale.com/smallzstd"
+	"tailscale.com/types/logger"
 	"tailscale.com/version"
 )
 
@@ -54,7 +56,7 @@ type Policy struct {
 func (c *Config) ToBytes() []byte {
 	data, err := json.MarshalIndent(c, "", "\t")
 	if err != nil {
-		log.Fatalf("logpolicy.Config marshal: %v\n", err)
+		log.Fatalf("logpolicy.Config marshal: %v", err)
 	}
 	return data
 }
@@ -101,21 +103,36 @@ func (l logWriter) Write(buf []byte) (int, error) {
 
 // logsDir returns the directory to use for log configuration and
 // buffer storage.
-func logsDir() string {
+func logsDir(logf logger.Logf) string {
+	// STATE_DIRECTORY is set by systemd 240+ but we support older
+	// systems-d. For example, Ubuntu 18.04 (Bionic Beaver) is 237.
 	systemdStateDir := os.Getenv("STATE_DIRECTORY")
 	if systemdStateDir != "" {
+		logf("logpolicy: using $STATE_DIRECTORY, %q", systemdStateDir)
 		return systemdStateDir
 	}
 
+	// Default to e.g. /var/lib/tailscale or /var/db/tailscale on Unix.
+	if d := paths.DefaultTailscaledStateFile(); d != "" {
+		d = filepath.Dir(d) // directory of e.g. "/var/lib/tailscale/tailscaled.state"
+		if err := os.MkdirAll(d, 0700); err == nil {
+			logf("logpolicy: using system state directory %q", d)
+			return d
+		}
+	}
+
 	cacheDir, err := os.UserCacheDir()
 	if err == nil {
-		return filepath.Join(cacheDir, "Tailscale")
+		d := filepath.Join(cacheDir, "Tailscale")
+		logf("logpolicy: using UserCacheDir, %q", d)
+		return d
 	}
 
 	// Use the current working directory, unless we're being run by a
 	// service manager that sets it to /.
 	wd, err := os.Getwd()
 	if err == nil && wd != "/" {
+		logf("logpolicy: using current directory, %q", wd)
 		return wd
 	}
 
@@ -126,6 +143,7 @@ func logsDir() string {
 	if err != nil {
 		panic("no safe place found to store log state")
 	}
+	logf("logpolicy: using temp directory, %q", tmp)
 	return tmp
 }
 
@@ -303,7 +321,13 @@ func New(collection string) *Policy {
 	}
 	console := log.New(stderrWriter{}, "", lflags)
 
-	dir := logsDir()
+	var earlyErrBuf bytes.Buffer
+	earlyLogf := func(format string, a ...interface{}) {
+		fmt.Fprintf(&earlyErrBuf, format, a...)
+		earlyErrBuf.WriteByte('\n')
+	}
+
+	dir := logsDir(earlyLogf)
 
 	cmdName := version.CmdName()
 	tryFixLogStateLocation(dir, cmdName)
@@ -312,13 +336,13 @@ func New(collection string) *Policy {
 	var oldc *Config
 	data, err := ioutil.ReadFile(cfgPath)
 	if err != nil {
-		log.Printf("logpolicy.Read %v: %v\n", cfgPath, err)
+		earlyLogf("logpolicy.Read %v: %v", cfgPath, err)
 		oldc = &Config{}
 		oldc.Collection = collection
 	} else {
 		oldc, err = ConfigFromBytes(data)
 		if err != nil {
-			log.Printf("logpolicy.Config unmarshal: %v\n", err)
+			earlyLogf("logpolicy.Config unmarshal: %v", err)
 			oldc = &Config{}
 		}
 	}
@@ -340,7 +364,7 @@ func New(collection string) *Policy {
 	newc.PublicID = newc.PrivateID.Public()
 	if newc != *oldc {
 		if err := newc.save(cfgPath); err != nil {
-			log.Printf("logpolicy.Config.Save: %v\n", err)
+			earlyLogf("logpolicy.Config.Save: %v", err)
 		}
 	}
 
@@ -366,14 +390,17 @@ func New(collection string) *Policy {
 	log.SetFlags(0) // other logflags are set on console, not here
 	log.SetOutput(lw)
 
-	log.Printf("Program starting: v%v, Go %v: %#v\n",
+	log.Printf("Program starting: v%v, Go %v: %#v",
 		version.LONG,
 		strings.TrimPrefix(runtime.Version(), "go"),
 		os.Args)
-	log.Printf("LogID: %v\n", newc.PublicID)
+	log.Printf("LogID: %v", newc.PublicID)
 	if filchErr != nil {
 		log.Printf("filch failed: %v", filchErr)
 	}
+	if earlyErrBuf.Len() != 0 {
+		log.Printf("%s", earlyErrBuf.Bytes())
+	}
 
 	return &Policy{
 		Logtail:  lw,
@@ -392,7 +419,7 @@ func (p *Policy) Close() {
 // log upload if it can be done before ctx is canceled.
 func (p *Policy) Shutdown(ctx context.Context) error {
 	if p.Logtail != nil {
-		log.Printf("flushing log.\n")
+		log.Printf("flushing log.")
 		return p.Logtail.Shutdown(ctx)
 	}
 	return nil

tailcfg/tailcfg.go

@@ -613,6 +613,7 @@ func (n *Node) Equal(n2 *Node) bool {
 		eqCIDRs(n.Addresses, n2.Addresses) &&
 		eqCIDRs(n.AllowedIPs, n2.AllowedIPs) &&
 		eqStrings(n.Endpoints, n2.Endpoints) &&
+		n.DERP == n2.DERP &&
 		n.Hostinfo.Equal(&n2.Hostinfo) &&
 		n.Created.Equal(n2.Created) &&
 		eqTimePtr(n.LastSeen, n2.LastSeen) &&

tailcfg/tailcfg_test.go

@@ -315,6 +315,11 @@ func TestNodeEqual(t *testing.T) {
 			&Node{LastSeen: &now},
 			true,
 		},
+		{
+			&Node{DERP: "foo"},
+			&Node{DERP: "bar"},
+			false,
+		},
 	}
 	for i, tt := range tests {
 		got := tt.a.Equal(tt.b)

version/mkversion.sh

@@ -5,89 +5,107 @@ set -eu
 mode=$1
 describe=$2
 
-long() {
-    ver="${describe#v}"
-    stem="${ver%%-*}"
-    case "$stem" in
-        *.*.*)
-            # Full SemVer, nothing to do.
-            semver="${stem}"
+# Git describe output overall looks like
+# MAJOR.MINOR.PATCH-NUMCOMMITS-GITHASH. Depending on the tag being
+# described and the state of the repo, ver can be missing PATCH,
+# NUMCOMMITS, or both.
+#
+# Valid values look like: 1.2.3-1234-abcdef, 0.98-1234-abcdef,
+# 1.0.0-abcdef, 0.99-abcdef.
+ver="${describe#v}"
+stem="${ver%%-*}" # Just the semver-ish bit e.g. 1.2.3, 0.98
+suffix="${ver#$stem}" # The rest e.g. -23-abcdef, -abcdef
+
+# Normalize the stem into a full major.minor.patch semver. We might
+# not use all those pieces depending on what kind of version we're
+# making, but it's good to have them all on hand.
+case "$stem" in
+    *.*.*)
+        # Full SemVer, nothing to do
+        stem="$stem"
         ;;
-        *.*)
-            # Old style major.minor, add a .0
-            semver="${stem}.0"
-            ;;
-        *)
-            echo "Unparseable version $stem" >&2
-            exit 1
-            ;;
-    esac
-    suffix="${ver#$stem}"
-    case "$suffix" in
-        -*-*)
-            # Has a change count in addition to the commit hash.
+    *.*)
+        # Old style major.minor, add a .0
+        stem="${stem}.0"
+        ;;
+    *)
+        echo "Unparseable version $stem" >&2
+        exit 1
+        ;;
+esac
+major=$(echo "$stem" | cut -f1 -d.)
+minor=$(echo "$stem" | cut -f2 -d.)
+patch=$(echo "$stem" | cut -f3 -d.)
+
+# Extract the change count and git ID from the suffix.
+case "$suffix" in
+    -*-*)
+        # Has both a change count and a commit hash.
+        changecount=$(echo "$suffix" | cut -f2 -d-)
+        githash=$(echo "$suffix" | cut -f3 -d-)
+        ;;
+    -*)
+        # Git hash only, change count is zero.
+        changecount="0"
+        githash=$(echo "$suffix" | cut -f2 -d-)
+        ;;
+    *)
+        echo "Unparseable version suffix $suffix" >&2
+        exit 1
+        ;;
+esac
+
+# Validate that the version data makes sense. Rules:
+#  - Odd number minors are unstable. Patch must be 0, and gets
+#    replaced by changecount.
+#  - Even number minors are stable. Changecount must be 0, and
+#    gets removed.
+#
+# After this section, we only use major/minor/patch, which have been
+# tweaked as needed.
+if expr "$minor" : "[0-9]*[13579]$" >/dev/null; then
+    # Unstable
+    if [ "$patch" != "0" ]; then
+        # This is a fatal error, because a non-zero patch number
+        # indicates that we created an unstable git tag in violation
+        # of our versioning policy, and we want to blow up loudly to
+        # get that fixed.
+        echo "Unstable release $describe has a non-zero patch number, which is not allowed" >&2
+        exit 1
+    fi
+    patch="$changecount"
+else
+    # Stable
+    if [ "$changecount" != "0" ]; then
+        # This is a commit that's sitting between two stable
+        # releases. We never want to release such a commit to the
+        # pbulic, but it's useful to be able to build it for
+        # debugging. Just force the version to 0.0.0, so that we're
+        # forced to rely on the git commit hash.
+        major=0
+        minor=0
+        patch=0
+    fi
+fi
+
+case "$1" in
+    long)
+        echo "${major}.${minor}.${patch}-${githash}"
+        ;;
+    short)
+        echo "${major}.${minor}.${patch}"
+        ;;
+    xcode)
+        # CFBundleShortVersionString: the "short name" used in the App
+        # Store.  eg. 0.92.98
+        echo "VERSION_NAME = ${major}.${minor}.${patch}"
+        # CFBundleVersion: the build number. Needs to be 3 numeric
+        # sections that increment for each release according to SemVer
+        # rules.
+        #
+        # We start counting at 100 because we submitted using raw
+        # build numbers before, and Apple doesn't let you start over.
+        # e.g. 0.98.3 -> 100.98.3
+        echo "VERSION_ID = $((major + 100)).${minor}.${patch}"        
         ;;
-        -*)
-            # Missing change count, add one.
-            suffix="-0${suffix}"
-            ;;
-        *)
-            echo "Unexpected version suffix" >&2
-            exit 1
-    esac
-    echo "${semver}${suffix}"
-}
-
-short() {
-    ver="$(long)"
-    case "$ver" in
-        *-*-*)
-            echo "${ver%-*}"
-            ;;
-        *-*)
-            echo "$ver"
-            ;;
-        *)
-            echo "Long version in invalid format" >&2
-            exit 1
-            ;;
-    esac
-}
-
-xcode() {
-    ver=$(short | sed -e 's/-/./')
-    major=$(echo "$ver" | cut -f1 -d.)
-    minor=$(echo "$ver" | cut -f2 -d.)
-    patch=$(echo "$ver" | cut -f3 -d.)
-    changecount=$(echo "$ver" | cut -f4 -d.)
-
-    # Apple version numbers must be major.minor.patch. We have 4 fields
-    # because we need major.minor.patch for go module compatibility, and
-    # changecount for automatic version numbering of unstable builds. To
-    # resolve this, for Apple builds, we combine changecount into patch:
-    patch=$((patch*10000 + changecount))
-
-    # CFBundleShortVersionString: the "short name" used in the App Store.
-    # eg. 0.92.98
-    echo "VERSION_NAME = $major.$minor.$patch"
-    # CFBundleVersion: the build number. Needs to be 3 numeric sections
-    # that increment for each release according to SemVer rules.
-    #
-    # We start counting at 100 because we submitted using raw build
-    # numbers before, and Apple doesn't let you start over.
-    # e.g. 0.98.3-123 -> 100.98.3123
-    major=$((major + 100))
-    echo "VERSION_ID = $major.$minor.$patch"
-}
-
-case "$mode" in
-    long)
-        long
-    ;;
-    short)
-        short
-    ;;
-    xcode)
-        xcode
-    ;;
 esac

version/mkversion_test.go

@@ -15,42 +15,60 @@ func xcode(short, long string) string {
 	return fmt.Sprintf("VERSION_NAME = %s\nVERSION_ID = %s", short, long)
 }
 
-func mkversion(t *testing.T, mode, in string) string {
+func mkversion(t *testing.T, mode, in string) (string, bool) {
 	t.Helper()
 	bs, err := exec.Command("./mkversion.sh", mode, in).CombinedOutput()
 	if err != nil {
 		t.Logf("mkversion.sh output: %s", string(bs))
-		t.Fatalf("mkversion.sh %s %s: %v", mode, in, err)
+		return "", false
 	}
-	return strings.TrimSpace(string(bs))
+	return strings.TrimSpace(string(bs)), true
 }
 
 func TestMkversion(t *testing.T) {
 	tests := []struct {
 		in    string
+		ok    bool
 		long  string
 		short string
 		xcode string
 	}{
-		{"v0.98-abcdef", "0.98.0-0-abcdef", "0.98.0-0", xcode("0.98.0", "100.98.0")},
-		{"v0.98-123-abcdef", "0.98.0-123-abcdef", "0.98.0-123", xcode("0.98.123", "100.98.123")},
-		{"v0.99.5-123-abcdef", "0.99.5-123-abcdef", "0.99.5-123", xcode("0.99.50123", "100.99.50123")},
-		{"v0.99.5-123-abcdef", "0.99.5-123-abcdef", "0.99.5-123", xcode("0.99.50123", "100.99.50123")},
-		{"v2.3-0-abcdef", "2.3.0-0-abcdef", "2.3.0-0", xcode("2.3.0", "102.3.0")},
-		{"1.2.3-4-abcdef", "1.2.3-4-abcdef", "1.2.3-4", xcode("1.2.30004", "101.2.30004")},
+		{"v0.98-abcdef", true, "0.98.0-abcdef", "0.98.0", xcode("0.98.0", "100.98.0")},
+		{"v0.98.1-abcdef", true, "0.98.1-abcdef", "0.98.1", xcode("0.98.1", "100.98.1")},
+		{"v1.1.0-37-abcdef", true, "1.1.37-abcdef", "1.1.37", xcode("1.1.37", "101.1.37")},
+		{"v1.2.9-abcdef", true, "1.2.9-abcdef", "1.2.9", xcode("1.2.9", "101.2.9")},
+		{"v1.2.9-0-abcdef", true, "1.2.9-abcdef", "1.2.9", xcode("1.2.9", "101.2.9")},
+		{"v1.15.0-129-abcdef", true, "1.15.129-abcdef", "1.15.129", xcode("1.15.129", "101.15.129")},
+
+		{"v0.98-123-abcdef", true, "0.0.0-abcdef", "0.0.0", xcode("0.0.0", "100.0.0")},
+		{"v1.0.0-37-abcdef", true, "0.0.0-abcdef", "0.0.0", xcode("0.0.0", "100.0.0")},
+
+		{"v0.99.5-0-abcdef", false, "", "", ""},   // unstable, patch not allowed
+		{"v0.99.5-123-abcdef", false, "", "", ""}, // unstable, patch not allowed
+		{"v1-abcdef", false, "", "", ""},          // bad semver
+		{"v1.0", false, "", "", ""},               // missing suffix
 	}
 
 	for _, test := range tests {
-		gotlong := mkversion(t, "long", test.in)
-		gotshort := mkversion(t, "short", test.in)
-		gotxcode := mkversion(t, "xcode", test.in)
-		if gotlong != test.long {
+		gotlong, longOK := mkversion(t, "long", test.in)
+		if longOK != test.ok {
+			t.Errorf("mkversion.sh long %q ok=%v, want %v", test.in, longOK, test.ok)
+		}
+		gotshort, shortOK := mkversion(t, "short", test.in)
+		if shortOK != test.ok {
+			t.Errorf("mkversion.sh short %q ok=%v, want %v", test.in, shortOK, test.ok)
+		}
+		gotxcode, xcodeOK := mkversion(t, "xcode", test.in)
+		if xcodeOK != test.ok {
+			t.Errorf("mkversion.sh xcode %q ok=%v, want %v", test.in, xcodeOK, test.ok)
+		}
+		if longOK && gotlong != test.long {
 			t.Errorf("mkversion.sh long %q: got %q, want %q", test.in, gotlong, test.long)
 		}
-		if gotshort != test.short {
+		if shortOK && gotshort != test.short {
 			t.Errorf("mkversion.sh short %q: got %q, want %q", test.in, gotshort, test.short)
 		}
-		if gotxcode != test.xcode {
+		if xcodeOK && gotxcode != test.xcode {
 			t.Errorf("mkversion.sh xcode %q: got %q, want %q", test.in, gotxcode, test.xcode)
 		}
 	}

wgengine/filter/filter.go

@@ -349,7 +349,10 @@ func omitDropLogging(p *packet.ParsedPacket, dir direction) bool {
 			if string(dst) == ipv6AllMLDv2CapableRouters {
 				return true
 			}
-			panic(fmt.Sprintf("Got proto=%2x; src=%x dst=%x", int(p.IPProto), src, dst))
+			// Actually, just catch all multicast.
+			if dst[0] == 0xff {
+				return true
+			}
 		}
 	}
 	return false

wgengine/filter/filter_test.go

@@ -343,6 +343,12 @@ func TestOmitDropLogging(t *testing.T) {
 			dir:  out,
 			want: true,
 		},
+		{
+			name: "v6_udp_multicast",
+			pkt:  parseHexPkt(t, "60 00 00 00 00 00 11 00  fe800000000000007dc6bc04499262a3 ff120000000000000000000000008384"),
+			dir:  out,
+			want: true,
+		},
 	}
 
 	for _, tt := range tests {

wgengine/magicsock/magicsock.go

@@ -125,19 +125,12 @@ type Conn struct {
 	packetListener nettype.PacketListener
 
 	// ============================================================
-	mu sync.Mutex // guards all following fields
-
-	// canCreateEPUnlocked tracks at one place whether mu is
-	// already held. It's then checked in CreateEndpoint to avoid
-	// double-locking mu and thus deadlocking. mu should be held
-	// while setting this; but can be read without mu held.
-	// TODO(bradfitz): delete this shameful hack; refactor the one use
-	canCreateEPUnlocked syncs.AtomicBool
+	mu     sync.Mutex // guards all following fields; see userspaceEngine lock ordering rules
+	muCond *sync.Cond
 
 	started bool // Start was called
 	closed  bool // Close was called
 
-	endpointsUpdateWaiter *sync.Cond
 	endpointsUpdateActive bool
 	wantEndpointsUpdate   string // true if non-empty; string is reason
 	lastEndpoints         []string
@@ -289,8 +282,10 @@ type Options struct {
 	// sole user just doesn't need or want it called on every
 	// packet, just every minute or two for Wireguard timeouts,
 	// and 10 seconds seems like a good trade-off between often
-	// enough and not too often.) The provided func likely calls
-	// Conn.CreateEndpoint, which acquires Conn.mu.
+	// enough and not too often.) The provided func is called while
+	// holding userspaceEngine.wgLock and likely calls
+	// Conn.CreateEndpoint, which acquires Conn.mu. As such, you should
+	// not hold
 	NoteRecvActivity func(tailcfg.DiscoKey)
 }
 
@@ -323,7 +318,7 @@ func newConn() *Conn {
 		sharedDiscoKey:  make(map[tailcfg.DiscoKey]*[32]byte),
 		discoOfAddr:     make(map[netaddr.IPPort]tailcfg.DiscoKey),
 	}
-	c.endpointsUpdateWaiter = sync.NewCond(&c.mu)
+	c.muCond = sync.NewCond(&c.mu)
 	return c
 }
 
@@ -395,7 +390,7 @@ func (c *Conn) updateEndpoints(why string) {
 			go c.updateEndpoints(why)
 		} else {
 			c.endpointsUpdateActive = false
-			c.endpointsUpdateWaiter.Broadcast()
+			c.muCond.Broadcast()
 		}
 
 	}()
@@ -1079,6 +1074,7 @@ func (c *Conn) derpWriteChanOfAddr(addr netaddr.IPPort, peer key.Public) chan<-
 		go func() {
 			dc.Connect(ctx)
 			close(c.derpStarted)
+			c.muCond.Broadcast()
 		}()
 	}
 
@@ -1353,13 +1349,15 @@ func wgRecvAddr(e conn.Endpoint, ipp netaddr.IPPort, addr *net.UDPAddr) *net.UDP
 	return ipp.UDPAddr()
 }
 
-// noteRecvActivity calls the magicsock.Conn.noteRecvActivity hook if
-// e is a discovery-capable peer.
+// noteRecvActivityFromEndpoint calls the c.noteRecvActivity hook if
+// e is a discovery-capable peer and this is the first receive activity
+// it's got in awhile (in last 10 seconds).
 //
 // This should be called whenever a packet arrives from e.
-func noteRecvActivity(e conn.Endpoint) {
-	if de, ok := e.(*discoEndpoint); ok {
-		de.onRecvActivity()
+func (c *Conn) noteRecvActivityFromEndpoint(e conn.Endpoint) {
+	de, ok := e.(*discoEndpoint)
+	if ok && c.noteRecvActivity != nil && de.isFirstRecvActivityInAwhile() {
+		c.noteRecvActivity(de.discoKey)
 	}
 }
 
@@ -1370,7 +1368,7 @@ Top:
 		c.bufferedIPv4From = netaddr.IPPort{}
 		addr = from.UDPAddr()
 		ep := c.findEndpoint(from, addr)
-		noteRecvActivity(ep)
+		c.noteRecvActivityFromEndpoint(ep)
 		return copy(b, c.bufferedIPv4Packet), ep, wgRecvAddr(ep, from, addr), nil
 	}
 
@@ -1482,7 +1480,7 @@ Top:
 		ep = c.findEndpoint(ipp, addr)
 	}
 	if !didNoteRecvActivity {
-		noteRecvActivity(ep)
+		c.noteRecvActivityFromEndpoint(ep)
 	}
 	return n, ep, wgRecvAddr(ep, ipp, addr), nil
 }
@@ -1510,7 +1508,7 @@ func (c *Conn) ReceiveIPv6(b []byte) (int, conn.Endpoint, *net.UDPAddr, error) {
 		}
 
 		ep := c.findEndpoint(ipp, addr)
-		noteRecvActivity(ep)
+		c.noteRecvActivityFromEndpoint(ep)
 		return n, ep, wgRecvAddr(ep, ipp, addr), nil
 	}
 }
@@ -1608,8 +1606,9 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool {
 		return false
 	}
 
-	de, ok := c.endpointOfDisco[sender]
-	if !ok {
+	needsRecvActivityCall := false
+	de, endpointFound0 := c.endpointOfDisco[sender]
+	if !endpointFound0 {
 		// We don't have an active endpoint for this sender but we knew about the node, so
 		// it's an idle endpoint that doesn't yet exist in the wireguard config. We now have
 		// to notify the userspace engine (via noteRecvActivity) so wireguard-go can create
@@ -1621,20 +1620,37 @@ func (c *Conn) handleDiscoMessage(msg []byte, src netaddr.IPPort) bool {
 			c.logf("magicsock: [unexpected] have node without endpoint, without c.noteRecvActivity hook")
 			return false
 		}
-		// noteRecvActivity calls back into CreateEndpoint, which we can't easily control,
-		// and CreateEndpoint expects to be called with c.mu held, but we hold it here, and
-		// it's too invasive for now to release it here and recheck invariants.  So instead,
-		// use this unfortunate hack: set canCreateEPUnlocked which CreateEndpoint then
-		// checks to conditionally acquire the mutex. I'm so sorry.
-		c.canCreateEPUnlocked.Set(true)
+		needsRecvActivityCall = true
+	} else {
+		needsRecvActivityCall = de.isFirstRecvActivityInAwhile()
+	}
+	if needsRecvActivityCall && c.noteRecvActivity != nil {
+		// We can't hold Conn.mu while calling noteRecvActivity.
+		// noteRecvActivity acquires userspaceEngine.wgLock (and per our
+		// lock ordering rules: wgLock must come first), and also calls
+		// back into our Conn.CreateEndpoint, which would double-acquire
+		// Conn.mu.
+		c.mu.Unlock()
 		c.noteRecvActivity(sender)
-		c.canCreateEPUnlocked.Set(false)
+		c.mu.Lock() // re-acquire
+
+		// Now, recheck invariants that might've changed while we'd
+		// released the lock, which isn't much:
+		if c.closed || c.privateKey.IsZero() {
+			return true
+		}
 		de, ok = c.endpointOfDisco[sender]
 		if !ok {
+			if _, ok := c.nodeOfDisco[sender]; !ok {
+				// They just disappeared while we'd released the lock.
+				return false
+			}
 			c.logf("magicsock: [unexpected] lazy endpoint not created for %v, %v", peerNode.Key.ShortString(), sender.ShortString())
 			return false
 		}
-		c.logf("magicsock: lazy endpoint created via disco message for %v, %v", peerNode.Key.ShortString(), sender.ShortString())
+		if !endpointFound0 {
+			c.logf("magicsock: lazy endpoint created via disco message for %v, %v", peerNode.Key.ShortString(), sender.ShortString())
+		}
 	}
 
 	// First, do we even know (and thus care) about this sender? If not,
@@ -1825,7 +1841,9 @@ func (c *Conn) SetPrivateKey(privateKey wgcfg.PrivateKey) error {
 	if oldKey.IsZero() {
 		c.everHadKey = true
 		c.logf("magicsock: SetPrivateKey called (init)")
-		go c.ReSTUN("set-private-key")
+		if c.started {
+			go c.ReSTUN("set-private-key")
+		}
 	} else if newKey.IsZero() {
 		c.logf("magicsock: SetPrivateKey called (zeroed)")
 		c.closeAllDerpLocked("zero-private-key")
@@ -2087,6 +2105,21 @@ func (c *Conn) Close() error {
 		c.pconn6.Close()
 	}
 	err := c.pconn4.Close()
+
+	// Wait on goroutines updating right at the end, once everything is
+	// already closed. We want everything else in the Conn to be
+	// consistently in the closed state before we release mu to wait
+	// on the endpoint updater & derphttp.Connect.
+	for c.goroutinesRunningLocked() {
+		c.muCond.Wait()
+	}
+	return err
+}
+
+func (c *Conn) goroutinesRunningLocked() bool {
+	if c.endpointsUpdateActive {
+		return true
+	}
 	// The goroutine running dc.Connect in derpWriteChanOfAddr may linger
 	// and appear to leak, as observed in https://github.com/tailscale/tailscale/issues/554.
 	// This is despite the underlying context being cancelled by connCtxCancel above.
@@ -2096,16 +2129,14 @@ func (c *Conn) Close() error {
 	// on the first run, it sets firstDerp := true and spawns the aforementioned goroutine.
 	// To detect this, we check activeDerp, which is initialized to non-nil on the first run.
 	if c.activeDerp != nil {
-		<-c.derpStarted
+		select {
+		case <-c.derpStarted:
+			break
+		default:
+			return true
+		}
 	}
-	// Wait on endpoints updating right at the end, once everything is
-	// already closed. We want everything else in the Conn to be
-	// consistently in the closed state before we release mu to wait
-	// on the endpoint updater.
-	for c.endpointsUpdateActive {
-		c.endpointsUpdateWaiter.Wait()
-	}
-	return err
+	return false
 }
 
 func maxIdleBeforeSTUNShutdown() time.Duration {
@@ -2623,6 +2654,9 @@ func (c *Conn) CreateBind(uint16) (conn.Bind, uint16, error) {
 //
 
 func (c *Conn) CreateEndpoint(pubKey [32]byte, addrs string) (conn.Endpoint, error) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
 	pk := key.Public(pubKey)
 	c.logf("magicsock: CreateEndpoint: key=%s: %s", pk.ShortString(), derpStr(addrs))
 
@@ -2632,10 +2666,6 @@ func (c *Conn) CreateEndpoint(pubKey [32]byte, addrs string) (conn.Endpoint, err
 		if err != nil {
 			return nil, fmt.Errorf("magicsock: invalid discokey endpoint %q for %v: %w", addrs, pk.ShortString(), err)
 		}
-		if !c.canCreateEPUnlocked.Get() { // sorry
-			c.mu.Lock()
-			defer c.mu.Unlock()
-		}
 		de := &discoEndpoint{
 			c:                  c,
 			publicKey:          tailcfg.NodeKey(pk),        // peer public key (for WireGuard + DERP)
@@ -2645,17 +2675,6 @@ func (c *Conn) CreateEndpoint(pubKey [32]byte, addrs string) (conn.Endpoint, err
 			sentPing:           map[stun.TxID]sentPing{},
 			endpointState:      map[netaddr.IPPort]*endpointState{},
 		}
-		lastRecvTime := new(int64) // atomic
-		de.onRecvActivity = func() {
-			now := time.Now().Unix()
-			old := atomic.LoadInt64(lastRecvTime)
-			if old == 0 || old <= now-10 {
-				atomic.StoreInt64(lastRecvTime, now)
-				if c.noteRecvActivity != nil {
-					c.noteRecvActivity(de.discoKey)
-				}
-			}
-		}
 		de.initFakeUDPAddr()
 		de.updateFromNode(c.nodeOfDisco[de.discoKey])
 		c.endpointOfDisco[de.discoKey] = de
@@ -2679,9 +2698,6 @@ func (c *Conn) CreateEndpoint(pubKey [32]byte, addrs string) (conn.Endpoint, err
 		}
 	}
 
-	c.mu.Lock()
-	defer c.mu.Unlock()
-
 	// If this endpoint is being updated, remember its old set of
 	// endpoints so we can remove any (from c.addrsByUDP) that are
 	// not in the new set.
@@ -2930,6 +2946,9 @@ func udpAddrDebugString(ua net.UDPAddr) string {
 // discoEndpoint is a wireguard/conn.Endpoint for new-style peers that
 // advertise a DiscoKey and participate in active discovery.
 type discoEndpoint struct {
+	// atomically accessed; declared first for alignment reasons
+	lastRecvUnixAtomic int64
+
 	// These fields are initialized once and never modified.
 	c                  *Conn
 	publicKey          tailcfg.NodeKey  // peer public key (for WireGuard + DERP)
@@ -2938,7 +2957,6 @@ type discoEndpoint struct {
 	fakeWGAddr         netaddr.IPPort   // the UDP address we tell wireguard-go we're using
 	fakeWGAddrStd      *net.UDPAddr     // the *net.UDPAddr form of fakeWGAddr
 	wgEndpointHostPort string           // string from CreateEndpoint: "<hex-discovery-key>.disco.tailscale:12345"
-	onRecvActivity     func()
 
 	// Owned by Conn.mu:
 	lastPingFrom netaddr.IPPort
@@ -3035,6 +3053,19 @@ func (de *discoEndpoint) initFakeUDPAddr() {
 	de.fakeWGAddrStd = de.fakeWGAddr.UDPAddr()
 }
 
+// isFirstRecvActivityInAwhile notes that receive activity has occured for this
+// endpoint and reports whether it's been at least 10 seconds since the last
+// receive activity (including having never received from this peer before).
+func (de *discoEndpoint) isFirstRecvActivityInAwhile() bool {
+	now := time.Now().Unix()
+	old := atomic.LoadInt64(&de.lastRecvUnixAtomic)
+	if old <= now-10 {
+		atomic.StoreInt64(&de.lastRecvUnixAtomic, now)
+		return true
+	}
+	return false
+}
+
 // String exists purely so wireguard-go internals can log.Printf("%v")
 // its internal conn.Endpoints and we don't end up with data races
 // from fmt (via log) reading mutex fields and such.

wgengine/magicsock/magicsock_test.go

@@ -10,16 +10,19 @@ import (
 	crand "crypto/rand"
 	"crypto/tls"
 	"encoding/binary"
+	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
 	"time"
+	"unsafe"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/tailscale/wireguard-go/device"
@@ -807,13 +810,17 @@ func testActiveDiscovery(t *testing.T, d *devices) {
 	// from DERP.
 
 	mustDirect := func(m1, m2 *magicStack) {
+		lastLog := time.Now().Add(-time.Minute)
 		for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(10 * time.Millisecond) {
 			pst := m1.Status().Peer[m2.Public()]
 			if pst.CurAddr != "" {
 				logf("direct link %s->%s found with addr %s", m1, m2, pst.CurAddr)
 				return
 			}
-			logf("no direct path %s->%s yet, addrs %v", m1, m2, pst.Addrs)
+			if now := time.Now(); now.Sub(lastLog) > time.Second {
+				logf("no direct path %s->%s yet, addrs %v", m1, m2, pst.Addrs)
+				lastLog = now
+			}
 		}
 		t.Errorf("magicsock did not find a direct path from %s to %s", m1, m2)
 	}
@@ -923,7 +930,7 @@ func testTwoDevicePing(t *testing.T, d *devices) {
 	})
 
 	// TODO: Remove this once the following tests are reliable.
-	if os.Getenv("RUN_CURSED_TESTS") == "" {
+	if run, _ := strconv.ParseBool(os.Getenv("RUN_CURSED_TESTS")); !run {
 		t.Skip("skipping following tests because RUN_CURSED_TESTS is not set.")
 	}
 
@@ -1021,10 +1028,8 @@ func testTwoDevicePing(t *testing.T, d *devices) {
 		defer setT(outerT)
 		defer func() {
 			if t.Failed() || true {
-				uapi1, _ := cfgs[0].ToUAPI()
-				logf("cfg0: %v", uapi1)
-				uapi2, _ := cfgs[1].ToUAPI()
-				logf("cfg1: %v", uapi2)
+				logf("cfg0: %v", stringifyConfig(cfgs[0]))
+				logf("cfg1: %v", stringifyConfig(cfgs[1]))
 			}
 		}()
 		pingSeq(t, 20, 0, true)
@@ -1310,3 +1315,25 @@ func TestDiscoStringLogRace(t *testing.T) {
 	}()
 	wg.Wait()
 }
+
+func stringifyConfig(cfg wgcfg.Config) string {
+	j, err := json.Marshal(cfg)
+	if err != nil {
+		panic(err)
+	}
+	return string(j)
+}
+
+func TestDiscoEndpointAlignment(t *testing.T) {
+	var de discoEndpoint
+	off := unsafe.Offsetof(de.lastRecvUnixAtomic)
+	if off%8 != 0 {
+		t.Fatalf("lastRecvUnixAtomic is not 8-byte aligned")
+	}
+	if !de.isFirstRecvActivityInAwhile() { // verify this doesn't panic on 32-bit
+		t.Error("expected true")
+	}
+	if de.isFirstRecvActivityInAwhile() {
+		t.Error("expected false on second call")
+	}
+}

wgengine/monitor/monitor_linux.go

@@ -79,7 +79,7 @@ func (c *nlConn) Receive() (message, error) {
 	case unix.RTM_NEWADDR, unix.RTM_DELADDR:
 		var rmsg rtnetlink.AddressMessage
 		if err := rmsg.UnmarshalBinary(msg.Data); err != nil {
-			c.logf("failed to parse type 0x%x: %v", msg.Header.Type, err)
+			c.logf("failed to parse type %v: %v", msg.Header.Type, err)
 			return unspecifiedMessage{}, nil
 		}
 		return &newAddrMessage{
@@ -99,8 +99,18 @@ func (c *nlConn) Receive() (message, error) {
 			Dst:     netaddrIP(rmsg.Attributes.Dst),
 			Gateway: netaddrIP(rmsg.Attributes.Gateway),
 		}, nil
+	case unix.RTM_DELROUTE:
+		var rmsg rtnetlink.RouteMessage
+		if err := rmsg.UnmarshalBinary(msg.Data); err != nil {
+			c.logf("RTM_DELROUTE: failed to parse: %v", err)
+			return unspecifiedMessage{}, nil
+		}
+		// Just log it for now, but don't bubble it up.
+		// (Debugging https://github.com/tailscale/tailscale/issues/643)
+		c.logf("RTM_DELROUTE: %+v", rmsg)
+		return unspecifiedMessage{}, nil
 	default:
-		c.logf("unhandled netlink msg type 0x%x: %+v, %q", msg.Header.Type, msg.Header, msg.Data)
+		c.logf("unhandled netlink msg type %+v, %q", msg.Header, msg.Data)
 		return unspecifiedMessage{}, nil
 	}
 }

wgengine/router/ifconfig_windows.go

@@ -116,6 +116,13 @@ func monitorDefaultRoutes(device *device.Device, autoMTU bool, tun *tun.NativeTu
 				return err
 			}
 			iface.NlMtu = mtu - 80
+			// If the TUN device was created with a smaller MTU,
+			// though, such as 1280, we don't want to go bigger than
+			// configured. (See the comment on minimalMTU in the
+			// wgengine package.)
+			if min, err := tun.MTU(); err == nil && min < int(iface.NlMtu) {
+				iface.NlMtu = uint32(min)
+			}
 			if iface.NlMtu < 576 {
 				iface.NlMtu = 576
 			}

wgengine/userspace.go

@@ -87,6 +87,7 @@ type userspaceEngine struct {
 	logf      logger.Logf
 	reqCh     chan struct{}
 	waitCh    chan struct{} // chan is closed when first Close call completes; contrast with closing bool
+	timeNow   func() time.Time
 	tundev    *tstun.TUN
 	wgdev     *device.Device
 	router    router.Router
@@ -94,6 +95,8 @@ type userspaceEngine struct {
 	magicConn *magicsock.Conn
 	linkMon   *monitor.Mon
 
+	testMaybeReconfigHook func() // for tests; if non-nil, fires if maybeReconfigWireguardLocked called
+
 	// localAddrs is the set of IP addresses assigned to the local
 	// tunnel interface. It's used to reflect local packets
 	// incorrectly sent to us.
@@ -116,7 +119,7 @@ type userspaceEngine struct {
 	pingers        map[wgcfg.Key]*pinger // legacy pingers for pre-discovery peers
 	linkState      *interfaces.State
 
-	// Lock ordering: wgLock, then mu.
+	// Lock ordering: magicsock.Conn.mu, wgLock, then mu.
 }
 
 // RouterGen is the signature for a function that creates a
@@ -199,6 +202,7 @@ func newUserspaceEngineAdvanced(conf EngineConfig) (_ Engine, reterr error) {
 	logf := conf.Logf
 
 	e := &userspaceEngine{
+		timeNow:  time.Now,
 		logf:     logf,
 		reqCh:    make(chan struct{}, 1),
 		waitCh:   make(chan struct{}),
@@ -401,7 +405,7 @@ func (e *userspaceEngine) isLocalAddr(ip packet.IP) bool {
 func (e *userspaceEngine) handleDNS(p *packet.ParsedPacket, t *tstun.TUN) filter.Response {
 	if p.DstIP == magicDNSIP && p.DstPort == magicDNSPort && p.IPProto == packet.UDP {
 		request := tsdns.Packet{
-			Payload: p.Payload(),
+			Payload: append([]byte(nil), p.Payload()...),
 			Addr:    netaddr.IPPort{IP: p.SrcIP.Netaddr(), Port: p.SrcPort},
 		}
 		err := e.resolver.EnqueueRequest(request)
@@ -622,12 +626,12 @@ func (e *userspaceEngine) noteReceiveActivity(dk tailcfg.DiscoKey) {
 	e.wgLock.Lock()
 	defer e.wgLock.Unlock()
 
-	now := time.Now()
 	was, ok := e.recvActivityAt[dk]
 	if !ok {
 		// Not a trimmable peer we care about tracking. (See isTrimmablePeer)
 		return
 	}
+	now := e.timeNow()
 	e.recvActivityAt[dk] = now
 
 	// If the last activity time jumped a bunch (say, at least
@@ -636,7 +640,7 @@ func (e *userspaceEngine) noteReceiveActivity(dk tailcfg.DiscoKey) {
 	// lazyPeerIdleThreshold without the divide by 2, but
 	// maybeReconfigWireguardLocked is cheap enough to call every
 	// couple minutes (just not on every packet).
-	if was.IsZero() || now.Sub(was) < -lazyPeerIdleThreshold/2 {
+	if was.IsZero() || now.Sub(was) > lazyPeerIdleThreshold/2 {
 		e.maybeReconfigWireguardLocked()
 	}
 }
@@ -677,6 +681,11 @@ func discoKeyFromPeer(p *wgcfg.Peer) tailcfg.DiscoKey {
 
 // e.wgLock must be held.
 func (e *userspaceEngine) maybeReconfigWireguardLocked() error {
+	if hook := e.testMaybeReconfigHook; hook != nil {
+		hook()
+		return nil
+	}
+
 	full := e.lastCfgFull
 
 	// Compute a minimal config to pass to wireguard-go
@@ -689,7 +698,7 @@ func (e *userspaceEngine) maybeReconfigWireguardLocked() error {
 	// the past 5 minutes. That's more than WireGuard's key
 	// rotation time anyway so it's no harm if we remove it
 	// later if it's been inactive.
-	activeCutoff := time.Now().Add(-lazyPeerIdleThreshold)
+	activeCutoff := e.timeNow().Add(-lazyPeerIdleThreshold)
 
 	// Not all peers can be trimmed from the network map (see
 	// isTrimmablePeer).  For those are are trimmable, keep track
@@ -765,7 +774,7 @@ func (e *userspaceEngine) updateActivityMapsLocked(trackDisco []tailcfg.DiscoKey
 		if fn == nil {
 			// This is the func that gets run on every outgoing packet for tracked IPs:
 			fn = func() {
-				now := time.Now().Unix()
+				now := e.timeNow().Unix()
 				old := atomic.LoadInt64(timePtr)
 
 				// How long's it been since we last sent a packet?
@@ -883,6 +892,11 @@ func (e *userspaceEngine) getStatusCallback() StatusCallback {
 // TODO: this function returns an error but it's always nil, and when
 // there's actually a problem it just calls log.Fatal. Why?
 func (e *userspaceEngine) getStatus() (*Status, error) {
+	// Grab derpConns before acquiring wgLock to not violate lock ordering;
+	// the DERPs method acquires magicsock.Conn.mu.
+	// (See comment in userspaceEngine's declaration.)
+	derpConns := e.magicConn.DERPs()
+
 	e.wgLock.Lock()
 	defer e.wgLock.Unlock()
 
@@ -998,7 +1012,7 @@ func (e *userspaceEngine) getStatus() (*Status, error) {
 	return &Status{
 		LocalAddrs: append([]string(nil), e.endpoints...),
 		Peers:      peers,
-		DERPs:      e.magicConn.DERPs(),
+		DERPs:      derpConns,
 	}, nil
 }
 

wgengine/userspace_test.go

@@ -0,0 +1,88 @@
+// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package wgengine
+
+import (
+	"bytes"
+	"fmt"
+	"testing"
+	"time"
+
+	"tailscale.com/tailcfg"
+	"tailscale.com/types/key"
+	"tailscale.com/wgengine/tstun"
+)
+
+func TestNoteReceiveActivity(t *testing.T) {
+	now := time.Unix(1, 0)
+	tick := func(d time.Duration) { now = now.Add(d) }
+	var logBuf bytes.Buffer
+
+	confc := make(chan bool, 1)
+	gotConf := func() bool {
+		select {
+		case <-confc:
+			return true
+		default:
+			return false
+		}
+	}
+	e := &userspaceEngine{
+		timeNow:        func() time.Time { return now },
+		recvActivityAt: map[tailcfg.DiscoKey]time.Time{},
+		logf: func(format string, a ...interface{}) {
+			fmt.Fprintf(&logBuf, format, a...)
+		},
+		tundev:                new(tstun.TUN),
+		testMaybeReconfigHook: func() { confc <- true },
+	}
+	ra := e.recvActivityAt
+
+	dk := tailcfg.DiscoKey(key.NewPrivate().Public())
+
+	// Activity on an untracked key should do nothing.
+	e.noteReceiveActivity(dk)
+	if len(ra) != 0 {
+		t.Fatalf("unexpected growth in map: now has %d keys; want 0", len(ra))
+	}
+	if logBuf.Len() != 0 {
+		t.Fatalf("unexpected log write (and thus activity): %s", logBuf.Bytes())
+	}
+
+	// Now track it and expect updates.
+	ra[dk] = time.Time{}
+	e.noteReceiveActivity(dk)
+	if len(ra) != 1 {
+		t.Fatalf("unexpected growth in map: now has %d keys; want 1", len(ra))
+	}
+	if got := ra[dk]; got != now {
+		t.Fatalf("time in map = %v; want %v", got, now)
+	}
+	if !gotConf() {
+		t.Fatalf("didn't get expected reconfig")
+	}
+
+	// With updates 1 second apart, don't expect a reconfig.
+	for i := 0; i < 300; i++ {
+		tick(time.Second)
+		e.noteReceiveActivity(dk)
+		if len(ra) != 1 {
+			t.Fatalf("map len = %d; want 1", len(ra))
+		}
+		if got := ra[dk]; got != now {
+			t.Fatalf("time in map = %v; want %v", got, now)
+		}
+		if gotConf() {
+			t.Fatalf("unexpected reconfig")
+		}
+	}
+
+	// But if there's a big jump it should get an update.
+	tick(3 * time.Minute)
+	e.noteReceiveActivity(dk)
+	if !gotConf() {
+		t.Fatalf("expected config")
+	}
+}