On Mon, 7 Nov 2022 12:08:59 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Sat, Nov 05, 2022 at 08:22:23AM +0100, Stefano Brivio wrote:I wouldn't be so sure of that "surely". In the Podman test case where I hit this issue, I use Podman to write to /etc/resolv.conf directly, no DHCP/NDP/DHCPv6 involved, and things work. That doesn't automatically imply a use case for *not* advertising it, but we have several ways this can work without advertising anything, so there are also chances somebody might not want to advertise that in some obscure use case.On Sat, 5 Nov 2022 12:19:55 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Right, that's what I meant.On Thu, Nov 03, 2022 at 07:42:51AM +0100, Stefano Brivio wrote:Now that we have ip{4,6}.dns_send[], yes.On Thu, 3 Nov 2022 14:42:13 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: > On Thu, Nov 03, 2022 at 12:04:43AM +0100, Stefano Brivio wrote: > > Now that we allow loopback DNS addresses to be used as targets for > > forwarding, we need to check if DNS answers come from those targets, > > before deciding to eventually remap traffic for local redirects. > > > > Otherwise, the source address won't match the one configured as > > forwarder, which means that the guest or the container will refuse > > those responses. > > > > Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> > > --- > > udp.c | 17 ++++++++--------- > > 1 file changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/udp.c b/udp.c > > index 4b201d3..7c77e09 100644 > > --- a/udp.c > > +++ b/udp.c > > @@ -680,8 +680,10 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n, > > src = ntohl(b->s_in.sin_addr.s_addr); > > src_port = ntohs(b->s_in.sin_port); > > > > - if (src >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET || > > - src == INADDR_ANY || src == ntohl(c->ip4.addr_seen)) { > > + if (c->ip4.dns_fwd && src == htonl(c->ip4.dns[0]) && src_port == 53) { > > I guess this is not a newly introduced bug, but for the case of > multiple host nameservers, don't you need to check against everything > in the ip4.dns[] array, not just entry 0? No, because that's the only one we're using as target for forwarded queries -- and DNS answers we want to check here are only the forwarded ones.*thinks* .. ok, that makes sense. But if that's the case, won't ip4.dns[0] be the only entry in ip4.dns[] we use for anything at all? Can we drop the table and just keep one entry?We could rename .dns_send[] back to .dns[] and change the currentRight, I think dns[] is a better name for it..dns[] to .own_dns, or .fwd_dns_target, something like that. Other naming ideas welcome.Yeah, I find the current dns_fwd name not very illuminating either. *thinks* does it even make sense for dns_fwd not to be in dns_send? We're intercepting queries the guest sends to @dns_fwd, so surely we should also be advertising it to the guest.So what about: @dns: Primary DNS server advertised to guest - may be a fake address we'll intercept @dns_extra[]:Additional DNS servers advertised to guest. Must be real servers the guest can address without translation @host_dns: Host side DNS server (may be localhost or another address that's not guest accessible) The DHCP code advertises both @dns and @dns_extra, and that's the *only* place @dns_extra is used.Also NDP and DHCPv6 use that, and checking one item plus one array in three places (difficult to share that code path) is uglier than just the array.UDP intercepts outbound packets for @dns and redirects them to @host_dns, likewise masquerading inbound packets from @host_dns to appear to have come from @dns.I see the general point, but we would still need a flag to allow users to disable the redirection. Given that, I'd rather go with dns, host_dns, and fwd_dns, it looks simpler. -- Stefano