policy: fix handling managed devices without default routeth/bgo735512_route_metric_v2

Before, we would only track a device in NMDefaultRouteManager
if it had a default route. Otherwise the entry for the device
was removed.

That was wrong, because having no entry meant that the interface
is assumed and hence we would not touch the interface. Instead we must
esplicitly track devices without default route to know when an interface
has no default route.

Signed-off-by: Thomas Haller <thaller@redhat.com>

2 files changed, 102 insertions, 41 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 2a93f3e0dc..2a3ef8da69 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -737,7 +737,7 @@ nm_device_get_ip4_default_route (NMDevice *self, gboolean *out_is_assumed)
 	priv = NM_DEVICE_GET_PRIVATE (self);
 
 	if (out_is_assumed)
-		*out_is_assumed = priv->default_route.v4_has && priv->default_route.v4_is_assumed;
+		*out_is_assumed = priv->default_route.v4_is_assumed;
 
 	return priv->default_route.v4_has ? &priv->default_route.v4 : NULL;
 }
@@ -752,7 +752,7 @@ nm_device_get_ip6_default_route (NMDevice *self, gboolean *out_is_assumed)
 	priv = NM_DEVICE_GET_PRIVATE (self);
 
 	if (out_is_assumed)
-		*out_is_assumed = priv->default_route.v6_has && priv->default_route.v6_is_assumed;
+		*out_is_assumed = priv->default_route.v6_is_assumed;
 
 	return priv->default_route.v6_has ? &priv->default_route.v6 : NULL;
 }
@@ -2792,6 +2792,7 @@ ip4_config_merge_and_apply (NMDevice *self,
 	 */
 	connection = nm_device_get_connection (self);
 	priv->default_route.v4_has = FALSE;
+	priv->default_route.v4_is_assumed = TRUE;
 	if (connection) {
 		gboolean assumed = nm_device_uses_assumed_connection (self);
 		NMPlatformIP4Route *route = &priv->default_route.v4;
@@ -2817,6 +2818,7 @@ ip4_config_merge_and_apply (NMDevice *self,
 		    && nm_default_route_manager_ip4_connection_has_default_route (nm_default_route_manager_get (), connection)) {
 			guint32 gateway = 0;
 
+			priv->default_route.v4_is_assumed = FALSE;
 			if (   (!commit && priv->ext_ip4_config_had_any_addresses)
 			    || ( commit && nm_ip4_config_get_num_addresses (composite))) {
 				/* For managed interfaces, we can only configure a gateway, if either the external config indicates
@@ -2832,7 +2834,6 @@ ip4_config_merge_and_apply (NMDevice *self,
 					route->metric = nm_device_get_ip4_route_metric (self);
 					route->mss = nm_ip4_config_get_mss (composite);
 					priv->default_route.v4_has = TRUE;
-					priv->default_route.v4_is_assumed = FALSE;
 
 					if (   gateway
 					    && !nm_ip4_config_get_subnet_for_host (composite, gateway)
@@ -2851,7 +2852,6 @@ ip4_config_merge_and_apply (NMDevice *self,
 			/* For interfaces that are assumed and that have no default-route by configuration, we assume
 			 * the default connection and pick up whatever is configured. */
 			priv->default_route.v4_has = _device_get_default_route_from_platform (self, AF_INET, (NMPlatformIPRoute *) route);
-			priv->default_route.v4_is_assumed = TRUE;
 		}
 	}
 
@@ -3351,6 +3351,7 @@ ip6_config_merge_and_apply (NMDevice *self,
 	 */
 	connection = nm_device_get_connection (self);
 	priv->default_route.v6_has = FALSE;
+	priv->default_route.v6_is_assumed = TRUE;
 	if (connection) {
 		gboolean assumed = nm_device_uses_assumed_connection (self);
 		NMPlatformIP6Route *route = &priv->default_route.v6;
@@ -3376,6 +3377,7 @@ ip6_config_merge_and_apply (NMDevice *self,
 		    && nm_default_route_manager_ip6_connection_has_default_route (nm_default_route_manager_get (), connection)) {
 			const struct in6_addr *gateway = NULL;
 
+			priv->default_route.v6_is_assumed = FALSE;
 			if (   (!commit && priv->ext_ip6_config_had_any_addresses)
 			    || ( commit && nm_ip6_config_get_num_addresses (composite))) {
 				/* For managed interfaces, we can only configure a gateway, if either the external config indicates
@@ -3390,7 +3392,6 @@ ip6_config_merge_and_apply (NMDevice *self,
 					route->metric = nm_device_get_ip6_route_metric (self);
 					route->mss = nm_ip6_config_get_mss (composite);
 					priv->default_route.v6_has = TRUE;
-					priv->default_route.v6_is_assumed = FALSE;
 
 					if (   gateway
 					    && !nm_ip6_config_get_subnet_for_host (composite, gateway)
@@ -3409,7 +3410,6 @@ ip6_config_merge_and_apply (NMDevice *self,
 			/* For interfaces that are assumed and that have no default-route by configuration, we assume
 			 * the default connection and pick up whatever is configured. */
 			priv->default_route.v6_has = _device_get_default_route_from_platform (self, AF_INET6, (NMPlatformIPRoute *) route);
-			priv->default_route.v6_is_assumed = TRUE;
 		}
 	}
 
@@ -6966,7 +6966,9 @@ _cleanup_generic_post (NMDevice *self, gboolean deconfigure)
 	NMDeviceStateReason ignored = NM_DEVICE_STATE_REASON_NONE;
 
 	priv->default_route.v4_has = FALSE;
+	priv->default_route.v4_is_assumed = TRUE;
 	priv->default_route.v6_has = FALSE;
+	priv->default_route.v6_is_assumed = TRUE;
 
 	nm_default_route_manager_ip4_update_default_route (nm_default_route_manager_get (), self);
 	nm_default_route_manager_ip6_update_default_route (nm_default_route_manager_get (), self);
@@ -7766,6 +7768,9 @@ nm_device_init (NMDevice *self)
 	priv->unmanaged_flags = NM_UNMANAGED_INTERNAL;
 	priv->available_connections = g_hash_table_new_full (g_direct_hash, g_direct_equal, g_object_unref, NULL);
 	priv->ip6_saved_properties = g_hash_table_new_full (g_str_hash, g_str_equal, NULL, g_free);
+
+	priv->default_route.v4_is_assumed = TRUE;
+	priv->default_route.v6_is_assumed = TRUE;
 }
 
 /*
diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c
index d0aa3f4bfe..76403b330c 100644
--- a/src/nm-default-route-manager.c
+++ b/src/nm-default-route-manager.c
@@ -76,12 +76,14 @@ static NMDefaultRouteManager *_instance;
 #define _LOGW(addr_family, ...)      _LOG (LOGL_WARN , addr_family, __VA_ARGS__)
 #define _LOGE(addr_family, ...)      _LOG (LOGL_ERR  , addr_family, __VA_ARGS__)
 
-#define LOG_ENTRY_FMT  "entry[%u/%s:%p:%s]"
+#define LOG_ENTRY_FMT  "entry[%u/%s:%p:%s:%c%c]"
 #define LOG_ENTRY_ARGS(entry_idx, entry) \
-		entry_idx, \
-		NM_IS_DEVICE (entry->source.pointer) ? "dev" : "vpn", \
-		entry->source.pointer, \
-		NM_IS_DEVICE (entry->source.pointer) ? nm_device_get_iface (entry->source.device) : nm_vpn_connection_get_connection_id (entry->source.vpn)
+		(entry_idx), \
+		NM_IS_DEVICE ((entry)->source.pointer) ? "dev" : "vpn", \
+		(entry)->source.pointer, \
+		NM_IS_DEVICE ((entry)->source.pointer) ? nm_device_get_iface ((entry)->source.device) : nm_vpn_connection_get_connection_id ((entry)->source.vpn), \
+		((entry)->never_default ? 'N' : 'n'), \
+		((entry)->synced ? 'S' : 's')
 
 /***********************************************************************************/
 
@@ -97,11 +99,32 @@ typedef struct {
 		NMVpnConnection *vpn;
 	} source;
 	NMPlatformIPXRoute route;
-	gboolean synced; /* if true, we synced the entry to platform. We don't sync assumed devices */
 
-	/* it makes sense to order sources based on their priority, without
+	/* Whether the route is synced to platform.
+	 *
+	 * For VPN sources, the means that the entry is ignored for most puposes.
+	 * It is only tracked by NMDefaultRouteManager for get_best_config() where
+	 * we want to prefer a VPN connection for DNS even if it does not receive
+	 * the default route.
+	 *
+	 * For Device sources, a non synced route means that the route is assumed
+	 * and we should not touch the corresponding interface (unless we have a
+	 * synced entry for the very same ifindex).
+	 * A synced entry can be never-default. In that case the interface will
+	 * still not get a default route, but it notes that the interface is
+	 * managed (not assumed) and we enforce not having a default route.
+	 * This distincion is necessary, because usually if we see a default
+	 * route on an interface that has no entry, we ignore it as because
+	 * it is assumed. Hence, a missing entry really means: ignore the route.
+	 * But a non-synced, never-default entry means: enforce no default route. */
+	gboolean synced;
+
+	/* For VPN sources, it makes sense to order sources based on their priority, without
 	 * actually adding a default route. This is useful to decide which
-	 * DNS server to prefer. never_default entries are not synced to platform. */
+	 * DNS server to prefer.
+	 *
+	 * For Device source, never_default entries enforce no default route on the
+	 * interface. */
 	gboolean never_default;
 
 	guint32 effective_metric;
@@ -252,7 +275,7 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g
 }
 
 static gboolean
-_platform_route_sync_flush (const VTableIP *vtable, NMDefaultRouteManager *self)
+_platform_route_sync_flush (const VTableIP *vtable, NMDefaultRouteManager *self, int ifindex_to_flush)
 {
 	NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self);
 	GPtrArray *entries = vtable->get_entries (priv);
@@ -275,13 +298,15 @@ _platform_route_sync_flush (const VTableIP *vtable, NMDefaultRouteManager *self)
 		for (j = 0; j < entries->len; j++) {
 			Entry *e = g_ptr_array_index (entries, j);
 
-			if (e->never_default)
+			if (   e->never_default
+			    && !NM_IS_DEVICE (e->source.object))
 				continue;
 
 			if (   e->route.rx.ifindex == route->ifindex
 			    && e->synced) {
 				has_ifindex_synced = TRUE;
-				if (e->effective_metric == route->metric)
+				if (   !e->never_default
+				    && e->effective_metric == route->metric)
 					entry = e;
 			}
 		}
@@ -293,7 +318,8 @@ _platform_route_sync_flush (const VTableIP *vtable, NMDefaultRouteManager *self)
 		 * Otherwise, don't delete the route because it's configured
 		 * externally (and will be assumed -- or already is assumed).
 		 */
-		if (has_ifindex_synced && !entry) {
+		if (   !entry
+		    && (has_ifindex_synced || ifindex_to_flush == route->ifindex)) {
 			vtable->platform_route_delete_default (route->ifindex, route->metric);
 			changed = TRUE;
 		}
@@ -370,8 +396,11 @@ _get_assumed_interface_metrics (const VTableIP *vtable, NMDefaultRouteManager *s
 		for (j = 0; j < entries->len; j++) {
 			Entry *e = g_ptr_array_index (entries, j);
 
-			if (   !e->never_default
-			    && e->synced
+			if (   e->never_default
+			    && !NM_IS_DEVICE (e->source.object))
+				continue;
+
+			if (   e->synced
 			    && e->route.rx.ifindex == route->ifindex) {
 				ifindex_has_synced_entry = TRUE;
 				break;
@@ -409,6 +438,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c
 	GHashTable *assumed_metrics;
 	GArray *routes;
 	gboolean changed = FALSE;
+	int ifindex_to_flush = 0;
 
 	g_assert (priv->resync.guard == 0);
 	priv->resync.guard++;
@@ -428,7 +458,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c
 
 	assumed_metrics = _get_assumed_interface_metrics (vtable, self, routes);
 
-	if (old_entry && old_entry->synced) {
+	if (old_entry && old_entry->synced && !old_entry->never_default) {
 		/* The old version obviously changed. */
 		g_array_append_val (changed_metrics, old_entry->effective_metric);
 	}
@@ -452,8 +482,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c
 			for (j = 0; j < entries->len; j++) {
 				const Entry *e = g_ptr_array_index (entries, j);
 
-				if (   !e->never_default
-				    && e->synced
+				if (   e->synced
 					&& e->route.rx.ifindex == entry->route.rx.ifindex) {
 					has_synced_entry = TRUE;
 					break;
@@ -539,7 +568,17 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c
 		last_metric = expected_metric;
 	}
 
-	changed |= _platform_route_sync_flush (vtable, self);
+	if (   old_entry
+	    && !changed_entry
+	    && old_entry->synced
+	    && !old_entry->never_default) {
+		/* If we entriely remove an entry that was synced before, we must make
+		 * sure to flush routes for this ifindex too. Otherwise they linger
+		 * around as "assumed" routes */
+		ifindex_to_flush = old_entry->route.rx.ifindex;
+	}
+
+	changed |= _platform_route_sync_flush (vtable, self, ifindex_to_flush);
 
 	g_array_free (changed_metrics, TRUE);
 	g_hash_table_unref (assumed_metrics);
@@ -563,14 +602,13 @@ _entry_at_idx_update (const VTableIP *vtable, NMDefaultRouteManager *self, guint
 	g_assert (   !old_entry
 	          || (entry->source.pointer == old_entry->source.pointer && entry->route.rx.ifindex == old_entry->route.rx.ifindex));
 
-	if (!entry->synced) {
+	if (!entry->synced && !entry->never_default)
 		entry->effective_metric = entry->route.rx.metric;
-		_LOGD (vtable->addr_family, LOG_ENTRY_FMT": %s %s%s",
-		       LOG_ENTRY_ARGS (entry_idx, entry),
-		       old_entry ? "update" : "add",
-		       vtable->platform_route_to_string (&entry->route.rx),
-		       entry->never_default ? " (never-default)" : (entry->synced ? "" : " (not synced)"));
-	}
+
+	_LOGD (vtable->addr_family, LOG_ENTRY_FMT": %s %s",
+	       LOG_ENTRY_ARGS (entry_idx, entry),
+	       old_entry ? "update" : "add",
+	       vtable->platform_route_to_string (&entry->route.rx));
 
 	g_ptr_array_sort_with_data (entries, _sort_entries_cmp, NULL);
 
@@ -590,9 +628,8 @@ _entry_at_idx_remove (const VTableIP *vtable, NMDefaultRouteManager *self, guint
 
 	entry = g_ptr_array_index (entries, entry_idx);
 
-	_LOGD (vtable->addr_family, LOG_ENTRY_FMT": remove %s (%u%s)", LOG_ENTRY_ARGS (entry_idx, entry),
-	       vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric,
-	       entry->synced ? "" : ", not synced");
+	_LOGD (vtable->addr_family, LOG_ENTRY_FMT": remove %s (%u)", LOG_ENTRY_ARGS (entry_idx, entry),
+	       vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric);
 
 	/* Remove the entry from the list (but don't free it yet) */
 	g_ptr_array_index (entries, entry_idx) = NULL;
@@ -618,8 +655,7 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self,
 	NMDevice *device = NULL;
 	NMVpnConnection *vpn = NULL;
 	gboolean never_default = FALSE;
-	gboolean synced;
-	gboolean is_assumed = FALSE;
+	gboolean synced = FALSE;
 
 	g_return_if_fail (NM_IS_DEFAULT_ROUTE_MANAGER (self));
 	if (NM_IS_DEVICE (source))
@@ -665,10 +701,29 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self,
 	/* get the @default_route from the device. */
 	if (ip_ifindex > 0) {
 		if (device) {
+			gboolean is_assumed;
+
 			if (VTABLE_IS_IP4)
 				default_route = (const NMPlatformIPRoute *) nm_device_get_ip4_default_route (device, &is_assumed);
 			else
 				default_route = (const NMPlatformIPRoute *) nm_device_get_ip6_default_route (device, &is_assumed);
+			if (!default_route && !is_assumed) {
+				/* the device has no default route, but it is not assumed. That means, NMDefaultRouteManager
+				 * enforces that the device has no default route.
+				 *
+				 * Hence we have to keep track of this entry, otherwise a missing entry tells us
+				 * that the interface is assumed and NM would not remove the default routes on
+				 * the device. */
+				memset (&rt, 0, sizeof (rt));
+				rt.rx.ifindex = ip_ifindex;
+				rt.rx.source = NM_IP_CONFIG_SOURCE_UNKNOWN;
+				rt.rx.metric = G_MAXUINT32;
+				default_route = &rt.rx;
+
+				never_default = TRUE;
+				synced = TRUE;
+			} else
+				synced = default_route && !is_assumed;
 		} else {
 			NMConnection *connection = nm_active_connection_get_connection ((NMActiveConnection *) vpn);
 
@@ -706,14 +761,11 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self,
 					}
 				}
 			}
+			synced = default_route && !never_default;
 		}
 	}
 	g_assert (!default_route || default_route->plen == 0);
 
-	/* if the source is never_default or the device uses an assumed connection,
-	 * we don't sync the route. */
-	synced = !never_default && !is_assumed;
-
 	if (!entry && !default_route)
 		/* nothing to do */;
 	else if (!entry) {
@@ -844,7 +896,8 @@ _ipx_get_best_device (const VTableIP *vtable, NMDefaultRouteManager *self, const
 		if (!NM_IS_DEVICE (entry->source.pointer))
 			continue;
 
-		g_assert (!entry->never_default);
+		if (entry->never_default)
+			continue;
 
 		if (g_slist_find ((GSList *) devices, entry->source.device))
 			return entry->source.pointer;
@@ -998,6 +1051,9 @@ _ipx_get_best_config (const VTableIP *vtable,
 			NMDevice *device = entry->source.device;
 			NMActRequest *req;
 
+			if (entry->never_default)
+				continue;
+
 			if (VTABLE_IS_IP4)
 				config_result = nm_device_get_ip4_config (device);
 			else