diff --git a/wgengine/router/osrouter/router_linux.go b/wgengine/router/osrouter/router_linux.go index 3cd789833..372e29282 100644 --- a/wgengine/router/osrouter/router_linux.go +++ b/wgengine/router/osrouter/router_linux.go @@ -13,6 +13,7 @@ import ( "net/netip" "os" "os/exec" + "path/filepath" "strconv" "strings" "sync" @@ -74,9 +75,21 @@ type linuxRouter struct { // Various feature checks for the network stack. ipRuleAvailable bool // whether kernel was built with IP_MULTIPLE_TABLES - v6Available bool // whether the kernel supports IPv6 fwmaskWorksLazy opt.Bool // whether we can use 'ip rule...fwmark /'; set lazily + // interfaceV6Usable reports whether the kernel has IPv6 enabled on the + // tunnel interface specifically (distinct from global IPv6 support, + // which the netfilter runner tracks). Always set: the constructor wires + // it to interfaceV6UsableForTun; tests override it. See #20447. + interfaceV6Usable func() bool + + // interfaceV6UsableMemo memoizes interfaceV6Usable for the duration of a + // single Set, so its many getV6Available calls don't each hit /proc. It's + // an atomic tri-state (see the memoV6 constants) because getV6Available is + // also reached, without holding mu, from the onIPRuleDeleted timer's + // justAddIPRules; the unset value there means "read live". See #20447. + interfaceV6UsableMemo atomic.Int32 + // ipPolicyPrefBase is the base priority at which ip rules are installed. ipPolicyPrefBase int @@ -124,6 +137,7 @@ func newUserspaceRouterAdvanced(logf logger.Logf, tunname string, netMon *netmon ipRuleFixLimiter: rate.NewLimiter(rate.Every(5*time.Second), 10), ipPolicyPrefBase: 5200, } + r.interfaceV6Usable = func() bool { return interfaceV6UsableForTun(r.tunname) } ec := bus.Client("router-linux") r.rulesAddedPub = eventbus.Publish[AddIPRules](ec) eventbus.SubscribeFunc(ec, func(rs netmon.RuleDeleted) { @@ -172,8 +186,6 @@ func newUserspaceRouterAdvanced(logf logger.Logf, tunname string, netMon *netmon r.logf("mwan3 on openWRT detected, switching policy base priority to 1300") } - r.v6Available = linuxfw.CheckIPv6(r.logf) == nil - r.fixupWSLMTU() return r, nil @@ -421,6 +433,11 @@ func (r *linuxRouter) setupNetfilterLocked(kind string) error { func (r *linuxRouter) Set(cfg *router.Config) error { r.mu.Lock() defer r.mu.Unlock() + // Memoize the tun's IPv6 usability for the duration of this Set so the + // per-address/route getV6Available calls don't each re-read /proc. + // snapshotV6Usable runs now; the closure it returns (trailing ()) is + // deferred to clear the memo on return. + defer r.snapshotV6Usable()() var errs []error if cfg == nil { cfg = &shutdownConfig @@ -893,11 +910,72 @@ func (r *linuxRouter) getV6FilteringAvailable() bool { // getV6Available reports whether the router can manage IPv6. r.nfr can be nil if // setupNetfilterLocked failed earlier in Set (which continues on error), so // treat a nil runner as no IPv6 rather than dereferencing it. +// +// It requires both global IPv6 support (the netfilter runner) and IPv6 on the +// tun interface itself, which can differ: the kernel may refuse IPv6 on the tun +// while global IPv6 is fine. The per-interface check consults /proc, so within +// a single Set (which calls this once per address and route) the result is +// snapshotted by snapshotV6Usable rather than re-read each time. See #20447. func (r *linuxRouter) getV6Available() bool { if r.nfr == nil { return false } - return r.nfr.HasIPV6() + switch memoV6(r.interfaceV6UsableMemo.Load()) { + case memoV6Usable: + return r.nfr.HasIPV6() + case memoV6Unusable: + return false + default: // memoV6Unset: no snapshot active, read live. + return r.nfr.HasIPV6() && r.interfaceV6Usable() + } +} + +// memoV6 is the state of a linuxRouter.interfaceV6UsableMemo snapshot. +type memoV6 int32 + +const ( + memoV6Unset memoV6 = iota // no snapshot active; read live + memoV6Usable // snapshot: IPv6 usable on the tun interface + memoV6Unusable // snapshot: IPv6 not usable on the tun interface +) + +// snapshotV6Usable memoizes interfaceV6Usable for the duration of a Set, so +// its many getV6Available calls don't each hit /proc. It returns a function +// that clears the snapshot, intended to be deferred. The snapshot is taken +// once: an IPv6-enabled flip concurrent with a single Set is rare and, since a +// stray v6 operation is no longer fatal, harmless until the next Set. See +// #20447. +func (r *linuxRouter) snapshotV6Usable() func() { + m := memoV6Unusable + if r.interfaceV6Usable() { + m = memoV6Usable + } + r.interfaceV6UsableMemo.Store(int32(m)) + return func() { r.interfaceV6UsableMemo.Store(int32(memoV6Unset)) } +} + +// interfaceV6UsableForTun reports whether the kernel has IPv6 enabled on the +// named interface. The kernel creates /proc/sys/net/ipv6/conf// only +// once IPv6 is up on the interface (e.g. an MTU below the 1280-byte IPv6 +// minimum removes it entirely), and disable_ipv6 within it reflects whether +// IPv6 has since been turned off explicitly. +func interfaceV6UsableForTun(tunname string) bool { + if tunname == "" { + return false + } + bs, err := os.ReadFile(filepath.Join("/proc/sys/net/ipv6/conf", tunname, "disable_ipv6")) + if err != nil { + // A missing directory/knob means IPv6 isn't up on the interface, so + // it's unavailable. Any other error (e.g. EACCES) means the knob + // exists but we couldn't read it; assume IPv6 is usable rather than + // skipping it on a transient error. + return !os.IsNotExist(err) + } + disabled, err := strconv.ParseBool(strings.TrimSpace(string(bs))) + if err != nil { + return true // unparseable; assume usable + } + return !disabled } // addAddress adds an IP/mask to the tunnel interface. Fails if the diff --git a/wgengine/router/osrouter/router_linux_test.go b/wgengine/router/osrouter/router_linux_test.go index 9c74eceba..eff59ada6 100644 --- a/wgengine/router/osrouter/router_linux_test.go +++ b/wgengine/router/osrouter/router_linux_test.go @@ -525,6 +525,10 @@ v6/nat/ts-postrouting -m mark --mark 0x40000/0xff0000 -j MASQUERADE ht := health.NewTracker(bus) router, err := newUserspaceRouterAdvanced(t.Logf, "tailscale0", mon, fake, ht, bus) router.(*linuxRouter).nfr = fake.nfr + // Don't consult the live /proc for the tun's IPv6 state in tests; the + // fake netfilter runner's HasIPV6 (noV6) is the authoritative v6 signal + // here. See #20447. + router.(*linuxRouter).interfaceV6Usable = func() bool { return true } if err != nil { t.Fatalf("failed to create router: %v", err) } @@ -1614,6 +1618,9 @@ func TestSetSkipsNetfilterAddonsWhenSetupFails(t *testing.T) { } lr := r.(*linuxRouter) lr.nfr = nfr + // Keep the fake netfilter runner's HasIPV6 the sole v6 signal; don't + // consult the live /proc for the tun's IPv6 state. See #20447. + lr.interfaceV6Usable = func() bool { return true } if err := lr.Up(); err != nil { t.Fatalf("Up: %v", err) } @@ -1661,6 +1668,9 @@ func newTestLinuxRouter(t *testing.T) (*linuxRouter, *fakeOS) { } lr := r.(*linuxRouter) lr.nfr = fake.nfr + // Keep the fake netfilter runner's HasIPV6 (noV6) the sole v6 signal; + // don't consult the live /proc for the tun's IPv6 state. See #20447. + lr.interfaceV6Usable = func() bool { return true } if err := lr.Up(); err != nil { t.Fatalf("Up: %v", err) } @@ -1933,3 +1943,57 @@ func TestSetSkipsV6OrphansWhenV6Unavailable(t *testing.T) { t.Errorf("v6 orphan should be left alone when v6 is unavailable; ips=%q", fake.ips) } } + +// TestSetSkipsV6WhenInterfaceV6Unusable verifies that when global IPv6 is +// available (the netfilter runner reports HasIPV6) but the kernel has not +// enabled IPv6 on the tunnel interface itself -- e.g. the tun MTU is below the +// 1280-byte IPv6 minimum -- Set skips the v6 address rather than failing. This +// is the #20447 scenario: historically the v6 addAddress errored, aborting the +// whole router pass, which in turn suppressed DNS configuration. +func TestSetSkipsV6WhenInterfaceV6Unusable(t *testing.T) { + lr, fake := newTestLinuxRouter(t) + // Global v6 is fine (fake nfr HasIPV6 defaults true), but the tun + // interface has no usable v6. + lr.interfaceV6Usable = func() bool { return false } + + cfg := &Config{ + LocalAddrs: mustCIDRs("100.64.0.1/32", "fd7a:115c:a1e0::1/128"), + NetfilterMode: netfilterOff, + } + if err := lr.Set(cfg); err != nil { + t.Fatalf("Set: %v", err) + } + + // The v4 address is programmed; the v6 address is skipped rather than + // attempted (which would have errored on a v6-less interface). + if !slices.Contains(fake.ips, "100.64.0.1/32 dev tailscale0") { + t.Errorf("v4 address was not programmed; ips=%q", fake.ips) + } + if slices.Contains(fake.ips, "fd7a:115c:a1e0::1/128 dev tailscale0") { + t.Errorf("v6 address should be skipped when the interface has no usable v6; ips=%q", fake.ips) + } +} + +// TestSetSnapshotsV6Usable verifies that a single Set consults the +// per-interface IPv6 check (which reads /proc) once, rather than once per +// address and route, even though many getV6Available calls happen within it. +func TestSetSnapshotsV6Usable(t *testing.T) { + lr, _ := newTestLinuxRouter(t) + var calls int + lr.interfaceV6Usable = func() bool { + calls++ + return true + } + + cfg := &Config{ + LocalAddrs: mustCIDRs("100.64.0.1/32", "fd7a:115c:a1e0::1/128"), + Routes: mustCIDRs("10.0.0.0/8", "fd00::/8"), + NetfilterMode: netfilterOff, + } + if err := lr.Set(cfg); err != nil { + t.Fatalf("Set: %v", err) + } + if calls != 1 { + t.Errorf("interfaceV6Usable called %d times during one Set; want 1 (snapshotted)", calls) + } +} diff --git a/wgengine/userspace.go b/wgengine/userspace.go index e2b1b85c0..c049e0e9b 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -884,12 +884,15 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, // per peer by [Engine.SyncDevicePeer]), and its private key is set // above when it changes. + // A router.Set error is recorded but must not abort the reconfig: DNS + // configuration below must be attempted independently. See #20447. + var routerErr error if routerChanged { e.logf("wgengine: Reconfig: configuring router") - err := e.router.Set(routerCfg) - e.health.SetRouterHealth(err) - if err != nil { - return err + routerErr = e.router.Set(routerCfg) + e.health.SetRouterHealth(routerErr) + if routerErr != nil { + e.logf("wgengine: Reconfig: router config failed (%v); continuing to DNS config so name resolution still works", routerErr) } } @@ -902,6 +905,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, // TODO(bradfitz): try to do the "configuring DNS" part below only if // dnsChanged, not routerChanged. The "resolver.ShouldUseRoutes" part // probably needs to keep happening for both. + var dnsErr, vpnErr error if buildfeatures.HasDNS && (routerChanged || dnsChanged) { if resolver.ShouldUseRoutes(e.controlKnobs) { e.logf("wgengine: Reconfig: user dialer") @@ -914,31 +918,32 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, // DNS managers refuse to apply settings if the device has no // assigned address. e.logf("wgengine: Reconfig: configuring DNS") - err := e.dns.Set(*dnsCfg) - e.health.SetDNSHealth(err) - if err != nil { - return err - } - if err := e.reconfigureVPNIfNecessary(); err != nil { - return err + dnsErr = e.dns.Set(*dnsCfg) + e.health.SetDNSHealth(dnsErr) + if dnsErr == nil { + vpnErr = e.reconfigureVPNIfNecessary() } } - // Let the network flow logger finish reacting after the router is - // configured, so that a stopping logger captures final packets. + // Let the network flow logger finish reacting, pairing the Reconfig + // call near the top of this function. This runs regardless of router + // or DNS errors above: skipping it would leave a stopping logger + // running until the next successful reconfig. // This may block to flush pending log messages. if e.netlogger != nil { e.netlogger.ReconfigDone() } - // Let the BIRD integration apply any protocol state change now, - // after the router is configured. + // Let the BIRD integration apply any protocol state change computed by + // its Reconfig call above. As with the netlogger, this runs even if + // router/DNS config failed, so BIRD's protocol state still tracks the + // primary-subnet-router transition. if e.bird != nil { e.bird.ReconfigDone() } e.logf("[v1] wgengine: Reconfig done") - return nil + return errors.Join(routerErr, dnsErr, vpnErr) } func (e *userspaceEngine) GetFilter() *filter.Filter { diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index fc1c89403..a5569d329 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -4,6 +4,7 @@ package wgengine import ( + "errors" "fmt" "math/rand" "net/netip" @@ -116,6 +117,72 @@ func TestUserspaceEngineReconfig(t *testing.T) { } } +// failingRouter is a router.Router whose Set always fails, used to verify that +// DNS configuration is still attempted when router configuration fails. +type failingRouter struct { + err error +} + +func (failingRouter) Up() error { return nil } +func (r failingRouter) Set(*router.Config) error { return r.err } +func (failingRouter) Close() error { return nil } + +// recordingOSConfigurator is a dns.OSConfigurator that records whether SetDNS +// was called. +type recordingOSConfigurator struct { + setDNSCalled bool +} + +func (c *recordingOSConfigurator) SetDNS(dns.OSConfig) error { c.setDNSCalled = true; return nil } +func (c *recordingOSConfigurator) SupportsSplitDNS() bool { return false } +func (c *recordingOSConfigurator) Close() error { return nil } +func (c *recordingOSConfigurator) GetBaseConfig() (dns.OSConfig, error) { + return dns.OSConfig{}, dns.ErrGetBaseConfigNotSupported +} + +// TestUserspaceEngineReconfigDNSAfterRouterError verifies that a router.Set +// failure does not prevent DNS from being configured. Historically Reconfig +// returned on router error before dns.Set ran, so MagicDNS was never +// configured on hosts where router config failed on every reconfig. See +// tailscale/tailscale#20447. +func TestUserspaceEngineReconfigDNSAfterRouterError(t *testing.T) { + bus := eventbustest.NewBus(t) + + ht := health.NewTracker(bus) + reg := new(usermetric.Registry) + e, err := NewFakeUserspaceEngine(t.Logf, 0, ht, reg, bus) + if err != nil { + t.Fatal(err) + } + t.Cleanup(e.Close) + ue := e.(*userspaceEngine) + + routerErr := fmt.Errorf("router boom") + ue.router = failingRouter{err: routerErr} + + osCfg := &recordingOSConfigurator{} + ue.dns = dns.NewManager(t.Logf, osCfg, ht, ue.dialer, nil, nil, runtime.GOOS, bus) + + nm := &netmap.NetworkMap{ + Peers: nodeViews([]*tailcfg.Node{{ID: 1, Key: nkFromHex("aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa")}}), + } + cfg := &wgcfg.Config{ + Addresses: []netip.Prefix{netip.PrefixFrom(netaddr.IPv4(100, 100, 99, 1), 32)}, + } + e.SetSelfNode(nm.SelfNode) + + err = e.Reconfig(cfg, &router.Config{}, &dns.Config{}) + + if !osCfg.setDNSCalled { + t.Error("SetDNS was not called after router.Set failed; DNS config must be independent of router success") + } + if err == nil { + t.Error("Reconfig returned nil; want the router error to be surfaced") + } else if !errors.Is(err, routerErr) { + t.Errorf("Reconfig error = %v; want it to wrap the router error %v", err, routerErr) + } +} + func TestUserspaceEnginePortReconfig(t *testing.T) { flakytest.Mark(t, "https://github.com/tailscale/tailscale/issues/2855") const defaultPort = 49983