Re: [PATCH 1/2] conf: Split add_dns_resolv() into separate IPv4 and IPv6 versions

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

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

On Fri, Apr 11, 2025 at 11:14:38AM +0200, Stefano Brivio wrote:
> Not really valuable by itself, but dropping one level of nested blocks
> makes the next change more convenient.
> 
> No functional changes intended.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Not in scope for this code motion, but I did spot another bug here..

> ---
>  conf.c | 101 ++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 60 insertions(+), 41 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 168646f..18ed11c 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -414,6 +414,62 @@ static unsigned add_dns6(struct ctx *c, const struct in6_addr *addr,
>  	return 1;
>  }
>  
> +/**
> + * add_dns_resolv4() - Possibly add one IPv4 nameserver from host's resolv.conf
> + * @c:		Execution context
> + * @ns:		Nameserver address
> + * @idx:	Pointer to index of current IPv4 resolver entry, set on return
> + */
> +static void add_dns_resolv4(struct ctx *c, struct in_addr *ns, unsigned *idx)
> +{
> +	if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_host))
> +		c->ip4.dns_host = *ns;
> +
> +	/* Special handling if guest or container can only access local
> +	 * addresses via redirect, or if the host gateway is also a resolver and
> +	 * we shadow its address
> +	 */
> +	if (IN4_IS_ADDR_LOOPBACK(ns) ||
> +	    IN4_ARE_ADDR_EQUAL(ns, &c->ip4.map_host_loopback)) {

The second bit here is wrong.  We check if the nameserver address is
the --map-host-loopback address - meaning we can't use it in the
guest - then try to use it in the guest anyway.  That path should
instead return, like the ns == 127.0.0.1 && map_host_loopback ==
0.0.0.0 case.

> +		if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.map_host_loopback))
> +			return;
> +
> +		*ns = c->ip4.map_host_loopback;
> +		if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match))
> +			c->ip4.dns_match = c->ip4.map_host_loopback;
> +	}
> +
> +	*idx += add_dns4(c, ns, *idx);
> +}
> +
> +/**
> + * add_dns_resolv6() - Possibly add one IPv6 nameserver from host's resolv.conf
> + * @c:		Execution context
> + * @ns:		Nameserver address
> + * @idx:	Pointer to index of current IPv6 resolver entry, set on return
> + */
> +static void add_dns_resolv6(struct ctx *c, struct in6_addr *ns, unsigned *idx)
> +{
> +	if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_host))
> +		c->ip6.dns_host = *ns;
> +
> +	/* Special handling if guest or container can only access local
> +	 * addresses via redirect, or if the host gateway is also a resolver and
> +	 * we shadow its address
> +	 */
> +	if (IN6_IS_ADDR_LOOPBACK(ns) ||
> +	    IN6_ARE_ADDR_EQUAL(ns, &c->ip6.map_host_loopback)) {

Same bug for IPv6.

> +		if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.map_host_loopback))
> +			return;
> +
> +		*ns = c->ip6.map_host_loopback;
> +		if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_match))
> +			c->ip6.dns_match = c->ip6.map_host_loopback;
> +	}
> +
> +	*idx += add_dns6(c, ns, *idx);
> +}
> +
>  /**
>   * add_dns_resolv() - Possibly add ns from host resolv.conf to configuration
>   * @c:		Execution context
> @@ -430,48 +486,11 @@ static void add_dns_resolv(struct ctx *c, const char *nameserver,
>  	struct in6_addr ns6;
>  	struct in_addr ns4;
>  
> -	if (idx4 && inet_pton(AF_INET, nameserver, &ns4)) {
> -		if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_host))
> -			c->ip4.dns_host = ns4;
> -
> -		/* Special handling if guest or container can only access local
> -		 * addresses via redirect, or if the host gateway is also a
> -		 * resolver and we shadow its address
> -		 */
> -		if (IN4_IS_ADDR_LOOPBACK(&ns4) ||
> -		    IN4_ARE_ADDR_EQUAL(&ns4, &c->ip4.map_host_loopback)) {
> -			if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.map_host_loopback))
> -				return;
> -
> -			ns4 = c->ip4.map_host_loopback;
> -			if (IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match))
> -				c->ip4.dns_match = c->ip4.map_host_loopback;
> -		}
> -
> -		*idx4 += add_dns4(c, &ns4, *idx4);
> -	}
> -
> -	if (idx6 && inet_pton(AF_INET6, nameserver, &ns6)) {
> -		if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_host))
> -			c->ip6.dns_host = ns6;
> -
> -		/* Special handling if guest or container can only access local
> -		 * addresses via redirect, or if the host gateway is also a
> -		 * resolver and we shadow its address
> -		 */
> -		if (IN6_IS_ADDR_LOOPBACK(&ns6) ||
> -		    IN6_ARE_ADDR_EQUAL(&ns6, &c->ip6.map_host_loopback)) {
> -			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.map_host_loopback))
> -				return;
> +	if (idx4 && inet_pton(AF_INET, nameserver, &ns4))
> +		add_dns_resolv4(c, &ns4, idx4);
>  
> -			ns6 = c->ip6.map_host_loopback;
> -
> -			if (IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_match))
> -				c->ip6.dns_match = c->ip6.map_host_loopback;
> -		}
> -
> -		*idx6 += add_dns6(c, &ns6, *idx6);
> -	}
> +	if (idx6 && inet_pton(AF_INET6, nameserver, &ns6))
> +		add_dns_resolv6(c, &ns6, idx6);
>  }
>  
>  /**

-- 
David Gibson (he or they)	| 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 --]