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

@@ -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
 			}
 		}
 	}

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