From 8a03faaefe4858b848102ccb596b906e0194ef96 Mon Sep 17 00:00:00 2001 From: Chiachang Date: Wed, 4 May 2022 07:45:37 +0000 Subject: 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 --- server/RouteController.cpp | 21 +++++++++++---------- tests/binder_test.cpp | 24 ++++++++++++++++++------ 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 { -- cgit v1.2.3