This is an automated email from the git hooks/post-receive script. dgoulet pushed a commit to branch main in repository tor. commit 347eaa32bfbcdfb13c9e34397d8b068ed6f6ba17 Author: David Goulet <dgoulet@torproject.org> AuthorDate: Mon Dec 13 10:56:43 2021 -0500 relay: Lower DNS Exit-side timeout Introduces two new consensus parameter: exit_dns_timeout: Number of seconds before libevent should consider the DNS request a timeout. exit_dns_num_attempts: Number of attempts that libeven should retry a previously failing query before calling it a timeout. Closes #40312 Signed-off-by: David Goulet <dgoulet@torproject.org> --- changes/ticket40312 | 5 ++++ src/feature/relay/dns.c | 68 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 67 insertions(+), 6 deletions(-) diff --git a/changes/ticket40312 b/changes/ticket40312 new file mode 100644 index 0000000000..ef572d0e69 --- /dev/null +++ b/changes/ticket40312 @@ -0,0 +1,5 @@ + o Major bugfixes (relay, DNS): + - Lower the DNS timeout from 3 attempts at 5 seconds each to 2 attempts at + 1 seconds each. Two new consensus parameters were added to control these + values. See ticket for more details on why. Fixes bug 40312; bugfix on + 0.3.5.1-alpha. diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c index 180f2cbdd1..9e504a7cfa 100644 --- a/src/feature/relay/dns.c +++ b/src/feature/relay/dns.c @@ -61,6 +61,7 @@ #include "core/or/relay.h" #include "feature/control/control_events.h" #include "feature/relay/dns.h" +#include "feature/nodelist/networkstatus.h" #include "feature/relay/router.h" #include "feature/relay/routermode.h" #include "feature/stats/rephist.h" @@ -1353,6 +1354,54 @@ configured_nameserver_address(const size_t idx) } #endif /* defined(HAVE_EVDNS_BASE_GET_NAMESERVER_ADDR) */ +/** Return a pointer to a stack allocated buffer containing the string + * representation of the exit_dns_timeout consensus parameter. */ +static const char * +get_consensus_param_exit_dns_timeout(void) +{ + static char str[4]; + + /* Get the Exit DNS timeout value from the consensus or default. This is in + * milliseconds. */ +#define EXIT_DNS_TIMEOUT_DEFAULT (1000) +#define EXIT_DNS_TIMEOUT_MIN (1) +#define EXIT_DNS_TIMEOUT_MAX (120000) + int32_t val = networkstatus_get_param(NULL, "exit_dns_timeout", + EXIT_DNS_TIMEOUT_DEFAULT, + EXIT_DNS_TIMEOUT_MIN, + EXIT_DNS_TIMEOUT_MAX); + /* NOTE: We convert it to seconds because libevent only supports that. In the + * future, if we support different resolver(s), we might want to specialize + * this call. */ + + /* NOTE: We also don't allow 0 and so we must cap the division to 1 second + * else all DNS request would fail if the consensus would ever tell us a + * value below 1000 (1 sec). */ + val = MAX(1, val / 1000); + + tor_snprintf(str, sizeof(str), "%d", val); + return str; +} + +/** Return a pointer to a stack allocated buffer containing the string + * representation of the exit_dns_num_attempts consensus parameter. */ +static const char * +get_consensus_param_exit_dns_attempts(void) +{ + static char str[4]; + + /* Get the Exit DNS number of attempt value from the consensus or default. */ +#define EXIT_DNS_NUM_ATTEMPTS_DEFAULT (2) +#define EXIT_DNS_NUM_ATTEMPTS_MIN (0) +#define EXIT_DNS_NUM_ATTEMPTS_MAX (255) + int32_t val = networkstatus_get_param(NULL, "exit_dns_num_attempts", + EXIT_DNS_NUM_ATTEMPTS_DEFAULT, + EXIT_DNS_NUM_ATTEMPTS_MIN, + EXIT_DNS_NUM_ATTEMPTS_MAX); + tor_snprintf(str, sizeof(str), "%d", val); + return str; +} + /** Configure eventdns nameservers if force is true, or if the configuration * has changed since the last time we called this function, or if we failed on * our last attempt. On Unix, this reads from /etc/resolv.conf or @@ -1489,12 +1538,19 @@ configure_nameservers(int force) // of a full queue. SET("max-inflight:", "8192"); - // Two retries at 5 and 10 seconds for bind9/named which relies on - // clients to handle retries. Second retry for retried circuits with - // extended 15 second timeout. Superfluous with local-system Unbound - // instance--has its own elaborate retry scheme. - SET("timeout:", "5"); - SET("attempts:","3"); + /* Set timeout to be 1 second. This tells libevent that it shouldn't wait + * more than N second to drop a DNS query and consider it "timed out". It is + * very important to differentiate here a libevent timeout and a DNS server + * timeout. And so, by setting this to N second, libevent sends back + * "DNS_ERR_TIMEOUT" if that N second is reached which does NOT indicate that + * the query itself timed out in transit. */ + SET("timeout:", get_consensus_param_exit_dns_timeout()); + + /* This tells libevent to attemps up to X times a DNS query if the previous + * one failed to complete within N second. We believe that this should be + * enough to catch temporary hiccups on the first query. But after that, it + * should signal us that it won't be able to resolve it. */ + SET("attempts:", get_consensus_param_exit_dns_attempts()); if (options->ServerDNSRandomizeCase) SET("randomize-case:", "1"); -- To stop receiving notification emails like this one, please contact the administrator of this repository.

This is an automated email from the git hooks/post-receive script. dgoulet pushed a commit to branch main in repository tor. commit 7ce17c2b008dee04b209ac698e7a380eae63987e Author: David Goulet <dgoulet@torproject.org> AuthorDate: Tue Mar 15 15:33:35 2022 -0400 relay: Reconfigure libevent options only on DNS params change Related #40312 Signed-off-by: David Goulet <dgoulet@torproject.org> --- src/feature/relay/dns.c | 103 +++++++++++++++++++++++++++--------------------- 1 file changed, 59 insertions(+), 44 deletions(-) diff --git a/src/feature/relay/dns.c b/src/feature/relay/dns.c index 8467b9c0a4..0af80d52ae 100644 --- a/src/feature/relay/dns.c +++ b/src/feature/relay/dns.c @@ -111,6 +111,7 @@ static int answer_is_wildcarded(const char *ip); static int evdns_err_is_transient(int err); static void inform_pending_connections(cached_resolve_t *resolve); static void make_pending_resolve_cached(cached_resolve_t *cached); +static void configure_libevent_options(void); #ifdef DEBUG_DNS_CACHE static void assert_cache_ok_(void); @@ -222,7 +223,7 @@ dns_new_consensus_params(const networkstatus_t *ns) /* Consensus has parameters for the Exit relay DNS side and so we only reset * the DNS nameservers if we are in server mode. */ if (server_mode(get_options())) { - dns_reset(); + configure_libevent_options(); } } @@ -1415,6 +1416,60 @@ get_consensus_param_exit_dns_attempts(void) return str; } +/** Configure the libevent options. This can be called after initialization. + * This should never be called without the evdns base pointer initialized. */ +static void +configure_libevent_options(void) +{ + if (BUG(!the_evdns_base)) { + return; + } + +#define SET(k,v) evdns_base_set_option(the_evdns_base, (k), (v)) + + // If we only have one nameserver, it does not make sense to back off + // from it for a timeout. Unfortunately, the value for max-timeouts is + // currently clamped by libevent to 255, but it does not hurt to set + // it higher in case libevent gets a patch for this. Higher-than- + // default maximum of 3 with multiple nameservers to avoid spuriously + // marking one down on bursts of timeouts resulting from scans/attacks + // against non-responding authoritative DNS servers. + if (evdns_base_count_nameservers(the_evdns_base) == 1) { + SET("max-timeouts:", "1000000"); + } else { + SET("max-timeouts:", "10"); + } + + // Elongate the queue of maximum inflight dns requests, so if a bunch + // remain pending at the resolver (happens commonly with Unbound) we won't + // stall every other DNS request. This potentially means some wasted + // CPU as there's a walk over a linear queue involved, but this is a + // much better tradeoff compared to just failing DNS requests because + // of a full queue. + SET("max-inflight:", "8192"); + + /* Set timeout to be 1 second. This tells libevent that it shouldn't wait + * more than N second to drop a DNS query and consider it "timed out". It is + * very important to differentiate here a libevent timeout and a DNS server + * timeout. And so, by setting this to N second, libevent sends back + * "DNS_ERR_TIMEOUT" if that N second is reached which does NOT indicate that + * the query itself timed out in transit. */ + SET("timeout:", get_consensus_param_exit_dns_timeout()); + + /* This tells libevent to attemps up to X times a DNS query if the previous + * one failed to complete within N second. We believe that this should be + * enough to catch temporary hiccups on the first query. But after that, it + * should signal us that it won't be able to resolve it. */ + SET("attempts:", get_consensus_param_exit_dns_attempts()); + + if (get_options()->ServerDNSRandomizeCase) + SET("randomize-case:", "1"); + else + SET("randomize-case:", "0"); + +#undef SET +} + /** Configure eventdns nameservers if force is true, or if the configuration * has changed since the last time we called this function, or if we failed on * our last attempt. On Unix, this reads from /etc/resolv.conf or @@ -1528,50 +1583,10 @@ configure_nameservers(int force) } #endif /* defined(_WIN32) */ -#define SET(k,v) evdns_base_set_option(the_evdns_base, (k), (v)) - - // If we only have one nameserver, it does not make sense to back off - // from it for a timeout. Unfortunately, the value for max-timeouts is - // currently clamped by libevent to 255, but it does not hurt to set - // it higher in case libevent gets a patch for this. Higher-than- - // default maximum of 3 with multiple nameservers to avoid spuriously - // marking one down on bursts of timeouts resulting from scans/attacks - // against non-responding authoritative DNS servers. - if (evdns_base_count_nameservers(the_evdns_base) == 1) { - SET("max-timeouts:", "1000000"); - } else { - SET("max-timeouts:", "10"); - } - - // Elongate the queue of maximum inflight dns requests, so if a bunch - // remain pending at the resolver (happens commonly with Unbound) we won't - // stall every other DNS request. This potentially means some wasted - // CPU as there's a walk over a linear queue involved, but this is a - // much better tradeoff compared to just failing DNS requests because - // of a full queue. - SET("max-inflight:", "8192"); - - /* Set timeout to be 1 second. This tells libevent that it shouldn't wait - * more than N second to drop a DNS query and consider it "timed out". It is - * very important to differentiate here a libevent timeout and a DNS server - * timeout. And so, by setting this to N second, libevent sends back - * "DNS_ERR_TIMEOUT" if that N second is reached which does NOT indicate that - * the query itself timed out in transit. */ - SET("timeout:", get_consensus_param_exit_dns_timeout()); - - /* This tells libevent to attemps up to X times a DNS query if the previous - * one failed to complete within N second. We believe that this should be - * enough to catch temporary hiccups on the first query. But after that, it - * should signal us that it won't be able to resolve it. */ - SET("attempts:", get_consensus_param_exit_dns_attempts()); - - if (options->ServerDNSRandomizeCase) - SET("randomize-case:", "1"); - else - SET("randomize-case:", "0"); - -#undef SET + /* Setup libevent options. */ + configure_libevent_options(); + /* Relaunch periodical DNS check event. */ dns_servers_relaunch_checks(); nameservers_configured = 1; -- To stop receiving notification emails like this one, please contact the administrator of this repository.