commit f4b7b1f63e6e8ed3cfeee09570709cad81419458
author Erik Kline <ek@google.com> Thu Jun 14 15:16:27 2018 +0900
committer Erik Kline <ek@google.com> Wed Jul 04 12:16:14 2018 +0900
tree 963815b1bc8912d9e67b83b121b0bd5e5b3fd9fc
parent 4cef4147bfc1c8708c32972191690e1cd449e2e1

Don't continuously reevaluate DNS-over-TLS unless strict mode

This is because sending TCP SYNs and not getting any replies can trip
the mobile data stall detector. The data stall detection mechanism
needs to be re-evaluated, but until such time only do reevaluation
if a network is using strict mode.

Additionally, revalidate failed servers whenever a DNS configuration is
pushed down to netd.

Test: as follows
    - built, flashed, booted
    - observed tcpdump traffic in opportunistic and strict mode
    - ./system/netd/tests/runtests.sh passes
Bug: 64133961
Bug: 72344805
Bug: 109928338
Merged-In: I729bc9cd7ba6cfc088aaf0aec3e770f14d1ac10d
Merged-In: If62c3348fe115f9791a136bf16ccf8bacccff36e
Change-Id: I15a9c2d328fec2910e47a477cbc1dcaa5323675a

server/ResolverController.cpp

diff --git a/server/ResolverController.cpp b/server/ResolverController.cpp
index f8e1fb3..0812e7a 100644
--- a/server/ResolverController.cpp
+++ b/server/ResolverController.cpp
@@ -168,9 +168,7 @@
 
         // Add any new or changed servers to the tracker, and initiate async checks for them.
         for (const auto& server : tlsServers) {
-            // Don't probe a server more than once.  This means that the only way to
-            // re-check a failed server is to remove it and re-add it from the netId.
-            if (tracker.count(server) == 0) {
+            if (needsValidation(tracker, server)) {
                 validatePrivateDnsProvider(server, tracker, netId);
             }
         }
@@ -305,7 +303,15 @@
             return DONT_REEVALUATE;
         }
 
-        bool reevaluationStatus = success ? DONT_REEVALUATE : NEEDS_REEVALUATION;
+        const auto mode = mPrivateDnsModes.find(netId);
+        if (mode == mPrivateDnsModes.end()) {
+            ALOGW("netId %u has no private DNS validation mode", netId);
+            return DONT_REEVALUATE;
+        }
+        const bool modeDoesReevaluation = (mode->second == PrivateDnsMode::STRICT);
+
+        bool reevaluationStatus = (success || !modeDoesReevaluation)
+                ? DONT_REEVALUATE : NEEDS_REEVALUATION;
 
         auto& tracker = netPair->second;
         auto serverPair = tracker.find(server);
@@ -348,9 +354,10 @@
             }
         } else {
             // Validation failure is expected if a user is on a captive portal.
-            // TODO: Trigger a second validation attempt after captive portal login
-            // succeeds.
-            tracker[server] = Validation::fail;
+            // A second validation attempt is triggered in opportunistic mode
+            // by the framework after captive portal login succeeds.
+            tracker[server] = (reevaluationStatus == NEEDS_REEVALUATION)
+                    ? Validation::in_process : Validation::fail;
             if (DBG) {
                 ALOGD("Validation failed for %s!", addrToString(&(server.ss)).c_str());
             }
@@ -359,6 +366,16 @@
         return reevaluationStatus;
     }
 
+
+    // Start validation for newly added servers as well as any servers that have
+    // landed in Validation::fail state. Note that servers that have failed
+    // multiple validation attempts but for which there is still a validating
+    // thread running are marked as being Validation::in_process.
+    static bool needsValidation(const PrivateDnsTracker& tracker, const DnsTlsServer& server) {
+        const auto& iter = tracker.find(server);
+        return (iter == tracker.end()) || (iter->second == Validation::fail);
+    }
+
     EventReporter mEventReporter;
 
     std::mutex mPrivateDnsLock;
@@ -368,7 +385,7 @@
     std::map<unsigned, PrivateDnsTracker> mPrivateDnsTransports GUARDED_BY(mPrivateDnsLock);
     android::sp<android::net::metrics::INetdEventListener>
             mNetdEventListener GUARDED_BY(mPrivateDnsLock);
-} privateDnsConfiguration;
+} sPrivateDnsConfiguration;
 
 }  // namespace
 
@@ -382,7 +399,7 @@
 
 ResolverController::PrivateDnsStatus
 ResolverController::getPrivateDnsStatus(unsigned netId) const {
-    return privateDnsConfiguration.getStatus(netId);
+    return sPrivateDnsConfiguration.getStatus(netId);
 }
 
 int ResolverController::clearDnsServers(unsigned netId) {
@@ -390,7 +407,7 @@
     if (DBG) {
         ALOGD("clearDnsServers netId = %u\n", netId);
     }
-    privateDnsConfiguration.clear(netId);
+    sPrivateDnsConfiguration.clear(netId);
     return 0;
 }
 
@@ -486,7 +503,7 @@
         return -EINVAL;
     }
 
-    const int err = privateDnsConfiguration.set(netId, tlsServers, tlsName, tlsFingerprints);
+    const int err = sPrivateDnsConfiguration.set(netId, tlsServers, tlsName, tlsFingerprints);
     if (err != 0) {
         return err;
     }
@@ -590,7 +607,7 @@
                     static_cast<unsigned>(params.max_samples));
         }
 
-        privateDnsConfiguration.dump(dw, netId);
+        sPrivateDnsConfiguration.dump(dw, netId);
     }
     dw.decIndent();
 }