Merge pull request #32 from coder/spike/fix-close-deadlock2

spikecurtis · web-flow · commit ebed8c967bd2 · 2023-08-15T10:05:14.000+04:00
Don't process activity during closing
diff --git a/wgengine/userspace.go b/wgengine/userspace.go
@@ -532,6 +532,16 @@ func (e *userspaceEngine) noteRecvActivity(nk key.NodePublic) {
 	e.wgLock.Lock()
 	defer e.wgLock.Unlock()
 
+	// Reconfiguring wireguard while closing can cause deadlocks, since this
+	// function could be called from receive functions that need to shut down
+	// in order to close the wireguard device.
+	e.mu.Lock()
+	if e.closing {
+		e.mu.Unlock()
+		return
+	}
+	e.mu.Unlock()
+
 	if _, ok := e.recvActivityAt[nk]; !ok {
 		// Not a trimmable peer we care about tracking. (See isTrimmablePeer)
 		if e.trimmedNodes[nk] {
@@ -1058,24 +1068,34 @@ func (e *userspaceEngine) RequestStatus() {
 }
 
 func (e *userspaceEngine) Close() {
+	// It's important here to hold wgLock as well, so that noteRecvActivity exits early once we start
+	// closing the wgdev.  Reprogramming wireguard from a recv function causes a deadlock while closing
+	// since the wireguard device waits for the recv function.
+	e.wgLock.Lock()
 	e.mu.Lock()
 	if e.closing {
 		e.mu.Unlock()
+		e.wgLock.Unlock()
 		return
 	}
 	e.closing = true
 	e.mu.Unlock()
+	e.wgLock.Unlock()
 
 	r := bufio.NewReader(strings.NewReader(""))
+
 	e.wgdev.IpcSetOperation(r)
+
 	e.magicConn.Close()
 	e.netMonUnregister()
 	if e.netMonOwned {
 		e.netMon.Close()
 	}
 	e.dns.Down()
 	e.router.Close()
+
 	e.wgdev.Close()
+
 	e.tundev.Close()
 	if e.birdClient != nil {
 		e.birdClient.DisableProtocol("tailscale")
