From 9859a42c2e80cbb6aa44458a982967945096fb09 Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Mon, 16 Sep 2024 11:38:47 +0000 Subject: [PATCH 1/2] fix: immediately rebind after WSAECONNRESET --- net/neterror/neterror.go | 16 +++++++++++++++- net/neterror/neterror_windows.go | 4 ++++ wgengine/magicsock/magicsock.go | 24 ++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 1 deletion(-) diff --git a/net/neterror/neterror.go b/net/neterror/neterror.go index e2387440d33d5..abdcf093fcf06 100644 --- a/net/neterror/neterror.go +++ b/net/neterror/neterror.go @@ -41,7 +41,10 @@ func TreatAsLostUDP(err error) bool { return false } -var packetWasTruncated func(error) bool // non-nil on Windows at least +var ( + packetWasTruncated func(error) bool // non-nil on Windows at least + socketWasReset func(error) bool // non-nil on Windows at least +) // PacketWasTruncated reports whether err indicates truncation but the RecvFrom // that generated err was otherwise successful. On Windows, Go's UDP RecvFrom @@ -59,6 +62,17 @@ func PacketWasTruncated(err error) bool { return packetWasTruncated(err) } +// SocketWasReset reports whether err is an error from a TCP or UDP send/recv +// operation that should be treated as a connection reset. On Windows, +// WSARecvFrom can return WSAECONNRESET when the remote side sends an ICMP error +// message. +func SocketWasReset(err error) bool { + if socketWasReset == nil { + return false + } + return socketWasReset(err) +} + var shouldDisableUDPGSO func(error) bool // non-nil on Linux func ShouldDisableUDPGSO(err error) bool { diff --git a/net/neterror/neterror_windows.go b/net/neterror/neterror_windows.go index bf112f5ed7ab7..55eba260d24f9 100644 --- a/net/neterror/neterror_windows.go +++ b/net/neterror/neterror_windows.go @@ -13,4 +13,8 @@ func init() { packetWasTruncated = func(err error) bool { return errors.Is(err, windows.WSAEMSGSIZE) } + + socketWasReset = func(err error) bool { + return errors.Is(err, windows.WSAECONNRESET) + } } diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index b27736822af30..0370b77851ab1 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -1213,17 +1213,32 @@ func (c *Conn) receiveIPv6() conn.ReceiveFunc { return c.mkReceiveFunc(&c.pconn6, &health.ReceiveIPv6, metricRecvDataIPv6) } +type permanentNetError struct { + error +} + +var _ net.Error = permanentNetError{} + +func (permanentNetError) Timeout() bool { return false } +func (permanentNetError) Temporary() bool { return false } + // mkReceiveFunc creates a ReceiveFunc reading from ruc. // The provided healthItem and metric are updated if non-nil. func (c *Conn) mkReceiveFunc(ruc *RebindingUDPConn, healthItem *health.ReceiveFuncStats, metric *clientmetric.Metric) conn.ReceiveFunc { // epCache caches an IPPort->endpoint for hot flows. var epCache ippEndpointCache + var connErr error return func(buffs [][]byte, sizes []int, eps []conn.Endpoint) (int, error) { if healthItem != nil { healthItem.Enter() defer healthItem.Exit() } + if connErr != nil { + // Just in case we get another call, we don't want to call ReadBatch + // again. + return 0, connErr + } if ruc == nil { panic("nil RebindingUDPConn") } @@ -1236,6 +1251,15 @@ func (c *Conn) mkReceiveFunc(ruc *RebindingUDPConn, healthItem *health.ReceiveFu if neterror.PacketWasTruncated(err) { continue } + if neterror.SocketWasReset(err) { + // Wrap the error in a permanentNetError so that Wireguard + // doesn't keep trying to read packets from us. + connErr = permanentNetError{error: err} + c.logf("magicsock: receive: rebind required due to socket reset: %v", err) + c.Rebind() + return 0, connErr + } + return 0, err } From fac0e1c778154a64787708e65972b683149c086a Mon Sep 17 00:00:00 2001 From: Dean Sheather Date: Tue, 1 Oct 2024 03:14:17 +0000 Subject: [PATCH 2/2] Comments --- wgengine/magicsock/magicsock.go | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 0370b77851ab1..72c356a3c6d38 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -1213,32 +1213,17 @@ func (c *Conn) receiveIPv6() conn.ReceiveFunc { return c.mkReceiveFunc(&c.pconn6, &health.ReceiveIPv6, metricRecvDataIPv6) } -type permanentNetError struct { - error -} - -var _ net.Error = permanentNetError{} - -func (permanentNetError) Timeout() bool { return false } -func (permanentNetError) Temporary() bool { return false } - // mkReceiveFunc creates a ReceiveFunc reading from ruc. // The provided healthItem and metric are updated if non-nil. func (c *Conn) mkReceiveFunc(ruc *RebindingUDPConn, healthItem *health.ReceiveFuncStats, metric *clientmetric.Metric) conn.ReceiveFunc { // epCache caches an IPPort->endpoint for hot flows. var epCache ippEndpointCache - var connErr error return func(buffs [][]byte, sizes []int, eps []conn.Endpoint) (int, error) { if healthItem != nil { healthItem.Enter() defer healthItem.Exit() } - if connErr != nil { - // Just in case we get another call, we don't want to call ReadBatch - // again. - return 0, connErr - } if ruc == nil { panic("nil RebindingUDPConn") } @@ -1252,12 +1237,9 @@ func (c *Conn) mkReceiveFunc(ruc *RebindingUDPConn, healthItem *health.ReceiveFu continue } if neterror.SocketWasReset(err) { - // Wrap the error in a permanentNetError so that Wireguard - // doesn't keep trying to read packets from us. - connErr = permanentNetError{error: err} c.logf("magicsock: receive: rebind required due to socket reset: %v", err) c.Rebind() - return 0, connErr + c.ReSTUN("socket-reset") } return 0, err