Use iptables-restore to set the incoming packet mark rule.

This speeds up network switching because one rule needs to be
added/removed per interface.

Bug: 28362720
Test: bullhead builds, boots
Test: netd_{unit,integration}_test pass
Test: watch -n1 "adb shell iptables -v -n -t mangle -L INPUT" while switching networks
Change-Id: Ie536db6a50d018c88bb03c5f069965e99e0d162e

7 files changed, 56 insertions, 8 deletions

diff --git a/server/IptablesBaseTest.cpp b/server/IptablesBaseTest.cpp
index f879fe67..ce6008a6 100644
--- a/server/IptablesBaseTest.cpp
+++ b/server/IptablesBaseTest.cpp
@@ -30,6 +30,8 @@
 #define LOG_TAG "IptablesBaseTest"
 #include <cutils/log.h>
 
+using android::base::StringPrintf;
+
 IptablesBaseTest::IptablesBaseTest() {
     sCmds.clear();
     sRestoreCmds.clear();
@@ -87,7 +89,7 @@ FILE *IptablesBaseTest::fake_popen(const char * /* cmd */, const char *type) {
         return NULL;
     }
 
-    std::string realCmd = android::base::StringPrintf("echo '%s'", sPopenContents.front().c_str());
+    std::string realCmd = StringPrintf("echo '%s'", sPopenContents.front().c_str());
     sPopenContents.pop_front();
     return popen(realCmd.c_str(), "r");
 }
@@ -109,6 +111,14 @@ int IptablesBaseTest::fakeExecIptablesRestore(IptablesTarget target, const std::
     return fakeExecIptablesRestoreWithOutput(target, commands, nullptr);
 }
 
+int IptablesBaseTest::fakeExecIptablesRestoreCommand(IptablesTarget target,
+                                                     const std::string& table,
+                                                     const std::string& command,
+                                                     std::string *output) {
+    std::string fullCmd = StringPrintf("-t %s %s", table.c_str(), command.c_str());
+    return fakeExecIptablesRestoreWithOutput(target, fullCmd, output);
+}
+
 int IptablesBaseTest::expectIptablesCommand(IptablesTarget target, int pos,
                                             const std::string& cmd) {
 
diff --git a/server/IptablesBaseTest.h b/server/IptablesBaseTest.h
index b8ce1e28..a8a511f0 100644
--- a/server/IptablesBaseTest.h
+++ b/server/IptablesBaseTest.h
@@ -32,6 +32,8 @@ public:
     static int fakeExecIptablesRestore(IptablesTarget target, const std::string& commands);
     static int fakeExecIptablesRestoreWithOutput(IptablesTarget target, const std::string& commands,
                                                  std::string *output);
+    static int fakeExecIptablesRestoreCommand(IptablesTarget target, const std::string& table,
+                                              const std::string& commands, std::string *output);
     static FILE *fake_popen(const char *cmd, const char *type);
     void expectIptablesCommands(const std::vector<std::string>& expectedCmds);
     void expectIptablesCommands(const ExpectedIptablesCommands& expectedCmds);
diff --git a/server/NetdConstants.cpp b/server/NetdConstants.cpp
index ff3fc2c9..99f4bf98 100644
--- a/server/NetdConstants.cpp
+++ b/server/NetdConstants.cpp
@@ -26,6 +26,7 @@
 
 #define LOG_TAG "Netd"
 
+#include <android-base/stringprintf.h>
 #include <cutils/log.h>
 #include <logwrap/logwrap.h>
 
@@ -128,6 +129,13 @@ int execIptablesRestore(IptablesTarget target, const std::string& commands) {
     return execIptablesRestoreWithOutput(target, commands, nullptr);
 }
 
+int execIptablesRestoreCommand(IptablesTarget target, const std::string& table,
+                               const std::string& command, std::string *output) {
+    std::string fullCmd = android::base::StringPrintf("*%s\n%s\nCOMMIT\n", table.c_str(),
+                                                      command.c_str());
+    return execIptablesRestoreWithOutput(target, fullCmd, output);
+}
+
 /*
  * Check an interface name for plausibility. This should e.g. help against
  * directory traversal.
diff --git a/server/NetdConstants.h b/server/NetdConstants.h
index 668b9bef..0a5136ed 100644
--- a/server/NetdConstants.h
+++ b/server/NetdConstants.h
@@ -47,6 +47,8 @@ int execIptablesSilently(IptablesTarget target, ...);
 int execIptablesRestore(IptablesTarget target, const std::string& commands);
 int execIptablesRestoreWithOutput(IptablesTarget target, const std::string& commands,
                                   std::string *output);
+int execIptablesRestoreCommand(IptablesTarget target, const std::string& table,
+                               const std::string& command, std::string *output);
 bool isIfaceName(const char *name);
 int parsePrefix(const char *prefix, uint8_t *family, void *address, int size, uint8_t *prefixlen);
 
diff --git a/server/RouteController.cpp b/server/RouteController.cpp
index aba1458e..85ab43a5 100644
--- a/server/RouteController.cpp
+++ b/server/RouteController.cpp
@@ -29,22 +29,27 @@
 
 #include "DummyNetwork.h"
 #include "Fwmark.h"
+#include "NetdConstants.h"
 #include "NetlinkCommands.h"
 #include "UidRanges.h"
 
 #include "android-base/file.h"
+#include <android-base/stringprintf.h>
 #define LOG_TAG "Netd"
 #include "log/log.h"
 #include "logwrap/logwrap.h"
 #include "netutils/ifc.h"
 #include "resolv_netid.h"
 
+using android::base::StringPrintf;
 using android::base::WriteStringToFile;
 using android::net::UidRange;
 
 namespace android {
 namespace net {
 
+auto RouteController::iptablesRestoreCommandFunction = execIptablesRestoreCommand;
+
 // BEGIN CONSTANTS --------------------------------------------------------------------------------
 
 const uint32_t RULE_PRIORITY_VPN_OVERRIDE_SYSTEM = 10000;
@@ -437,11 +442,9 @@ WARN_UNUSED_RESULT int modifyIncomingPacketMark(unsigned netId, const char* inte
     fwmark.protectedFromVpn = true;
     fwmark.permission = permission;
 
-    char markString[UINT32_HEX_STRLEN];
-    snprintf(markString, sizeof(markString), "0x%x", fwmark.intValue);
-
-    if (execIptables(V4V6, "-t", "mangle", add ? "-A" : "-D", "INPUT", "-i", interface, "-j",
-                     "MARK", "--set-mark", markString, NULL)) {
+    std::string cmd = StringPrintf("%s INPUT -i %s -j MARK --set-mark 0x%x",
+                                   add ? "-A" : "-D", interface, fwmark.intValue);
+    if (RouteController::iptablesRestoreCommandFunction(V4V6, "mangle", cmd, nullptr) != 0) {
         ALOGE("failed to change iptables rule that sets incoming packet mark");
         return -EREMOTEIO;
     }
diff --git a/server/RouteController.h b/server/RouteController.h
index 48239d7e..579cfe22 100644
--- a/server/RouteController.h
+++ b/server/RouteController.h
@@ -91,6 +91,10 @@ public:
                                             Permission permission) WARN_UNUSED_RESULT;
     static int removeVirtualNetworkFallthrough(unsigned vpnNetId, const char* physicalInterface,
                                                Permission permission) WARN_UNUSED_RESULT;
+
+    // For testing.
+    static int (*iptablesRestoreCommandFunction)(IptablesTarget, const std::string&,
+                                                 const std::string&, std::string *);
 };
 
 // Public because they are called by by RouteControllerTest.cpp.
@@ -100,6 +104,8 @@ int modifyIpRoute(uint16_t action, uint32_t table, const char* interface, const
                   const char* nexthop) WARN_UNUSED_RESULT;
 int flushRoutes(uint32_t table) WARN_UNUSED_RESULT;
 uint32_t getRulePriority(const nlmsghdr *nlh);
+WARN_UNUSED_RESULT int modifyIncomingPacketMark(unsigned netId, const char* interface,
+                                                Permission permission, bool add);
 
 }  // namespace net
 }  // namespace android
diff --git a/server/RouteControllerTest.cpp b/server/RouteControllerTest.cpp
index 6bcb231e..a8468a62 100644
--- a/server/RouteControllerTest.cpp
+++ b/server/RouteControllerTest.cpp
@@ -18,13 +18,21 @@
 
 #include <gtest/gtest.h>
 
+#include "IptablesBaseTest.h"
 #include "NetlinkCommands.h"
 #include "RouteController.h"
 
 namespace android {
 namespace net {
 
-TEST(RouteControllerTest, TestGetRulePriority) {
+class RouteControllerTest : public IptablesBaseTest {
+public:
+    RouteControllerTest() {
+        RouteController::iptablesRestoreCommandFunction = fakeExecIptablesRestoreCommand;
+    }
+};
+
+TEST_F(RouteControllerTest, TestGetRulePriority) {
     // Expect a rule dump for these two families to contain at least the following priorities.
     for (int family : {AF_INET, AF_INET6 }) {
         std::set<uint32_t> expectedPriorities = {
@@ -53,7 +61,7 @@ TEST(RouteControllerTest, TestGetRulePriority) {
     }
 }
 
-TEST(RouteControllerTest, TestRouteFlush) {
+TEST_F(RouteControllerTest, TestRouteFlush) {
     // Pick a table number that's not used by the system.
     const uint32_t table1 = 500;
     const uint32_t table2 = 600;
@@ -76,5 +84,14 @@ TEST(RouteControllerTest, TestRouteFlush) {
               modifyIpRoute(RTM_DELROUTE, table2, "lo", "192.0.2.4/32", NULL));
 }
 
+TEST_F(RouteControllerTest, TestModifyIncomingPacketMark) {
+    static constexpr int TEST_NETID = 30;
+    EXPECT_EQ(0, modifyIncomingPacketMark(TEST_NETID, "netdtest0", PERMISSION_NONE, true));
+    expectIptablesRestoreCommands({ "-t mangle -A INPUT -i netdtest0 -j MARK --set-mark 0x3001e" });
+
+    EXPECT_EQ(0, modifyIncomingPacketMark(TEST_NETID, "netdtest0", PERMISSION_NONE, false));
+    expectIptablesRestoreCommands({ "-t mangle -D INPUT -i netdtest0 -j MARK --set-mark 0x3001e" });
+}
+
 }  // namespace net
 }  // namespace android