From 5bcceac53ca1c21a7700ddb9d22082138150656a Mon Sep 17 00:00:00 2001 From: JackDoan Date: Thu, 15 Oct 2026 10:01:05 -0500 Subject: [PATCH] linux: Don't replace another interface's route with the same destination and metric --- overlay/tun_linux.go | 99 +++++++++++++++++++++++- overlay/tun_linux_route_test.go | 133 ++++++++++++++++++++++++++++++++ 2 files changed, 229 insertions(+), 3 deletions(-) create mode 100644 overlay/tun_linux_route_test.go diff --git a/overlay/tun_linux.go b/overlay/tun_linux.go index 5e27ad9b..6cdd9d16 100644 --- a/overlay/tun_linux.go +++ b/overlay/tun_linux.go @@ -552,13 +552,17 @@ func (t *tun) setDefaultRoute(cidr netip.Prefix) error { if cidr.Addr().Is6() { nr.Priority = 256 } - err := netlink.RouteReplace(&nr) + held, err := t.otherLinkRoutes() + if err != nil { + return err + } + err = t.replaceRoute(&nr, held) if err != nil { t.l.Warn("Failed to set default route MTU, retrying", "error", err, "cidr", cidr) //retry twice more -- on some systems there appears to be a race condition where if we set routes too soon, netlink says `invalid argument` for range 2 { time.Sleep(100 * time.Millisecond) - err = netlink.RouteReplace(&nr) + err = t.replaceRoute(&nr, held) if err == nil { break } else { @@ -578,6 +582,15 @@ func (t *tun) setDefaultRoute(cidr netip.Prefix) error { } func (t *tun) addRoutes(logErrors bool) error { + held, err := t.otherLinkRoutes() + if err != nil { + if logErrors { + t.l.Error("Failed to add routes", "error", err) + return nil + } + return err + } + // Path routes routes := *t.Routes.Load() for _, r := range routes { @@ -602,7 +615,7 @@ func (t *tun) addRoutes(logErrors bool) error { nr.Priority = r.Metric } - err := netlink.RouteReplace(&nr) + err := t.replaceRoute(&nr, held) if err != nil { retErr := util.NewContextualError("Failed to add route", map[string]any{"route": r}, err) if logErrors { @@ -618,6 +631,86 @@ func (t *tun) addRoutes(logErrors bool) error { return nil } +// ip6RoutePrioUser is the metric the kernel gives an IPv6 route added without one +const ip6RoutePrioUser = 1024 + +// routeKey is where the main table files a route. Replacing a route replaces the first one with the same key, whatever +// device it is on +type routeKey struct { + dst netip.Prefix + priority int +} + +func routeKeyOf(dst *net.IPNet, family, priority int) (routeKey, bool) { + var p netip.Prefix + if dst == nil { + // A default route + if family == unix.AF_INET6 { + p = netip.PrefixFrom(netip.IPv6Unspecified(), 0) + } else { + p = netip.PrefixFrom(netip.IPv4Unspecified(), 0) + } + } else { + addr, ok := netip.AddrFromSlice(dst.IP) + if !ok { + return routeKey{}, false + } + ones, _ := dst.Mask.Size() + p = netip.PrefixFrom(addr.Unmap(), ones).Masked() + } + if priority == 0 && p.Addr().Is6() { + priority = ip6RoutePrioUser + } + return routeKey{dst: p, priority: priority}, true +} + +// otherLinkRoutes returns the keys of the main table routes that are not on our device +func (t *tun) otherLinkRoutes() (map[routeKey]struct{}, error) { + routes, err := netlink.RouteListFiltered(netlink.FAMILY_ALL, &netlink.Route{Table: unix.RT_TABLE_MAIN}, netlink.RT_FILTER_TABLE) + // An interrupted dump may miss a route, which then gets replaced like it always used to be + if err != nil && !errors.Is(err, netlink.ErrDumpInterrupted) { + return nil, fmt.Errorf("failed to list the system routes: %w", err) + } + held := make(map[routeKey]struct{}, len(routes)) + for _, r := range routes { + if r.LinkIndex == t.deviceIndex { + continue + } + if k, ok := routeKeyOf(r.Dst, r.Family, r.Priority); ok { + held[k] = struct{}{} + } + } + return held, nil +} + +// replaceRoute puts nr in the main table in place of our own route with its key. Replacing would take the route of +// another device holding that key, so ours goes in behind it instead. Theirs keeps routing and the kernel falls back +// to ours once theirs is removed, like when leaving the lan that had it. A different metric decides which one wins. +func (t *tun) replaceRoute(nr *netlink.Route, held map[routeKey]struct{}) error { + family := unix.AF_INET6 + if nr.Dst.IP.To4() != nil { + family = unix.AF_INET + } + k, ok := routeKeyOf(nr.Dst, family, nr.Priority) + if _, taken := held[k]; !ok || !taken { + return netlink.RouteReplace(nr) + } + + // Appending doesn't update a route that is already ours, it adds a second one + ours := netlink.Route{ + LinkIndex: nr.LinkIndex, + Dst: nr.Dst, + Scope: nr.Scope, + Table: nr.Table, + Priority: k.priority, + } + if err := netlink.RouteDel(&ours); err != nil && !errors.Is(err, unix.ESRCH) { + return err + } + t.l.Warn("Another interface already routes this destination at the same metric, our route is used once theirs is removed", "destination", k.dst, "metric", k.priority) + return netlink.RouteAppend(nr) +} + func (t *tun) removeRoutes(routes []Route) { for _, r := range routes { if !r.Install { diff --git a/overlay/tun_linux_route_test.go b/overlay/tun_linux_route_test.go new file mode 100644 index 00000000..a2a0db23 --- /dev/null +++ b/overlay/tun_linux_route_test.go @@ -0,0 +1,133 @@ +//go:build linux && !android && !e2e_testing + +package overlay + +import ( + "net" + "net/netip" + "os" + "testing" + + "github.com/DefinedNet/netlink" + "github.com/slackhq/nebula/config" + "github.com/slackhq/nebula/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +// An unsafe_route with the same destination and metric as a lan's route must not replace it. Ours waits behind theirs +// and takes over when the lan goes away. Run as root, or unprivileged with `unshare -rn`. +func TestTunRouteLeavesAnotherDevicesRoute(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("creating links and adding routes needs root") + } + + lan := &netlink.Veth{LinkAttrs: netlink.LinkAttrs{Name: "nebrt0"}, PeerName: "nebrt1"} + require.NoError(t, netlink.LinkAdd(lan)) + t.Cleanup(func() { _ = netlink.LinkDel(lan) }) + peer, err := netlink.LinkByName("nebrt1") + require.NoError(t, err) + require.NoError(t, netlink.LinkSetUp(lan)) + require.NoError(t, netlink.LinkSetUp(peer)) + + lanAddrs := []*netlink.Addr{ + {IPNet: &net.IPNet{IP: net.ParseIP("198.51.100.5"), Mask: net.CIDRMask(24, 32)}}, + {IPNet: &net.IPNet{IP: net.ParseIP("2001:db8:1::5"), Mask: net.CIDRMask(64, 128)}, Flags: unix.IFA_F_NODAD}, + } + for _, a := range lanAddrs { + require.NoError(t, netlink.AddrAdd(lan, a)) + } + + l := test.NewLogger() + c := config.NewC(l) + // The kernel puts a v4 lan route at metric 0, our default, and a v6 one at 256 + require.NoError(t, c.LoadString(` +tun: + dev: nebrt2 + unsafe_routes: + - route: 198.51.100.0/24 + via: 10.250.0.2 + - route: 2001:db8:1::/64 + via: fd00:250::2 + metric: 256 +`)) + tn, err := newTun(c, l, []netip.Prefix{netip.MustParsePrefix("10.250.0.1/24"), netip.MustParsePrefix("fd00:250::1/64")}, false) + require.NoError(t, err) + t.Cleanup(func() { _ = tn.Close() }) + require.NoError(t, tn.Activate()) + + via := func(dst string) int { + routes, err := netlink.RouteGet(net.ParseIP(dst)) + require.NoError(t, err) + require.NotEmpty(t, routes) + return routes[0].LinkIndex + } + devices := func(dst string) []int { + _, n, err := net.ParseCIDR(dst) + require.NoError(t, err) + routes, err := netlink.RouteListFiltered(netlink.FAMILY_ALL, &netlink.Route{Dst: n}, netlink.RT_FILTER_DST) + require.NoError(t, err) + var out []int + for _, r := range routes { + out = append(out, r.LinkIndex) + } + return out + } + + for _, dst := range []string{"198.51.100.0/24", "2001:db8:1::/64"} { + assert.ElementsMatch(t, []int{lan.Index, tn.deviceIndex}, devices(dst), dst) + } + assert.Equal(t, lan.Index, via("198.51.100.9")) + assert.Equal(t, lan.Index, via("2001:db8:1::9")) + + // Leaving the lan + for _, a := range lanAddrs { + require.NoError(t, netlink.AddrDel(lan, a)) + } + assert.Equal(t, tn.deviceIndex, via("198.51.100.9")) + assert.Equal(t, tn.deviceIndex, via("2001:db8:1::9")) + + // Dropping the unsafe_routes removes only ours + for _, a := range lanAddrs { + require.NoError(t, netlink.AddrAdd(lan, a)) + } + require.NoError(t, c.ReloadConfigString("tun:\n dev: nebrt2\n")) + for _, dst := range []string{"198.51.100.0/24", "2001:db8:1::/64"} { + assert.Equal(t, []int{lan.Index}, devices(dst), dst) + } +} + +// The route for our own vpn network is replaced to set its mtu, which must not take a lan's route for the same network +func TestTunVpnNetworkLeavesAnotherDevicesRoute(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("creating links and adding routes needs root") + } + + lan := &netlink.Veth{LinkAttrs: netlink.LinkAttrs{Name: "nebrt0"}, PeerName: "nebrt1"} + require.NoError(t, netlink.LinkAdd(lan)) + t.Cleanup(func() { _ = netlink.LinkDel(lan) }) + require.NoError(t, netlink.LinkSetUp(lan)) + require.NoError(t, netlink.AddrAdd(lan, &netlink.Addr{IPNet: &net.IPNet{IP: net.ParseIP("10.250.0.5"), Mask: net.CIDRMask(24, 32)}})) + + l := test.NewLogger() + c := config.NewC(l) + require.NoError(t, c.LoadString("tun:\n dev: nebrt2\n")) + tn, err := newTun(c, l, []netip.Prefix{netip.MustParsePrefix("10.250.0.1/24")}, false) + require.NoError(t, err) + t.Cleanup(func() { _ = tn.Close() }) + require.NoError(t, tn.Activate()) + + _, n, err := net.ParseCIDR("10.250.0.0/24") + require.NoError(t, err) + routes, err := netlink.RouteListFiltered(netlink.FAMILY_V4, &netlink.Route{Dst: n}, netlink.RT_FILTER_DST) + require.NoError(t, err) + var devices []int + for _, r := range routes { + devices = append(devices, r.LinkIndex) + if r.LinkIndex == tn.deviceIndex { + assert.Equal(t, tn.DefaultMTU, r.MTU, "ours still carries the mtu") + } + } + assert.ElementsMatch(t, []int{lan.Index, tn.deviceIndex}, devices) +}