Add local rule only for default network

The existing flow would add local rules for each physical
interface. The order of the rules in the routing table is
depending on the order that interfaces were added. It may
cause non-deterministic routing depending on the racing of
registering networks. The rule should only be needed for
default network, so update the flow to update rules with
fall through rules updates while switching the default network.

Bug: 184750836
Test: cd system/netd ; atest
Change-Id: I632f249ead6b418df40fa9639104043a66726d23
(cherry picked from commit 2271c127ad8e8e99c675b38b1414ffa092726d25)
Merged-In: I632f249ead6b418df40fa9639104043a66726d23

2 files changed, 29 insertions, 16 deletions

diff --git a/server/RouteController.cpp b/server/RouteController.cpp
index 5ed33cdd..d63dbd2e 100644
--- a/server/RouteController.cpp
+++ b/server/RouteController.cpp
@@ -1252,10 +1252,6 @@ int RouteController::addInterfaceToPhysicalNetwork(unsigned netId, const char* i
                                         MODIFY_NON_UID_BASED_RULES)) {
         return ret;
     }
-    // TODO: Consider to remove regular table if adding local table failed.
-    if (int ret = modifyVpnLocalExclusionRule(true, interface)) {
-        return ret;
-    }
 
     maybeModifyQdiscClsact(interface, ACTION_ADD);
     updateTableNamesFile();
@@ -1270,10 +1266,7 @@ int RouteController::removeInterfaceFromPhysicalNetwork(unsigned netId, const ch
         return ret;
     }
 
-    int ret = modifyVpnLocalExclusionRule(false, interface);
-    // Always perform flushRoute even if removing local exclusion rules failed.
-    ret |= flushRoutes(interface);
-    if (ret) {
+    if (int ret = flushRoutes(interface)) {
         return ret;
     }
 
@@ -1385,13 +1378,21 @@ int RouteController::disableTethering(const char* inputInterface, const char* ou
 
 int RouteController::addVirtualNetworkFallthrough(unsigned vpnNetId, const char* physicalInterface,
                                                   Permission permission) {
-    return modifyVpnFallthroughRule(RTM_NEWRULE, vpnNetId, physicalInterface, permission);
+    if (int ret = modifyVpnFallthroughRule(RTM_NEWRULE, vpnNetId, physicalInterface, permission)) {
+        return ret;
+    }
+
+    return modifyVpnLocalExclusionRule(true /* add */, physicalInterface);
 }
 
 int RouteController::removeVirtualNetworkFallthrough(unsigned vpnNetId,
                                                      const char* physicalInterface,
                                                      Permission permission) {
-    return modifyVpnFallthroughRule(RTM_DELRULE, vpnNetId, physicalInterface, permission);
+    if (int ret = modifyVpnFallthroughRule(RTM_DELRULE, vpnNetId, physicalInterface, permission)) {
+        return ret;
+    }
+
+    return modifyVpnLocalExclusionRule(false /* add */, physicalInterface);
 }
 
 int RouteController::addUsersToPhysicalNetwork(unsigned netId, const char* interface,
diff --git a/tests/binder_test.cpp b/tests/binder_test.cpp
index 519effea..c01d78e9 100644
--- a/tests/binder_test.cpp
+++ b/tests/binder_test.cpp
@@ -225,7 +225,8 @@ class NetdBinderTest : public ::testing::Test {
                                      unique_fd* acceptedSocket);
 
     void createVpnNetworkWithUid(bool secure, uid_t uid, int vpnNetId = TEST_NETID2,
-                                 int fallthroughNetId = TEST_NETID1);
+                                 int fallthroughNetId = TEST_NETID1,
+                                 int nonDefaultNetId = TEST_NETID3);
 
     void createAndSetDefaultNetwork(int netId, const std::string& interface,
                                     int permission = INetd::PERMISSION_NONE);
@@ -3272,18 +3273,26 @@ TEST_F(NetdBinderTest, OemNetdRelated) {
 }
 
 void NetdBinderTest::createVpnNetworkWithUid(bool secure, uid_t uid, int vpnNetId,
-                                             int fallthroughNetId) {
+                                             int fallthroughNetId, int nonDefaultNetId) {
     // Re-init sTun* to ensure route rule exists.
     sTun.destroy();
     sTun.init();
     sTun2.destroy();
     sTun2.init();
+    sTun3.destroy();
+    sTun3.init();
 
     // Create physical network with fallthroughNetId but not set it as default network
     auto config = makeNativeNetworkConfig(fallthroughNetId, NativeNetworkType::PHYSICAL,
                                           INetd::PERMISSION_NONE, false, false);
     EXPECT_TRUE(mNetd->networkCreate(config).isOk());
     EXPECT_TRUE(mNetd->networkAddInterface(fallthroughNetId, sTun.name()).isOk());
+    // Create a physical network to test that local network access does not include the non-default
+    // networks.
+    auto nonDefaultNetworkConfig = makeNativeNetworkConfig(
+            nonDefaultNetId, NativeNetworkType::PHYSICAL, INetd::PERMISSION_NONE, false, false);
+    EXPECT_TRUE(mNetd->networkCreate(nonDefaultNetworkConfig).isOk());
+    EXPECT_TRUE(mNetd->networkAddInterface(nonDefaultNetId, sTun3.name()).isOk());
 
     // Create VPN with vpnNetId
     config.netId = vpnNetId;
@@ -3464,7 +3473,8 @@ void expectVpnFallthroughRuleExists(const std::string& ifName, int vpnNetId) {
 
 void expectVpnFallthroughWorks(android::net::INetd* netdService, bool bypassable, uid_t uid,
                                const TunInterface& fallthroughNetwork,
-                               const TunInterface& vpnNetwork, int vpnNetId = TEST_NETID2,
+                               const TunInterface& vpnNetwork,
+                               const TunInterface& nonDefaultNetwork, int vpnNetId = TEST_NETID2,
                                int fallthroughNetId = TEST_NETID1) {
     // Set default network to NETID_UNSET
     EXPECT_TRUE(netdService->networkSetDefault(NETID_UNSET).isOk());
@@ -3499,8 +3509,10 @@ void expectVpnFallthroughWorks(android::net::INetd* netdService, bool bypassable
     // Check if fallthrough rule exists
     expectVpnFallthroughRuleExists(fallthroughNetwork.name(), vpnNetId);
 
-    // Check if local exclusion rule exists
+    // Check if local exclusion rule exists for default network
     expectVpnLocalExclusionRuleExists(fallthroughNetwork.name(), true);
+    // No local exclusion rule for non-default network
+    expectVpnLocalExclusionRuleExists(nonDefaultNetwork.name(), false);
 
     // Expect fallthrough to default network
     // The fwmark differs depending on whether the VPN is bypassable or not.
@@ -3548,14 +3560,14 @@ TEST_F(NetdBinderTest, SecureVPNFallthrough) {
     createVpnNetworkWithUid(true /* secure */, TEST_UID1);
     // Get current default network NetId
     ASSERT_TRUE(mNetd->networkGetDefault(&mStoredDefaultNetwork).isOk());
-    expectVpnFallthroughWorks(mNetd.get(), false /* bypassable */, TEST_UID1, sTun, sTun2);
+    expectVpnFallthroughWorks(mNetd.get(), false /* bypassable */, TEST_UID1, sTun, sTun2, sTun3);
 }
 
 TEST_F(NetdBinderTest, BypassableVPNFallthrough) {
     createVpnNetworkWithUid(false /* secure */, TEST_UID1);
     // Get current default network NetId
     ASSERT_TRUE(mNetd->networkGetDefault(&mStoredDefaultNetwork).isOk());
-    expectVpnFallthroughWorks(mNetd.get(), true /* bypassable */, TEST_UID1, sTun, sTun2);
+    expectVpnFallthroughWorks(mNetd.get(), true /* bypassable */, TEST_UID1, sTun, sTun2, sTun3);
 }
 
 namespace {