Move all init code to iptables-restore.

This gets rid of one of the last few uses of iptables, and also
reduces startup time from ~750ms to ~150ms.

Bug: 28362720
Test: bullhead builds,boots
Test: netd_{unit,integration}_test pass
Test: rules after "killall netd" look identical
Change-Id: Idf4d8dbc1292cb0017d4546976ad645a4ac7fa08
Merged-In: Ifc7b7045f00f7803b31a22d96a04e208917af5a5

3 files changed, 254 insertions, 77 deletions

diff --git a/server/Controllers.cpp b/server/Controllers.cpp
index a25e05a6..2b4079cb 100644
--- a/server/Controllers.cpp
+++ b/server/Controllers.cpp
@@ -14,6 +14,11 @@
  * limitations under the License.
  */
 
+#include <regex>
+#include <set>
+#include <string>
+
+#include <android-base/strings.h>
 #include <android-base/stringprintf.h>
 
 #define LOG_TAG "Netd"
@@ -29,101 +34,153 @@
 namespace android {
 namespace net {
 
+using android::base::Join;
+using android::base::StringPrintf;
+using android::base::StringAppendF;
+
 auto Controllers::execIptablesRestore  = ::execIptablesRestore;
-auto Controllers::execIptablesSilently = ::execIptablesSilently;
+auto Controllers::execIptablesRestoreWithOutput = ::execIptablesRestoreWithOutput;
 
 namespace {
+
 /**
  * List of module chains to be created, along with explicit ordering. ORDERING
  * IS CRITICAL, AND SHOULD BE TRIPLE-CHECKED WITH EACH CHANGE.
  */
-static const char* FILTER_INPUT[] = {
+static const std::vector<const char*> FILTER_INPUT = {
         // Bandwidth should always be early in input chain, to make sure we
         // correctly count incoming traffic against data plan.
         BandwidthController::LOCAL_INPUT,
         FirewallController::LOCAL_INPUT,
-        NULL,
 };
 
-static const char* FILTER_FORWARD[] = {
+static const std::vector<const char*> FILTER_FORWARD = {
         OEM_IPTABLES_FILTER_FORWARD,
         FirewallController::LOCAL_FORWARD,
         BandwidthController::LOCAL_FORWARD,
         NatController::LOCAL_FORWARD,
-        NULL,
 };
 
-static const char* FILTER_OUTPUT[] = {
+static const std::vector<const char*> FILTER_OUTPUT = {
         OEM_IPTABLES_FILTER_OUTPUT,
         FirewallController::LOCAL_OUTPUT,
         StrictController::LOCAL_OUTPUT,
         BandwidthController::LOCAL_OUTPUT,
-        NULL,
 };
 
-static const char* RAW_PREROUTING[] = {
+static const std::vector<const char*> RAW_PREROUTING = {
         BandwidthController::LOCAL_RAW_PREROUTING,
         IdletimerController::LOCAL_RAW_PREROUTING,
         NatController::LOCAL_RAW_PREROUTING,
-        NULL,
 };
 
-static const char* MANGLE_POSTROUTING[] = {
+static const std::vector<const char*> MANGLE_POSTROUTING = {
         OEM_IPTABLES_MANGLE_POSTROUTING,
         BandwidthController::LOCAL_MANGLE_POSTROUTING,
         IdletimerController::LOCAL_MANGLE_POSTROUTING,
-        NULL,
 };
 
-static const char* MANGLE_INPUT[] = {
+static const std::vector<const char*> MANGLE_INPUT = {
         WakeupController::LOCAL_MANGLE_INPUT,
         RouteController::LOCAL_MANGLE_INPUT,
-        NULL,
 };
 
-static const char* MANGLE_FORWARD[] = {
+static const std::vector<const char*> MANGLE_FORWARD = {
         NatController::LOCAL_MANGLE_FORWARD,
-        NULL,
 };
 
-static const char* NAT_PREROUTING[] = {
+static const std::vector<const char*> NAT_PREROUTING = {
         OEM_IPTABLES_NAT_PREROUTING,
-        NULL,
 };
 
-static const char* NAT_POSTROUTING[] = {
+static const std::vector<const char*> NAT_POSTROUTING = {
         NatController::LOCAL_NAT_POSTROUTING,
-        NULL,
 };
 
+// Commands to create child chains and to match created chains in iptables -S output. Keep in sync.
+static const char* CHILD_CHAIN_TEMPLATE = "-A %s -j %s\n";
+static const std::regex CHILD_CHAIN_REGEX("^-A ([^ ]+) -j ([^ ]+)$",
+                                          std::regex_constants::extended);
+
 }  // namespace
 
 /* static */
+std::set<std::string> Controllers::findExistingChildChains(const IptablesTarget target,
+                                                           const char* table,
+                                                           const char* parentChain) {
+    if (target == V4V6) {
+        ALOGE("findExistingChildChains only supports one protocol at a time");
+        abort();
+    }
+
+    std::set<std::string> existing;
+
+    // List the current contents of parentChain.
+    //
+    // TODO: there is no guarantee that nothing else modifies the chain in the few milliseconds
+    // between when we list the existing rules and when we delete them. However:
+    // - Since this code is only run on startup, nothing else in netd will be running.
+    // - While vendor code is known to add its own rules to chains created by netd, it should never
+    //   be modifying the rules in childChains or the rules that hook said chains into their parent
+    //   chains.
+    std::string command = StringPrintf("*%s\n-S %s\nCOMMIT\n", table, parentChain);
+    std::string output;
+    if (Controllers::execIptablesRestoreWithOutput(target, command, &output) == -1) {
+        ALOGE("Error listing chain %s in table %s\n", parentChain, table);
+        return existing;
+    }
+
+    // The only rules added by createChildChains are of the simple form "-A <parent> -j <child>".
+    // Find those rules and add each one's child chain to existing.
+    std::smatch matches;
+    std::stringstream stream(output);
+    std::string rule;
+    while (std::getline(stream, rule, '\n')) {
+        if (std::regex_search(rule, matches, CHILD_CHAIN_REGEX) && matches[1] == parentChain) {
+            existing.insert(matches[2]);
+        }
+    }
+
+    return existing;
+}
+
+/* static */
 void Controllers::createChildChains(IptablesTarget target, const char* table,
                                     const char* parentChain,
-                                    const char** childChains,
+                                    const std::vector<const char*>& childChains,
                                     bool exclusive) {
-    std::string command = android::base::StringPrintf("*%s\n", table);
-
-    // If we're the exclusive owner of this chain, clear it entirely. This saves us from having to
-    // run one execIptablesSilently command to delete each child chain. We can't use -D in
-    // iptables-restore because it's a fatal error if the rule doesn't exist.
+    std::string command = StringPrintf("*%s\n", table);
+
+    // We cannot just clear all the chains we create because vendor code modifies filter OUTPUT and
+    // mangle POSTROUTING directly. So:
+    //
+    // - If we're the exclusive owner of this chain, simply clear it entirely.
+    // - If not, then list the chain's current contents to ensure that if we restart after a crash,
+    //   we leave the existing rules alone in the positions they currently occupy. This is faster
+    //   than blindly deleting our rules and recreating them, because deleting a rule that doesn't
+    //   exists causes iptables-restore to quit, which takes ~30ms per delete. It's also more
+    //   correct, because if we delete rules and re-add them, they'll be in the wrong position with
+    //   regards to the vendor rules.
+    //
     // TODO: Make all chains exclusive once vendor code uses the oem_* rules.
+    std::set<std::string> existingChildChains;
     if (exclusive) {
         // Just running ":chain -" flushes user-defined chains, but not built-in chains like INPUT.
         // Since at this point we don't know if parentChain is a built-in chain, do both.
-        command += android::base::StringPrintf(":%s -\n", parentChain);
-        command += android::base::StringPrintf("-F %s\n", parentChain);
+        StringAppendF(&command, ":%s -\n", parentChain);
+        StringAppendF(&command, "-F %s\n", parentChain);
+    } else {
+        existingChildChains = findExistingChildChains(target, table, parentChain);
     }
 
-    const char** childChain = childChains;
-    do {
-        if (!exclusive) {
-            execIptablesSilently(target, "-t", table, "-D", parentChain, "-j", *childChain, NULL);
+    for (const auto& childChain : childChains) {
+        // Always clear the child chain.
+        StringAppendF(&command, ":%s -\n", childChain);
+        // But only add it to the parent chain if it's not already there.
+        if (existingChildChains.find(childChain) == existingChildChains.end()) {
+            StringAppendF(&command, CHILD_CHAIN_TEMPLATE, parentChain, childChain);
         }
-        command += android::base::StringPrintf(":%s -\n", *childChain);
-        command += android::base::StringPrintf("-A %s -j %s\n", parentChain, *childChain);
-    } while (*(++childChain) != NULL);
+    }
     command += "COMMIT\n";
     execIptablesRestore(target, command);
 }
@@ -163,10 +220,10 @@ void Controllers::initChildChains() {
     createChildChains(V4, "nat", "PREROUTING", NAT_PREROUTING, true);
     createChildChains(V4, "nat", "POSTROUTING", NAT_POSTROUTING, true);
 
-    // We cannot use createChildChainsFast for all chains because vendor code modifies filter OUTPUT
-    // and mangle POSTROUTING directly.
-    createChildChains(V4V6, "filter", "OUTPUT", FILTER_OUTPUT, false);
-    createChildChains(V4V6, "mangle", "POSTROUTING", MANGLE_POSTROUTING, false);
+    createChildChains(V4, "filter", "OUTPUT", FILTER_OUTPUT, false);
+    createChildChains(V6, "filter", "OUTPUT", FILTER_OUTPUT, false);
+    createChildChains(V4, "mangle", "POSTROUTING", MANGLE_POSTROUTING, false);
+    createChildChains(V6, "mangle", "POSTROUTING", MANGLE_POSTROUTING, false);
 }
 
 void Controllers::initIptablesRules() {
diff --git a/server/Controllers.h b/server/Controllers.h
index 0754932c..53854cff 100644
--- a/server/Controllers.h
+++ b/server/Controllers.h
@@ -63,10 +63,13 @@ private:
     friend class ControllersTest;
     void initIptablesRules();
     static void initChildChains();
+    static std::set<std::string> findExistingChildChains(const IptablesTarget target,
+                                                         const char* table,
+                                                         const char* parentChain);
     static void createChildChains(IptablesTarget target, const char* table, const char* parentChain,
-                                  const char** childChains, bool exclusive);
-    static int (*execIptablesSilently)(IptablesTarget target, ...);
+                                  const std::vector<const char*>& childChains, bool exclusive);
     static int (*execIptablesRestore)(IptablesTarget, const std::string&);
+    static int (*execIptablesRestoreWithOutput)(IptablesTarget, const std::string&, std::string *);
 };
 
 extern Controllers* gCtls;
diff --git a/server/ControllersTest.cpp b/server/ControllersTest.cpp
index 6f417982..3ca5d81e 100644
--- a/server/ControllersTest.cpp
+++ b/server/ControllersTest.cpp
@@ -16,30 +16,60 @@
  * ControllersTest.cpp - unit tests for Controllers.cpp
  */
 
+#include <set>
 #include <string>
 #include <vector>
 
+#include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
+#include <android-base/strings.h>
+
 #include "Controllers.h"
 #include "IptablesBaseTest.h"
 
+using testing::ContainerEq;
+
 namespace android {
 namespace net {
 
 class ControllersTest : public IptablesBaseTest {
   public:
     ControllersTest() {
-        Controllers::execIptablesSilently = fakeExecIptables;
         Controllers::execIptablesRestore = fakeExecIptablesRestore;
+        Controllers::execIptablesRestoreWithOutput = fakeExecIptablesRestoreWithOutput;
     }
 
   protected:
     void initChildChains() { Controllers::initChildChains(); };
+    std::set<std::string> findExistingChildChains(IptablesTarget a, const char* b, const char*c) {
+        return Controllers::findExistingChildChains(a, b, c);
+    }
 };
 
+TEST_F(ControllersTest, TestFindExistingChildChains) {
+    ExpectedIptablesCommands expectedCmds = {
+        { V6, "*raw\n-S PREROUTING\nCOMMIT\n" },
+    };
+    sIptablesRestoreOutput.push_back(
+        "-P PREROUTING ACCEPT\n"
+        "-A PREROUTING -j bw_raw_PREROUTING\n"
+        "-A PREROUTING -j idletimer_raw_PREROUTING\n"
+        "-A PREROUTING -j natctrl_raw_PREROUTING\n"
+    );
+    std::set<std::string> expectedChains = {
+        "bw_raw_PREROUTING",
+        "idletimer_raw_PREROUTING",
+        "natctrl_raw_PREROUTING",
+    };
+    std::set<std::string> actual = findExistingChildChains(V6, "raw", "PREROUTING");
+    EXPECT_THAT(expectedChains, ContainerEq(actual));
+    expectIptablesRestoreCommands(expectedCmds);
+}
+
 TEST_F(ControllersTest, TestInitIptablesRules) {
-    ExpectedIptablesCommands expectedRestoreCommands = {
+    // Test what happens when we boot and there are no rules.
+    ExpectedIptablesCommands expected = {
         { V4V6, "*filter\n"
                 ":INPUT -\n"
                 "-F INPUT\n"
@@ -103,47 +133,134 @@ TEST_F(ControllersTest, TestInitIptablesRules) {
                 "-A POSTROUTING -j natctrl_nat_POSTROUTING\n"
                 "COMMIT\n"
         },
-        { V4V6, "*filter\n"
-                ":oem_out -\n"
-                "-A OUTPUT -j oem_out\n"
-                ":fw_OUTPUT -\n"
-                "-A OUTPUT -j fw_OUTPUT\n"
-                ":st_OUTPUT -\n"
-                "-A OUTPUT -j st_OUTPUT\n"
-                ":bw_OUTPUT -\n"
-                "-A OUTPUT -j bw_OUTPUT\n"
-                "COMMIT\n"
+        { V4, "*filter\n"
+              "-S OUTPUT\n"
+              "COMMIT\n" },
+        { V4, "*filter\n"
+              ":oem_out -\n"
+              "-A OUTPUT -j oem_out\n"
+              ":fw_OUTPUT -\n"
+              "-A OUTPUT -j fw_OUTPUT\n"
+              ":st_OUTPUT -\n"
+              "-A OUTPUT -j st_OUTPUT\n"
+              ":bw_OUTPUT -\n"
+              "-A OUTPUT -j bw_OUTPUT\n"
+              "COMMIT\n"
         },
-        { V4V6, "*mangle\n"
-                ":oem_mangle_post -\n"
-                "-A POSTROUTING -j oem_mangle_post\n"
-                ":bw_mangle_POSTROUTING -\n"
-                "-A POSTROUTING -j bw_mangle_POSTROUTING\n"
-                ":idletimer_mangle_POSTROUTING -\n"
-                "-A POSTROUTING -j idletimer_mangle_POSTROUTING\n"
-                "COMMIT\n"
+        { V6, "*filter\n"
+              "-S OUTPUT\n"
+              "COMMIT\n" },
+        { V6, "*filter\n"
+              ":oem_out -\n"
+              "-A OUTPUT -j oem_out\n"
+              ":fw_OUTPUT -\n"
+              "-A OUTPUT -j fw_OUTPUT\n"
+              ":st_OUTPUT -\n"
+              "-A OUTPUT -j st_OUTPUT\n"
+              ":bw_OUTPUT -\n"
+              "-A OUTPUT -j bw_OUTPUT\n"
+              "COMMIT\n"
+        },
+        { V4, "*mangle\n"
+              "-S POSTROUTING\n"
+              "COMMIT\n" },
+        { V4, "*mangle\n"
+              ":oem_mangle_post -\n"
+              "-A POSTROUTING -j oem_mangle_post\n"
+              ":bw_mangle_POSTROUTING -\n"
+              "-A POSTROUTING -j bw_mangle_POSTROUTING\n"
+              ":idletimer_mangle_POSTROUTING -\n"
+              "-A POSTROUTING -j idletimer_mangle_POSTROUTING\n"
+              "COMMIT\n"
+        },
+        { V6, "*mangle\n"
+              "-S POSTROUTING\n"
+              "COMMIT\n" },
+        { V6, "*mangle\n"
+              ":oem_mangle_post -\n"
+              "-A POSTROUTING -j oem_mangle_post\n"
+              ":bw_mangle_POSTROUTING -\n"
+              "-A POSTROUTING -j bw_mangle_POSTROUTING\n"
+              ":idletimer_mangle_POSTROUTING -\n"
+              "-A POSTROUTING -j idletimer_mangle_POSTROUTING\n"
+              "COMMIT\n"
         },
     };
+
+    // Check that we run these commands and these only.
     initChildChains();
-    expectIptablesRestoreCommands(expectedRestoreCommands);
-
-    std::vector<std::string> expectedIptablesCommands = {
-        "-t filter -D OUTPUT -j oem_out",
-        "-t filter -D OUTPUT -j fw_OUTPUT",
-        "-t filter -D OUTPUT -j st_OUTPUT",
-        "-t filter -D OUTPUT -j bw_OUTPUT",
-        "-t mangle -D POSTROUTING -j oem_mangle_post",
-        "-t mangle -D POSTROUTING -j bw_mangle_POSTROUTING",
-        "-t mangle -D POSTROUTING -j idletimer_mangle_POSTROUTING",
-    };
-    expectIptablesCommands(expectedIptablesCommands);
+    expectIptablesRestoreCommands(expected);
+    expectIptablesRestoreCommands(ExpectedIptablesCommands{});
+
+    // Now test what happens when some rules exist (e.g., if we crash and restart).
+
+    // First, explicitly tell the iptables test code to return empty output to all the commands we
+    // send. This allows us to tell it to return non-empty output to particular commands in the
+    // following code.
+    for (size_t i = 0; i < expected.size(); i++) {
+        sIptablesRestoreOutput.push_back("");
+    }
+
+    // Define a macro to remove a substring from a string. We use a macro instead of a function so
+    // we can assert in it. In the following code, we use ASSERT_* to check for programming errors
+    // in the test code, and EXPECT_* to check for errors in the actual code.
+#define DELETE_SUBSTRING(substr, str) {                      \
+        size_t start = (str).find((substr));                 \
+        ASSERT_NE(std::string::npos, start);                 \
+        (str).erase(start, strlen((substr)));                \
+        ASSERT_EQ(std::string::npos, (str).find((substr)));  \
+    }
+
+    // Now set test expectations.
+
+    // 1. Test that if we find rules that we don't create ourselves, we ignore them.
+    // First check that command #7 is where we list the OUTPUT chain in the (IPv4) filter table:
+    ASSERT_NE(std::string::npos, expected[7].second.find("*filter\n-S OUTPUT\n"));
+    // ... and pretend that when we run that command, we find the following rules. Because we don't
+    // create any of these rules ourselves, our behaviour is unchanged.
+    sIptablesRestoreOutput[7] =
+        "-P OUTPUT ACCEPT\n"
+        "-A OUTPUT -o r_rmnet_data8 -p udp -m udp --dport 1900 -j DROP\n";
 
-    // ... and nothing more.
-    expectedRestoreCommands = {};
-    expectIptablesRestoreCommands(expectedRestoreCommands);
+    // 2. Test that rules that we create ourselves are not added if they already exist.
+    // Pretend that when we list the OUTPUT chain in the (IPv6) filter table, we find the oem_out
+    // and st_OUTPUT chains:
+    ASSERT_NE(std::string::npos, expected[9].second.find("*filter\n-S OUTPUT\n"));
+    sIptablesRestoreOutput[9] =
+        "-A OUTPUT -j oem_out\n"
+        "-A OUTPUT -j st_OUTPUT\n";
+    // ... and expect that when we populate the OUTPUT chain, we do not re-add them.
+    DELETE_SUBSTRING("-A OUTPUT -j oem_out\n", expected[10].second);
+    DELETE_SUBSTRING("-A OUTPUT -j st_OUTPUT\n", expected[10].second);
 
-    expectedIptablesCommands = {};
-    expectIptablesCommands(expectedIptablesCommands);
+    // 3. Now test that when we list the POSTROUTING chain in the mangle table, we find a mixture of
+    // netd-created rules and vendor rules:
+    ASSERT_NE(std::string::npos, expected[13].second.find("*mangle\n-S POSTROUTING\n"));
+    sIptablesRestoreOutput[13] =
+        "-P POSTROUTING ACCEPT\n"
+        "-A POSTROUTING -j oem_mangle_post\n"
+        "-A POSTROUTING -j bw_mangle_POSTROUTING\n"
+        "-A POSTROUTING -j idletimer_mangle_POSTROUTING\n"
+        "-A POSTROUTING -j qcom_qos_reset_POSTROUTING\n"
+        "-A POSTROUTING -j qcom_qos_filter_POSTROUTING\n";
+    // and expect that we don't re-add the netd-created rules that already exist.
+    DELETE_SUBSTRING("-A POSTROUTING -j oem_mangle_post\n", expected[14].second);
+    DELETE_SUBSTRING("-A POSTROUTING -j bw_mangle_POSTROUTING\n", expected[14].second);
+    DELETE_SUBSTRING("-A POSTROUTING -j idletimer_mangle_POSTROUTING\n", expected[14].second);
+
+    // In this last case, also check that our expectations are reasonable.
+    std::string expectedCmd14 =
+        "*mangle\n"
+        ":oem_mangle_post -\n"
+        ":bw_mangle_POSTROUTING -\n"
+        ":idletimer_mangle_POSTROUTING -\n"
+        "COMMIT\n";
+    ASSERT_EQ(expectedCmd14, expected[14].second);
+
+    // Finally, actually test that initChildChains runs the expected commands, and nothing more.
+    initChildChains();
+    expectIptablesRestoreCommands(expected);
+    expectIptablesRestoreCommands(ExpectedIptablesCommands{});
 }
 
 }  // namespace net