From 5835a3f553f678944c0ae8600bb5cd8127dfdc86 Mon Sep 17 00:00:00 2001
From: Josh Bleecher Snyder <josharian@gmail.com>
Date: Thu, 22 Apr 2021 12:37:53 -0700
Subject: [PATCH] health, wgengine/magicsock: avoid receive function false
 positives

Avery reported a sub-ms health transition from "receiveIPv4 not running" to "ok".

To avoid these transient false-positives, be more precise about
the expected lifetime of receive funcs. The problematic case is one in which
they were started but exited prior to a call to connBind.Close.
Explicitly represent started vs running state, taking care with the order of updates.

Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
---
 health/health.go                | 8 ++++++--
 wgengine/magicsock/magicsock.go | 8 +++++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/health/health.go b/health/health.go
index 84e336dce..a9d29b7ba 100644
--- a/health/health.go
+++ b/health/health.go
@@ -37,9 +37,11 @@ var (
 	ipnWantRunning          bool
 	anyInterfaceUp          = true // until told otherwise
 
+	receiveIPv4Started bool
 	receiveIPv4Running bool
 	receiveIPv6Started bool
 	receiveIPv6Running bool
+	receiveDERPStarted bool
 	receiveDERPRunning bool
 )
 
@@ -218,9 +220,11 @@ func SetAnyInterfaceUp(up bool) {
 	selfCheckLocked()
 }
 
+func SetReceiveIPv4Started(running bool) { setHealthBool(&receiveIPv4Started, running) }
 func SetReceiveIPv4Running(running bool) { setHealthBool(&receiveIPv4Running, running) }
 func SetReceiveIPv6Started(running bool) { setHealthBool(&receiveIPv6Started, running) }
 func SetReceiveIPv6Running(running bool) { setHealthBool(&receiveIPv6Running, running) }
+func SetReceiveDERPStarted(running bool) { setHealthBool(&receiveDERPStarted, running) }
 func SetReceiveDERPRunning(running bool) { setHealthBool(&receiveDERPRunning, running) }
 
 func setHealthBool(dst *bool, b bool) {
@@ -280,10 +284,10 @@ func overallErrorLocked() error {
 	_ = lastMapRequestHeard
 
 	var errs []error
-	if !receiveIPv4Running {
+	if receiveIPv4Started && !receiveIPv4Running {
 		errs = append(errs, errors.New("receiveIPv4 not running"))
 	}
-	if !receiveDERPRunning {
+	if receiveDERPStarted && !receiveDERPRunning {
 		errs = append(errs, errors.New("receiveDERP not running"))
 	}
 	if receiveIPv6Started && !receiveIPv6Running {
diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go
index 9acdc4f47..b2c873624 100644
--- a/wgengine/magicsock/magicsock.go
+++ b/wgengine/magicsock/magicsock.go
@@ -1581,8 +1581,8 @@ func (c *Conn) noteRecvActivityFromEndpoint(e conn.Endpoint) {
 
 // receiveIPv6 receives a UDP IPv6 packet. It is called by wireguard-go.
 func (c *Conn) receiveIPv6(b []byte) (int, conn.Endpoint, error) {
-	health.SetReceiveIPv6Started(true)
 	health.SetReceiveIPv6Running(true)
+	health.SetReceiveIPv6Started(true)
 	defer health.SetReceiveIPv6Running(false)
 	for {
 		n, ipp, err := c.pconn6.ReadFromNetaddr(b)
@@ -1598,6 +1598,7 @@ func (c *Conn) receiveIPv6(b []byte) (int, conn.Endpoint, error) {
 // receiveIPv4 receives a UDP IPv4 packet. It is called by wireguard-go.
 func (c *Conn) receiveIPv4(b []byte) (n int, ep conn.Endpoint, err error) {
 	health.SetReceiveIPv4Running(true)
+	health.SetReceiveIPv4Started(true)
 	defer health.SetReceiveIPv4Running(false)
 	for {
 		n, ipp, err := c.pconn4.ReadFromNetaddr(b)
@@ -1652,6 +1653,7 @@ func (c *Conn) receiveIP(b []byte, ipp netaddr.IPPort, cache *ippEndpointCache)
 // found, the returned error is errLoopAgain.
 func (c *connBind) receiveDERP(b []byte) (n int, ep conn.Endpoint, err error) {
 	health.SetReceiveDERPRunning(true)
+	health.SetReceiveDERPStarted(true)
 	defer health.SetReceiveDERPRunning(false)
 	for dm := range c.derpRecvCh {
 		if c.Closed() {
@@ -2441,6 +2443,9 @@ func (c *connBind) Close() error {
 	}
 	c.closed = true
 	// Unblock all outstanding receives.
+	// Tell the health checker that we're closing the connections
+	// before actually closing them to avoid false positives.
+	health.SetReceiveIPv4Started(false)
 	c.pconn4.Close()
 	if c.pconn6 != nil {
 		health.SetReceiveIPv6Started(false)
@@ -2448,6 +2453,7 @@ func (c *connBind) Close() error {
 	}
 	// Send an empty read result to unblock receiveDERP,
 	// which will then check connBind.Closed.
+	health.SetReceiveDERPStarted(false)
 	c.derpRecvCh <- derpReadResult{}
 	return nil
 }