From 949d9a3ea4ea6048b619c6af27815113f17de5e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 25 May 2020 14:34:56 +0200 Subject: [PATCH 1/6] Remove fctx->reason and a misleading log message The following message: success resolving '' (in ''?) after reducing the advertised EDNS UDP packet size to 512 octets can currently be logged even if the EDNS UDP buffer size advertised in queries sent to a given server had already been set to 512 octets before the fetch context was created (e.g. due to the server responding intermittently). In other words, this log message may be misleading as lowering the advertised EDNS UDP buffer size may not be the actual cause of being successfully resolved. Remove the log message in question to prevent confusion. As this log message is the only existing user of the "reason" field in struct fetchctx, remove that field as well, along with all the code related to it. --- lib/dns/resolver.c | 34 ---------------------------------- 1 file changed, 34 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index a518a1f613..f73f8b70f6 100644 --- a/lib/dns/resolver.c +++ b/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); @@ -2624,8 +2594,6 @@ resquery_send(resquery_t *query) { query->addrinfo); } else if (tried->count >= 2U) { query->options |= DNS_FETCHOPT_EDNS512; - fctx->reason = "reducing the advertised EDNS " - "UDP packet size to 512 octets"; } } } @@ -4583,7 +4551,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 +4963,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; From d7583e7926e317c1eeacd695eca6fb3ad661a8af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 25 May 2020 14:34:56 +0200 Subject: [PATCH 2/6] Restore semantic meaning of DNS_FETCHOPT_EDNS512 When the DNS_FETCHOPT_EDNS512 flag was first introduced [1], it enforced advertising a 512-byte UDP buffer size in an outgoing query. Ever since EDNS processing code got updated [2], that flag has still been set upon detection of certain query timeout patterns, but it has no longer been affecting the calculations of the advertised UDP buffer size in outgoing queries. Restore original semantic meaning of DNS_FETCHOPT_EDNS512 by ensuring the advertised UDP buffer size is set to 512 bytes when that flag is set. Update existing comments and add new ones to improve code readability. [1] see commit 08c90261660649ca7d92065f6f13a61ec5a9a86d [2] see commit 8e15d5eb3a000f1341e6bea0ddbc28d6dd2a0591 --- lib/dns/resolver.c | 39 ++++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index f73f8b70f6..77c6a99912 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -2588,6 +2588,15 @@ 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, @@ -2613,9 +2622,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); @@ -2632,13 +2649,6 @@ resquery_send(resquery_t *query) { udpsize = 512; } - /* - * Was the size forced to 512 in the configuration? - */ - if (udpsize == 512U) { - query->options |= DNS_FETCHOPT_EDNS512; - } - /* * We have talked to this server before. */ @@ -2647,10 +2657,13 @@ resquery_send(resquery_t *query) { } /* - * We know nothing about the peer's capabilities - * so start with minimal EDNS UDP size. + * 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 (udpsize == 0U) { + if (udpsize == 0U || + (query->options & DNS_FETCHOPT_EDNS512) != 0) { udpsize = 512; } From d27f96cc98094cb44af0d03a2ed6d7238c9c2f8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 25 May 2020 14:34:56 +0200 Subject: [PATCH 3/6] Ensure server-specific "edns-udp-size" is obeyed If "edns-udp-size" is set in a "server" block matching the queried server, it is accounted for in the process of determining the advertised UDP buffer size, but its value may still be overridden before the query is sent. This behavior contradicts the ARM which claims that when set, the server-specific "edns-udp-size" value is used for all EDNS queries sent to a given server. Furthermore, calling dns_peer_getudpsize() with the "udpsize" variable as an argument makes the code hard to follow as that call may either update the value of "udpsize" or leave it untouched. Ensure the code matches the documentation by moving the dns_peer_getudpsize() call below all other blocks of code potentially affecting the advertised UDP buffer size, which is where it was located when server-specific "edns-udp-size" support was first implemented [1]. Improve code readability by calling dns_peer_getudpsize() with a helper variable instead of "udpsize". [1] see commit 1c153afce556ff3c687986fb7c4a0b0a7f5e7cd8 --- doc/arm/reference.rst | 3 +++ lib/dns/resolver.c | 20 ++++++++++++-------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 0ea798f90b..582a2c77b2 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -3412,6 +3412,9 @@ Tuning 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. Valid values are 512 to 4096 (values outside this range will be diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 77c6a99912..99278a177f 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -2614,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; @@ -2641,14 +2642,6 @@ resquery_send(resquery_t *query) { } } - if (peer != NULL) { - (void)dns_peer_getudpsize(peer, &udpsize); - } - - if (udpsize == 0U && res->udpsize == 512U) { - udpsize = 512; - } - /* * We have talked to this server before. */ @@ -2667,6 +2660,17 @@ resquery_send(resquery_t *query) { udpsize = 512; } + /* + * If a fixed EDNS UDP buffer size is configured for + * this server, make sure we obey that. + */ + if (peer != NULL) { + (void)dns_peer_getudpsize(peer, &peerudpsize); + if (peerudpsize != 0) { + udpsize = peerudpsize; + } + } + if ((flags & DNS_FETCHOPT_EDNSVERSIONSET) != 0) { version = flags & DNS_FETCHOPT_EDNSVERSIONMASK; version >>= DNS_FETCHOPT_EDNSVERSIONSHIFT; From fb123df2b21b8e5d7f89d0ccf070a6536ef7425e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 25 May 2020 14:34:56 +0200 Subject: [PATCH 4/6] Improve the "hint" variable comment Replace an existing comment with a more verbose explanation of when the "hint" variable is set in resquery_send() and how its value affects the advertised UDP buffer size in outgoing queries. --- lib/dns/resolver.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 99278a177f..e97bc41b5d 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -2643,7 +2643,12 @@ resquery_send(resquery_t *query) { } /* - * We have talked to this server before. + * 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 (hint != 0U) { udpsize = hint; From 8ddd5c2f9cb53105b1950d99baf802e1be7e8cee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 25 May 2020 14:34:56 +0200 Subject: [PATCH 5/6] Update "edns-udp-size" documentation in the ARM Update the description of the process for determining the advertised UDP buffer size in outgoing queries so that it matches the code. --- doc/arm/reference.rst | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 582a2c77b2..42b234fa75 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -3401,16 +3401,23 @@ 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. + 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). - 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.) + 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. From 862a51d8918b9b38a7ecfecc88d3a269ffa9eabe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 25 May 2020 14:34:56 +0200 Subject: [PATCH 6/6] Add CHANGES entry 5417. [cleanup] The code determining the advertised UDP buffer size in outgoing EDNS queries has been refactored to improve its clarity. [GL #1868] --- CHANGES | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGES b/CHANGES index 25924a89fe..c3b3cb3e21 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +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