From 0d4c8cb2e168a3cf95b858ff69e88e24870e3ede Mon Sep 17 00:00:00 2001 From: Josh Bleecher Snyder Date: Mon, 26 Apr 2021 16:35:09 -0700 Subject: [PATCH] health: delete ReceiveFunc health checks They were not doing their job. They need yet another conceptual re-think. Start by clearing the decks. Signed-off-by: Josh Bleecher Snyder --- health/health.go | 71 --------------------------------- wgengine/magicsock/magicsock.go | 25 ------------ 2 files changed, 96 deletions(-) diff --git a/health/health.go b/health/health.go index d545e885a..662114f2d 100644 --- a/health/health.go +++ b/health/health.go @@ -36,10 +36,6 @@ var ( ipnState string ipnWantRunning bool anyInterfaceUp = true // until told otherwise - - ReceiveIPv4 = ReceiveFuncState{name: "IPv4"} - ReceiveIPv6 = ReceiveFuncState{name: "IPv6"} - ReceiveDERP = ReceiveFuncState{name: "DERP"} ) // Subsystem is the name of a subsystem whose health can be monitored. @@ -217,68 +213,6 @@ func SetAnyInterfaceUp(up bool) { selfCheckLocked() } -// ReceiveFuncState tracks the state of a wireguard-go conn.ReceiveFunc. -type ReceiveFuncState struct { - // name is a mnemonic for the receive func, used in error messages. - name string - // started indicates whether magicsock.connBind.Open - // has requested that wireguard-go start its receive func - // goroutine (without a corresponding connBind.Close). - started bool - // running models whether wireguard-go's receive func - // goroutine is actually running. We cannot easily introspect that, - // so it is based on our knowledge of wireguard-go's internals. - running bool -} - -// err returns the error state (if any) that s represents. -func (s ReceiveFuncState) err() error { - // Possible states: - // | started | running | notes - // | ------- | ------- | ----- - // | true | true | normal operation - // | true | false | we prematurely returned a permanent error from this receive func - // | false | true | we have told package health that we're closing the bind, but the receive funcs haven't closed yet (transient) - // | false | false | not running - - // The problematic case is started && !running. - // If that happens, wireguard-go will no longer request packets, - // and we'll lose an entire communication channel. - if s.started && !s.running { - return fmt.Errorf("receive%s started but not running", s.name) - } - return nil -} - -// Open tells r that connBind.Open has requested wireguard-go open a conn.Bind that includes r. -func (r *ReceiveFuncState) Open() { - mu.Lock() - defer mu.Unlock() - r.started = true - r.running = true - selfCheckLocked() -} - -// Stop tells r that we have returned a permanent error to wireguard-go. -// wireguard-go's receive func goroutine for r will soon stop. -func (r *ReceiveFuncState) Stop() { - mu.Lock() - defer mu.Unlock() - r.running = false - selfCheckLocked() -} - -// Close tells r that connBind.Close has requested wireguard-go close the bind for r. -// This will stop the corresponding receive func goroutine. -// Close must be called before actually closing the underlying connection, -// to avoid a small window of false positives. -func (r *ReceiveFuncState) Close() { - mu.Lock() - defer mu.Unlock() - r.started = false - selfCheckLocked() -} - func timerSelfCheck() { mu.Lock() defer mu.Unlock() @@ -329,11 +263,6 @@ func overallErrorLocked() error { _ = lastMapRequestHeard var errs []error - for _, recv := range []ReceiveFuncState{ReceiveIPv4, ReceiveIPv6, ReceiveDERP} { - if err := recv.err(); err != nil { - errs = append(errs, err) - } - } for sys, err := range sysErr { if err == nil || sys == SysOverall { continue diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 8b90f2fbb..51b5c0af1 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -1584,9 +1584,6 @@ func (c *Conn) receiveIPv6(b []byte) (int, conn.Endpoint, error) { for { n, ipp, err := c.pconn6.ReadFromNetaddr(b) if err != nil { - if isPermanentNetError(err) { - health.ReceiveIPv6.Stop() - } return 0, nil, err } if ep, ok := c.receiveIP(b[:n], ipp, &c.ippEndpoint6); ok { @@ -1600,9 +1597,6 @@ func (c *Conn) receiveIPv4(b []byte) (n int, ep conn.Endpoint, err error) { for { n, ipp, err := c.pconn4.ReadFromNetaddr(b) if err != nil { - if isPermanentNetError(err) { - health.ReceiveIPv4.Stop() - } return 0, nil, err } if ep, ok := c.receiveIP(b[:n], ipp, &c.ippEndpoint4); ok { @@ -1663,7 +1657,6 @@ func (c *connBind) receiveDERP(b []byte) (n int, ep conn.Endpoint, err error) { } return n, ep, nil } - health.ReceiveDERP.Stop() return 0, nil, net.ErrClosed } @@ -1734,18 +1727,6 @@ func (c *Conn) processDERPReadResult(dm derpReadResult, b []byte) (n int, ep con return n, ep } -// isPermanentNetError reports whether err is permanent. -// It matches an equivalent check in wireguard-go's RoutineReceiveIncoming. -func isPermanentNetError(err error) bool { - // Once this module requires Go 1.17, the comparison to net.ErrClosed can be removed. - // See https://github.com/golang/go/issues/45357. - if errors.Is(err, net.ErrClosed) { - return true - } - neterr, ok := err.(net.Error) - return ok && !neterr.Temporary() -} - // discoLogLevel controls the verbosity of discovery log messages. type discoLogLevel int @@ -2430,11 +2411,8 @@ func (c *connBind) Open(ignoredPort uint16) ([]conn.ReceiveFunc, uint16, error) } c.closed = false fns := []conn.ReceiveFunc{c.receiveIPv4, c.receiveDERP} - health.ReceiveIPv4.Open() - health.ReceiveDERP.Open() if c.pconn6 != nil { fns = append(fns, c.receiveIPv6) - health.ReceiveIPv6.Open() } // TODO: Combine receiveIPv4 and receiveIPv6 and receiveIP into a single // closure that closes over a *RebindingUDPConn? @@ -2456,15 +2434,12 @@ func (c *connBind) Close() error { } c.closed = true // Unblock all outstanding receives. - health.ReceiveIPv4.Close() c.pconn4.Close() if c.pconn6 != nil { - health.ReceiveIPv6.Close() c.pconn6.Close() } // Send an empty read result to unblock receiveDERP, // which will then check connBind.Closed. - health.ReceiveDERP.Close() c.derpRecvCh <- derpReadResult{} return nil }