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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 3 Nov 2022 14:37:18 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> 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.

Hopefully clarified enough in v2.

> > 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.

Oops, right, fixed in v2.

> 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.

Probably not relevant now that I fixed the case you mentioned above.

-- 
Stefano