Re: [musl] [PATCH 1/1] musl/resolver: serialise querying name servers

[Rich Felker <dalias@aerifal.cx>]

On Sun, Oct 23, 2022 at 09:18:01PM +1000, Qingtao Cao wrote:
> Querying all configured DNS servers in parallel boost performance but makes
> it impossible to prefer private DNS servers to public ones. Serialise DNS
> servers access from the top of the nameserver specified in resolv.conf and
> only move on to the next, or less preferred DNS server when the current one
> either timed out or replied NOERROR and NXDOMAIN for all queries
> 
> When querying the next or less perferred DNS servers, only unsuccessfully
> resolved host names were queried. When replies are received, they won't replace
> a perviously successful answer. This ensures successful answers from higher
> prioritised DNS servers won't be overridden
> 
> This eliminates a racy condition when different DNS servers replies differently
> to a query, the last received one wins
> 
> Last but not least, using a local array to save the currently received reply to
> simplify the logic to re-shuffle answers[next] to the corresponding answers[i]
> 
> Signed-off-by: Qingtao Cao <qingtao.cao@digi.com>

This patch is being submitted as one to change a very intentional and
longstanding behavior (and with that impose a new contract) and is not
going to be accepted. A basic premise of musl's resolver, that's not
just arbitrary but a consequence of a "fallback" model, is that all
the nameservers provided present a consistent view of a global dns
namespace. 

Some people mistake "fallback" for "search" because it kinda appears
to work that way if you don't actually question what happens in the
corner cases. But if you're using fallback as search, failure of the
primary server to answer in time will *change the results* of the
query. This is not how fallback is supposed to work.

It would have been a lot more productive to start from a standpoint of
what you're trying to do. I assume that's "unioning" where you have a
local nameserver serving fake names that don't actually exist in the
real DNS namespace, and a real nameserver that's serving the real DNS
namespace. There are at least 2 valid ways to achieve this that don't
break in the above manner.

1. Run a unioning nameserver locally. The unioning nameserver handles
   all the complex logic of handling clashes and which upstream source
   takes precedence, querying both and only answering if it gets
   sufficient answers to draw a conclusive result about which result
   it can use (and ServFail otherwise).

2. Put your local names under an actual DNS domain (or subdomain zone)
   you actually own. Set the public DNS delegations to point to
   nameservers that don't exist. Now the public DNS will always
   ServFail. Set your private local nameserver to only return results
   for this local zone, and ServFail otherwise. As a result, neither
   is providing conflicting answers for the part of the namespace the
   other is responsible for.

There are likely other ways too.

> ---
>  src/network/res_msend.c | 100 +++++++++++++++++++++++++++-------------
>  1 file changed, 68 insertions(+), 32 deletions(-)
> 
> diff --git a/src/network/res_msend.c b/src/network/res_msend.c
> index 9adaea13..130ac4fe 100644
> --- a/src/network/res_msend.c
> +++ b/src/network/res_msend.c
> @@ -10,6 +10,7 @@
>  #include <unistd.h>
>  #include <errno.h>
>  #include <pthread.h>
> +#include <arpa/nameser.h>
>  #include "stdio_impl.h"
>  #include "syscall.h"
>  #include "lookup.h"
> @@ -41,11 +42,12 @@ int __res_msend_rc(int nqueries, const unsigned char *const *queries,
>  	int nns = 0;
>  	int family = AF_INET;
>  	int rlen;
> -	int next;
> -	int i, j;
> +	int i;
>  	int cs;
>  	struct pollfd pfd;
>  	unsigned long t0, t1, t2;
> +	int ns_index, rcode, all_done, unanswered;
> +	unsigned char answer[512];
>  
>  	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
>  
> @@ -115,19 +117,27 @@ int __res_msend_rc(int nqueries, const unsigned char *const *queries,
>  	pfd.fd = fd;
>  	pfd.events = POLLIN;
>  	retry_interval = timeout / attempts;
> -	next = 0;
> +
> +	ns_index = 0; /* Start from the top name server */
> +
> +next_ns:
>  	t0 = t2 = mtime();
>  	t1 = t2 - retry_interval;
>  
> +	unanswered = 0;
> +
>  	for (; t2-t0 < timeout; t2=mtime()) {
>  		if (t2-t1 >= retry_interval) {
> -			/* Query all configured namservers in parallel */
> -			for (i=0; i<nqueries; i++)
> -				if (!alens[i])
> -					for (j=0; j<nns; j++)
> -						sendto(fd, queries[i],
> -							qlens[i], MSG_NOSIGNAL,
> -							(void *)&ns[j], sl);
> +			for (i=0; i<nqueries; i++) {
> +				/* Only query unresolved or negative resolved host names */
> +				rcode = answers[i][3] & 15;
> +				if (!alens[i] || rcode != NOERROR) {
> +					sendto(fd, queries[i],
> +						qlens[i], MSG_NOSIGNAL,
> +						(void *)&ns[ns_index], sl);
> +					unanswered++;
> +				}
> +			}
>  			t1 = t2;
>  			servfail_retry = 2 * nqueries;
>  		}
> @@ -135,51 +145,77 @@ int __res_msend_rc(int nqueries, const unsigned char *const *queries,
>  		/* Wait for a response, or until time to retry */
>  		if (poll(&pfd, 1, t1+retry_interval-t2) <= 0) continue;
>  
> -		while ((rlen = recvfrom(fd, answers[next], asize, 0,
> +		while ((rlen = recvfrom(fd, answer, 512, 0,
>  		  (void *)&sa, (socklen_t[1]){sl})) >= 0) {
>  
>  			/* Ignore non-identifiable packets */
>  			if (rlen < 4) continue;
>  
>  			/* Ignore replies from addresses we didn't send to */
> -			for (j=0; j<nns && memcmp(ns+j, &sa, sl); j++);
> -			if (j==nns) continue;
> +			if (memcmp(ns+ns_index, &sa, sl))
> +				continue;
>  
>  			/* Find which query this answer goes with, if any */
> -			for (i=next; i<nqueries && (
> -				answers[next][0] != queries[i][0] ||
> -				answers[next][1] != queries[i][1] ); i++);
> +			for (i=0; i<nqueries && (
> +				answer[0] != queries[i][0] ||
> +				answer[1] != queries[i][1] ); i++);
>  			if (i==nqueries) continue;
> -			if (alens[i]) continue;
> +
> +			rcode = answers[i][3] & 15;
> +			if (alens[i] && rcode == NOERROR) {
> +				/* Do not override successful answers from a more prioritied server */
> +				continue;
> +			}
>  
>  			/* Only accept positive or negative responses;
>  			 * retry immediately on server failure, and ignore
>  			 * all other codes such as refusal. */
> -			switch (answers[next][3] & 15) {
> -			case 0:
> -			case 3:
> -				break;
> -			case 2:
> +			rcode = answer[3] & 15;
> +			switch (rcode) {
> +			case NOERROR:
> +			case NXDOMAIN:
> +				alens[i] = rlen;
> +				memcpy(answers[i], answer, rlen);
> +				unanswered--;
> +				continue;
> +			case SERVFAIL:
>  				if (servfail_retry && servfail_retry--)
>  					sendto(fd, queries[i],
>  						qlens[i], MSG_NOSIGNAL,
> -						(void *)&ns[j], sl);
> +						(void *)&ns[ns_index], sl);
>  			default:
>  				continue;
>  			}
> +		}
>  
> -			/* Store answer in the right slot, or update next
> -			 * available temp slot if it's already in place. */
> -			alens[i] = rlen;
> -			if (i == next)
> -				for (; next<nqueries && alens[next]; next++);
> -			else
> -				memcpy(answers[i], answers[next], rlen);
> +		/* Check if all sent queries have got NOERROR or NXDOMAIN replies from this server,
> +		 * no need to retry it anymore even when not timed out yet. When the unanswered counter
> +		 * drops to zero, any remaining NXDOMAIN replies were sure sent from this server */
> +		if (!unanswered) {
> +			all_done = 1;
> +			for (i = 0; i < nqueries; i++) {
> +				rcode = answers[i][3] & 15;
> +				if (!(alens[i] && ((rcode == NOERROR) || (rcode == NXDOMAIN)))) {
> +					all_done = 0;
> +					break;
> +				}
> +			}
>  
> -			if (next == nqueries) goto out;
> +			if (all_done)
> +				break;
>  		}
>  	}
> -out:
> +
> +	/* Check if any queries is unresolved or unsuccessful (regardless of failure code)
> +	 * try the next DNS server if available */
> +	if (++ns_index < conf->nns) {
> +		for (i = 0; i < nqueries; i++) {
> +			rcode = answers[i][3] & 15;
> +			if (!alens[i] || rcode != NOERROR)
> +				goto next_ns;
> +		}
> +	}
> +
>  	pthread_cleanup_pop(1);
>  
>  	return 0;
> -- 
> 2.34.1