wgengine: don't Reconfig on boring link changes

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

wgengine/magicsock/magicsock.go

@@ -64,9 +64,6 @@ type Conn struct {
 	connCtx       context.Context // closed on Conn.Close
 	connCtxCancel func()          // closes connCtx
 
-	linkChangeMu sync.Mutex
-	linkState    *interfaces.State
-
 	// addrsByUDP is a map of every remote ip:port to a priority
 	// list of endpoint addresses for a peer.
 	// The priority list is provided by wgengine configuration.
@@ -205,7 +202,6 @@ func Listen(opts Options) (*Conn, error) {
 		derpTLSConfig: opts.derpTLSConfig,
 		derps:         opts.DERPs,
 	}
-	c.linkState, _ = getLinkState()
 	if c.derps == nil {
 		c.derps = derpmap.Prod()
 	}
@@ -218,7 +214,7 @@ func Listen(opts Options) (*Conn, error) {
 
 	c.ignoreSTUNPackets()
 	c.pconn.Reset(packetConn.(*net.UDPConn))
-	c.reSTUN("initial")
+	c.ReSTUN("initial")
 
 	c.goroutines.Add(1)
 	go func() {
@@ -1110,42 +1106,18 @@ func (c *Conn) Close() error {
 	return err
 }
 
-func (c *Conn) reSTUN(why string) {
+// ReSTUN triggers an address discovery.
+// The provided why string is for debug logging only.
+func (c *Conn) ReSTUN(why string) {
 	select {
 	case c.startEpUpdate <- why:
 	case <-c.donec():
 	}
 }
 
-// LinkChange should be called whenever something changed with the
-// network, no matter how minor. The LinkChange method then looks
-// at the state of the network and decides whether the change from
-// before is interesting enough to warrant taking action on.
-func (c *Conn) LinkChange() {
-	defer c.reSTUN("link-change")
-
-	c.linkChangeMu.Lock()
-	defer c.linkChangeMu.Unlock()
-
-	cur, err := getLinkState()
-	if err != nil {
-		return
-	}
-	if c.linkState != nil && !cur.Equal(c.linkState) {
-		c.linkState = cur
-		c.rebind()
-	}
-}
-
-func getLinkState() (*interfaces.State, error) {
-	s, err := interfaces.GetState()
-	if s != nil {
-		s.RemoveTailscaleInterfaces()
-	}
-	return s, err
-}
-
-func (c *Conn) rebind() {
+// Rebind closes and re-binds the UDP sockets.
+// It should be followed by a call to ReSTUN.
+func (c *Conn) Rebind() {
 	if c.pconnPort != 0 {
 		c.pconn.mu.Lock()
 		if err := c.pconn.pconn.Close(); err != nil {

wgengine/userspace.go

@@ -18,6 +18,7 @@ import (
 	"github.com/tailscale/wireguard-go/device"
 	"github.com/tailscale/wireguard-go/tun"
 	"github.com/tailscale/wireguard-go/wgcfg"
+	"tailscale.com/net/interfaces"
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/logger"
 	"tailscale.com/wgengine/filter"
@@ -46,6 +47,7 @@ type userspaceEngine struct {
 	peerSequence   []wgcfg.Key
 	endpoints      []string
 	pingers        map[wgcfg.Key]context.CancelFunc // mu must be held to call CancelFunc
+	linkState      *interfaces.State
 }
 
 type Loggify struct {
@@ -103,6 +105,7 @@ func newUserspaceEngineAdvanced(logf logger.Logf, tundev tun.Device, routerGen R
 		tundev:  tundev,
 		pingers: make(map[wgcfg.Key]context.CancelFunc),
 	}
+	e.linkState, _ = getLinkState()
 
 	mon, err := monitor.New(logf, func() { e.LinkChange(false) })
 	if err != nil {
@@ -585,13 +588,41 @@ func (e *userspaceEngine) Wait() {
 	<-e.waitCh
 }
 
+func (e *userspaceEngine) setLinkState(st *interfaces.State) (changed bool) {
+	if st == nil {
+		return false
+	}
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	changed = e.linkState == nil || !st.Equal(e.linkState)
+	e.linkState = st
+	return changed
+}
+
 func (e *userspaceEngine) LinkChange(isExpensive bool) {
-	e.logf("LinkChange(isExpensive=%v): rebinding socket", isExpensive)
+	cur, err := getLinkState()
+	if err != nil {
+		e.logf("LinkChange: interfaces.GetState: %v", err)
+		return
+	}
+	needRebind := e.setLinkState(cur)
+
+	e.logf("LinkChange(isExpensive=%v); needsRebind=%v", isExpensive, needRebind)
+
+	why := "link-change-minor"
+	if needRebind {
+		why = "link-change-major"
+		e.magicConn.Rebind()
+	}
+	e.magicConn.ReSTUN(why)
+	if !needRebind {
+		return
+	}
+
 	e.wgLock.Lock()
 	defer e.wgLock.Unlock()
 
 	// TODO(crawshaw): use isExpensive=true to switch into "client mode" on macOS?
-	e.magicConn.LinkChange()
 
 	// TODO(crawshaw): when we have an incremental notion of reconfig,
 	// be gentler here. No need to smash in-progress connections,
@@ -606,6 +637,14 @@ func (e *userspaceEngine) LinkChange(isExpensive bool) {
 	}
 }
 
+func getLinkState() (*interfaces.State, error) {
+	s, err := interfaces.GetState()
+	if s != nil {
+		s.RemoveTailscaleInterfaces()
+	}
+	return s, err
+}
+
 func (e *userspaceEngine) SetNetInfoCallback(cb NetInfoCallback) {
 	e.magicConn.SetNetInfoCallback(cb)
 }

wgengine/wgengine.go

@@ -125,6 +125,12 @@ type Engine interface {
 	// link has changed. The isExpensive parameter is set on links
 	// where sending packets uses substantial power or money,
 	// such as mobile data on a phone.
+	//
+	// LinkChange should be called whenever something changed with
+	// the network, no matter how minor. The implementation should
+	// look at the state of the network and decide whether the
+	// change from before is interesting enough to warrant taking
+	// action on.
 	LinkChange(isExpensive bool)
 
 	// SetDERPEnabled controls whether DERP is enabled.