From 12d5c99b04dce9e8d5394e0d9f87ea502491de16 Mon Sep 17 00:00:00 2001 From: Jordan Whited Date: Thu, 9 Nov 2023 11:34:41 -0800 Subject: [PATCH] client/tailscale,ipn/{ipnlocal,localapi}: check UDP GRO config (#10071) Updates tailscale/corp#9990 Signed-off-by: Jordan Whited --- client/tailscale/localclient.go | 20 ++++++++ cmd/tailscale/cli/up.go | 9 +++- cmd/tailscaled/depaware.txt | 2 + go.mod | 1 + go.sum | 2 + ipn/ipnlocal/local.go | 40 ++++++++++++++++ ipn/localapi/localapi.go | 18 ++++++++ net/netkernelconf/netkernelconf.go | 5 ++ net/netkernelconf/netkernelconf_default.go | 12 +++++ net/netkernelconf/netkernelconf_linux.go | 54 ++++++++++++++++++++++ 10 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 net/netkernelconf/netkernelconf.go create mode 100644 net/netkernelconf/netkernelconf_default.go create mode 100644 net/netkernelconf/netkernelconf_linux.go diff --git a/client/tailscale/localclient.go b/client/tailscale/localclient.go index 4b61b3151..e5d42dd9c 100644 --- a/client/tailscale/localclient.go +++ b/client/tailscale/localclient.go @@ -679,6 +679,26 @@ func (lc *LocalClient) CheckIPForwarding(ctx context.Context) error { return nil } +// CheckUDPGROForwarding asks the local Tailscale daemon whether it looks like +// the machine is optimally configured to forward UDP packets as a subnet router +// or exit node. +func (lc *LocalClient) CheckUDPGROForwarding(ctx context.Context) error { + body, err := lc.get200(ctx, "/localapi/v0/check-udp-gro-forwarding") + if err != nil { + return err + } + var jres struct { + Warning string + } + if err := json.Unmarshal(body, &jres); err != nil { + return fmt.Errorf("invalid JSON from check-udp-gro-forwarding: %w", err) + } + if jres.Warning != "" { + return errors.New(jres.Warning) + } + return nil +} + // CheckPrefs validates the provided preferences, without making any changes. // // The CLI uses this before a Start call to fail fast if the preferences won't diff --git a/cmd/tailscale/cli/up.go b/cmd/tailscale/cli/up.go index 2a1fb3380..266dbd0f4 100644 --- a/cmd/tailscale/cli/up.go +++ b/cmd/tailscale/cli/up.go @@ -443,9 +443,16 @@ func runUp(ctx context.Context, cmd string, args []string, upArgs upArgsT) (retE } if len(prefs.AdvertiseRoutes) > 0 { - if err := localClient.CheckIPForwarding(context.Background()); err != nil { + // TODO(jwhited): compress CheckIPForwarding and CheckUDPGROForwarding + // into a single HTTP req. + if err := localClient.CheckIPForwarding(ctx); err != nil { warnf("%v", err) } + if runtime.GOOS == "linux" { + if err := localClient.CheckUDPGROForwarding(ctx); err != nil { + warnf("%v", err) + } + } } curPrefs, err := localClient.GetPrefs(ctx) diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 046d4f5c5..29174fd26 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -133,6 +133,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de github.com/pkg/errors from github.com/gorilla/csrf LD github.com/pkg/sftp from tailscale.com/ssh/tailssh LD github.com/pkg/sftp/internal/encoding/ssh/filexfer from github.com/pkg/sftp + L 💣 github.com/safchain/ethtool from tailscale.com/net/netkernelconf W 💣 github.com/tailscale/certstore from tailscale.com/control/controlclient W 💣 github.com/tailscale/go-winio from tailscale.com/safesocket W 💣 github.com/tailscale/go-winio/internal/fs from github.com/tailscale/go-winio @@ -278,6 +279,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/net/netaddr from tailscale.com/ipn+ tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock tailscale.com/net/neterror from tailscale.com/net/dns/resolver+ + tailscale.com/net/netkernelconf from tailscale.com/ipn/ipnlocal tailscale.com/net/netknob from tailscale.com/net/netns+ tailscale.com/net/netmon from tailscale.com/cmd/tailscaled+ tailscale.com/net/netns from tailscale.com/derp/derphttp+ diff --git a/go.mod b/go.mod index eab461c57..0f63d5ae0 100644 --- a/go.mod +++ b/go.mod @@ -56,6 +56,7 @@ require ( github.com/pkg/sftp v1.13.6 github.com/prometheus/client_golang v1.17.0 github.com/prometheus/common v0.44.0 + github.com/safchain/ethtool v0.3.0 github.com/skip2/go-qrcode v0.0.0-20200617195104-da1b6568686e github.com/tailscale/certstore v0.1.1-0.20231020161753-77811a65f4ff github.com/tailscale/depaware v0.0.0-20210622194025-720c4b409502 diff --git a/go.sum b/go.sum index 34640abe2..6cbfc70c7 100644 --- a/go.sum +++ b/go.sum @@ -783,6 +783,8 @@ github.com/ryancurrah/gomodguard v1.3.0 h1:q15RT/pd6UggBXVBuLps8BXRvl5GPBcwVA7BJ github.com/ryancurrah/gomodguard v1.3.0/go.mod h1:ggBxb3luypPEzqVtq33ee7YSN35V28XeGnid8dnni50= github.com/ryanrolds/sqlclosecheck v0.4.0 h1:i8SX60Rppc1wRuyQjMciLqIzV3xnoHB7/tXbr6RGYNI= github.com/ryanrolds/sqlclosecheck v0.4.0/go.mod h1:TBRRjzL31JONc9i4XMinicuo+s+E8yKZ5FN8X3G6CKQ= +github.com/safchain/ethtool v0.3.0 h1:gimQJpsI6sc1yIqP/y8GYgiXn/NjgvpM0RNoWLVVmP0= +github.com/safchain/ethtool v0.3.0/go.mod h1:SA9BwrgyAqNo7M+uaL6IYbxpm5wk3L7Mm6ocLW+CJUs= github.com/sanposhiho/wastedassign/v2 v2.0.7 h1:J+6nrY4VW+gC9xFzUc+XjPD3g3wF3je/NsJFwFK7Uxc= github.com/sanposhiho/wastedassign/v2 v2.0.7/go.mod h1:KyZ0MWTwxxBmfwn33zh3k1dmsbF2ud9pAAGfoLfjhtI= github.com/sashamelentyev/interfacebloat v1.1.0 h1:xdRdJp0irL086OyW1H/RTZTr1h/tMEOsumirXcOJqAw= diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 3654faa87..b48c23773 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -54,6 +54,7 @@ import ( "tailscale.com/net/dnscache" "tailscale.com/net/dnsfallback" "tailscale.com/net/interfaces" + "tailscale.com/net/netkernelconf" "tailscale.com/net/netmon" "tailscale.com/net/netns" "tailscale.com/net/netutil" @@ -4871,6 +4872,45 @@ func (b *LocalBackend) CheckIPForwarding() error { return warn } +// CheckUDPGROForwarding checks if the machine is optimally configured to +// forward UDP packets between the default route and Tailscale TUN interfaces. +// It returns an error if the check fails or if suboptimal configuration is +// detected. No error is returned if we are unable to gather the interface +// names from the relevant subsystems. +func (b *LocalBackend) CheckUDPGROForwarding() error { + if b.sys.IsNetstackRouter() { + return nil + } + // We return nil when the interface name or subsystem it's tied to can't be + // fetched. This is intentional as answering the question "are netdev + // features optimal for performance?" is a low priority in that situation. + tunSys, ok := b.sys.Tun.GetOK() + if !ok { + return nil + } + tunInterface, err := tunSys.Name() + if err != nil { + return nil + } + netmonSys, ok := b.sys.NetMon.GetOK() + if !ok { + return nil + } + state := netmonSys.InterfaceState() + if state == nil { + return nil + } + // We return warn or err. If err is non-nil there was a problem + // communicating with the kernel via ethtool semantics/ioctl. ethtool ioctl + // errors are interesting for our future selves as we consider tweaking + // netdev features automatically using similar API infra. + warn, err := netkernelconf.CheckUDPGROForwarding(tunInterface, state.DefaultRouteInterface) + if err != nil { + return err + } + return warn +} + // DERPMap returns the current DERPMap in use, or nil if not connected. func (b *LocalBackend) DERPMap() *tailcfg.DERPMap { b.mu.Lock() diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 4137d89e2..a0066f17c 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -71,6 +71,7 @@ var handler = map[string]localAPIHandler{ // without a trailing slash: "bugreport": (*Handler).serveBugReport, "check-ip-forwarding": (*Handler).serveCheckIPForwarding, + "check-udp-gro-forwarding": (*Handler).serveCheckUDPGROForwarding, "check-prefs": (*Handler).serveCheckPrefs, "component-debug-logging": (*Handler).serveComponentDebugLogging, "debug": (*Handler).serveDebug, @@ -983,6 +984,23 @@ func (h *Handler) serveCheckIPForwarding(w http.ResponseWriter, r *http.Request) }) } +func (h *Handler) serveCheckUDPGROForwarding(w http.ResponseWriter, r *http.Request) { + if !h.PermitRead { + http.Error(w, "UDP GRO forwarding check access denied", http.StatusForbidden) + return + } + var warning string + if err := h.b.CheckUDPGROForwarding(); err != nil { + warning = err.Error() + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(struct { + Warning string + }{ + Warning: warning, + }) +} + func (h *Handler) serveStatus(w http.ResponseWriter, r *http.Request) { if !h.PermitRead { http.Error(w, "status access denied", http.StatusForbidden) diff --git a/net/netkernelconf/netkernelconf.go b/net/netkernelconf/netkernelconf.go new file mode 100644 index 000000000..3ea502b37 --- /dev/null +++ b/net/netkernelconf/netkernelconf.go @@ -0,0 +1,5 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +// Package netkernelconf contains code for checking kernel netdev config. +package netkernelconf diff --git a/net/netkernelconf/netkernelconf_default.go b/net/netkernelconf/netkernelconf_default.go new file mode 100644 index 000000000..644ec1252 --- /dev/null +++ b/net/netkernelconf/netkernelconf_default.go @@ -0,0 +1,12 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +//go:build !linux + +package netkernelconf + +// CheckUDPGROForwarding is unimplemented for non-Linux platforms. Refer to the +// docstring in _linux.go. +func CheckUDPGROForwarding(tunInterface, defaultRouteInterface string) (warn, err error) { + return nil, nil +} diff --git a/net/netkernelconf/netkernelconf_linux.go b/net/netkernelconf/netkernelconf_linux.go new file mode 100644 index 000000000..6ea479e8d --- /dev/null +++ b/net/netkernelconf/netkernelconf_linux.go @@ -0,0 +1,54 @@ +// Copyright (c) Tailscale Inc & AUTHORS +// SPDX-License-Identifier: BSD-3-Clause + +package netkernelconf + +import ( + "fmt" + + "github.com/safchain/ethtool" +) + +// CheckUDPGROForwarding checks if the machine is optimally configured to +// forward UDP packets between the default route and Tailscale TUN interfaces. +// It returns a non-nil warn in the case that the configuration is suboptimal. +// It returns a non-nil err in the case that an error is encountered while +// performing the check. +func CheckUDPGROForwarding(tunInterface, defaultRouteInterface string) (warn, err error) { + const txFeature = "tx-udp-segmentation" + const rxWantFeature = "rx-udp-gro-forwarding" + const rxDoNotWantFeature = "rx-gro-list" + const kbLink = "\nSee https://tailscale.com/s/ethtool-config-udp-gro" + errWithPrefix := func(format string, a ...any) error { + const errPrefix = "couldn't check system's UDP GRO forwarding configuration, " + return fmt.Errorf(errPrefix+format, a...) + } + e, err := ethtool.NewEthtool() + if err != nil { + return nil, errWithPrefix("failed to init ethtool: %v", err) + } + defer e.Close() + tunFeatures, err := e.Features(tunInterface) + if err != nil { + return nil, errWithPrefix("failed to retrieve TUN device features: %v", err) + } + if !tunFeatures[txFeature] { + // if txFeature is disabled/nonexistent on the TUN then UDP GRO + // forwarding doesn't matter, we won't be taking advantage of it. + return nil, nil + } + defaultFeatures, err := e.Features(defaultRouteInterface) + if err != nil { + return nil, errWithPrefix("failed to retrieve default route interface features: %v", err) + } + defaultHasRxWant, ok := defaultFeatures[rxWantFeature] + if !ok { + // unlikely the feature is nonexistant with txFeature in the TUN driver + // being added to the kernel later than rxWantFeature, but let's be sure + return nil, nil + } + if !defaultHasRxWant || defaultFeatures[rxDoNotWantFeature] { + return fmt.Errorf("UDP GRO forwarding is suboptimally configured on %s, UDP forwarding throughput capability will increase with a configuration change.%s", defaultRouteInterface, kbLink), nil + } + return nil, nil +}