diff --git a/client/firewall/iptables/chains_linux.go b/client/firewall/iptables/chains_linux.go index 65f921576..58bfa8c6a 100644 --- a/client/firewall/iptables/chains_linux.go +++ b/client/firewall/iptables/chains_linux.go @@ -61,21 +61,21 @@ func (r *family) createContainers() error { func (r *family) addJumpRules() error { // Jump to nat chain - natRule := []string{"-j", chainRTNAT} + natRule := jumpRuleSpec(chainRTNAT) if err := r.iptablesClient.Insert(tableNat, chainPostrouting, 1, natRule...); err != nil { return fmt.Errorf("add nat postrouting jump rule: %w", err) } r.rules[jumpNATPost] = natRule // Jump to mangle prerouting chain - preRule := []string{"-j", chainRTPre} + preRule := jumpRuleSpec(chainRTPre) if err := r.iptablesClient.Insert(tableMangle, chainPrerouting, 1, preRule...); err != nil { return fmt.Errorf("add mangle prerouting jump rule: %w", err) } r.rules[jumpManglePre] = preRule // Jump to nat prerouting chain - rdrRule := []string{"-j", chainRTRdr} + rdrRule := jumpRuleSpec(chainRTRdr) if err := r.iptablesClient.Insert(tableNat, chainPrerouting, 1, rdrRule...); err != nil { return fmt.Errorf("add nat prerouting jump rule: %w", err) } @@ -162,7 +162,7 @@ func (r *family) appendToEntries(chain chainKey, spec ruleSpec) { } func (r *family) createDefaultChains() error { - if err := r.iptablesClient.NewChain(tableName, chainACLInput); err != nil { + if err := r.iptablesClient.NewChain(tableFilter, chainACLInput); err != nil { return fmt.Errorf("create %s chain: %w", chainACLInput, err) } @@ -172,7 +172,7 @@ func (r *family) createDefaultChains() error { continue } for _, rule := range rules { - if err := r.iptablesClient.InsertUnique(tableName, string(chain), 1, rule...); err != nil { + if err := r.iptablesClient.InsertUnique(tableFilter, string(chain), 1, rule...); err != nil { return fmt.Errorf("insert jump rule into %s: %w", chain, err) } } @@ -180,7 +180,7 @@ func (r *family) createDefaultChains() error { for chain, entries := range r.optionalEntries { for _, entry := range entries { - if err := r.iptablesClient.InsertUnique(tableName, string(chain), entry.position, entry.spec...); err != nil { + if err := r.iptablesClient.InsertUnique(tableFilter, string(chain), entry.position, entry.spec...); err != nil { log.Errorf("failed to insert optional entry %v: %v", entry.spec, err) continue } @@ -237,21 +237,32 @@ func (r *family) cleanUpDefaultForwardRules() error { } func (r *family) cleanJumpRules() error { - // locations maps each tracked jump rule to the built-in table and - // chain it was inserted into. - locations := map[firewall.RuleID]struct{ table, chain string }{ - jumpNATPost: {tableNat, chainPostrouting}, - jumpManglePre: {tableMangle, chainPrerouting}, - jumpNATPre: {tableNat, chainPrerouting}, - jumpMSSClamp: {tableMangle, chainForward}, - jumpNATOutput: {tableNat, chainOutput}, + // locations maps each jump rule to the built-in table and chain it + // was inserted into, plus the netbird chain it targets. + locations := map[firewall.RuleID]struct{ table, chain, target string }{ + jumpNATPost: {tableNat, chainPostrouting, chainRTNAT}, + jumpManglePre: {tableMangle, chainPrerouting, chainRTPre}, + jumpNATPre: {tableNat, chainPrerouting, chainRTRdr}, + jumpMSSClamp: {tableMangle, chainForward, chainRTMSSClamp}, + jumpNATOutput: {tableNat, chainOutput, chainNATOutput}, } var merr *multierror.Error for ruleID, loc := range locations { rule, exists := r.rules[ruleID] if !exists { - continue + // Untracked (e.g. fresh start after an unclean shutdown with no + // restored state): if the target chain survived, remove the stale + // jump to it so the chain can be deleted. + ok, err := r.iptablesClient.ChainExists(loc.table, loc.target) + if err != nil { + merr = multierror.Append(merr, fmt.Errorf("check chain %s in table %s: %w", loc.target, loc.table, err)) + continue + } + if !ok { + continue + } + rule = jumpRuleSpec(loc.target) } if err := r.iptablesClient.DeleteIfExists(loc.table, loc.chain, rule...); err != nil { merr = multierror.Append(merr, fmt.Errorf("delete rule from chain %s in table %s: %w", loc.chain, loc.table, err)) @@ -262,6 +273,12 @@ func (r *family) cleanJumpRules() error { return nberrors.FormatErrorOrNil(merr) } +// jumpRuleSpec builds the iptables rule spec that jumps to target. Create +// and cleanup sites share it so the installed and deleted specs cannot drift. +func jumpRuleSpec(target string) []string { + return []string{"-j", target} +} + func (r *family) cleanAclChains() error { var merr *multierror.Error @@ -269,10 +286,6 @@ func (r *family) cleanAclChains() error { merr = multierror.Append(merr, err) } - if err := r.cleanPreroutingEntries(); err != nil { - merr = multierror.Append(merr, err) - } - for _, rule := range r.entries[mangleForwardKey] { if err := r.iptablesClient.DeleteIfExists(tableMangle, chainForward, rule...); err != nil { merr = multierror.Append(merr, fmt.Errorf("delete mangle %s guard rule %v: %w", chainForward, rule, err)) @@ -283,7 +296,7 @@ func (r *family) cleanAclChains() error { } func (r *family) cleanInputAclChain() error { - ok, err := r.iptablesClient.ChainExists(tableName, chainACLInput) + ok, err := r.iptablesClient.ChainExists(tableFilter, chainACLInput) if err != nil { return fmt.Errorf("check chain %s: %w", chainACLInput, err) } @@ -293,43 +306,24 @@ func (r *family) cleanInputAclChain() error { var merr *multierror.Error for _, rule := range r.entries[chainInput] { - if err := r.iptablesClient.DeleteIfExists(tableName, chainInput, rule...); err != nil { + if err := r.iptablesClient.DeleteIfExists(tableFilter, chainInput, rule...); err != nil { merr = multierror.Append(merr, fmt.Errorf("delete %s rule %v: %w", chainInput, rule, err)) } } for _, rule := range r.entries[chainForward] { - if err := r.iptablesClient.DeleteIfExists(tableName, chainForward, rule...); err != nil { + if err := r.iptablesClient.DeleteIfExists(tableFilter, chainForward, rule...); err != nil { merr = multierror.Append(merr, fmt.Errorf("delete %s rule %v: %w", chainForward, rule, err)) } } - if err := r.iptablesClient.ClearAndDeleteChain(tableName, chainACLInput); err != nil { + if err := r.iptablesClient.ClearAndDeleteChain(tableFilter, chainACLInput); err != nil { merr = multierror.Append(merr, fmt.Errorf("clear and delete %s chain: %w", chainACLInput, err)) } return nberrors.FormatErrorOrNil(merr) } -func (r *family) cleanPreroutingEntries() error { - ok, err := r.iptablesClient.ChainExists(tableMangle, chainPrerouting) - if err != nil { - return fmt.Errorf("check chain %s in %s: %w", chainPrerouting, tableMangle, err) - } - if !ok { - return nil - } - - var merr *multierror.Error - for _, rule := range r.entries[chainPrerouting] { - if err := r.iptablesClient.DeleteIfExists(tableMangle, chainPrerouting, rule...); err != nil { - merr = multierror.Append(merr, fmt.Errorf("delete %s rule %v: %w", chainPrerouting, rule, err)) - } - } - - return nberrors.FormatErrorOrNil(merr) -} - func (r *family) cleanupDataPlaneMark() error { var merr *multierror.Error if preRule, exists := r.rules[markManglePre]; exists { diff --git a/client/firewall/iptables/dnat_linux.go b/client/firewall/iptables/dnat_linux.go index ad7b83b85..ced6ae6b1 100644 --- a/client/firewall/iptables/dnat_linux.go +++ b/client/firewall/iptables/dnat_linux.go @@ -225,7 +225,7 @@ func (r *family) ensureNATOutputChain() error { } } - jumpRule := []string{"-j", chainNATOutput} + jumpRule := jumpRuleSpec(chainNATOutput) if err := r.iptablesClient.Insert(tableNat, chainOutput, 1, jumpRule...); err != nil { if !chainExists { if delErr := r.iptablesClient.ClearAndDeleteChain(tableNat, chainNATOutput); delErr != nil { diff --git a/client/firewall/iptables/family_linux.go b/client/firewall/iptables/family_linux.go index fa57ce25b..7276acc5d 100644 --- a/client/firewall/iptables/family_linux.go +++ b/client/firewall/iptables/family_linux.go @@ -22,7 +22,6 @@ import ( // constants needed to manage and create iptable rules const ( tableFilter = "filter" - tableName = tableFilter tableNat = "nat" tableMangle = "mangle" diff --git a/client/firewall/iptables/filter_linux.go b/client/firewall/iptables/filter_linux.go index 852d31db0..5fc578700 100644 --- a/client/firewall/iptables/filter_linux.go +++ b/client/firewall/iptables/filter_linux.go @@ -96,9 +96,14 @@ func (r *family) DeleteFilterRule(rule firewall.Rule) error { return nberrors.FormatErrorOrNil(merr) } - r.dropSourceMatch(pr.specs) + // The rule is gone from iptables, so untrack it regardless of how the + // refcount decrement goes, but surface decrement failures so callers + // see the ipset desync. delete(r.filters, ruleID) r.updateState() + if err := r.decrementSetCounter(pr.specs); err != nil { + return fmt.Errorf("drop source set references: %w", err) + } return nil } @@ -152,10 +157,10 @@ func (r *family) applySourceMatch(network firewall.Network, prefixes []netip.Pre } } -// dropSourceMatch undoes whatever applySourceMatch reserved. Safe to -// call when the spec is empty or holds only inline matchers. Decrement -// errors are logged but not returned: the filter rule has already been -// deleted at that point and we don't want to leak the deletion. +// dropSourceMatch undoes whatever applySourceMatch reserved when +// installing a rule fails. Safe to call when the spec is empty or holds +// only inline matchers. Decrement errors are logged but not returned: +// the install error is what the caller needs to see. func (r *family) dropSourceMatch(srcMatch []string) { if r.ipsetCounter == nil { return diff --git a/client/firewall/iptables/manager_linux.go b/client/firewall/iptables/manager_linux.go index 43e0b7635..7356056a5 100644 --- a/client/firewall/iptables/manager_linux.go +++ b/client/firewall/iptables/manager_linux.go @@ -266,6 +266,9 @@ func (m *Manager) RemoveNatRule(pair firewall.RouterPair) error { } func (m *Manager) SetLegacyManagement(isLegacy bool) error { + m.mutex.Lock() + defer m.mutex.Unlock() + if err := firewall.SetLegacyManagement(m.family4, isLegacy); err != nil { return err } diff --git a/client/firewall/iptables/routing_linux.go b/client/firewall/iptables/routing_linux.go index 49fd03bbf..1d6fdb14a 100644 --- a/client/firewall/iptables/routing_linux.go +++ b/client/firewall/iptables/routing_linux.go @@ -157,9 +157,7 @@ func (r *family) addMSSClampingRules() error { mss := r.mtu - overhead // Add jump rule from FORWARD chain in mangle table to our custom chain - jumpRule := []string{ - "-j", chainRTMSSClamp, - } + jumpRule := jumpRuleSpec(chainRTMSSClamp) if err := r.iptablesClient.Insert(tableMangle, chainForward, 1, jumpRule...); err != nil { return fmt.Errorf("add jump to MSS clamp chain: %w", err) }