Merge "Revert "Support evaluating private DNS by latency"" am: d1b3b80399 (c9ae70a7) · Commits · e / os / android_packages_modules_DnsResolver

DnsTlsDispatcher.cpp

+2 −19

Original line number	Diff line number	Diff line
		@@ -182,18 +182,9 @@ DnsTlsTransport::Response DnsTlsDispatcher::query(const DnsTlsServer& server, un
		// stuck, this function also gets blocked.
		const int connectCounter = xport->transport.getConnectCounter();

		Stopwatch stopwatch;
		const auto& result = queryInternal(*xport, query);
		const int64_t timeTaken = saturate_cast<int64_t>(stopwatch.timeTakenUs() / 1000);
		*connectTriggered = (xport->transport.getConnectCounter() > connectCounter);

		const int64_t targetTime = server.latencyThreshold().value_or(INT64_MAX);
		const bool latencyTooHigh = timeTaken > targetTime;
		if (latencyTooHigh) {
		LOG(WARNING) << "DoT query took too long: " << timeTaken << " ms (threshold: " << targetTime
		<< "ms)";
		}

		DnsTlsTransport::Response code = result.code;
		if (code == DnsTlsTransport::Response::success) {
		if (result.response.size() > ans.size()) {
		@@ -215,7 +206,7 @@ DnsTlsTransport::Response DnsTlsDispatcher::query(const DnsTlsServer& server, un
		xport->lastUsed = now;

		// DoT revalidation specific feature.
		if (xport->checkRevalidationNecessary(code, latencyTooHigh)) {
		if (xport->checkRevalidationNecessary(code)) {
		// Even if the revalidation passes, it doesn't guarantee that DoT queries
		// to the xport can stop failing because revalidation creates a new connection
		// to probe while the xport still uses an existing connection. So far, there isn't
		@@ -342,21 +333,13 @@ DnsTlsDispatcher::Transport* DnsTlsDispatcher::getTransport(const Key& key) {
		return (it == mStore.end() ? nullptr : it->second.get());
		}

		bool DnsTlsDispatcher::Transport::checkRevalidationNecessary(DnsTlsTransport::Response code,
		bool latencyTooHigh) {
		bool DnsTlsDispatcher::Transport::checkRevalidationNecessary(DnsTlsTransport::Response code) {
		if (!revalidationEnabled) return false;

		if (code == DnsTlsTransport::Response::network_error) {
		continuousfailureCount++;
		LOG(WARNING) << "continuousfailureCount incremented: network_error, count = "
		<< continuousfailureCount;
		} else if (latencyTooHigh) {
		continuousfailureCount++;
		LOG(WARNING) << "continuousfailureCount incremented: latency too High, count = "
		<< continuousfailureCount;
		} else {
		continuousfailureCount = 0;
		LOG(WARNING) << "continuousfailureCount reset";
		}

		// triggerThreshold must be greater than 0 because the value of revalidationEnabled is true.

DnsTlsDispatcher.h

+1 −2

Original line number	Diff line number	Diff line
		@@ -107,8 +107,7 @@ class DnsTlsDispatcher : public PrivateDnsValidationObserver {
		// whether or not this Transport is usable.
		bool usable() const REQUIRES(sLock);

		bool checkRevalidationNecessary(DnsTlsTransport::Response code, bool latencyTooHigh)
		REQUIRES(sLock);
		bool checkRevalidationNecessary(DnsTlsTransport::Response code) REQUIRES(sLock);

		std::chrono::milliseconds timeout() const { return mTimeout; }

DnsTlsServer.h

+0 −9

Original line number	Diff line number	Diff line
		@@ -86,19 +86,10 @@ struct DnsTlsServer : public IPrivateDnsServer {
		bool active() const override { return mActive; }
		void setActive(bool val) override { mActive = val; }

		// Setter and getter for the latency constraint.
		void setLatencyThreshold(std::optional<int64_t> val) { mLatencyThresholdMs = val; }
		std::optional<int64_t> latencyThreshold() const { return mLatencyThresholdMs; }

		private:
		// State, unrelated to the comparison of DnsTlsServer objects.
		Validation mValidation = Validation::unknown_server;
		bool mActive = false;

		// The DNS response time threshold. If it is set, the latency of this server will be
		// considered when evaluating the server. It is used for the DoT engine to evaluate whether
		// this server, compared to cleartext DNS servers, has relatively high latency or not.
		std::optional<int64_t> mLatencyThresholdMs = std::nullopt;
		};

		// This comparison only checks the IP address. It ignores ports, names, and fingerprints.

Experiments.h

+4 −12

Original line number	Diff line number	Diff line
		@@ -49,19 +49,11 @@ class Experiments {
		// TODO: Migrate other experiment flags to here.
		// (retry_count, retransmission_time_interval)
		static constexpr const char* const kExperimentFlagKeyList[] = {
		"keep_listening_udp",
		"parallel_lookup_release",
		"parallel_lookup_sleep_time",
		"sort_nameservers",
		"dot_async_handshake",
		"dot_connect_timeout_ms",
		"dot_maxtries",
		"dot_revalidation_threshold",
		"dot_xport_unusable_threshold",
		"keep_listening_udp", "parallel_lookup_release", "parallel_lookup_sleep_time",
		"sort_nameservers", "dot_async_handshake", "dot_connect_timeout_ms",
		"dot_maxtries", "dot_revalidation_threshold", "dot_xport_unusable_threshold",
		"dot_query_timeout_ms",
		"avoid_bad_private_dns",
		"min_private_dns_latency_threshold_ms",
		"max_private_dns_latency_threshold_ms"};
		};
		// This value is used in updateInternal as the default value if any flags can't be found.
		static constexpr int kFlagIntDefault = INT_MIN;
		// For testing.

PrivateDnsConfiguration.cpp

+12 −71

Original line number	Diff line number	Diff line
		@@ -21,23 +21,18 @@
		#include <android-base/format.h>
		#include <android-base/logging.h>
		#include <android-base/stringprintf.h>
		#include <netdutils/Stopwatch.h>
		#include <netdutils/ThreadUtil.h>
		#include <sys/socket.h>

		#include "DnsTlsTransport.h"
		#include "Experiments.h"
		#include "ResolverEventReporter.h"
		#include "netd_resolv/resolv.h"
		#include "resolv_cache.h"
		#include "resolv_private.h"
		#include "util.h"

		using aidl::android::net::resolv::aidl::IDnsResolverUnsolicitedEventListener;
		using aidl::android::net::resolv::aidl::PrivateDnsValidationEventParcel;
		using android::base::StringPrintf;
		using android::netdutils::setThreadName;
		using android::netdutils::Stopwatch;
		using std::chrono::milliseconds;

		namespace android {
		@@ -200,23 +195,6 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un
		std::thread validate_thread([this, identity, server, netId, isRevalidation] {
		setThreadName(StringPrintf("TlsVerify_%u", netId).c_str());

		const bool avoidBadPrivateDns =
		Experiments::getInstance()->getFlag("avoid_bad_private_dns", 0);
		std::optional<int64_t> latencyThreshold;
		if (avoidBadPrivateDns) {
		const int maxLatency = Experiments::getInstance()->getFlag(
		"max_private_dns_latency_threshold_ms", kMaxPrivateDnsLatencyThresholdMs);
		const int minLatency = Experiments::getInstance()->getFlag(
		"min_private_dns_latency_threshold_ms", kMinPrivateDnsLatencyThresholdMs);
		const auto do53Latency = resolv_stats_get_average_response_time(netId, PROTO_UDP);
		const int target =
		do53Latency.has_value() ? (3 * do53Latency.value().count() / 1000) : 0;

		// The time is limited to the range [minLatency, maxLatency].
		latencyThreshold = std::clamp(target, minLatency, maxLatency);
		}
		const bool isOpportunisticMode = server.name.empty();

		// cat /proc/sys/net/ipv4/tcp_syn_retries yields "6".
		//
		// Start with a 1 minute delay and backoff to once per hour.
		@@ -237,24 +215,12 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un
		// It can take milliseconds to minutes, up to the SYN retry limit.
		LOG(WARNING) << "Validating DnsTlsServer " << server.toIpString() << " with mark 0x"
		<< std::hex << server.validationMark();
		const bool success = DnsTlsTransport::validate(server, server.validationMark());
		LOG(WARNING) << "validateDnsTlsServer returned " << success << " for "
		<< server.toIpString();

		Stopwatch stopwatch;
		const bool gotAnswer = DnsTlsTransport::validate(server, server.validationMark());
		const int32_t timeTaken = saturate_cast<int32_t>(stopwatch.timeTakenUs() / 1000);
		LOG(WARNING) << fmt::format("validateDnsTlsServer returned {} for {}, took {}ms",
		gotAnswer, server.toIpString(), timeTaken);

		const int64_t targetTime = latencyThreshold.value_or(INT64_MAX);
		bool latencyTooHigh = false;
		if (isOpportunisticMode && timeTaken > targetTime) {
		latencyTooHigh = true;
		LOG(WARNING) << "validateDnsTlsServer took too long: threshold is " << targetTime
		<< "ms";
		}

		// TODO: combine these boolean variables into a bitwise variable.
		const bool needs_reeval = this->recordPrivateDnsValidation(
		identity, netId, gotAnswer, isRevalidation, latencyTooHigh);
		const bool needs_reeval =
		this->recordPrivateDnsValidation(identity, netId, success, isRevalidation);

		if (!needs_reeval) {
		break;
		@@ -267,8 +233,6 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un
		break;
		}
		}

		this->updateServerLatencyThreshold(identity, latencyThreshold, netId);
		});
		validate_thread.detach();
		}
		@@ -304,8 +268,8 @@ void PrivateDnsConfiguration::sendPrivateDnsValidationEvent(const ServerIdentity
		}

		bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& identity,
		unsigned netId, bool gotAnswer,
		bool isRevalidation, bool latencyTooHigh) {
		unsigned netId, bool success,
		bool isRevalidation) {
		constexpr bool NEEDS_REEVALUATION = true;
		constexpr bool DONT_REEVALUATE = false;

		@@ -326,17 +290,14 @@ bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& i
		}

		bool reevaluationStatus = NEEDS_REEVALUATION;
		if (gotAnswer) {
		if (!latencyTooHigh) {
		if (success) {
		reevaluationStatus = DONT_REEVALUATE;
		}
		} else if (mode->second == PrivateDnsMode::OFF) {
		reevaluationStatus = DONT_REEVALUATE;
		} else if (mode->second == PrivateDnsMode::OPPORTUNISTIC && !isRevalidation) {
		reevaluationStatus = DONT_REEVALUATE;
		}

		bool success = gotAnswer;
		auto& tracker = netPair->second;
		auto serverPair = tracker.find(identity);
		if (serverPair == tracker.end()) {
		@@ -351,12 +312,10 @@ bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& i
		reevaluationStatus = DONT_REEVALUATE;
		}

		const bool succeededQuickly = success && !latencyTooHigh;

		// Send private dns validation result to listeners.
		sendPrivateDnsValidationEvent(identity, netId, succeededQuickly);
		sendPrivateDnsValidationEvent(identity, netId, success);

		if (succeededQuickly) {
		if (success) {
		updateServerState(identity, Validation::success, netId);
		} else {
		// Validation failure is expected if a user is on a captive portal.
		@@ -366,7 +325,7 @@ bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& i
		: Validation::fail;
		updateServerState(identity, result, netId);
		}
		LOG(WARNING) << "Validation " << (succeededQuickly ? "success" : "failed");
		LOG(WARNING) << "Validation " << (success ? "success" : "failed");

		return reevaluationStatus;
		}
		@@ -426,24 +385,6 @@ base::Result<IPrivateDnsServer*> PrivateDnsConfiguration::getPrivateDnsLocked(
		return iter->second.get();
		}

		void PrivateDnsConfiguration::updateServerLatencyThreshold(const ServerIdentity& identity,
		std::optional<int64_t> latencyThreshold,
		uint32_t netId) {
		std::lock_guard guard(mPrivateDnsLock);

		const auto result = getPrivateDnsLocked(identity, netId);
		if (!result.ok()) return;

		if (result.value()->isDot()) {
		DnsTlsServer& server = static_cast<DnsTlsServer>(result.value());
		server.setLatencyThreshold(latencyThreshold);
		LOG(INFO) << "Set latencyThreshold "
		<< (latencyThreshold ? std::to_string(latencyThreshold.value()) + "ms"
		: "nullopt")
		<< " to " << server.toIpString();
		}
		}

		void PrivateDnsConfiguration::setObserver(PrivateDnsValidationObserver* observer) {
		std::lock_guard guard(mPrivateDnsLock);
		mObserver = observer;