release-request-a41f1f54-b764-45d4-b9ce-16ae3001be88-for-git_oc-mr1-release-4362737 snap-temp-L87100000106492125

Change-Id: I3b6e0dcc1f36529c1fb978b42e8342fa2187b1f7

6 files changed, 85 insertions, 25 deletions

diff --git a/server/BandwidthController.cpp b/server/BandwidthController.cpp
index 903390bd..4962b7ca 100644
--- a/server/BandwidthController.cpp
+++ b/server/BandwidthController.cpp
@@ -52,6 +52,7 @@
 
 #include <netdutils/Syscalls.h>
 #include "BandwidthController.h"
+#include "FirewallController.h" /* For makeCriticalCommands */
 #include "NatController.h" /* For LOCAL_TETHER_COUNTERS_CHAIN */
 #include "NetdConstants.h"
 #include "ResponseCode.h"
@@ -248,12 +249,25 @@ int BandwidthController::disableBandwidthControl() {
     return 0;
 }
 
-int BandwidthController::enableDataSaver(bool enable) {
-    std::string cmd = StringPrintf(
+std::string BandwidthController::makeDataSaverCommand(IptablesTarget target, bool enable) {
+    std::string cmd;
+    const char *chainName = "bw_data_saver";
+    const char *op = jumpToString(enable ? IptJumpReject : IptJumpReturn);
+    std::string criticalCommands = enable ?
+            FirewallController::makeCriticalCommands(target, chainName) : "";
+    StringAppendF(&cmd,
         "*filter\n"
-        "-R bw_data_saver 1%s\n"
-        "COMMIT\n", jumpToString(enable ? IptJumpReject : IptJumpReturn));
-    return iptablesRestoreFunction(V4V6, cmd, nullptr);
+        ":%s -\n"
+        "%s"
+        "-A %s%s\n"
+        "COMMIT\n", chainName, criticalCommands.c_str(), chainName, op);
+    return cmd;
+}
+
+int BandwidthController::enableDataSaver(bool enable) {
+    int ret = iptablesRestoreFunction(V4, makeDataSaverCommand(V4, enable), nullptr);
+    ret |= iptablesRestoreFunction(V6, makeDataSaverCommand(V6, enable), nullptr);
+    return ret;
 }
 
 int BandwidthController::addNaughtyApps(int numUids, char *appUids[]) {
diff --git a/server/BandwidthController.h b/server/BandwidthController.h
index 1522a567..0eef5811 100644
--- a/server/BandwidthController.h
+++ b/server/BandwidthController.h
@@ -133,6 +133,8 @@ public:
     enum IptFailureLog { IptFailShow, IptFailHide = IptFailShow };
 #endif
 
+    std::string makeDataSaverCommand(IptablesTarget target, bool enable);
+
     int manipulateSpecialApps(const std::vector<std::string>& appStrUids, const std::string& chain,
                               IptJumpOp jumpHandling, IptOp appOp);
 
diff --git a/server/BandwidthControllerTest.cpp b/server/BandwidthControllerTest.cpp
index a0a57da7..8681be43 100644
--- a/server/BandwidthControllerTest.cpp
+++ b/server/BandwidthControllerTest.cpp
@@ -221,20 +221,38 @@ TEST_F(BandwidthControllerTest, TestDisableBandwidthControl) {
 
 TEST_F(BandwidthControllerTest, TestEnableDataSaver) {
     mBw.enableDataSaver(true);
-    std::vector<std::string> expected = {
+    std::string expected4 =
         "*filter\n"
-        "-R bw_data_saver 1 --jump REJECT\n"
-        "COMMIT\n"
-    };
-    expectIptablesRestoreCommands(expected);
+        ":bw_data_saver -\n"
+        "-A bw_data_saver --jump REJECT\n"
+        "COMMIT\n";
+    std::string expected6 =
+        "*filter\n"
+        ":bw_data_saver -\n"
+        "-A bw_data_saver -p icmpv6 --icmpv6-type packet-too-big -j RETURN\n"
+        "-A bw_data_saver -p icmpv6 --icmpv6-type router-solicitation -j RETURN\n"
+        "-A bw_data_saver -p icmpv6 --icmpv6-type router-advertisement -j RETURN\n"
+        "-A bw_data_saver -p icmpv6 --icmpv6-type neighbour-solicitation -j RETURN\n"
+        "-A bw_data_saver -p icmpv6 --icmpv6-type neighbour-advertisement -j RETURN\n"
+        "-A bw_data_saver -p icmpv6 --icmpv6-type redirect -j RETURN\n"
+        "-A bw_data_saver --jump REJECT\n"
+        "COMMIT\n";
+    expectIptablesRestoreCommands({
+        {V4, expected4},
+        {V6, expected6},
+    });
 
     mBw.enableDataSaver(false);
-    expected = {
+    std::string expected = {
         "*filter\n"
-        "-R bw_data_saver 1 --jump RETURN\n"
+        ":bw_data_saver -\n"
+        "-A bw_data_saver --jump RETURN\n"
         "COMMIT\n"
     };
-    expectIptablesRestoreCommands(expected);
+    expectIptablesRestoreCommands({
+        {V4, expected},
+        {V6, expected},
+    });
 }
 
 std::string kIPv4TetherCounters = Join(std::vector<std::string> {
diff --git a/server/FirewallController.cpp b/server/FirewallController.cpp
index 8e32bc94..f5da0691 100644
--- a/server/FirewallController.cpp
+++ b/server/FirewallController.cpp
@@ -239,6 +239,19 @@ int FirewallController::createChain(const char* chain, FirewallType type) {
     return replaceUidChain(chain, type == WHITELIST, NO_UIDS);
 }
 
+/* static */
+std::string FirewallController::makeCriticalCommands(IptablesTarget target, const char* chainName) {
+    // Allow ICMPv6 packets necessary to make IPv6 connectivity work. http://b/23158230 .
+    std::string commands;
+    if (target == V6) {
+        for (size_t i = 0; i < ARRAY_SIZE(ICMPV6_TYPES); i++) {
+            StringAppendF(&commands, "-A %s -p icmpv6 --icmpv6-type %s -j RETURN\n",
+                   chainName, ICMPV6_TYPES[i]);
+        }
+    }
+    return commands;
+}
+
 std::string FirewallController::makeUidRules(IptablesTarget target, const char *name,
         bool isWhitelist, const std::vector<int32_t>& uids) {
     std::string commands;
@@ -264,13 +277,7 @@ std::string FirewallController::makeUidRules(IptablesTarget target, const char *
     StringAppendF(&commands, "-A %s -p tcp --tcp-flags RST RST -j RETURN\n", name);
 
     if (isWhitelist) {
-        // Allow ICMPv6 packets necessary to make IPv6 connectivity work. http://b/23158230 .
-        if (target == V6) {
-            for (size_t i = 0; i < ARRAY_SIZE(ICMPV6_TYPES); i++) {
-                StringAppendF(&commands, "-A %s -p icmpv6 --icmpv6-type %s -j RETURN\n",
-                       name, ICMPV6_TYPES[i]);
-            }
-        }
+        commands.append(makeCriticalCommands(target, name));
     }
 
     // Blacklist chains have UIDs at the end, and new UIDs are added with '-A'.
diff --git a/server/FirewallController.h b/server/FirewallController.h
index 041aa40c..1da9e70c 100644
--- a/server/FirewallController.h
+++ b/server/FirewallController.h
@@ -64,6 +64,8 @@ public:
 
     int replaceUidChain(const char*, bool, const std::vector<int32_t>&);
 
+    static std::string makeCriticalCommands(IptablesTarget target, const char* chainName);
+
     static const char* TABLE;
 
     static const char* LOCAL_INPUT;
diff --git a/tests/binder_test.cpp b/tests/binder_test.cpp
index b2f362ee..02ec1ffc 100644
--- a/tests/binder_test.cpp
+++ b/tests/binder_test.cpp
@@ -57,6 +57,7 @@
 using namespace android;
 using namespace android::base;
 using namespace android::binder;
+using android::base::StartsWith;
 using android::net::INetd;
 using android::net::TunInterface;
 using android::net::UidRange;
@@ -212,13 +213,29 @@ static int bandwidthDataSaverEnabled(const char *binary) {
     // Chain bw_data_saver (1 references)
     // target     prot opt source               destination
     // RETURN     all  --  0.0.0.0/0            0.0.0.0/0
-    EXPECT_EQ(3U, lines.size());
-    if (lines.size() != 3) return -1;
+    //
+    // or:
+    //
+    // Chain bw_data_saver (1 references)
+    // target     prot opt source               destination
+    // ... possibly connectivity critical packet rules here ...
+    // REJECT     all  --  ::/0            ::/0
+
+    EXPECT_GE(lines.size(), 3U);
 
-    EXPECT_TRUE(android::base::StartsWith(lines[2], "RETURN ") ||
-                android::base::StartsWith(lines[2], "REJECT "));
+    if (lines.size() == 3 && StartsWith(lines[2], "RETURN ")) {
+        // Data saver disabled.
+        return 0;
+    }
+
+    size_t minSize = (std::string(binary) == IPTABLES_PATH) ? 3 : 9;
+
+    if (lines.size() >= minSize && StartsWith(lines[lines.size() -1], "REJECT ")) {
+        // Data saver enabled.
+        return 1;
+    }
 
-    return android::base::StartsWith(lines[2], "REJECT");
+    return -1;
 }
 
 bool enableDataSaver(sp<INetd>& netd, bool enable) {