From 95635857dc23d442fb43d0cabc13ad73486492cf Mon Sep 17 00:00:00 2001 From: Val Date: Mon, 18 Sep 2023 21:21:46 +0200 Subject: [PATCH] wgengine/magicsock: replace CanPMTUD() with ShouldPMTUD() Replace CanPMTUD() with ShouldPMTUD() to check if peer path MTU discovery should be enabled, in preparation for adding support for enabling/disabling peer MTU dynamically. Updated #311 Signed-off-by: Val --- wgengine/magicsock/debugknobs.go | 8 ++- wgengine/magicsock/debugknobs_stubs.go | 3 +- wgengine/magicsock/magicsock.go | 13 ++-- wgengine/magicsock/peermtu.go | 99 ++++++++++++++++++++++++++ wgengine/magicsock/peermtu_stubs.go | 14 +++- wgengine/magicsock/peermtu_unix.go | 4 -- wgengine/userspace.go | 7 +- 7 files changed, 128 insertions(+), 20 deletions(-) create mode 100644 wgengine/magicsock/peermtu.go diff --git a/wgengine/magicsock/debugknobs.go b/wgengine/magicsock/debugknobs.go index ee831a976..824c677f6 100644 --- a/wgengine/magicsock/debugknobs.go +++ b/wgengine/magicsock/debugknobs.go @@ -53,9 +53,11 @@ var ( // discovery on UDP connections between peers. Currently (2023-09-05) // this only turns on the don't fragment bit for the magicsock UDP // sockets. - debugEnablePMTUD = envknob.RegisterBool("TS_DEBUG_ENABLE_PMTUD") - // Hey you! Adding a new debugknob? Make sure to stub it out in the debugknob_stubs.go - // file too. + debugEnablePMTUD = envknob.RegisterOptBool("TS_DEBUG_ENABLE_PMTUD") + // debugPMTUD prints extra debugging about peer MTU path discovery. + debugPMTUD = envknob.RegisterBool("TS_DEBUG_PMTUD") + // Hey you! Adding a new debugknob? Make sure to stub it out in the + // debugknobs_stubs.go file too. ) // inTest reports whether the running program is a test that set the diff --git a/wgengine/magicsock/debugknobs_stubs.go b/wgengine/magicsock/debugknobs_stubs.go index 57cbdeae2..de49865bf 100644 --- a/wgengine/magicsock/debugknobs_stubs.go +++ b/wgengine/magicsock/debugknobs_stubs.go @@ -20,10 +20,11 @@ func debugAlwaysDERP() bool { return false } func debugUseDERPHTTP() bool { return false } func debugEnableSilentDisco() bool { return false } func debugSendCallMeUnknownPeer() bool { return false } -func debugEnablePMTUD() bool { return false } +func debugPMTUD() bool { return false } func debugUseDERPAddr() string { return "" } func debugUseDerpRouteEnv() string { return "" } func debugUseDerpRoute() opt.Bool { return "" } +func debugEnablePMTUD() opt.Bool { return "" } func debugRingBufferMaxSizeBytes() int { return 0 } func inTest() bool { return false } func debugPeerMap() bool { return false } diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index f6f5d85d6..cdb793e39 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -165,6 +165,9 @@ type Conn struct { // port is the preferred port from opts.Port; 0 means auto. port atomic.Uint32 + // peerMTUEnabled is whether path MTU discovery to peers is enabled. + peerMTUEnabled atomic.Bool + // stats maintains per-connection counters. stats atomic.Pointer[connstats.Statistics] @@ -2310,15 +2313,6 @@ func (c *Conn) bindSocket(ruc *RebindingUDPConn, network string, curPortFate cur } trySetSocketBuffer(pconn, c.logf) - if CanPMTUD() { - err = c.setDontFragment(network, true) - if err != nil { - c.logf("magicsock: set dontfragment failed for %v port %d: %v", network, port, err) - // TODO disable PMTUD in this case. We don't expect the setsockopt to fail on - // supported platforms, but we might as well be paranoid. - } - } - // Success. if debugBindSocket() { c.logf("magicsock: bindSocket: successfully listened %v port %d", network, port) @@ -2358,6 +2352,7 @@ func (c *Conn) rebind(curPortFate currentPortFate) error { return fmt.Errorf("magicsock: Rebind IPv4 failed: %w", err) } c.portMapper.SetLocalPort(c.LocalPort()) + c.UpdatePMTUD() return nil } diff --git a/wgengine/magicsock/peermtu.go b/wgengine/magicsock/peermtu.go new file mode 100644 index 000000000..12ea8ea4e --- /dev/null +++ b/wgengine/magicsock/peermtu.go @@ -0,0 +1,99 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +//go:build (darwin && !ios) || (linux && !android) + +package magicsock + +// Peer path MTU routines shared by platforms that implement it. + +// DontFragSetting returns true if at least one of the underlying sockets of +// this connection is a UDP socket with the don't fragment bit set, otherwise it +// returns false. It also returns an error if either connection returned an error +// other than errUnsupportedConnType. +func (c *Conn) DontFragSetting() (bool, error) { + df4, err4 := c.getDontFragment("udp4") + df6, err6 := c.getDontFragment("udp6") + df := df4 || df6 + err := err4 + if err4 != nil && err4 != errUnsupportedConnType { + err = err6 + } + if err == errUnsupportedConnType { + err = nil + } + return df, err +} + +// ShouldPMTUD returns true if this client should try to enable peer MTU +// discovery, false otherwise. +func (c *Conn) ShouldPMTUD() bool { + if v, ok := debugEnablePMTUD().Get(); ok { + if debugPMTUD() { + c.logf("magicsock: peermtu: peer path MTU discovery set via envknob to %v", v) + } + return v + } + if debugPMTUD() { + c.logf("magicsock: peermtu: peer path MTU discovery set by default to false") + } + return false // Until we feel confident PMTUD is solid. +} + +// PeerMTUEnabled returns true if this Conn is has peer path MTU discovery enabled. +func (c *Conn) PeerMTUEnabled() bool { + return c.peerMTUEnabled.Load() +} + +// UpdatePMTUD configures underlying sockets of this Conn to enable or disable +// peer path MTU discovery according to the current configuration. +// +// Enabling or disabling peer path MTU discovery requires setting the don't +// fragment bit on its two underlying pconns. There are three distinct results +// for this operation on each pconn: +// +// 1. Success +// 2. Failure (not supported on this platform, or supported but failed) +// 3. Not a UDP socket (most likely one of IPv4 or IPv6 couldn't be used) +// +// To simplify the fast path for the most common case, we set the PMTUD status +// of the overall Conn according to the results of setting the sockopt on pconn +// as follows: +// +// 1. Both setsockopts succeed: PMTUD status update succeeds +// 2. One succeeds, one returns not a UDP socket: PMTUD status update succeeds +// 4. Neither setsockopt succeeds: PMTUD disabled +// 3. Either setsockopt fails: PMTUD disabled +// +// If the PMTUD settings changed, it resets the endpoint state so that it will +// re-probe path MTUs to this peer. +func (c *Conn) UpdatePMTUD() { + if debugPMTUD() { + df4, err4 := c.getDontFragment("udp4") + df6, err6 := c.getDontFragment("udp6") + c.logf("magicsock: peermtu: peer MTU status %v DF bit status: v4: %v (%v) v6: %v (%v)", c.peerMTUEnabled.Load(), df4, err4, df6, err6) + } + + enable := c.ShouldPMTUD() + if c.peerMTUEnabled.Load() == enable { + c.logf("magicsock: peermtu: peer MTU status is %v", enable) + return + } + + newStatus := enable + err4 := c.setDontFragment("udp4", enable) + err6 := c.setDontFragment("udp6", enable) + anySuccess := err4 == nil || err6 == nil + noFailures := (err4 == nil || err4 == errUnsupportedConnType) && (err6 == nil || err6 == errUnsupportedConnType) + + if anySuccess && noFailures { + c.logf("magicsock: peermtu: peer MTU status updated to %v", newStatus) + } else { + c.logf("[unexpected] magicsock: peermtu: updating peer MTU status to %v failed (v4: %v, v6: %v), disabling", enable, err4, err6) + _ = c.setDontFragment("udp4", false) + _ = c.setDontFragment("udp6", false) + newStatus = false + } + c.peerMTUEnabled.Store(newStatus) + c.resetEndpointStates() +} diff --git a/wgengine/magicsock/peermtu_stubs.go b/wgengine/magicsock/peermtu_stubs.go index 60c619f29..6981f28c3 100644 --- a/wgengine/magicsock/peermtu_stubs.go +++ b/wgengine/magicsock/peermtu_stubs.go @@ -30,7 +30,17 @@ func (c *Conn) getDontFragment(network string) (bool, error) { return false, nil } -// CanPMTUD returns whether this platform supports performing peet path MTU discovery. -func CanPMTUD() bool { +func (c *Conn) DontFragSetting() (bool, error) { + return false, nil +} + +func (c *Conn) ShouldPMTUD() bool { + return false +} + +func (c *Conn) PeerMTUEnabled() bool { return false } + +func (c *Conn) UpdatePMTUD() { +} diff --git a/wgengine/magicsock/peermtu_unix.go b/wgengine/magicsock/peermtu_unix.go index 6b89ef93a..eec3d744f 100644 --- a/wgengine/magicsock/peermtu_unix.go +++ b/wgengine/magicsock/peermtu_unix.go @@ -40,7 +40,3 @@ func (c *Conn) connControl(network string, fn func(fd uintptr)) error { } return rc.Control(fn) } - -func CanPMTUD() bool { - return debugEnablePMTUD() -} diff --git a/wgengine/userspace.go b/wgengine/userspace.go index c90a4b2be..397348ee2 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -804,6 +804,8 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, listenPort = 0 } + peerMTUEnable := e.magicConn.ShouldPMTUD() + isSubnetRouter := false if e.birdClient != nil && nm != nil && nm.SelfNode.Valid() { isSubnetRouter = hasOverlap(nm.SelfNode.PrimaryRoutes(), nm.SelfNode.Hostinfo().RoutableIPs()) @@ -818,7 +820,9 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, RouterConfig *router.Config DNSConfig *dns.Config }{routerCfg, dnsCfg}) - if !engineChanged && !routerChanged && listenPort == e.magicConn.LocalPort() && !isSubnetRouterChanged { + listenPortChanged := listenPort != e.magicConn.LocalPort() + peerMTUChanged := peerMTUEnable != e.magicConn.PeerMTUEnabled() + if !engineChanged && !routerChanged && !listenPortChanged && !isSubnetRouterChanged && !peerMTUChanged { return ErrNoChanges } newLogIDs := cfg.NetworkLogging @@ -874,6 +878,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, } e.magicConn.UpdatePeers(peerSet) e.magicConn.SetPreferredPort(listenPort) + e.magicConn.UpdatePMTUD() if err := e.maybeReconfigWireguardLocked(discoChanged); err != nil { return err