From 5ab7e4a475e59d44641f349b47a1a93196703356 Mon Sep 17 00:00:00 2001 From: Simon Kelley Date: Mon, 14 Jun 2021 23:56:21 +0100 Subject: [PATCH] Improve efficiency of DNSSEC. The sharing point for DNSSEC RR data used to be when it entered the cache, having been validated. After that queries requiring the KEY or DS records would share the cached values. There is a common case in dual-stack hosts that queries for A and AAAA records for the same domain are made simultaneously. If required keys were not in the cache, this would result in two requests being sent upstream for the same key data (and all the subsequent chain-of-trust queries.) Now we combine these requests and elide the duplicates, resulting in fewer queries upstream and better performance. To keep a better handle on what's going on, the "extra" logging mode has been modified to associate queries and answers for DNSSEC queries in the same way as ordinary queries. The requesting address and port have been removed from DNSSEC logging lines, since this is no longer strictly defined. --- CHANGELOG | 15 +++ src/cache.c | 8 +- src/dnsmasq.h | 2 +- src/forward.c | 327 +++++++++++++++++++++++++++++--------------------- 4 files changed, 212 insertions(+), 140 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index da5139d..c5865ee 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -49,6 +49,21 @@ version 2.86 common case, where only default servers are declared, there is no effective change. + Improve efficiency of DNSSEC. The sharing point for DNSSEC RR data + used to be when it entered the cache, having been validated. After + that queries requiring the KEY or DS records would share the cached + values. There is a common case in dual-stack hosts that queries for + A and AAAA records for the same domain are made simultaneously. + If required keys were not in the cache, this would result in two + requests being sent upstream for the same key data (and all the + subsequent chain-of-trust queries.) Now we combine these requests + and elide the duplicates, resulting in fewer queries upstream + and better performance. To keep a better handle on what's + going on, the "extra" logging mode has been modified to associate + queries and answers for DNSSEC queries in the same way as ordinary + queries. The requesting address and port have been removed from + DNSSEC logging lines, since this is no longer strictly defined. + version 2.85 Fix problem with DNS retries in 2.83/2.84. diff --git a/src/cache.c b/src/cache.c index 3f59cd3..e671fa2 100644 --- a/src/cache.c +++ b/src/cache.c @@ -1965,11 +1965,13 @@ void log_query(unsigned int flags, char *name, union all_addr *addr, char *arg) if (option_bool(OPT_EXTRALOG)) { - int port = prettyprint_addr(daemon->log_source_addr, daemon->addrbuff2); if (flags & F_NOEXTRA) - my_syslog(LOG_INFO, "* %s/%u %s %s %s %s", daemon->addrbuff2, port, source, name, verb, dest); + my_syslog(LOG_INFO, "%u %s %s %s %s", daemon->log_display_id, source, name, verb, dest); else - my_syslog(LOG_INFO, "%u %s/%u %s %s %s %s", daemon->log_display_id, daemon->addrbuff2, port, source, name, verb, dest); + { + int port = prettyprint_addr(daemon->log_source_addr, daemon->addrbuff2); + my_syslog(LOG_INFO, "%u %s/%u %s %s %s %s", daemon->log_display_id, daemon->addrbuff2, port, source, name, verb, dest); + } } else my_syslog(LOG_INFO, "%s %s %s %s", source, name, verb, dest); diff --git a/src/dnsmasq.h b/src/dnsmasq.h index fbf1b52..696408a 100644 --- a/src/dnsmasq.h +++ b/src/dnsmasq.h @@ -690,7 +690,6 @@ struct hostsfile { #define STAT_SECURE_WILDCARD 7 #define STAT_OK 8 #define STAT_ABANDONED 9 -#define STAT_INPROGRESS 10 #define FREC_NOREBIND 1 #define FREC_CHECKING_DISABLED 2 @@ -727,6 +726,7 @@ struct frec { struct blockdata *stash; /* Saved reply, whilst we validate */ size_t stash_len; struct frec *dependent; /* Query awaiting internally-generated DNSKEY or DS query */ + struct frec *next_dependent; /* list of above. */ struct frec *blocking_query; /* Query which is blocking us. */ #endif struct frec *next; diff --git a/src/forward.c b/src/forward.c index ef289fd..de36ff2 100644 --- a/src/forward.c +++ b/src/forward.c @@ -16,14 +16,16 @@ #include "dnsmasq.h" -static struct frec *get_new_frec(time_t now, struct server *serv, struct frec *force); +static struct frec *get_new_frec(time_t now, struct server *serv, int force); static struct frec *lookup_frec(unsigned short id, int fd, void *hash, int *firstp, int *lastp); -static struct frec *lookup_frec_by_query(void *hash, unsigned int flags); +static struct frec *lookup_frec_by_query(void *hash, unsigned int flags, unsigned int flagmask); static unsigned short get_id(void); static void free_frec(struct frec *f); static void query_full(time_t now, char *domain); +static void return_reply(time_t now, struct frec *forward, struct dns_header *header, ssize_t n, int status); + /* Send a UDP packet with its source address set as "source" unless nowild is true, when we just send it with the kernel default */ int send_from(int fd, int nowild, char *packet, size_t len, @@ -189,10 +191,17 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr, fwd_flags |= FREC_DO_QUESTION; #endif - /* Check for retry on existing query */ + /* Check for retry on existing query. + FREC_DNSKEY and FREC_DS_QUERY are never set in flags, so the test below + ensures that no frec created for internal DNSSEC query can be returned here. + + Similarly FREC_NO_CACHE is never set in flags, so a query which is + contigent on a particular source address EDNS0 option will never be matched. */ if (forward) old_src = 1; - else if ((forward = lookup_frec_by_query(hash, fwd_flags))) + else if ((forward = lookup_frec_by_query(hash, fwd_flags, + FREC_CHECKING_DISABLED | FREC_AD_QUESTION | FREC_DO_QUESTION | + FREC_HAS_PHEADER | FREC_DNSKEY_QUERY | FREC_DS_QUERY | FREC_NO_CACHE))) { struct frec_src *src; @@ -270,7 +279,7 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr, master = daemon->serverarray[first]; - if (!(forward = get_new_frec(now, master, NULL))) + if (!(forward = get_new_frec(now, master, 0))) goto reply; /* table full - flags == 0, return REFUSED */ @@ -378,13 +387,13 @@ static int forward_query(int udpfd, union mysockaddr *udpaddr, for this server */ forward->flags |= FREC_TEST_PKTSZ; } - + + /* If a query is retried, use the log_id for the retry when logging the answer. */ + forward->frec_src.log_id = daemon->log_id; + /* We may be resending a DNSSEC query here, for which the below processing is not necessary. */ if (!is_dnssec) { - /* If a query is retried, use the log_id for the retry when logging the answer. */ - forward->frec_src.log_id = daemon->log_id; - header->id = htons(forward->new_id); plen = add_edns0_config(header, plen, ((unsigned char *)header) + PACKETSZ, &forward->frec_src.source, now, &subnet, &cacheable); @@ -715,15 +724,14 @@ static size_t process_reply(struct dns_header *header, time_t now, struct server } #ifdef HAVE_DNSSEC -static int dnssec_validate(struct frec **forwardp, struct dns_header *header, - ssize_t plen, struct server *server, time_t now) +static void dnssec_validate(struct frec *forward, struct dns_header *header, + ssize_t plen, int status, time_t now) { - int status = 0; - struct frec *forward = *forwardp; + daemon->log_display_id = forward->frec_src.log_id; /* We've had a reply already, which we're validating. Ignore this duplicate */ if (forward->blocking_query) - return STAT_INPROGRESS; + return; /* Truncated answer can't be validated. If this is an answer to a DNSSEC-generated query, we still @@ -737,101 +745,111 @@ static int dnssec_validate(struct frec **forwardp, struct dns_header *header, if (RCODE(header) == REFUSED) status = STAT_ABANDONED; - while (1) + /* As soon as anything returns BOGUS, we stop and unwind, to do otherwise + would invite infinite loops, since the answers to DNSKEY and DS queries + will not be cached, so they'll be repeated. */ + if (status != STAT_BOGUS && status != STAT_TRUNCATED && status != STAT_ABANDONED) { - /* As soon as anything returns BOGUS, we stop and unwind, to do otherwise - would invite infinite loops, since the answers to DNSKEY and DS queries - will not be cached, so they'll be repeated. */ - if (status != STAT_BOGUS && status != STAT_TRUNCATED && status != STAT_ABANDONED) - { - if (forward->flags & FREC_DNSKEY_QUERY) - status = dnssec_validate_by_ds(now, header, plen, daemon->namebuff, daemon->keyname, forward->class); - else if (forward->flags & FREC_DS_QUERY) - status = dnssec_validate_ds(now, header, plen, daemon->namebuff, daemon->keyname, forward->class); - else - status = dnssec_validate_reply(now, header, plen, daemon->namebuff, daemon->keyname, &forward->class, - !option_bool(OPT_DNSSEC_IGN_NS) && (forward->sentto->flags & SERV_DO_DNSSEC), - NULL, NULL, NULL); + if (forward->flags & FREC_DNSKEY_QUERY) + status = dnssec_validate_by_ds(now, header, plen, daemon->namebuff, daemon->keyname, forward->class); + else if (forward->flags & FREC_DS_QUERY) + status = dnssec_validate_ds(now, header, plen, daemon->namebuff, daemon->keyname, forward->class); + else + status = dnssec_validate_reply(now, header, plen, daemon->namebuff, daemon->keyname, &forward->class, + !option_bool(OPT_DNSSEC_IGN_NS) && (forward->sentto->flags & SERV_DO_DNSSEC), + NULL, NULL, NULL); #ifdef HAVE_DUMPFILE - if (status == STAT_BOGUS) - dump_packet((forward->flags & (FREC_DNSKEY_QUERY | FREC_DS_QUERY)) ? DUMP_SEC_BOGUS : DUMP_BOGUS, - header, (size_t)plen, &server->addr, NULL); + if (status == STAT_BOGUS) + dump_packet((forward->flags & (FREC_DNSKEY_QUERY | FREC_DS_QUERY)) ? DUMP_SEC_BOGUS : DUMP_BOGUS, + header, (size_t)plen, &forward->sentto->addr, NULL); #endif - } + } + + /* Can't validate, as we're missing key data. Put this + answer aside, whilst we get that. */ + if (status == STAT_NEED_DS || status == STAT_NEED_KEY) + { + struct frec *new = NULL; + int serverind; + struct blockdata *stash; - /* Can't validate, as we're missing key data. Put this - answer aside, whilst we get that. */ - if (status == STAT_NEED_DS || status == STAT_NEED_KEY) + /* Now save reply pending receipt of key data */ + if ((serverind = dnssec_server(forward->sentto, daemon->keyname, NULL, NULL)) != -1 && + (stash = blockdata_alloc((char *)header, plen))) { - struct frec *new = NULL, *orig; - int serverind; - - /* Free any saved query */ - if (forward->stash) - blockdata_free(forward->stash); - - /* Now save reply pending receipt of key data */ - if (!(forward->stash = blockdata_alloc((char *)header, plen))) - return STAT_ABANDONED; - forward->stash_len = plen; + struct server *server = daemon->serverarray[serverind]; + struct frec *orig; + unsigned int flags; + void *hash; + size_t nn; + + /* validate routines leave name of required record in daemon->keyname */ + nn = dnssec_generate_query(header, ((unsigned char *) header) + server->edns_pktsz, + daemon->keyname, forward->class, + status == STAT_NEED_KEY ? T_DNSKEY : T_DS, server->edns_pktsz); + flags = (status == STAT_NEED_KEY) ? FREC_DNSKEY_QUERY : FREC_DS_QUERY; + hash = hash_questions(header, nn, daemon->namebuff); + + if ((new = lookup_frec_by_query(hash, flags, FREC_DNSKEY_QUERY | FREC_DS_QUERY))) + { + forward->next_dependent = new->dependent; + new->dependent = forward; + /* Make consistent, only replace query copy with unvalidated answer + when we set ->blocking_query. */ + if (forward->stash) + blockdata_free(forward->stash); + forward->blocking_query = new; + forward->stash_len = plen; + forward->stash = stash; + return; + } + /* Find the original query that started it all.... */ for (orig = forward; orig->dependent; orig = orig->dependent); /* Make sure we don't expire and free the orig frec during the - allocation of a new one. */ - if (--orig->work_counter == 0 || - (serverind = dnssec_server(server, daemon->keyname, NULL, NULL)) == -1 || - !(server = daemon->serverarray[serverind]) || - !(new = get_new_frec(now, server, orig))) - { - status = STAT_ABANDONED; - if (new) - free_frec(new); - } + allocation of a new one: third arg of get_new_frec() does that. */ + if (--orig->work_counter == 0 || !(new = get_new_frec(now, server, 1))) + blockdata_free(stash); /* don't leak this on failure. */ else { - int querytype, fd; + int fd; struct frec *next = new->next; - size_t nn; - + *new = *forward; /* copy everything, then overwrite */ new->next = next; new->blocking_query = NULL; + new->frec_src.log_id = daemon->log_display_id = ++daemon->log_id; new->sentto = server; new->rfds = NULL; new->frec_src.next = NULL; new->flags &= ~(FREC_DNSKEY_QUERY | FREC_DS_QUERY | FREC_HAS_EXTRADATA); + new->flags |= flags; new->forwardall = 0; + forward->next_dependent = NULL; new->dependent = forward; /* to find query awaiting new one. */ - forward->blocking_query = new; /* for garbage cleaning */ - /* validate routines leave name of required record in daemon->keyname */ - if (status == STAT_NEED_KEY) - { - new->flags |= FREC_DNSKEY_QUERY; - querytype = T_DNSKEY; - } - else - { - new->flags |= FREC_DS_QUERY; - querytype = T_DS; - } + + /* Make consistent, only replace query copy with unvalidated answer + when we set ->blocking_query. */ + forward->blocking_query = new; + if (forward->stash) + blockdata_free(forward->stash); + forward->stash_len = plen; + forward->stash = stash; - nn = dnssec_generate_query(header,((unsigned char *) header) + server->edns_pktsz, - daemon->keyname, forward->class, querytype, server->edns_pktsz); - - memcpy(new->hash, hash_questions(header, nn, daemon->namebuff), HASH_SIZE); + memcpy(new->hash, hash, HASH_SIZE); new->new_id = get_id(); header->id = htons(new->new_id); /* Save query for retransmission */ new->stash = blockdata_alloc((char *)header, nn); new->stash_len = nn; - + /* Don't resend this. */ daemon->srv_save = NULL; - + if ((fd = allocate_rfd(&new->rfds, server)) != -1) { #ifdef HAVE_CONNTRACK @@ -840,28 +858,38 @@ static int dnssec_validate(struct frec **forwardp, struct dns_header *header, #endif server_send_log(server, fd, header, nn, DUMP_SEC_QUERY, F_NOEXTRA | F_DNSSEC, daemon->keyname, - querystr("dnssec-query", querytype)); + querystr("dnssec-query", status == STAT_NEED_KEY ? T_DNSKEY : T_DS)); server->queries++; } - } - return STAT_INPROGRESS; + + return; + } } - - /* Validated original answer, all done. */ - if (!forward->dependent) - break; - - /* validated subsidiary query, (and cached result) - pop that and return to the previous query we were working on. */ - struct frec *prev = forward->dependent; - free_frec(forward); - *forwardp = forward = prev; - forward->blocking_query = NULL; /* already gone */ - blockdata_retrieve(forward->stash, forward->stash_len, (void *)header); - plen = forward->stash_len; + + /* sending DNSSEC query failed. */ + status = STAT_ABANDONED; } - return status; + /* Validated original answer, all done. */ + if (!forward->dependent) + return_reply(now, forward, header, plen, status); + else + { + /* validated subsidiary query/queries, (and cached result) + pop that and return to the previous query/queries we were working on. */ + struct frec *prev, *nxt = forward->dependent; + + free_frec(forward); + + while ((prev = nxt)) + { + /* ->next_dependent will have changed after return from recursive call below. */ + nxt = prev->next_dependent; + prev->blocking_query = NULL; /* already gone */ + blockdata_retrieve(prev->stash, prev->stash_len, (void *)header); + dnssec_validate(prev, header, prev->stash_len, status, now); + } + } } #endif @@ -875,12 +903,10 @@ void reply_query(int fd, time_t now) struct frec *forward; socklen_t addrlen = sizeof(serveraddr); ssize_t n = recvfrom(fd, daemon->packet, daemon->packet_buff_sz, 0, &serveraddr.sa, &addrlen); - size_t nn; struct server *server; void *hash; int first, last, c; - int check_rebind = 0, no_cache_dnssec = 0, cache_secure = 0, bogusanswer = 0; - + /* packet buffer overwritten */ daemon->srv_save = NULL; @@ -944,10 +970,14 @@ void reply_query(int fd, time_t now) unsigned short udp_size = PACKETSZ; /* default if no EDNS0 */ size_t plen; int is_sign; - + size_t nn = 0; + #ifdef HAVE_DNSSEC - /* DNSSEC queries have a copy of the original query stashed. */ - if (forward->flags & (FREC_DNSKEY_QUERY | FREC_DS_QUERY)) + /* DNSSEC queries have a copy of the original query stashed. + The query MAY have got a good answer, and be awaiting + the results of further queries, in which case + The Stash contains something else and we don't need to retry anyway. */ + if ((forward->flags & (FREC_DNSKEY_QUERY | FREC_DS_QUERY)) && !forward->blocking_query) { blockdata_retrieve(forward->stash, forward->stash_len, (void *)header); nn = forward->stash_len; @@ -956,8 +986,6 @@ void reply_query(int fd, time_t now) else #endif { - nn = 0; - /* recreate query from reply */ if ((pheader = find_pseudoheader(header, (size_t)n, &plen, &udpsz, &is_sign, NULL))) GETSHORT(udp_size, udpsz); @@ -1015,25 +1043,36 @@ void reply_query(int fd, time_t now) my_syslog(LOG_WARNING, _("reducing DNS packet size for nameserver %s to %d"), daemon->addrbuff, SAFE_PKTSZ); } - /* Don't cache replies where DNSSEC validation was turned off, either - the upstream server told us so, or the original query specified it. */ - if ((header->hb4 & HB4_CD) || (forward->flags & FREC_CHECKING_DISABLED)) - no_cache_dnssec = 1; + forward->sentto = server; #ifdef HAVE_DNSSEC if ((forward->sentto->flags & SERV_DO_DNSSEC) && option_bool(OPT_DNSSEC_VALID) && !(forward->flags & FREC_CHECKING_DISABLED)) - { - /* Note that the value of forward may change here: - we can start with a DNSSEC query reply and - return with a query that was suspended pending - that DNSSEC query. */ - int status = dnssec_validate(&forward, header, n, server, now); + dnssec_validate(forward, header, n, STAT_OK, now); + else +#endif + return_reply(now, forward, header, n, STAT_OK); +} - if (status == STAT_INPROGRESS) - return; - +static void return_reply(time_t now, struct frec *forward, struct dns_header *header, ssize_t n, int status) +{ + int check_rebind = 0, no_cache_dnssec = 0, cache_secure = 0, bogusanswer = 0; + size_t nn; + + (void)status; + + daemon->log_display_id = forward->frec_src.log_id; + daemon->log_source_addr = &forward->frec_src.source; + + /* Don't cache replies where DNSSEC validation was turned off, either + the upstream server told us so, or the original query specified it. */ + if ((header->hb4 & HB4_CD) || (forward->flags & FREC_CHECKING_DISABLED)) + no_cache_dnssec = 1; + +#ifdef HAVE_DNSSEC + if (status != STAT_OK) + { no_cache_dnssec = 0; if (status == STAT_TRUNCATED) @@ -1080,7 +1119,7 @@ void reply_query(int fd, time_t now) if (forward->flags & FREC_NO_CACHE) no_cache_dnssec = 1; - if ((nn = process_reply(header, now, server, (size_t)n, check_rebind, no_cache_dnssec, cache_secure, bogusanswer, + if ((nn = process_reply(header, now, forward->sentto, (size_t)n, check_rebind, no_cache_dnssec, cache_secure, bogusanswer, forward->flags & FREC_AD_QUESTION, forward->flags & FREC_DO_QUESTION, forward->flags & FREC_ADDED_PHEADER, forward->flags & FREC_HAS_SUBNET, &forward->frec_src.source))) { @@ -1469,6 +1508,7 @@ static ssize_t tcp_talk(int first, int last, int start, unsigned char *packet, (void)have_mark; memcpy(hash, hash_questions(header, (unsigned int)qsize, daemon->namebuff), HASH_SIZE); + while (1) { int data_sent = 0; @@ -1572,7 +1612,8 @@ static int tcp_key_recurse(time_t now, int status, struct dns_header *header, si while (1) { - size_t m; + size_t m; + int log_save; /* limit the amount of work we do, to avoid cycling forever on loops in the DNS */ if (--(*keycount) == 0) @@ -1612,11 +1653,16 @@ static int tcp_key_recurse(time_t now, int status, struct dns_header *header, si new_status = STAT_ABANDONED; break; } + + log_save = daemon->log_display_id; + daemon->log_display_id = ++daemon->log_id; log_query_mysockaddr(F_NOEXTRA | F_DNSSEC, keyname, &server->addr, querystr("dnssec-query", new_status == STAT_NEED_KEY ? T_DNSKEY : T_DS)); new_status = tcp_key_recurse(now, new_status, new_header, m, class, name, keyname, server, have_mark, mark, keycount); + + daemon->log_display_id = log_save; if (new_status != STAT_OK) break; @@ -2136,9 +2182,27 @@ static void free_frec(struct frec *f) /* Anything we're waiting on is pointless now, too */ if (f->blocking_query) - free_frec(f->blocking_query); + { + struct frec *n, **up; + + /* unlink outselves from the blocking query's dependents list. */ + for (n = f->blocking_query->dependent, up = &f->blocking_query->dependent; n; n = n->next_dependent) + if (n == f) + { + *up = n->next_dependent; + break; + } + else + up = &n->next_dependent; + + /* If we were the only/last dependent, free the blocking query too. */ + if (!f->blocking_query->dependent) + free_frec(f->blocking_query); + } + f->blocking_query = NULL; f->dependent = NULL; + f->next_dependent = NULL; #endif } @@ -2146,10 +2210,11 @@ static void free_frec(struct frec *f) /* Impose an absolute limit of 4*TIMEOUT before we wipe things (for random sockets). - If force is non-NULL, always return a result, even if we have - to allocate above the limit, and never free the record pointed - to by the force argument. */ -static struct frec *get_new_frec(time_t now, struct server *master, struct frec *force) + If force is set, always return a result, even if we have + to allocate above the limit, and don'y free any records. + This is set when allocating for DNSSEC to avoid cutting off + the branch we are sitting on. */ +static struct frec *get_new_frec(time_t now, struct server *master, int force) { struct frec *f, *oldest, *target; int count; @@ -2165,7 +2230,7 @@ static struct frec *get_new_frec(time_t now, struct server *master, struct frec /* Don't free DNSSEC sub-queries here, as we may end up with dangling references to them. They'll go when their "real" query is freed. */ - if (!f->dependent && f != force) + if (!f->dependent && !force) #endif { if (difftime(now, f->time) >= 4*TIMEOUT) @@ -2173,8 +2238,7 @@ static struct frec *get_new_frec(time_t now, struct server *master, struct frec free_frec(f); target = f; } - - if (!oldest || difftime(f->time, oldest->time) <= 0) + else if (!oldest || difftime(f->time, oldest->time) <= 0) oldest = f; } } @@ -2255,22 +2319,13 @@ static struct frec *lookup_frec(unsigned short id, int fd, void *hash, int *firs return NULL; } -static struct frec *lookup_frec_by_query(void *hash, unsigned int flags) +static struct frec *lookup_frec_by_query(void *hash, unsigned int flags, unsigned int flagmask) { struct frec *f; - /* FREC_DNSKEY and FREC_DS_QUERY are never set in flags, so the test below - ensures that no frec created for internal DNSSEC query can be returned here. - - Similarly FREC_NO_CACHE is never set in flags, so a query which is - contigent on a particular source address EDNS0 option will never be matched. */ - -#define FLAGMASK (FREC_CHECKING_DISABLED | FREC_AD_QUESTION | FREC_DO_QUESTION \ - | FREC_HAS_PHEADER | FREC_DNSKEY_QUERY | FREC_DS_QUERY | FREC_NO_CACHE) - for(f = daemon->frec_list; f; f = f->next) if (f->sentto && - (f->flags & FLAGMASK) == flags && + (f->flags & flagmask) == flags && memcmp(hash, f->hash, HASH_SIZE) == 0) return f;