wgengine/router/osrouter: skip netfilter add-ons when chain setup fails (#19757)

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

Signed-off-by: Fernando Serboncini <fserb@tailscale.com>

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

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)
+	}
+}