policy: fix handling managed devices without default route

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, 107 insertions, 42 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..c006cff4df 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,36 @@ 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
-	 * actually adding a default route. This is useful to decide which
-	 * DNS server to prefer. never_default entries are not synced to platform. */
+	/* Whether the route is synced to platform and has a default route.
+	 *
+	 * ( synced && !never_default): the interface gets a default route that
+	 *     is enforced and managed by NMDefaultRouteManager.
+	 *
+	 * (!synced && !never_default): the interface has this route, but it is assumed.
+	 *     Assumed interfaces are those that have no tracked entry or that only have
+	 *     (!synced && !never_default) entries. NMDefaultRouteManager will not touch
+	 *     default routes on these interfaces.
+	 *     This combination makes only sense for device sources.
+	 *     They are tracked so that assumed devices can also be the best device.
+	 *
+	 * ( synced &&  never_default): entries of this kind are a placeholder
+	 *     to indicate that the ifindex is managed but has no default-route.
+	 *     Missing entries also indicate that a certain ifindex has no default-route.
+	 *     The difference is that missing entries are considered assumed while on
+	 *     (synced && never_default) entires the absence of the default route
+	 *     is enforced. NMDefaultRouteManager will actively remove any default
+	 *     route on such ifindexes.
+	 *     This combination makes only sense for device sources.
+	 *
+	 * (!synced &&  never_default): this combination makes only sense for VPN sources.
+	 *     If a VPN gets no default route, we still track it so that we can choose
+	 *     it for DNS configuration.
+	 *     Effectively, we ignore any default routes on such ifindexes and don't configure
+	 *     them ourselfes. The VPN is tracked with its configured priority (regardless
+	 *     of whether any default routes are actually present on the interface).
+	 */
+	gboolean synced;
 	gboolean never_default;
 
 	guint32 effective_metric;
@@ -252,7 +279,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 +302,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 +322,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 +400,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 +442,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 +462,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 +486,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 +572,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 +606,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 +632,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 +659,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 +705,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 +765,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 +900,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 +1055,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