From 666f0d28208a60c96fa2ac2a0be848b591abb70b Mon Sep 17 00:00:00 2001 From: Fernando Serboncini Date: Fri, 15 May 2026 09:34:44 -0400 Subject: [PATCH] wgengine/router/osrouter: skip netfilter add-ons when chain setup fails (#19761) linuxRouter has two blocks (connmark rules and the CGNAT drop rule) that gate on cfg.NetfilterMode, the requested config state. This may cause an error when setNetfilterModeLocked fails, since it may keep assuming this config is valid. We now gate both blocks on r.netfilterMode, matching the pattern used by SNAT, stateful, and loopback paths. Fixes #19737 Change-Id: Ia6003a082db99c376e662132d725661afbac0ee9 (cherry picked from commit 20b814893b2dbf42793fe18e6dfff4413d0c4ee2) Signed-off-by: Fernando Serboncini --- wgengine/router/osrouter/router_linux.go | 8 ++- wgengine/router/osrouter/router_linux_test.go | 59 +++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) diff --git a/wgengine/router/osrouter/router_linux.go b/wgengine/router/osrouter/router_linux.go index 41a2a7b90..73f65cdf1 100644 --- a/wgengine/router/osrouter/router_linux.go +++ b/wgengine/router/osrouter/router_linux.go @@ -490,7 +490,9 @@ func (r *linuxRouter) Set(cfg *router.Config) error { // Connmark rules for rp_filter compatibility. // Always enabled when netfilter is ON to handle all rp_filter=1 scenarios // (normal operation, exit nodes, subnet routers, and clients using exit nodes). - netfilterOn := cfg.NetfilterMode == netfilterOn + // Gate on r.netfilterMode (actual state) rather than cfg.NetfilterMode + // (desired state) so we don't call into the runner when chain setup failed. + netfilterOn := r.netfilterMode == netfilterOn switch { case netfilterOn == r.connmarkEnabled: // state already correct, nothing to do. @@ -530,8 +532,8 @@ func (r *linuxRouter) Set(cfg *router.Config) error { r.enableIPForwarding() } - // Remove the rule to drop off-tailnet CGNAT traffic, if asked. - if netfilterOn || cfg.NetfilterMode == netfilterNoDivert { + // Remove the rule to drop off-tailnet CGNAT traffic, if needed. + if netfilterOn || r.netfilterMode == netfilterNoDivert { var cgnatMode linuxfw.CGNATMode if cfg.RemoveCGNATDropRule { cgnatMode = linuxfw.CGNATModeReturn diff --git a/wgengine/router/osrouter/router_linux_test.go b/wgengine/router/osrouter/router_linux_test.go index 8830822e0..340ebb148 100644 --- a/wgengine/router/osrouter/router_linux_test.go +++ b/wgengine/router/osrouter/router_linux_test.go @@ -562,6 +562,10 @@ type fakeIPTablesRunner struct { ipt4 map[string][]string ipt6 map[string][]string // we always assume ipv6 and ipv6 nat are enabled when testing + + addChainsErr error // if non-nil, AddChains returns it instead of setting up chains + addConnmarkSaveCalls int + addExternalCGNATCalls int } func newIPTablesRunner(t *testing.T) linuxfw.NetfilterRunner { @@ -794,6 +798,9 @@ func (n *fakeIPTablesRunner) DelHooks(logf logger.Logf) error { } func (n *fakeIPTablesRunner) AddChains() error { + if n.addChainsErr != nil { + return n.addChainsErr + } for _, ipt := range []map[string][]string{n.ipt4, n.ipt6} { for _, chain := range []string{"filter/ts-input", "filter/ts-forward", "nat/ts-postrouting"} { ipt[chain] = nil @@ -922,6 +929,7 @@ func (n *fakeIPTablesRunner) DelMagicsockPortRule(port uint16, network string) e } func (n *fakeIPTablesRunner) AddConnmarkSaveRule() error { + n.addConnmarkSaveCalls++ // PREROUTING rule: restore mark from conntrack prerouteRule := "-m conntrack --ctstate ESTABLISHED,RELATED -j CONNMARK --restore-mark --nfmask 0xff0000 --ctmask 0xff0000" for _, ipt := range []map[string][]string{n.ipt4, n.ipt6} { @@ -970,6 +978,7 @@ func buildExternalCGNATRules(mode linuxfw.CGNATMode, tunname string) ([]iptRule, } func (n *fakeIPTablesRunner) AddExternalCGNATRules(mode linuxfw.CGNATMode, tunname string) error { + n.addExternalCGNATCalls++ rules, err := buildExternalCGNATRules(mode, tunname) if err != nil { return err @@ -1550,3 +1559,53 @@ func TestUpdateMagicsockPortChange(t *testing.T) { oldPortRule, nfr.ipt4["filter/ts-input"]) } } + +// TestSetSkipsNetfilterAddonsWhenSetupFails verifies that Set does not invoke +// rule-management methods that depend on the ts-* chains existing when chain +// setup failed. +func TestSetSkipsNetfilterAddonsWhenSetupFails(t *testing.T) { + nfr := newIPTablesRunner(t).(*fakeIPTablesRunner) + nfr.addChainsErr = errors.New("kernel lacks netfilter support") + + bus := eventbus.New() + defer bus.Close() + mon, err := netmon.New(bus, logger.Discard) + if err != nil { + t.Fatal(err) + } + mon.Start() + defer mon.Close() + + fake := NewFakeOS(t) + ht := health.NewTracker(bus) + r, err := newUserspaceRouterAdvanced(logger.Discard, "tailscale0", mon, fake, ht, bus) + if err != nil { + t.Fatalf("newUserspaceRouterAdvanced: %v", err) + } + lr := r.(*linuxRouter) + lr.nfr = nfr + if err := lr.Up(); err != nil { + t.Fatalf("Up: %v", err) + } + defer lr.Close() + + cfg := &Config{ + LocalAddrs: mustCIDRs("100.101.102.103/10"), + NetfilterMode: netfilterOn, + } + // Set must return an error (chain setup failed) but must not panic. + if err := lr.Set(cfg); err == nil { + t.Fatal("Set returned nil; want error because AddChains failed") + } + if lr.netfilterMode != netfilterOff { + t.Errorf("netfilterMode = %v; want netfilterOff after failed AddChains", lr.netfilterMode) + } + if nfr.addConnmarkSaveCalls != 0 { + t.Errorf("AddConnmarkSaveRule called %d times; want 0 when chain setup failed", + nfr.addConnmarkSaveCalls) + } + if nfr.addExternalCGNATCalls != 0 { + t.Errorf("AddExternalCGNATRules called %d times; want 0 when chain setup failed", + nfr.addExternalCGNATCalls) + } +}