From f7acbefbbb30a26848bae73041012693c2e7a64b Mon Sep 17 00:00:00 2001 From: Aaron Klotz Date: Tue, 13 Feb 2024 10:32:48 -0700 Subject: [PATCH] wgengine/router: make the Windows ifconfig implementation reuse existing MibIPforwardRow2 when possible Looking at profiles, we spend a lot of time in winipcfg.LUID.DeleteRoute looking up the routing table entry for the provided RouteData. But we already have the row! We previously obtained that data via the full table dump we did in getInterfaceRoutes. We can make this a lot faster by hanging onto a reference to the wipipcfg.MibIPforwardRow2 and executing the delete operation directly on that. Fixes #11123 Signed-off-by: Aaron Klotz --- wgengine/router/ifconfig_windows.go | 96 +++++++++++------- wgengine/router/ifconfig_windows_test.go | 124 ++++++++++++----------- 2 files changed, 125 insertions(+), 95 deletions(-) diff --git a/wgengine/router/ifconfig_windows.go b/wgengine/router/ifconfig_windows.go index 414a8b708..87ff69b3f 100644 --- a/wgengine/router/ifconfig_windows.go +++ b/wgengine/router/ifconfig_windows.go @@ -331,7 +331,7 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) (retErr error) { } } - var routes []*winipcfg.RouteData + var routes []*routeData foundDefault4 := false foundDefault6 := false for _, route := range cfg.Routes { @@ -361,10 +361,12 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) (retErr error) { } else if route.Addr().Is6() { gateway = firstGateway6 } - r := &winipcfg.RouteData{ - Destination: route, - NextHop: gateway, - Metric: 0, + r := &routeData{ + RouteData: winipcfg.RouteData{ + Destination: route, + NextHop: gateway, + Metric: 0, + }, } if r.Destination.Addr().Unmap() == gateway { // no need to add a route for the interface's @@ -393,9 +395,9 @@ func configureInterface(cfg *Config, tun *tun.NativeTun) (retErr error) { return fmt.Errorf("syncAddresses: %w", err) } - slices.SortFunc(routes, routeDataCompare) + slices.SortFunc(routes, (*routeData).Compare) - deduplicatedRoutes := []*winipcfg.RouteData{} + deduplicatedRoutes := []*routeData{} for i := 0; i < len(routes); i++ { // There's only one way to get to a given IP+Mask, so delete // all matches after the first. @@ -596,18 +598,26 @@ func syncAddresses(ifc *winipcfg.IPAdapterAddresses, want []netip.Prefix) error return erracc } -func routeDataLess(a, b *winipcfg.RouteData) bool { - return routeDataCompare(a, b) < 0 +// routeData wraps winipcfg.RouteData with an additional field that permits +// caching of the associated MibIPForwardRow2; by keeping it around, we can +// avoid unnecessary (and slow) lookups of information that we already have. +type routeData struct { + winipcfg.RouteData + Row *winipcfg.MibIPforwardRow2 } -func routeDataCompare(a, b *winipcfg.RouteData) int { - v := a.Destination.Addr().Compare(b.Destination.Addr()) +func (rd *routeData) Less(other *routeData) bool { + return rd.Compare(other) < 0 +} + +func (rd *routeData) Compare(other *routeData) int { + v := rd.Destination.Addr().Compare(other.Destination.Addr()) if v != 0 { return v } // Narrower masks first - b1, b2 := a.Destination.Bits(), b.Destination.Bits() + b1, b2 := rd.Destination.Bits(), other.Destination.Bits() if b1 != b2 { if b1 > b2 { return -1 @@ -616,31 +626,31 @@ func routeDataCompare(a, b *winipcfg.RouteData) int { } // No nexthop before non-empty nexthop - v = a.NextHop.Compare(b.NextHop) + v = rd.NextHop.Compare(other.NextHop) if v != 0 { return v } // Lower metrics first - if a.Metric < b.Metric { + if rd.Metric < other.Metric { return -1 - } else if a.Metric > b.Metric { + } else if rd.Metric > other.Metric { return 1 } return 0 } -func deltaRouteData(a, b []*winipcfg.RouteData) (add, del []*winipcfg.RouteData) { - add = make([]*winipcfg.RouteData, 0, len(b)) - del = make([]*winipcfg.RouteData, 0, len(a)) - slices.SortFunc(a, routeDataCompare) - slices.SortFunc(b, routeDataCompare) +func deltaRouteData(a, b []*routeData) (add, del []*routeData) { + add = make([]*routeData, 0, len(b)) + del = make([]*routeData, 0, len(a)) + slices.SortFunc(a, (*routeData).Compare) + slices.SortFunc(b, (*routeData).Compare) i := 0 j := 0 for i < len(a) && j < len(b) { - switch routeDataCompare(a[i], b[j]) { + switch a[i].Compare(b[j]) { case -1: // a < b, delete del = append(del, a[i]) @@ -677,7 +687,7 @@ func getInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses, family winipcfg.Addres return } -func getAllInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses) ([]*winipcfg.RouteData, error) { +func getAllInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses) ([]*routeData, error) { routes4, err := getInterfaceRoutes(ifc, windows.AF_INET) if err != nil { return nil, err @@ -688,19 +698,27 @@ func getAllInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses) ([]*winipcfg.RouteD // TODO: what if v6 unavailable? return nil, err } - rd := make([]*winipcfg.RouteData, 0, len(routes4)+len(routes6)) + + rd := make([]*routeData, 0, len(routes4)+len(routes6)) for _, r := range routes4 { - rd = append(rd, &winipcfg.RouteData{ - Destination: r.DestinationPrefix.Prefix(), - NextHop: r.NextHop.Addr(), - Metric: r.Metric, + rd = append(rd, &routeData{ + RouteData: winipcfg.RouteData{ + Destination: r.DestinationPrefix.Prefix(), + NextHop: r.NextHop.Addr(), + Metric: r.Metric, + }, + Row: r, }) } + for _, r := range routes6 { - rd = append(rd, &winipcfg.RouteData{ - Destination: r.DestinationPrefix.Prefix(), - NextHop: r.NextHop.Addr(), - Metric: r.Metric, + rd = append(rd, &routeData{ + RouteData: winipcfg.RouteData{ + Destination: r.DestinationPrefix.Prefix(), + NextHop: r.NextHop.Addr(), + Metric: r.Metric, + }, + Row: r, }) } return rd, nil @@ -708,7 +726,7 @@ func getAllInterfaceRoutes(ifc *winipcfg.IPAdapterAddresses) ([]*winipcfg.RouteD // filterRoutes removes routes that have been added by Windows and should not // be managed by us. -func filterRoutes(routes []*winipcfg.RouteData, dontDelete []netip.Prefix) []*winipcfg.RouteData { +func filterRoutes(routes []*routeData, dontDelete []netip.Prefix) []*routeData { ddm := make(map[netip.Prefix]bool) for _, dd := range dontDelete { // See issue 1448: we don't want to touch the routes added @@ -727,7 +745,7 @@ func filterRoutes(routes []*winipcfg.RouteData, dontDelete []netip.Prefix) []*wi lastIP := netipx.RangeOfPrefix(nr).To() ddm[netip.PrefixFrom(lastIP, lastIP.BitLen())] = true } - filtered := make([]*winipcfg.RouteData, 0, len(routes)) + filtered := make([]*routeData, 0, len(routes)) for _, r := range routes { rr := r.Destination if rr.IsValid() && ddm[rr] { @@ -742,7 +760,7 @@ func filterRoutes(routes []*winipcfg.RouteData, dontDelete []netip.Prefix) []*wi // This avoids a full ifc.FlushRoutes call. // dontDelete is a list of interface address routes that the // synchronization logic should never delete. -func syncRoutes(ifc *winipcfg.IPAdapterAddresses, want []*winipcfg.RouteData, dontDelete []netip.Prefix) error { +func syncRoutes(ifc *winipcfg.IPAdapterAddresses, want []*routeData, dontDelete []netip.Prefix) error { existingRoutes, err := getAllInterfaceRoutes(ifc) if err != nil { return err @@ -753,7 +771,15 @@ func syncRoutes(ifc *winipcfg.IPAdapterAddresses, want []*winipcfg.RouteData, do var errs []error for _, a := range del { - err := ifc.LUID.DeleteRoute(a.Destination, a.NextHop) + var err error + if a.Row == nil { + // DeleteRoute requires a routing table lookup, so only do that if + // a does not already have the row. + err = ifc.LUID.DeleteRoute(a.Destination, a.NextHop) + } else { + // Otherwise, delete the row directly. + err = a.Row.Delete() + } if err != nil { dstStr := a.Destination.String() if dstStr == "169.254.255.255/32" { diff --git a/wgengine/router/ifconfig_windows_test.go b/wgengine/router/ifconfig_windows_test.go index 4679a149d..64c0ff0e9 100644 --- a/wgengine/router/ifconfig_windows_test.go +++ b/wgengine/router/ifconfig_windows_test.go @@ -19,58 +19,62 @@ func randIP() netip.Addr { return netip.AddrFrom4([4]byte{b, b, b, b}) } -func randRouteData() *winipcfg.RouteData { - return &winipcfg.RouteData{ - Destination: netip.PrefixFrom(randIP(), rand.Intn(30)+1), - NextHop: randIP(), - Metric: uint32(rand.Intn(3)), +func randRouteData() *routeData { + return &routeData{ + RouteData: winipcfg.RouteData{ + Destination: netip.PrefixFrom(randIP(), rand.Intn(30)+1), + NextHop: randIP(), + Metric: uint32(rand.Intn(3)), + }, } } +type W = winipcfg.RouteData + func TestRouteLess(t *testing.T) { - type D = winipcfg.RouteData + type D = routeData ipnet := netip.MustParsePrefix tests := []struct { - ri, rj *winipcfg.RouteData + ri, rj *routeData want bool }{ { - ri: &D{Metric: 1}, - rj: &D{Metric: 2}, + ri: &D{RouteData: W{Metric: 1}}, + rj: &D{RouteData: W{Metric: 2}}, want: true, }, { - ri: &D{Destination: ipnet("1.1.0.0/16"), Metric: 2}, - rj: &D{Destination: ipnet("2.2.0.0/16"), Metric: 1}, + ri: &D{RouteData: W{Destination: ipnet("1.1.0.0/16"), Metric: 2}}, + rj: &D{RouteData: W{Destination: ipnet("2.2.0.0/16"), Metric: 1}}, want: true, }, { - ri: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1}, - rj: &D{Destination: ipnet("2.2.0.0/16"), Metric: 1}, + ri: &D{RouteData: W{Destination: ipnet("1.1.0.0/16"), Metric: 1}}, + rj: &D{RouteData: W{Destination: ipnet("2.2.0.0/16"), Metric: 1}}, want: true, }, { - ri: &D{Destination: ipnet("1.1.0.0/32"), Metric: 2}, - rj: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1}, + ri: &D{RouteData: W{Destination: ipnet("1.1.0.0/32"), Metric: 2}}, + rj: &D{RouteData: W{Destination: ipnet("1.1.0.0/16"), Metric: 1}}, want: true, }, { - ri: &D{Destination: ipnet("1.1.0.0/32"), Metric: 1}, - rj: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1}, + ri: &D{RouteData: W{Destination: ipnet("1.1.0.0/32"), Metric: 1}}, + rj: &D{RouteData: W{Destination: ipnet("1.1.0.0/16"), Metric: 1}}, want: true, }, { - ri: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: netip.MustParseAddr("3.3.3.3")}, - rj: &D{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: netip.MustParseAddr("4.4.4.4")}, + ri: &D{RouteData: W{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: netip.MustParseAddr("3.3.3.3")}}, + rj: &D{RouteData: W{Destination: ipnet("1.1.0.0/16"), Metric: 1, NextHop: netip.MustParseAddr("4.4.4.4")}}, want: true, }, } for i, tt := range tests { - got := routeDataLess(tt.ri, tt.rj) + got := tt.ri.Less(tt.rj) if got != tt.want { t.Errorf("%v. less = %v; want %v", i, got, tt.want) } - back := routeDataLess(tt.rj, tt.ri) + back := tt.rj.Less(tt.ri) if back && got { t.Errorf("%v. less both ways", i) } @@ -81,7 +85,7 @@ func TestRouteDataLessConsistent(t *testing.T) { for i := 0; i < 10000; i++ { ri := randRouteData() rj := randRouteData() - if routeDataLess(ri, rj) && routeDataLess(rj, ri) { + if ri.Less(rj) && rj.Less(ri) { t.Fatalf("both compare less to each other:\n\t%#v\nand\n\t%#v", ri, rj) } } @@ -139,7 +143,7 @@ func TestDeltaNets(t *testing.T) { } } -func formatRouteData(rds []*winipcfg.RouteData) string { +func formatRouteData(rds []*routeData) string { var b strings.Builder for _, rd := range rds { b.WriteString(fmt.Sprintf("%+v", rd)) @@ -147,12 +151,12 @@ func formatRouteData(rds []*winipcfg.RouteData) string { return b.String() } -func equalRouteDatas(a, b []*winipcfg.RouteData) bool { +func equalRouteDatas(a, b []*routeData) bool { if len(a) != len(b) { return false } for i := range a { - if routeDataCompare(a[i], b[i]) != 0 { + if a[i].Compare(b[i]) != 0 { return false } } @@ -166,33 +170,33 @@ func ipnet4(ip string, bits int) netip.Prefix { func TestFilterRoutes(t *testing.T) { var h0 netip.Addr - in := []*winipcfg.RouteData{ + in := []*routeData{ // LinkLocal and Loopback routes. - {ipnet4("169.254.0.0", 16), h0, 1}, - {ipnet4("169.254.255.255", 32), h0, 1}, - {ipnet4("127.0.0.0", 8), h0, 1}, - {ipnet4("127.255.255.255", 32), h0, 1}, + {RouteData: W{ipnet4("169.254.0.0", 16), h0, 1}}, + {RouteData: W{ipnet4("169.254.255.255", 32), h0, 1}}, + {RouteData: W{ipnet4("127.0.0.0", 8), h0, 1}}, + {RouteData: W{ipnet4("127.255.255.255", 32), h0, 1}}, // Local LAN routes. - {ipnet4("192.168.0.0", 24), h0, 1}, - {ipnet4("192.168.0.255", 32), h0, 1}, - {ipnet4("192.168.1.0", 25), h0, 1}, - {ipnet4("192.168.1.127", 32), h0, 1}, + {RouteData: W{ipnet4("192.168.0.0", 24), h0, 1}}, + {RouteData: W{ipnet4("192.168.0.255", 32), h0, 1}}, + {RouteData: W{ipnet4("192.168.1.0", 25), h0, 1}}, + {RouteData: W{ipnet4("192.168.1.127", 32), h0, 1}}, // Some random other route. - {ipnet4("192.168.2.23", 32), h0, 1}, + {RouteData: W{ipnet4("192.168.2.23", 32), h0, 1}}, // Our own tailscale address. - {ipnet4("100.100.100.100", 32), h0, 1}, + {RouteData: W{ipnet4("100.100.100.100", 32), h0, 1}}, // Other tailscale addresses. - {ipnet4("100.100.100.101", 32), h0, 1}, - {ipnet4("100.100.100.102", 32), h0, 1}, + {RouteData: W{ipnet4("100.100.100.101", 32), h0, 1}}, + {RouteData: W{ipnet4("100.100.100.102", 32), h0, 1}}, } - want := []*winipcfg.RouteData{ - {ipnet4("169.254.0.0", 16), h0, 1}, - {ipnet4("127.0.0.0", 8), h0, 1}, - {ipnet4("192.168.0.0", 24), h0, 1}, - {ipnet4("192.168.1.0", 25), h0, 1}, - {ipnet4("192.168.2.23", 32), h0, 1}, - {ipnet4("100.100.100.101", 32), h0, 1}, - {ipnet4("100.100.100.102", 32), h0, 1}, + want := []*routeData{ + {RouteData: W{ipnet4("169.254.0.0", 16), h0, 1}}, + {RouteData: W{ipnet4("127.0.0.0", 8), h0, 1}}, + {RouteData: W{ipnet4("192.168.0.0", 24), h0, 1}}, + {RouteData: W{ipnet4("192.168.1.0", 25), h0, 1}}, + {RouteData: W{ipnet4("192.168.2.23", 32), h0, 1}}, + {RouteData: W{ipnet4("100.100.100.101", 32), h0, 1}}, + {RouteData: W{ipnet4("100.100.100.102", 32), h0, 1}}, } got := filterRoutes(in, mustCIDRs("100.100.100.100/32")) @@ -206,25 +210,25 @@ func TestDeltaRouteData(t *testing.T) { h1 := netip.MustParseAddr("99.99.99.99") h2 := netip.MustParseAddr("99.99.9.99") - a := []*winipcfg.RouteData{ - {ipnet4("1.2.3.4", 32), h0, 1}, - {ipnet4("1.2.3.4", 24), h1, 2}, - {ipnet4("1.2.3.4", 24), h2, 1}, - {ipnet4("1.2.3.5", 32), h0, 1}, + a := []*routeData{ + {RouteData: W{ipnet4("1.2.3.4", 32), h0, 1}}, + {RouteData: W{ipnet4("1.2.3.4", 24), h1, 2}}, + {RouteData: W{ipnet4("1.2.3.4", 24), h2, 1}}, + {RouteData: W{ipnet4("1.2.3.5", 32), h0, 1}}, } - b := []*winipcfg.RouteData{ - {ipnet4("1.2.3.5", 32), h0, 1}, - {ipnet4("1.2.3.4", 24), h1, 2}, - {ipnet4("1.2.3.4", 24), h2, 2}, + b := []*routeData{ + {RouteData: W{ipnet4("1.2.3.5", 32), h0, 1}}, + {RouteData: W{ipnet4("1.2.3.4", 24), h1, 2}}, + {RouteData: W{ipnet4("1.2.3.4", 24), h2, 2}}, } add, del := deltaRouteData(a, b) - wantAdd := []*winipcfg.RouteData{ - {ipnet4("1.2.3.4", 24), h2, 2}, + wantAdd := []*routeData{ + {RouteData: W{ipnet4("1.2.3.4", 24), h2, 2}}, } - wantDel := []*winipcfg.RouteData{ - {ipnet4("1.2.3.4", 32), h0, 1}, - {ipnet4("1.2.3.4", 24), h2, 1}, + wantDel := []*routeData{ + {RouteData: W{ipnet4("1.2.3.4", 32), h0, 1}}, + {RouteData: W{ipnet4("1.2.3.4", 24), h2, 1}}, } if !equalRouteDatas(add, wantAdd) {