From 289dc7ce1d718b479e2a43282e443c79def0b133 Mon Sep 17 00:00:00 2001 From: Peter Johnston Date: Mon, 16 Oct 2023 08:07:04 -0700 Subject: [PATCH] Prefer routes with local addr on outgoing NIC only for local traffic Currently gVisor's routing logic always prefers routes that use a local address that is assigned to the outgoing interface. This preference is applied even for forwarded traffic, where the local address is left unspecified on route lookup because the source address of the packet belongs to some other node as opposed to the stack itself. This means that when forwarding incoming traffic, the netstack will prefer routes that go through a NIC with a local address endpoint over those that do not, even if a better route exists. Change this logic such that the preference for routes with a local address assigned to the outgoing interface only applies for locally- generated traffic. PiperOrigin-RevId: 573812452 --- pkg/tcpip/stack/stack.go | 61 ++++++++++++++++++------- pkg/tcpip/stack/stack_test.go | 85 +++++++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 17 deletions(-) diff --git a/pkg/tcpip/stack/stack.go b/pkg/tcpip/stack/stack.go index 33b3a503e..e3a7f788b 100644 --- a/pkg/tcpip/stack/stack.go +++ b/pkg/tcpip/stack/stack.go @@ -1290,6 +1290,28 @@ func isNICForwarding(nic *nic, proto tcpip.NetworkProtocolNumber) bool { } } +// findRouteWithLocalAddrFromAnyInterfaceRLocked returns a route to the given +// destination address, leaving through the given NIC. +// +// Rather than preferring to find a route that uses a local address assigned to +// the outgoing interface, it finds any NIC that holds a matching local address +// endpoint. +// +// +checklocksread:s.mu +func (s *Stack) findRouteWithLocalAddrFromAnyInterfaceRLocked(outgoingNIC *nic, localAddr, remoteAddr, gateway tcpip.Address, netProto tcpip.NetworkProtocolNumber, multicastLoop bool) *Route { + for _, aNIC := range s.nics { + addressEndpoint := s.getAddressEP(aNIC, localAddr, remoteAddr, netProto) + if addressEndpoint == nil { + continue + } + + if r := constructAndValidateRoute(netProto, addressEndpoint, aNIC /* localAddressNIC */, outgoingNIC, gateway, localAddr, remoteAddr, s.handleLocal, multicastLoop); r != nil { + return r + } + } + return nil +} + // FindRoute creates a route to the given destination address, leaving through // the given NIC and local address (if provided). // @@ -1379,15 +1401,27 @@ func (s *Stack) FindRoute(id tcpip.NICID, localAddr, remoteAddr tcpip.Address, n } } - // If the stack has forwarding enabled and we haven't found a valid route - // to the remote address yet, keep track of the first valid route. We - // keep iterating because we prefer routes that let us use a local - // address that is assigned to the outgoing interface. There is no - // requirement to do this from any RFC but simply a choice made to better - // follow a strong host model which the netstack follows at the time of - // writing. + // If the stack has forwarding enabled, we haven't found a valid route to + // the remote address yet, and we are routing locally generated traffic, + // keep track of the first valid route. We keep iterating because we + // prefer routes that let us use a local address that is assigned to the + // outgoing interface. There is no requirement to do this from any RFC + // but simply a choice made to better follow a strong host model which + // the netstack follows at the time of writing. + // + // Note that for incoming traffic that we are forwarding (for which the + // NIC and local address are unspecified), we do not keep iterating, as + // there is no reason to prefer routes that let us use a local address + // when routing forwarded (as opposed to locally-generated) traffic. + locallyGenerated := (id != 0 || localAddr != tcpip.Address{}) if onlyGlobalAddresses && chosenRoute.Equal(tcpip.Route{}) && isNICForwarding(nic, netProto) { - chosenRoute = route + if locallyGenerated { + chosenRoute = route + continue + } + if r := s.findRouteWithLocalAddrFromAnyInterfaceRLocked(nic, localAddr, remoteAddr, route.Gateway, netProto, multicastLoop); r != nil { + return r + } } } @@ -1427,15 +1461,8 @@ func (s *Stack) FindRoute(id tcpip.NICID, localAddr, remoteAddr tcpip.Address, n if id == 0 { // If an interface is not specified, try to find a NIC that holds the local // address endpoint to construct a route. - for _, aNIC := range s.nics { - addressEndpoint := s.getAddressEP(aNIC, localAddr, remoteAddr, netProto) - if addressEndpoint == nil { - continue - } - - if r := constructAndValidateRoute(netProto, addressEndpoint, aNIC /* localAddressNIC */, nic /* outgoingNIC */, gateway, localAddr, remoteAddr, s.handleLocal, multicastLoop); r != nil { - return r, nil - } + if r := s.findRouteWithLocalAddrFromAnyInterfaceRLocked(nic, localAddr, remoteAddr, gateway, netProto, multicastLoop); r != nil { + return r, nil } } } diff --git a/pkg/tcpip/stack/stack_test.go b/pkg/tcpip/stack/stack_test.go index 0ed358e51..10def3921 100644 --- a/pkg/tcpip/stack/stack_test.go +++ b/pkg/tcpip/stack/stack_test.go @@ -4836,6 +4836,91 @@ func TestFindRouteWithForwarding(t *testing.T) { } } +func TestFindRoutePrefersLocalAddrOnlyForLocallyGeneratedTraffic(t *testing.T) { + const ( + nicID1 = 1 + nicID2 = 2 + ) + var ( + nic1Addr = tcpip.AddrFromSlice([]byte("\x01\x00\x00\x00")) + nic2Addr = tcpip.AddrFromSlice([]byte("\x02\x00\x00\x00")) + gatewayAddr = tcpip.AddrFromSlice([]byte("\x03\x00\x00\x00")) + ) + + tests := []struct { + name string + localAddr tcpip.Address + remoteAddr tcpip.Address + wantOutgoingNIC tcpip.NICID + }{ + { + name: "locally generated traffic routed through default gateway because we prefer local address on outgoing interface", + localAddr: nic1Addr, + remoteAddr: nic2Addr, + wantOutgoingNIC: nicID1, + }, + { + name: "forwarded traffic routed through NIC 2 because local address preference only applies to locally generated traffic", + localAddr: tcpip.Address{}, + remoteAddr: nic2Addr, + wantOutgoingNIC: nicID2, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + s := stack.New(stack.Options{ + NetworkProtocols: []stack.NetworkProtocolFactory{fakeNetFactory}, + }) + + ep1 := channel.New(1, defaultMTU, "") + if err := s.CreateNIC(nicID1, ep1); err != nil { + t.Fatalf("CreateNIC(%d, _): %s:", nicID1, err) + } + + ep2 := channel.New(1, defaultMTU, "") + if err := s.CreateNIC(nicID2, ep2); err != nil { + t.Fatalf("CreateNIC(%d, _): %s:", nicID2, err) + } + + // NB: we do *not* assign nic2Addr on NIC 2. We are exercising the scenario when we are forwarding + // traffic to an address that we do not own. + protocolAddr1 := tcpip.ProtocolAddress{ + Protocol: fakeNetNumber, + AddressWithPrefix: tcpip.AddressWithPrefix{Address: nic1Addr, PrefixLen: fakeDefaultPrefixLen}, + } + if err := s.AddProtocolAddress(nicID1, protocolAddr1, stack.AddressProperties{}); err != nil { + t.Fatalf("AddProtocolAddress(%d, %+v, {}): %s", nicID1, protocolAddr1, err) + } + + if err := s.SetForwardingDefaultAndAllNICs(fakeNetNumber, true); err != nil { + t.Fatalf("SetForwardingDefaultAndAllNICs(%d, %t): %s", fakeNetNumber, true, err) + } + + unspecifiedSubnet := func() tcpip.Subnet { + unspecifiedSubnet, err := tcpip.NewSubnet(tcpip.AddrFrom4Slice([]byte("\x00\x00\x00\x00")), tcpip.MaskFrom("\x00\x00\x00\x00")) + if err != nil { + t.Fatal(err) + } + return unspecifiedSubnet + }() + s.SetRouteTable([]tcpip.Route{{Destination: nic2Addr.WithPrefix().Subnet(), NIC: nicID2}, {Destination: unspecifiedSubnet, Gateway: gatewayAddr, NIC: nicID1}}) + + r, err := s.FindRoute(0, test.localAddr, test.remoteAddr, fakeNetNumber, false /* multicastLoop */) + if err != nil { + t.Fatalf("FindRoute(0, %s, %s, %d, false): got %s, want nil", test.localAddr, test.remoteAddr, fakeNetNumber, err) + } + if r.NICID() != test.wantOutgoingNIC { + t.Errorf("got r.NICID() = %d, want = %d", r.NICID(), test.wantOutgoingNIC) + } + + if t.Failed() { + t.FailNow() + } + }) + } +} + func TestAddMulticastRoute(t *testing.T) { const ( incomingNICID = 1