From c761d0d1cd48b0db20a994fec32e7226d441621c Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 20 May 2026 13:15:15 +0200 Subject: [PATCH] Address review: VLAN-safe procfs path, rollback partial v6 enable, aggregate routing errors --- client/firewall/iptables/manager_linux.go | 20 ++++++----- client/firewall/nftables/manager_linux.go | 20 ++++++----- .../routemanager/ipfwdstate/ipfwdstate.go | 27 ++++++--------- .../systemops/systemops_android.go | 2 +- .../routemanager/systemops/systemops_ios.go | 2 +- .../routemanager/systemops/systemops_linux.go | 34 +++++++------------ .../systemops/systemops_nonlinux.go | 2 +- 7 files changed, 50 insertions(+), 57 deletions(-) diff --git a/client/firewall/iptables/manager_linux.go b/client/firewall/iptables/manager_linux.go index 19a313f7b..90ba540f4 100644 --- a/client/firewall/iptables/manager_linux.go +++ b/client/firewall/iptables/manager_linux.go @@ -405,26 +405,30 @@ func (m *Manager) EnableRouting() error { if err := m.router.ipFwdState.RequestForwarding(false); err != nil { return fmt.Errorf("enable IPv4 forwarding: %w", err) } - // Only flip v6 forwarding when the WG interface actually has v6, so that - // v4-only routing setups don't disable RA acceptance on the host. - if m.router6 != nil { - if err := m.router.ipFwdState.RequestForwarding(true); err != nil { - return fmt.Errorf("enable IPv6 forwarding: %w", err) + // v6 only when the overlay actually has v6. + if m.router6 == nil { + return nil + } + if err := m.router.ipFwdState.RequestForwarding(true); err != nil { + if rerr := m.router.ipFwdState.ReleaseForwarding(false); rerr != nil { + log.Warnf("rollback v4 forwarding: %v", rerr) } + return fmt.Errorf("enable IPv6 forwarding: %w", err) } return nil } func (m *Manager) DisableRouting() error { + var merr *multierror.Error if err := m.router.ipFwdState.ReleaseForwarding(false); err != nil { - return fmt.Errorf("disable IPv4 forwarding: %w", err) + merr = multierror.Append(merr, fmt.Errorf("disable IPv4 forwarding: %w", err)) } if m.router6 != nil { if err := m.router.ipFwdState.ReleaseForwarding(true); err != nil { - return fmt.Errorf("disable IPv6 forwarding: %w", err) + merr = multierror.Append(merr, fmt.Errorf("disable IPv6 forwarding: %w", err)) } } - return nil + return nberrors.FormatErrorOrNil(merr) } // AddDNATRule adds a DNAT rule diff --git a/client/firewall/nftables/manager_linux.go b/client/firewall/nftables/manager_linux.go index 109ab6ea4..d25b60585 100644 --- a/client/firewall/nftables/manager_linux.go +++ b/client/firewall/nftables/manager_linux.go @@ -533,26 +533,30 @@ func (m *Manager) EnableRouting() error { if err := m.router.ipFwdState.RequestForwarding(false); err != nil { return fmt.Errorf("enable IPv4 forwarding: %w", err) } - // Only flip v6 forwarding when the WG interface actually has v6, so that - // v4-only routing setups don't disable RA acceptance on the host. - if m.router6 != nil { - if err := m.router.ipFwdState.RequestForwarding(true); err != nil { - return fmt.Errorf("enable IPv6 forwarding: %w", err) + // v6 only when the overlay actually has v6. + if m.router6 == nil { + return nil + } + if err := m.router.ipFwdState.RequestForwarding(true); err != nil { + if rerr := m.router.ipFwdState.ReleaseForwarding(false); rerr != nil { + log.Warnf("rollback v4 forwarding: %v", rerr) } + return fmt.Errorf("enable IPv6 forwarding: %w", err) } return nil } func (m *Manager) DisableRouting() error { + var merr *multierror.Error if err := m.router.ipFwdState.ReleaseForwarding(false); err != nil { - return fmt.Errorf("disable IPv4 forwarding: %w", err) + merr = multierror.Append(merr, fmt.Errorf("disable IPv4 forwarding: %w", err)) } if m.router6 != nil { if err := m.router.ipFwdState.ReleaseForwarding(true); err != nil { - return fmt.Errorf("disable IPv6 forwarding: %w", err) + merr = multierror.Append(merr, fmt.Errorf("disable IPv6 forwarding: %w", err)) } } - return nil + return nberrors.FormatErrorOrNil(merr) } // Flush rule/chain/set operations from the buffer diff --git a/client/internal/routemanager/ipfwdstate/ipfwdstate.go b/client/internal/routemanager/ipfwdstate/ipfwdstate.go index 6af8753e9..7838979a4 100644 --- a/client/internal/routemanager/ipfwdstate/ipfwdstate.go +++ b/client/internal/routemanager/ipfwdstate/ipfwdstate.go @@ -9,31 +9,23 @@ import ( "github.com/netbirdio/netbird/client/internal/routemanager/systemops" ) -// IPForwardingState tracks per-family IP-forwarding sysctl enables with a -// refcount. v4 and v6 are independent so a v4-only routing setup doesn't flip -// net.ipv6.conf.all.forwarding, which on Linux disables RA acceptance by -// default and lets host RA-installed defaults silently expire. +// IPForwardingState tracks v4 and v6 IP-forwarding sysctl enables with +// independent refcounts so a v4-only routing setup doesn't flip v6 sysctls. type IPForwardingState struct { mu sync.Mutex v4Count int v6Count int - // wgIfaceName is excluded from the v6 accept_ra bump since the overlay - // interface doesn't carry upstream RAs. wgIfaceName string - // v6Saved records the sysctl values captured when v6 forwarding was - // enabled (forwarding + per-interface accept_ra), restored on the last - // release. - v6Saved map[string]int + v6Saved map[string]int } func NewIPForwardingState(wgIfaceName string) *IPForwardingState { return &IPForwardingState{wgIfaceName: wgIfaceName} } -// RequestForwarding bumps the per-family counter, enabling the underlying -// sysctl on the first request. +// RequestForwarding enables the family's forwarding sysctl on first request. func (f *IPForwardingState) RequestForwarding(v6 bool) error { f.mu.Lock() defer f.mu.Unlock() @@ -44,9 +36,9 @@ func (f *IPForwardingState) RequestForwarding(v6 bool) error { return f.requestV4() } -// ReleaseForwarding decrements the per-family counter. The last v6 release -// also restores the sysctls v6 enable captured. v4 stays on: net.ipv4.ip_forward -// is a global knob other tools (docker, k8s, libvirt) co-own. +// ReleaseForwarding decrements the family counter. The last v6 release restores +// what enable captured. v4 stays on: net.ipv4.ip_forward is co-owned by other +// tooling (docker, k8s, libvirt). func (f *IPForwardingState) ReleaseForwarding(v6 bool) error { f.mu.Lock() defer f.mu.Unlock() @@ -78,10 +70,13 @@ func (f *IPForwardingState) releaseV4() { func (f *IPForwardingState) requestV6() error { if f.v6Count == 0 { saved, err := systemops.EnableV6IPForwarding(f.wgIfaceName) - f.v6Saved = saved if err != nil { + if rerr := systemops.DisableV6IPForwarding(saved); rerr != nil { + log.Warnf("rollback partial v6 sysctls: %v", rerr) + } return fmt.Errorf("enable IPv6 forwarding: %w", err) } + f.v6Saved = saved log.Info("IPv6 forwarding enabled") } f.v6Count++ diff --git a/client/internal/routemanager/systemops/systemops_android.go b/client/internal/routemanager/systemops/systemops_android.go index 3840ad444..97b4ed8ec 100644 --- a/client/internal/routemanager/systemops/systemops_android.go +++ b/client/internal/routemanager/systemops/systemops_android.go @@ -39,7 +39,7 @@ func EnableV4IPForwarding() error { func EnableV6IPForwarding(string) (map[string]int, error) { log.Infof("Enable IPv6 forwarding is not implemented on %s", runtime.GOOS) - return nil, nil + return map[string]int{}, nil } func DisableV6IPForwarding(map[string]int) error { diff --git a/client/internal/routemanager/systemops/systemops_ios.go b/client/internal/routemanager/systemops/systemops_ios.go index fc1f58da5..0cccd4962 100644 --- a/client/internal/routemanager/systemops/systemops_ios.go +++ b/client/internal/routemanager/systemops/systemops_ios.go @@ -65,7 +65,7 @@ func EnableV4IPForwarding() error { func EnableV6IPForwarding(string) (map[string]int, error) { log.Infof("Enable IPv6 forwarding is not implemented on %s", runtime.GOOS) - return nil, nil + return map[string]int{}, nil } func DisableV6IPForwarding(map[string]int) error { diff --git a/client/internal/routemanager/systemops/systemops_linux.go b/client/internal/routemanager/systemops/systemops_linux.go index 8689a35d1..6d20dbaee 100644 --- a/client/internal/routemanager/systemops/systemops_linux.go +++ b/client/internal/routemanager/systemops/systemops_linux.go @@ -9,7 +9,6 @@ import ( "net" "net/netip" "os" - "strings" "syscall" "github.com/hashicorp/go-multierror" @@ -56,11 +55,11 @@ const ( ipv4ForwardingPath = "net.ipv4.ip_forward" // ipv6ForwardingPath is the path to the file containing the IPv6 forwarding setting. ipv6ForwardingPath = "net.ipv6.conf.all.forwarding" - // acceptRAInterfacePath toggles per-interface IPv6 RA acceptance. - // 1 (kernel default) accepts RAs only when forwarding is off; 2 keeps - // RA processing enabled even when forwarding is on, so RA-installed host - // defaults survive our v6 forwarding flip. - acceptRAInterfacePath = "net.ipv6.conf.%s.accept_ra" + // 1 (default) accepts RAs only while forwarding is off; 2 keeps RA + // acceptance on regardless, so RA-installed host defaults survive our + // v6 forwarding flip. + acceptRAInterfacePath = "net.ipv6.conf.%s.accept_ra" + acceptRAProcPathFormat = "/proc/sys/net/ipv6/conf/%s/accept_ra" ) var ErrTableIDExists = errors.New("ID exists with different name") @@ -776,14 +775,9 @@ func EnableV4IPForwarding() error { return nil } -// EnableV6IPForwarding bumps accept_ra=2 on every non-loopback v6 interface -// before flipping forwarding=1, so RA-installed host defaults survive the flip. -// wgIfaceName is excluded since the overlay interface doesn't carry upstream RAs. -// -// The returned map records prior sysctl values for entries we actually changed -// (forwarding + per-interface accept_ra); DisableV6IPForwarding restores from -// it. Entries we found already at the target value are omitted so another -// process's sysctls aren't reset by our cleanup. +// EnableV6IPForwarding bumps accept_ra=2 on host v6 interfaces before flipping +// forwarding=1, so RA-installed host defaults survive. Returns the prior values +// of sysctls we actually changed; entries already at the target are omitted. func EnableV6IPForwarding(wgIfaceName string) (map[string]int, error) { saved := map[string]int{} bumpAcceptRA(saved, wgIfaceName) @@ -798,9 +792,7 @@ func EnableV6IPForwarding(wgIfaceName string) (map[string]int, error) { return saved, nil } -// DisableV6IPForwarding restores every sysctl value EnableV6IPForwarding -// captured. v4 is intentionally not disabled: net.ipv4.ip_forward is a global -// knob other tools (docker, k8s, libvirt) co-own. +// DisableV6IPForwarding restores what EnableV6IPForwarding captured. func DisableV6IPForwarding(saved map[string]int) error { var result *multierror.Error for key, value := range saved { @@ -827,13 +819,11 @@ func bumpAcceptRA(saved map[string]int, wgIfaceName string) { func bumpAcceptRAForInterface(saved map[string]int, name string) { key := fmt.Sprintf(acceptRAInterfacePath, name) - procPath := "/proc/sys/" + strings.ReplaceAll(key, ".", "/") - if _, err := os.Stat(procPath); err != nil { - // No IPv6 stack on this interface. + // Build procfs path from name, not the dotted key: VLAN names like eth0.100. + if _, err := os.Stat(fmt.Sprintf(acceptRAProcPathFormat, name)); err != nil { return } - // onlyIfOne=true: only bump from the kernel default; preserves admin - // overrides of 0 (don't accept RAs) or 2 (already what we want). + // onlyIfOne=true: leave admin overrides (0, 2) alone. oldVal, err := sysctl.Set(key, 2, true) if err != nil { log.Warnf("bump %s: %v", key, err) diff --git a/client/internal/routemanager/systemops/systemops_nonlinux.go b/client/internal/routemanager/systemops/systemops_nonlinux.go index 2c3377775..837ac0cd2 100644 --- a/client/internal/routemanager/systemops/systemops_nonlinux.go +++ b/client/internal/routemanager/systemops/systemops_nonlinux.go @@ -50,7 +50,7 @@ func EnableV4IPForwarding() error { func EnableV6IPForwarding(string) (map[string]int, error) { log.Infof("Enable IPv6 forwarding is not implemented on %s", runtime.GOOS) - return nil, nil + return map[string]int{}, nil } func DisableV6IPForwarding(map[string]int) error {