(In reply to Tore Anderson from comment #0)
>   server=10.0.79.14@wlo1 # not really necessary since wlo1 is already
> preferred, but doesn't hurt and seems more consistent
>   server=fe80::120d:7fff:fe6d:a593%wlo1 # ipv6 link-locals already provide
> interface so these need no change
>   server=2001:4600:4:fff::52@wwp0s26u1u5i6 # ensures wwan interface, and
> crucially wwan source ip address, is used

How does this work in the face of https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=c328cf52f2b219feb5ae03fd40175f6b6ca73975 ?

We should do this, but it only fixes the problem for dnsmasq.


> Other approaches include setting up explicit host routes to the DNS servers
> out their appropriate interface, or to only configure dnsmasq with the DNS
> servers belonging to the primary selected connection (for each address
> family).

We probably should do this too.

IPv6 link-locals need to be specified with the '%' delimiter to work with current versions of dnsmasq (as explained in c328cf5). This issue impacts non-link-locals only. So you'll want something that works like this:

dnsmasq_conf = sprintf("server=%s%s%s", addr, IN6_IS_ADDR_LINKLOCAL(addr) ? "%" : "@", iface)

The issue doesn't really affect the glibc stub resolver because it checks with each server in resolv.conf in the order given. NM sorts that list according to the associated connection preference, so most likely the server tried first is a usable server.

(In reply to Tore Anderson from comment #0)
> I've done some testing, and one way to fix the problem is to suffix
> "@<interface>" to the dnsmasq.conf server=foo statements.

The main problem I see with this approach is that multiple connections
can have the same DNS server, but we can only put a single
"server=IP@iface" entry in dnsmasq configuration for that address.

In case of duplicate entries probably we should look at routes to
decide which interface is the best for a given name server (however
this is not trivial to do right), or simply omit the "@interface"
suffix (but then the original issue would be still present).

(In reply to Beniamino Galvani from comment #3)
> (In reply to Tore Anderson from comment #0)
> > I've done some testing, and one way to fix the problem is to suffix
> > "@<interface>" to the dnsmasq.conf server=foo statements.
> 
> The main problem I see with this approach is that multiple connections
> can have the same DNS server, but we can only put a single
> "server=IP@iface" entry in dnsmasq configuration for that address.
> 


> In case of duplicate entries probably we should look at routes to
> decide which interface is the best for a given name server (however
> this is not trivial to do right), or simply omit the "@interface"
> suffix (but then the original issue would be still present).

Reenacting what kernel routing does would be much too complicated. There is RTM_GETROUTE netlink request for that though...

But that still seems complicated to do, because routing can change, and then it's unclear what do to about that (and how to detect that).


Adding a direct route to the DNS server might be a good idea. But not trival either: what would be the gateway for that route? Guess for that we could use RTM_GETROUTE...

(In reply to Beniamino Galvani from comment #3)

> The main problem I see with this approach is that multiple connections
> can have the same DNS server, but we can only put a single
> "server=IP@iface" entry in dnsmasq configuration for that address.

Actually it seems to work. Well, sort of. For example:

$ dnsmasq -d -R -S 84.208.20.110@wlo1 -S 84.208.20.110@enp0s25

...will send out forwarded requests to the server via both interfaces simultaneously. It only seems to process responses coming in via the interface belonging to the primary connection for some reason (maybe rp_filter?), but at least that's no worse than what we have today.

Created attachment 326327 [details] [review]
dns: specify egress interface for each dnsmasq upstream server

(In reply to Tore Anderson from comment #5)
> ...will send out forwarded requests to the server via both interfaces
> simultaneously. It only seems to process responses coming in via the
> interface belonging to the primary connection for some reason (maybe
> rp_filter?), but at least that's no worse than what we have today.

This sounds fine. I think we should always specify an interface to
avoid the issues you reported. How about this patch? I did some basic
tests, but probably more are needed.

I've tested that patch on top of git master and functionality-wise it looks good to me, the REFUSED lookups are completely gone. (I have no opinion of the code itself though.)

Looks good. Just some minor nits:



	iface = g_object_get_data (G_OBJECT (ip4), IP_CONFIG_IFACE_TAG);
	g_assert (iface);

The assert is quite harsh. I'd drop the assert (also from the v6 case).
Or alternatively: keep the assert but add a g_return_if_fail(iface) to nm_dns_manager_add_ip6_config(), to catch the missing interface name when it is set, not later when it is used. There should be the assert early when the wrong thing happens (not when we later want to use it).






Btw, there is also nm_sprintf_buf(), but g_snprintf is fine too.









  buf = nm_utils_inet4_ntop (addr, NULL);
  /*use buf later*/

IMO using the static-buffer is error prone when the result is not used right away.

preferable would be

  char buf[NM_UTILS_INET_ADDRSTRLEN];
  nm_utils_inet4_ntop (addr, buf);
  /*use buf later*/

or

+              snprintf (buf, sizeof (buf), "%s%s%s",
+                        nm_utils_inet4_ntop (addr, NULL);
+                        iface[0] ? "@" : "",
+                        iface[0] ? iface : "");

(as it applies).



overall, lgtm

(In reply to Thomas Haller from comment #8)
> The assert is quite harsh. I'd drop the assert (also from the v6 case).
> Or alternatively: keep the assert but add a g_return_if_fail(iface) to
> nm_dns_manager_add_ip6_config(), to catch the missing interface name when it
> is set, not later when it is used. There should be the assert early when the
> wrong thing happens (not when we later want to use it).
 
> Btw, there is also nm_sprintf_buf(), but g_snprintf is fine too.

> IMO using the static-buffer is error prone when the result is not used right
> away.

Fixed and re-pushed (with additional commit) to bg/dnsmasq-interfaces-bgo765153.

lgtm

+1

Merged to master:

https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=6280839c16022c2cbd8358cbdea670f3fb638f7a