From e61ba5865f801adc769df6967dc8ca9d6fa024b5 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Thu, 19 Dec 2024 14:22:54 +0000 Subject: [PATCH 1/4] Use a suitable response in tcp_connected() when initiating a read When 'ISC_R_TIMEDOUT' is received in 'tcp_recv()', it times out the oldest response in the active responses queue, and only after that it checks whether other active responses have also timed out. So when setting a timeout value for a read operation after a successful connection, it makes sense to take the timeout value from the oldest response in the active queue too, because, theoretically, the responses can have different timeout values, e.g. when the TCP dispatch is shared. Currently 'resp' is always NULL. Previously when connect and read timeouts were not separated in dispatch this affected only logging, but now since we are setting a new timeout after a successful connection, we need to choose a suitable response from the active queue. --- lib/dns/dispatch.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/lib/dns/dispatch.c b/lib/dns/dispatch.c index 53c8b4b8a4..12f3d9c5a2 100644 --- a/lib/dns/dispatch.c +++ b/lib/dns/dispatch.c @@ -1871,18 +1871,17 @@ tcp_connected(isc_nmhandle_t *handle, isc_result_t eresult, void *arg) { } } - if (ISC_LIST_EMPTY(disp->active)) { + /* Take the oldest active response. */ + resp = ISC_LIST_HEAD(disp->active); + if (resp == NULL) { /* All responses have been canceled */ disp->state = DNS_DISPATCHSTATE_CANCELED; } else if (eresult == ISC_R_SUCCESS) { disp->state = DNS_DISPATCHSTATE_CONNECTED; isc_nmhandle_attach(handle, &disp->handle); - if (resp != NULL) { - isc_nmhandle_cleartimeout(disp->handle); - if (resp->timeout != 0) { - isc_nmhandle_settimeout(disp->handle, - resp->timeout); - } + isc_nmhandle_cleartimeout(disp->handle); + if (resp->timeout != 0) { + isc_nmhandle_settimeout(disp->handle, resp->timeout); } tcp_startrecv(disp, resp); } else { From 87c453850c716fb8ac283b10668aa95a9cef421e Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Fri, 20 Dec 2024 08:41:03 +0000 Subject: [PATCH 2/4] Fix rtt calculation bug for TCP in the resolver When TCP is used, 'fctx_query()' adds one second to the rtt (round-trip time) value, but there's a bug when the decision about using TCP is made already after the calculation. Move the block of the code which looks up the peers list to decide whether to use TCP into a place that's before the rtt calculation is performed. This commit doesn't add or remove any code, it just moves the code and adds a comment block. --- lib/dns/resolver.c | 115 +++++++++++++++++++++++---------------------- 1 file changed, 60 insertions(+), 55 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 746fe6d947..2aa4ca381a 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -1911,6 +1911,66 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, } } + /* + * Maybe apply DNS64 mappings to IPv4 addresses. + */ + sockaddr = addrinfo->sockaddr; + dns64 = ISC_LIST_HEAD(fctx->res->view->dns64); + if (isc_sockaddr_pf(&sockaddr) == AF_INET && + fctx->res->view->usedns64 && dns64 != NULL) + { + struct in6_addr aaaa; + + result = dns_dns64_aaaafroma( + dns64, NULL, NULL, fctx->res->view->aclenv, 0, + (unsigned char *)&sockaddr.type.sin.sin_addr.s_addr, + aaaa.s6_addr); + if (result == ISC_R_SUCCESS) { + char sockaddrbuf1[ISC_SOCKADDR_FORMATSIZE]; + char sockaddrbuf2[ISC_SOCKADDR_FORMATSIZE]; + + /* format old address */ + isc_sockaddr_format(&sockaddr, sockaddrbuf1, + sizeof(sockaddrbuf1)); + + /* replace address */ + isc_sockaddr_fromin6(&sockaddr, &aaaa, + ntohs(sockaddr.type.sin.sin_port)); + addrinfo->sockaddr = sockaddr; + + /* format new address */ + isc_sockaddr_format(&sockaddr, sockaddrbuf2, + sizeof(sockaddrbuf2)); + isc_log_write(DNS_LOGCATEGORY_RESOLVER, + DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(3), + "Using DNS64 address %s to talk to %s\n", + sockaddrbuf2, sockaddrbuf1); + } + } + + /* + * Check if the address is in the peers list and has a special + * confguration. + */ + if (res->view->peers != NULL) { + dns_peer_t *peer = NULL; + isc_netaddr_t dstip; + bool usetcp = false; + isc_netaddr_fromsockaddr(&dstip, &sockaddr); + result = dns_peerlist_peerbyaddr(res->view->peers, &dstip, + &peer); + if (result == ISC_R_SUCCESS) { + result = dns_peer_getquerysource(peer, &addr); + if (result == ISC_R_SUCCESS) { + have_addr = true; + } + result = dns_peer_getforcetcp(peer, &usetcp); + if (result == ISC_R_SUCCESS && usetcp) { + options |= DNS_FETCHOPT_TCP; + } + } + } + /* * Allow an additional second for the kernel to resend the SYN * (or SYN without ECN in the case of stupid firewalls blocking @@ -1960,61 +2020,6 @@ fctx_query(fetchctx_t *fctx, dns_adbaddrinfo_t *addrinfo, DNS_MESSAGE_INTENTPARSE, &query->rmessage); query->start = isc_time_now(); - /* - * Maybe apply DNS64 mappings to IPv4 addresses. - */ - sockaddr = addrinfo->sockaddr; - dns64 = ISC_LIST_HEAD(fctx->res->view->dns64); - if (isc_sockaddr_pf(&sockaddr) == AF_INET && - fctx->res->view->usedns64 && dns64 != NULL) - { - struct in6_addr aaaa; - - result = dns_dns64_aaaafroma( - dns64, NULL, NULL, fctx->res->view->aclenv, 0, - (unsigned char *)&sockaddr.type.sin.sin_addr.s_addr, - aaaa.s6_addr); - if (result == ISC_R_SUCCESS) { - char sockaddrbuf1[ISC_SOCKADDR_FORMATSIZE]; - char sockaddrbuf2[ISC_SOCKADDR_FORMATSIZE]; - - /* format old address */ - isc_sockaddr_format(&sockaddr, sockaddrbuf1, - sizeof(sockaddrbuf1)); - - /* replace address */ - isc_sockaddr_fromin6(&sockaddr, &aaaa, - ntohs(sockaddr.type.sin.sin_port)); - addrinfo->sockaddr = sockaddr; - - /* format new address */ - isc_sockaddr_format(&sockaddr, sockaddrbuf2, - sizeof(sockaddrbuf2)); - isc_log_write(DNS_LOGCATEGORY_RESOLVER, - DNS_LOGMODULE_RESOLVER, ISC_LOG_DEBUG(3), - "Using DNS64 address %s to talk to %s\n", - sockaddrbuf2, sockaddrbuf1); - } - } - if (res->view->peers != NULL) { - dns_peer_t *peer = NULL; - isc_netaddr_t dstip; - bool usetcp = false; - isc_netaddr_fromsockaddr(&dstip, &sockaddr); - result = dns_peerlist_peerbyaddr(res->view->peers, &dstip, - &peer); - if (result == ISC_R_SUCCESS) { - result = dns_peer_getquerysource(peer, &addr); - if (result == ISC_R_SUCCESS) { - have_addr = true; - } - result = dns_peer_getforcetcp(peer, &usetcp); - if (result == ISC_R_SUCCESS && usetcp) { - query->options |= DNS_FETCHOPT_TCP; - } - } - } - /* * If this is a TCP query, then we need to make a socket and * a dispatch for it here. Otherwise we use the resolver's From 5367ccb561aa62ca711cd31ec2a8f0c41bd63efd Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Fri, 20 Dec 2024 10:39:26 +0000 Subject: [PATCH 3/4] Adjust the resolver-query-timeout test Since the read timeout now works, the resolver time outs from the dispatch level instead of from the "hung fetch" timer, and so the EDE value in 'fctx_expired()' is not being set. Remove the expected EDE value from the test. --- bin/tests/system/resolver/tests.sh | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/bin/tests/system/resolver/tests.sh b/bin/tests/system/resolver/tests.sh index ab94c17c10..c2c2fd4d6f 100755 --- a/bin/tests/system/resolver/tests.sh +++ b/bin/tests/system/resolver/tests.sh @@ -53,16 +53,14 @@ grep -F "no servers could be reached" dig.out.ns1.test${n} >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) -# 'resolver-query-timeout' is set to 5 seconds in ns1, which is lower than the -# current single query timeout value MAX_SINGLE_QUERY_TIMEOUT of 9 seconds, so -# the "hung fetch" timer should kick in, interrupt the non-responsive query and -# send a SERVFAIL answer. +# 'resolver-query-timeout' is set to 5 seconds in ns1, so named should +# interrupt the non-responsive query and send a SERVFAIL answer before dig's +# own timeout fires, which is set to 7 seconds. n=$((n + 1)) echo_i "checking no response handling with a longer than resolver-query-timeout timeout ($n)" ret=0 dig_with_opts +tcp +tries=1 +timeout=7 noresponse.example.net @10.53.0.1 a >dig.out.ns1.test${n} || ret=1 grep -F "status: SERVFAIL" dig.out.ns1.test${n} >/dev/null || ret=1 -grep -F "EDE: 22 (No Reachable Authority)" dig.out.ns1.test${n} >/dev/null || ret=1 if [ $ret != 0 ]; then echo_i "failed"; fi status=$((status + ret)) From a6d6c3cb4539dc2f37b3a8286dd77deca89732d0 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Sat, 21 Dec 2024 09:10:20 +0000 Subject: [PATCH 4/4] Clean up fctx->next_timeout Since the support for non-zero values of stale-answer-client-timeout was removed in bd7463914fe6375e3e9157f305c60d0172f2b312, 'next_timeout' is unused. Clean it up. --- lib/dns/resolver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/dns/resolver.c b/lib/dns/resolver.c index 2aa4ca381a..80b6cbe388 100644 --- a/lib/dns/resolver.c +++ b/lib/dns/resolver.c @@ -360,7 +360,6 @@ struct fetchctx { atomic_uint_fast32_t attributes; isc_timer_t *timer; isc_time_t expires; - isc_time_t next_timeout; isc_interval_t interval; dns_message_t *qmessage; ISC_LIST(resquery_t) queries; @@ -1875,7 +1874,6 @@ fctx_setretryinterval(fetchctx_t *fctx, unsigned int rtt) { seconds = us / US_PER_SEC; us -= seconds * US_PER_SEC; isc_interval_set(&fctx->interval, seconds, us * NS_PER_US); - isc_time_nowplusinterval(&fctx->next_timeout, &fctx->interval); } static isc_result_t