From 40fa2a420c3681bda68762c1ba461852b868ab0c Mon Sep 17 00:00:00 2001 From: James Tucker Date: Thu, 30 Mar 2023 10:37:06 -0700 Subject: [PATCH] envknob,net/tstun,wgengine: use TS_DEBUG_MTU consistently Noted on #5915 TS_DEBUG_MTU was not used consistently everywhere. Extract the default into a function that can apply this centrally and use it everywhere. Added envknob.Lookup{Int,Uint}Sized to make it easier to keep CodeQL happy when using converted values. Updates #5915 Signed-off-by: James Tucker --- envknob/envknob.go | 40 +++++++++++++++++++++++++++++ net/tstun/constants.go | 15 ----------- net/tstun/mtu.go | 33 ++++++++++++++++++++++++ net/tstun/mtu_test.go | 28 ++++++++++++++++++++ net/tstun/tun.go | 7 +---- wgengine/netstack/netstack.go | 5 ++-- wgengine/router/ifconfig_windows.go | 2 +- 7 files changed, 105 insertions(+), 25 deletions(-) delete mode 100644 net/tstun/constants.go create mode 100644 net/tstun/mtu.go create mode 100644 net/tstun/mtu_test.go diff --git a/envknob/envknob.go b/envknob/envknob.go index 04913ae45..cd7440f94 100644 --- a/envknob/envknob.go +++ b/envknob/envknob.go @@ -330,6 +330,46 @@ func LookupInt(envVar string) (v int, ok bool) { panic("unreachable") } +// LookupIntSized returns the integer value of the named environment value +// parsed in base and with a maximum bit size bitSize. +// The ok result is whether a value was set. +// If the value isn't a valid int, it exits the program with a failure. +func LookupIntSized(envVar string, base, bitSize int) (v int, ok bool) { + assertNotInInit() + val := os.Getenv(envVar) + if val == "" { + return 0, false + } + i, err := strconv.ParseInt(val, base, bitSize) + if err == nil { + v = int(i) + noteEnv(envVar, val) + return v, true + } + log.Fatalf("invalid integer environment variable %s: %v", envVar, val) + panic("unreachable") +} + +// LookupUintSized returns the unsigned integer value of the named environment +// value parsed in base and with a maximum bit size bitSize. +// The ok result is whether a value was set. +// If the value isn't a valid int, it exits the program with a failure. +func LookupUintSized(envVar string, base, bitSize int) (v uint, ok bool) { + assertNotInInit() + val := os.Getenv(envVar) + if val == "" { + return 0, false + } + i, err := strconv.ParseUint(val, base, bitSize) + if err == nil { + v = uint(i) + noteEnv(envVar, val) + return v, true + } + log.Fatalf("invalid unsigned integer environment variable %s: %v", envVar, val) + panic("unreachable") +} + // UseWIPCode is whether TAILSCALE_USE_WIP_CODE is set to permit use // of Work-In-Progress code. func UseWIPCode() bool { return Bool("TAILSCALE_USE_WIP_CODE") } diff --git a/net/tstun/constants.go b/net/tstun/constants.go deleted file mode 100644 index 656eff7ac..000000000 --- a/net/tstun/constants.go +++ /dev/null @@ -1,15 +0,0 @@ -// Copyright (c) Tailscale Inc & AUTHORS -// SPDX-License-Identifier: BSD-3-Clause - -package tstun - -// DefaultMTU is the Tailscale default MTU for now. -// -// wireguard-go defaults to 1420 bytes, which only works if the -// "outer" MTU is 1500 bytes. This breaks on DSL connections -// (typically 1492 MTU) and on GCE (1460 MTU?!). -// -// 1280 is the smallest MTU allowed for IPv6, which is a sensible -// "probably works everywhere" setting until we develop proper PMTU -// discovery. -const DefaultMTU = 1280 diff --git a/net/tstun/mtu.go b/net/tstun/mtu.go new file mode 100644 index 000000000..2307d47f9 --- /dev/null +++ b/net/tstun/mtu.go @@ -0,0 +1,33 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause +package tstun + +import "tailscale.com/envknob" + +const ( + maxMTU uint32 = 65536 + defaultMTU uint32 = 1280 +) + +// DefaultMTU returns either the constant default MTU of 1280, or the value set +// in TS_DEBUG_MTU clamped to a maximum of 65536. +func DefaultMTU() uint32 { + // DefaultMTU is the Tailscale default MTU for now. + // + // wireguard-go defaults to 1420 bytes, which only works if the + // "outer" MTU is 1500 bytes. This breaks on DSL connections + // (typically 1492 MTU) and on GCE (1460 MTU?!). + // + // 1280 is the smallest MTU allowed for IPv6, which is a sensible + // "probably works everywhere" setting until we develop proper PMTU + // discovery. + tunMTU := defaultMTU + if mtu, ok := envknob.LookupUintSized("TS_DEBUG_MTU", 10, 32); ok { + mtu := uint32(mtu) + if mtu > maxMTU { + mtu = maxMTU + } + tunMTU = mtu + } + return tunMTU +} diff --git a/net/tstun/mtu_test.go b/net/tstun/mtu_test.go new file mode 100644 index 000000000..f3aea4697 --- /dev/null +++ b/net/tstun/mtu_test.go @@ -0,0 +1,28 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause +package tstun + +import ( + "os" + "testing" +) + +func TestDefaultMTU(t *testing.T) { + orig := os.Getenv("TS_DEBUG_MTU") + defer os.Setenv("TS_DEBUG_MTU", orig) + + os.Setenv("TS_DEBUG_MTU", "") + if DefaultMTU() != 1280 { + t.Errorf("DefaultMTU() = %d, want 1280", DefaultMTU()) + } + + os.Setenv("TS_DEBUG_MTU", "9000") + if DefaultMTU() != 9000 { + t.Errorf("DefaultMTU() = %d, want 9000", DefaultMTU()) + } + + os.Setenv("TS_DEBUG_MTU", "123456789") + if DefaultMTU() != maxMTU { + t.Errorf("DefaultMTU() = %d, want %d", DefaultMTU(), maxMTU) + } +} diff --git a/net/tstun/tun.go b/net/tstun/tun.go index e00a07239..57912c79a 100644 --- a/net/tstun/tun.go +++ b/net/tstun/tun.go @@ -14,7 +14,6 @@ import ( "time" "github.com/tailscale/wireguard-go/tun" - "tailscale.com/envknob" "tailscale.com/types/logger" ) @@ -45,11 +44,7 @@ func New(logf logger.Logf, tunName string) (tun.Device, string, error) { } dev, err = createTAP(tapName, bridgeName) } else { - tunMTU := DefaultMTU - if mtu, ok := envknob.LookupInt("TS_DEBUG_MTU"); ok { - tunMTU = mtu - } - dev, err = tun.CreateTUN(tunName, tunMTU) + dev, err = tun.CreateTUN(tunName, int(DefaultMTU())) } if err != nil { return nil, "", err diff --git a/wgengine/netstack/netstack.go b/wgengine/netstack/netstack.go index 103db9bda..db7fea470 100644 --- a/wgengine/netstack/netstack.go +++ b/wgengine/netstack/netstack.go @@ -146,7 +146,6 @@ type Impl struct { } const nicID = 1 -const mtu = tstun.DefaultMTU // maxUDPPacketSize is the maximum size of a UDP packet we copy in startPacketCopy // when relaying UDP packets. We don't use the 'mtu' const in anticipation of @@ -179,7 +178,7 @@ func Create(logf logger.Logf, tundev *tstun.Wrapper, e wgengine.Engine, mc *magi if tcpipErr != nil { return nil, fmt.Errorf("could not enable TCP SACK: %v", tcpipErr) } - linkEP := channel.New(512, mtu, "") + linkEP := channel.New(512, tstun.DefaultMTU(), "") if tcpipProblem := ipstack.CreateNIC(nicID, linkEP); tcpipProblem != nil { return nil, fmt.Errorf("could not create netstack NIC: %v", tcpipProblem) } @@ -1078,7 +1077,7 @@ func (ns *Impl) acceptUDP(r *udp.ForwarderRequest) { func (ns *Impl) handleMagicDNSUDP(srcAddr netip.AddrPort, c *gonet.UDPConn) { // In practice, implementations are advised not to exceed 512 bytes // due to fragmenting. Just to be sure, we bump all the way to the MTU. - const maxUDPReqSize = mtu + var maxUDPReqSize = tstun.DefaultMTU() // Packets are being generated by the local host, so there should be // very, very little latency. 150ms was chosen as something of an upper // bound on resource usage, while hopefully still being long enough for diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go index c1c71a975..1cd01eee1 100644 --- a/wgengine/router/ifconfig_windows.go +++ b/wgengine/router/ifconfig_windows.go @@ -241,7 +241,7 @@ func interfaceFromLUID(luid winipcfg.LUID, flags winipcfg.GAAFlags) (*winipcfg.I var networkCategoryWarning = health.NewWarnable(health.WithMapDebugFlag("warn-network-category-unhealthy")) func configureInterface(cfg *Config, tun *tun.NativeTun) (retErr error) { - const mtu = tstun.DefaultMTU + var mtu = tstun.DefaultMTU() luid := winipcfg.LUID(tun.LUID()) iface, err := interfaceFromLUID(luid, // Issue 474: on early boot, when the network is still