Re: [PATCH 2/3] conf: Split the notions of read DNS addresses and offered ones

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8878 bytes --]

On Thu, Nov 03, 2022 at 12:04:42AM +0100, Stefano Brivio wrote:
> With --dns-forward, if the host has a loopback address configured as
> DNS server, we should actually use it to forward queries, but, if
> --no-map-gw is passed, we shouldn't offer the same address via DHCP,
> NDP and DHCPv6, because it's not going to be reachable.
> 
> Problematic configuration: systemd-resolved configuring the usual
> 127.0.0.53 on the host, and --dns-forward specified with an unrelated
> address. We still want to forward queries to 127.0.0.53, so we can't
> drop it from the addresses in IPv4 and IPv6 context,

I'm not entirely sure what you mean by that.

> but we shouldn't
> offer that address either.
> 
> With this change, I'm only covering the case of automatically
> configured DNS servers from /etc/resolv.conf. We could extend this to
> addresses configured with command-line options, but I don't really
> see a likely use case at this point.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  conf.c   | 50 ++++++++++++++++++++++++++++++++++----------------
>  dhcp.c   |  5 +++--
>  dhcpv6.c |  5 +++--
>  ndp.c    |  6 +++---
>  passt.h  |  8 ++++++--
>  5 files changed, 49 insertions(+), 25 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 5b88547..c4e1030 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -355,10 +355,11 @@ overlap:
>   */
>  static void get_dns(struct ctx *c)
>  {
> +	uint32_t *dns4 = &c->ip4.dns[0], *dns4_send = &c->ip4.dns_send[0];
> +	struct in6_addr *dns6_send = &c->ip6.dns_send[0];
>  	int dns4_set, dns6_set, dnss_set, dns_set, fd;
>  	struct in6_addr *dns6 = &c->ip6.dns[0];
>  	struct fqdn *s = c->dns_search;
> -	uint32_t *dns4 = &c->ip4.dns[0];
>  	struct lineread resolvconf;
>  	int line_len;
>  	char *line, *p, *end;
> @@ -388,30 +389,45 @@ static void get_dns(struct ctx *c)
>  			if (!dns4_set &&
>  			    dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) - 1 &&
>  			    inet_pton(AF_INET, p + 1, dns4)) {
> -				/* We can only access local addresses via the gw redirect */
> -				if (ntohl(*dns4) >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET) {
> -					if (c->no_map_gw) {
> -						*dns4 = 0;
> +				/* Guest or container can only access local
> +				 * addresses via local redirect
> +				 */
> +				if (IPV4_IS_LOOPBACK(ntohl(*dns4))) {
> +					if (c->no_map_gw)
>  						continue;

In this case shouldn't you still be recording the local address in the
dns[] array (but not dns_send[]) since it's a valid nameserver for the
host.  In which case you'd need to advance the dns4 pointer.

If I'm mistaken and you don't want to record it in the dns[] array,
then shouldn't you clear it (because otherwise you will record it if
this is the last "nameserver" line).

> -					}
> -					*dns4 = c->ip4.gw;
> +
> +					*dns4_send = c->ip4.gw;
> +				} else {
> +					*dns4_send = *dns4;
>  				}

I think it would be clearer to update *dns4 if necessary, then
set *dns4_send = *dns4 outside the if statement.

> +
> +				dns4_send++;
>  				dns4++;
> -				*dns4 = 0;
> +
> +				*dns4 = *dns4_send = 0;
>  			}
>  
>  			if (!dns6_set &&
>  			    dns6 - &c->ip6.dns[0] < ARRAY_SIZE(c->ip6.dns) - 1 &&
>  			    inet_pton(AF_INET6, p + 1, dns6)) {
> -				/* We can only access local addresses via the gw redirect */
> +				/* Guest or container can only access local
> +				 * addresses via local redirect
> +				 */
>  				if (IN6_IS_ADDR_LOOPBACK(dns6)) {
> -					if (c->no_map_gw) {
> -						memset(dns6, 0, sizeof(*dns6));
> +					if (c->no_map_gw)
>  						continue;
> -					}
> -					memcpy(dns6, &c->ip6.gw, sizeof(*dns6));
> +
> +					memcpy(dns6_send, &c->ip6.gw,
> +					       sizeof(*dns6_send));
> +				} else {
> +					memcpy(dns6_send, &c->ip6.gw,
> +					       sizeof(*dns6_send));
>  				}
> +
> +				dns6_send++;
>  				dns6++;
> +
> +				memset(dns6_send, 0, sizeof(*dns6_send));
>  				memset(dns6, 0, sizeof(*dns6));
>  			}
>  		} else if (!dnss_set && strstr(line, "search ") == line &&
> @@ -832,10 +848,11 @@ static void conf_print(const struct ctx *c)
>  			     inet_ntop(AF_INET, &c->ip4.gw,   buf4, sizeof(buf4)));
>  		}
>  
> -		for (i = 0; c->ip4.dns[i]; i++) {
> +		for (i = 0; c->ip4.dns_send[i]; i++) {
>  			if (!i)
>  				info("DNS:");
> -			inet_ntop(AF_INET, &c->ip4.dns[i], buf4, sizeof(buf4));
> +			inet_ntop(AF_INET, &c->ip4.dns_send[i], buf4,
> +				  sizeof(buf4));
>  			info("    %s", buf4);
>  		}
>  
> @@ -866,7 +883,8 @@ static void conf_print(const struct ctx *c)
>  		     inet_ntop(AF_INET6, &c->ip6.addr_ll, buf6, sizeof(buf6)));
>  
>  dns6:
> -		for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
> +		for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_send[i]);
> +		     i++) {
>  			if (!i)
>  				info("DNS:");
>  			inet_ntop(AF_INET6, &c->ip6.dns[i], buf6, sizeof(buf6));
> diff --git a/dhcp.c b/dhcp.c
> index d22698a..e816eb6 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -355,8 +355,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  		opts[26].s[1] = c->mtu % 256;
>  	}
>  
> -	for (i = 0, opts[6].slen = 0; !c->no_dhcp_dns && c->ip4.dns[i]; i++) {
> -		((uint32_t *)opts[6].s)[i] = c->ip4.dns[i];
> +	opts[6].slen = 0;
> +	for (i = 0; !c->no_dhcp_dns && c->ip4.dns_send[i]; i++) {
> +		((uint32_t *)opts[6].s)[i] = c->ip4.dns_send[i];
>  		opts[6].slen += sizeof(uint32_t);
>  	}
>  
> diff --git a/dhcpv6.c b/dhcpv6.c
> index e763aed..67262e6 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -379,7 +379,7 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
>  	if (c->no_dhcp_dns)
>  		goto search;
>  
> -	for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[i]); i++) {
> +	for (i = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_send[i]); i++) {
>  		if (!i) {
>  			srv = (struct opt_dns_servers *)(buf + offset);
>  			offset += sizeof(struct opt_hdr);
> @@ -387,7 +387,8 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
>  			srv->hdr.l = 0;
>  		}
>  
> -		memcpy(&srv->addr[i], &c->ip6.dns[i], sizeof(srv->addr[i]));
> +		memcpy(&srv->addr[i], &c->ip6.dns_send[i],
> +		       sizeof(srv->addr[i]));
>  		srv->hdr.l += sizeof(srv->addr[i]);
>  		offset += sizeof(srv->addr[i]);
>  	}
> diff --git a/ndp.c b/ndp.c
> index 80e1f19..6d79477 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -121,7 +121,7 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr)
>  		if (c->no_dhcp_dns)
>  			goto dns_done;
>  
> -		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
> +		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_send[n]); n++);
>  		if (n) {
>  			*p++ = 25;				/* RDNSS */
>  			*p++ = 1 + 2 * n;			/* length */
> @@ -130,8 +130,8 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr)
>  			p += 4;
>  
>  			for (i = 0; i < n; i++) {
> -				memcpy(p, &c->ip6.dns[i], 16);	/* address */
> -				p += 16;
> +				memcpy(p, &c->ip6.dns_send[i], 16);
> +				p += 16;			/* address */
>  			}
>  
>  			for (n = 0; *c->dns_search[n].n; n++)
> diff --git a/passt.h b/passt.h
> index 67281db..9f9bf3b 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -101,7 +101,8 @@ enum passt_modes {
>   * @addr_seen:		Latest IPv4 address seen as source from tap
>   * @mask:		IPv4 netmask, network order
>   * @gw:			Default IPv4 gateway, network order
> - * @dns:		IPv4 DNS addresses, zero-terminated, network order
> + * @dns:		Host IPv4 DNS addresses, zero-terminated, network order
> + * @dns_send:		Offered IPv4 DNS, zero-terminated, network order
>   * @dns_fwd:		Address forwarded (UDP) to first IPv4 DNS, network order
>   */
>  struct ip4_ctx {
> @@ -110,6 +111,7 @@ struct ip4_ctx {
>  	uint32_t mask;
>  	uint32_t gw;
>  	uint32_t dns[MAXNS + 1];
> +	uint32_t dns_send[MAXNS + 1];
>  	uint32_t dns_fwd;
>  };
>  
> @@ -120,7 +122,8 @@ struct ip4_ctx {
>   * @addr_seen:		Latest IPv6 global/site address seen as source from tap
>   * @addr_ll_seen:	Latest IPv6 link-local address seen as source from tap
>   * @gw:			Default IPv6 gateway
> - * @dns:		IPv6 DNS addresses, zero-terminated
> + * @dns:		Host IPv6 DNS addresses, zero-terminated
> + * @dns_send:		Offered IPv6 DNS addresses, zero-terminated
>   * @dns_fwd:		Address forwarded (UDP) to first IPv6 DNS, network order
>   */
>  struct ip6_ctx {
> @@ -130,6 +133,7 @@ struct ip6_ctx {
>  	struct in6_addr addr_ll_seen;
>  	struct in6_addr gw;
>  	struct in6_addr dns[MAXNS + 1];
> +	struct in6_addr dns_send[MAXNS + 1];
>  	struct in6_addr dns_fwd;
>  };
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]