Merge branch '1868-edns-udp-buffer-size-tweaks' into 'master'

EDNS UDP buffer size tweaks

Closes #1868

See merge request !3576

CHANGES

+5417.	[cleanup]	The code determining the advertised UDP buffer size in
+			outgoing EDNS queries has been refactored to improve its
+			clarity. [GL #1868]
+
 5416.	[bug]		Fix a lock order inversion in unix/socket.c. [GL #1859]
 
 5415.	[test]		Address race in dnssec system test that led to

doc/arm/reference.rst

@@ -3401,16 +3401,26 @@ Tuning
    buffer size of 512, as this has the greatest chance of success on the
    first try.
 
-   If the initial query is successful with EDNS advertising a buffer size of
-   512, then ``named`` will advertise progressively larger buffer sizes on
-   successive queries, until responses begin timing out or ``edns-udp-size`` is
-   reached.
-
-   The default buffer sizes used by ``named`` are 512, 1232, 1432, and
-   4096, but never exceeding ``edns-udp-size``. (The values 1232 and
-   1432 are chosen to allow for an IPv4/IPv6 encapsulated UDP message to
-   be sent without fragmentation at the minimum MTU sizes for Ethernet
-   and IPv6 networks.)
+   If the initial query is successful with EDNS advertising a buffer
+   size of 512, then ``named`` will switch to advertising a buffer size
+   of 4096 bytes (unless ``edns-udp-size`` is lower, in which case the
+   latter will be used).
+
+   Query timeouts observed for any given server will affect the buffer
+   size advertised in queries sent to that server.  Depending on
+   observed packet dropping patterns, the advertised buffer size will be
+   lowered to 1432 bytes, 1232 bytes, 512 bytes, or the size of the
+   largest UDP response ever received from a given server, and then
+   clamped to the ``<512, edns-udp-size>`` range.  Per-server EDNS
+   statistics are only retained in memory for the lifetime of a given
+   server's ADB entry.
+
+   (The values 1232 and 1432 are chosen to allow for an IPv4/IPv6
+   encapsulated UDP message to be sent without fragmentation at the
+   minimum MTU sizes for Ethernet and IPv6 networks.)
+
+   Any server-specific ``edns-udp-size`` setting has precedence over all
+   the above rules.
 
 ``max-udp-size``
    Sets the maximum EDNS UDP message size ``named`` will send in bytes.

lib/dns/resolver.c

@@ -369,12 +369,6 @@ struct fetchctx {
 	 */
 	unsigned int nqueries; /* Bucket lock. */
 
-	/*%
-	 * The reason to print when logging a successful
-	 * response to a query.
-	 */
-	const char *reason;
-
 	/*%
 	 * Random numbers to use for mixing up server addresses.
 	 */
@@ -1748,25 +1742,6 @@ fctx_sendevents(fetchctx_t *fctx, isc_result_t result, int line) {
 	}
 }
 
-static inline void
-log_edns(fetchctx_t *fctx) {
-	char domainbuf[DNS_NAME_FORMATSIZE];
-
-	if (fctx->reason == NULL) {
-		return;
-	}
-
-	/*
-	 * We do not know if fctx->domain is the actual domain the record
-	 * lives in or a parent domain so we have a '?' after it.
-	 */
-	dns_name_format(&fctx->domain, domainbuf, sizeof(domainbuf));
-	isc_log_write(dns_lctx, DNS_LOGCATEGORY_EDNS_DISABLED,
-		      DNS_LOGMODULE_RESOLVER, ISC_LOG_INFO,
-		      "success resolving '%s' (in '%s'?) after %s", fctx->info,
-		      domainbuf, fctx->reason);
-}
-
 static void
 fctx_done(fetchctx_t *fctx, isc_result_t result, int line) {
 	dns_resolver_t *res;
@@ -1780,10 +1755,6 @@ fctx_done(fetchctx_t *fctx, isc_result_t result, int line) {
 	res = fctx->res;
 
 	if (result == ISC_R_SUCCESS) {
-		/*%
-		 * Log any deferred EDNS timeout messages.
-		 */
-		log_edns(fctx);
 		no_response = true;
 		if (fctx->qmin_warning != ISC_R_SUCCESS) {
 			isc_log_write(dns_lctx, DNS_LOGCATEGORY_LAME_SERVERS,
@@ -1799,7 +1770,6 @@ fctx_done(fetchctx_t *fctx, isc_result_t result, int line) {
 	}
 
 	fctx->qmin_warning = ISC_R_SUCCESS;
-	fctx->reason = NULL;
 
 	fctx_stopqueries(fctx, no_response, age_untried);
 
@@ -2618,14 +2588,21 @@ resquery_send(resquery_t *query) {
 		isc_sockaddr_t *sockaddr = &query->addrinfo->sockaddr;
 		struct tried *tried;
 
+		/*
+		 * If this is the first timeout for this server in this fetch
+		 * context, try setting EDNS UDP buffer size to the largest UDP
+		 * response size we have seen from this server so far.
+		 *
+		 * If this server has already timed out twice or more in this
+		 * fetch context, force setting the advertised UDP buffer size
+		 * to 512 bytes.
+		 */
 		if ((tried = triededns(fctx, sockaddr)) != NULL) {
 			if (tried->count == 1U) {
 				hint = dns_adb_getudpsize(fctx->adb,
 							  query->addrinfo);
 			} else if (tried->count >= 2U) {
 				query->options |= DNS_FETCHOPT_EDNS512;
-				fctx->reason = "reducing the advertised EDNS "
-					       "UDP packet size to 512 octets";
 			}
 		}
 	}
@@ -2637,6 +2614,7 @@ resquery_send(resquery_t *query) {
 	 */
 	if ((query->options & DNS_FETCHOPT_NOEDNS0) == 0) {
 		if ((query->addrinfo->flags & DNS_FETCHOPT_NOEDNS0) == 0) {
+			uint16_t peerudpsize = 0;
 			unsigned int version = DNS_EDNS_VERSION;
 			unsigned int flags = query->addrinfo->flags;
 			bool reqnsid = res->view->requestnsid;
@@ -2645,9 +2623,17 @@ resquery_send(resquery_t *query) {
 			unsigned char cookie[64];
 			uint16_t padding = 0;
 
-			if ((flags & FCTX_ADDRINFO_EDNSOK) != 0 &&
-			    (query->options & DNS_FETCHOPT_EDNS512) == 0)
-			{
+			/*
+			 * If we ever received an EDNS response from this
+			 * server, initialize 'udpsize' with a value between
+			 * 512 and 4096, based on any potential EDNS timeouts
+			 * observed for this particular server in the past and
+			 * the total number of query timeouts observed for this
+			 * fetch context so far.  Clamp 'udpsize' to the global
+			 * 'edns-udp-size' value (if unset, the latter defaults
+			 * to 4096 bytes).
+			 */
+			if ((flags & FCTX_ADDRINFO_EDNSOK) != 0) {
 				udpsize = dns_adb_probesize(fctx->adb,
 							    query->addrinfo,
 							    fctx->timeouts);
@@ -2656,34 +2642,38 @@ resquery_send(resquery_t *query) {
 				}
 			}
 
-			if (peer != NULL) {
-				(void)dns_peer_getudpsize(peer, &udpsize);
-			}
-
-			if (udpsize == 0U && res->udpsize == 512U) {
-				udpsize = 512;
-			}
-
 			/*
-			 * Was the size forced to 512 in the configuration?
+			 * This server timed out for the first time in this
+			 * fetch context and we received a response from it
+			 * before (either in this fetch context or in a
+			 * different one).  Set 'udpsize' to the size of the
+			 * largest UDP response we have received from this
+			 * server so far.
 			 */
-			if (udpsize == 512U) {
-				query->options |= DNS_FETCHOPT_EDNS512;
+			if (hint != 0U) {
+				udpsize = hint;
 			}
 
 			/*
-			 * We have talked to this server before.
+			 * If we have not received any responses from this
+			 * server before or if this server has already timed
+			 * out twice or more in this fetch context, use an EDNS
+			 * UDP buffer size of 512 bytes.
 			 */
-			if (hint != 0U) {
-				udpsize = hint;
+			if (udpsize == 0U ||
+			    (query->options & DNS_FETCHOPT_EDNS512) != 0) {
+				udpsize = 512;
 			}
 
 			/*
-			 * We know nothing about the peer's capabilities
-			 * so start with minimal EDNS UDP size.
+			 * If a fixed EDNS UDP buffer size is configured for
+			 * this server, make sure we obey that.
 			 */
-			if (udpsize == 0U) {
-				udpsize = 512;
+			if (peer != NULL) {
+				(void)dns_peer_getudpsize(peer, &peerudpsize);
+				if (peerudpsize != 0) {
+					udpsize = peerudpsize;
+				}
 			}
 
 			if ((flags & DNS_FETCHOPT_EDNSVERSIONSET) != 0) {
@@ -4583,7 +4573,6 @@ fctx_timeout(isc_task_t *task, isc_event_t *event) {
 	inc_stats(fctx->res, dns_resstatscounter_querytimeout);
 
 	if (event->ev_type == ISC_TIMEREVENT_LIFE) {
-		fctx->reason = NULL;
 		fctx_done(fctx, ISC_R_TIMEDOUT, __LINE__);
 	} else {
 		isc_result_t result;
@@ -4996,7 +4985,6 @@ fctx_create(dns_resolver_t *res, const dns_name_t *name, dns_rdatatype_t type,
 	atomic_init(&fctx->attributes, 0);
 	fctx->spilled = false;
 	fctx->nqueries = 0;
-	fctx->reason = NULL;
 	fctx->rand_buf = 0;
 	fctx->rand_bits = 0;
 	fctx->timeout = false;