From 81446b5ad32386f648c1d6633d3f88b94a677ef2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 10 Nov 2014 14:23:44 +0100 Subject: [PATCH 01/13] platform: add typedef for unions NMPlatformIPXAddress and NMPlatformIPXRoute Signed-off-by: Thomas Haller --- src/platform/nm-platform.h | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 3514dd084d..34ed384af7 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -175,6 +175,12 @@ struct _NMPlatformIP6Address { }; G_STATIC_ASSERT (G_STRUCT_OFFSET (NMPlatformIPAddress, address_ptr) == G_STRUCT_OFFSET (NMPlatformIP6Address, address)); +typedef union { + NMPlatformIPAddress ax; + NMPlatformIP4Address a4; + NMPlatformIP6Address a6; +} NMPlatformIPXAddress; + #undef __NMPlatformIPAddress_COMMON @@ -215,6 +221,12 @@ struct _NMPlatformIP6Route { }; G_STATIC_ASSERT (G_STRUCT_OFFSET (NMPlatformIPRoute, network_ptr) == G_STRUCT_OFFSET (NMPlatformIP6Route, network)); +typedef union { + NMPlatformIPRoute rx; + NMPlatformIP4Route r4; + NMPlatformIP6Route r6; +} NMPlatformIPXRoute; + #undef __NMPlatformIPRoute_COMMON From a43ed6cb80546c03d117dac13916bdbd15709ebb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 10 Nov 2014 14:36:50 +0100 Subject: [PATCH 02/13] policy: refactor NMDefaultRouteManager to use union type NMPlatformIPXRoute Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 117 +++++++++++++++++---------------- 1 file changed, 59 insertions(+), 58 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 8afdbb487a..08fbdadb31 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -85,11 +85,7 @@ typedef struct { NMDevice *device; NMVpnConnection *vpn; } source; - union { - NMPlatformIPRoute route; - NMPlatformIP4Route route4; - NMPlatformIP6Route route6; - }; + 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 @@ -178,24 +174,26 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g return; if (VTABLE_IS_IP4) { - success = nm_platform_ip4_route_add (entry->route.ifindex, - entry->route.source, + success = nm_platform_ip4_route_add (entry->route.rx.ifindex, + entry->route.rx.source, 0, 0, - entry->route4.gateway, + entry->route.r4.gateway, entry->effective_metric, - entry->route.mss); + entry->route.rx.mss); } else { - success = nm_platform_ip6_route_add (entry->route.ifindex, - entry->route.source, + success = nm_platform_ip6_route_add (entry->route.rx.ifindex, + entry->route.rx.source, in6addr_any, 0, - entry->route6.gateway, + entry->route.r6.gateway, entry->effective_metric, - entry->route.mss); + entry->route.rx.mss); + } + if (!success) { + _LOGW (vtable->addr_family, "failed to add default route %s with effective metric %u", + vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric); } - if (!success) - _LOGW (vtable->addr_family, "failed to add default route %s with effective metric %u", vtable->platform_route_to_string (&entry->route), (guint) entry->effective_metric); } static void @@ -230,7 +228,7 @@ _platform_route_sync_flush (const VTableIP *vtable, NMDefaultRouteManager *self) if (e->never_default) continue; - if ( e->route.ifindex == route->ifindex + if ( e->route.rx.ifindex == route->ifindex && e->synced) { has_ifindex_synced = TRUE; if (e->effective_metric == route->metric) @@ -259,8 +257,8 @@ _sort_entries_cmp (gconstpointer a, gconstpointer b, gpointer user_data) const Entry *e_b = *((const Entry **) b); /* when comparing routes, we consider the (original) metric. */ - m_a = e_a->route.metric; - m_b = e_b->route.metric; + m_a = e_a->route.rx.metric; + m_b = e_b->route.rx.metric; /* we normalize route.metric already in _ipx_update_default_route(). * so we can just compare the metrics numerically */ @@ -327,7 +325,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c continue; } - expected_metric = entry->route.metric; + expected_metric = entry->route.rx.metric; if ((gint64) expected_metric <= last_metric) expected_metric = last_metric == G_MAXUINT32 ? G_MAXUINT32 : last_metric + 1; @@ -335,14 +333,20 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c /* for the changed entry, the previous metric was either old_entry->effective_metric, * or none. Hence, we only have to remember what is going to change. */ g_hash_table_add (changed_metrics, GUINT_TO_POINTER (expected_metric)); - if (old_entry) - _LOGD (vtable->addr_family, LOG_ENTRY_FMT": update %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), vtable->platform_route_to_string (&entry->route), (guint) old_entry->effective_metric, (guint) expected_metric); - else - _LOGD (vtable->addr_family, LOG_ENTRY_FMT": add %s (%u)", LOG_ENTRY_ARGS (i, entry), vtable->platform_route_to_string (&entry->route), (guint) expected_metric); + if (old_entry) { + _LOGD (vtable->addr_family, LOG_ENTRY_FMT": update %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), + vtable->platform_route_to_string (&entry->route.rx), (guint) old_entry->effective_metric, + (guint) expected_metric); + } else { + _LOGD (vtable->addr_family, LOG_ENTRY_FMT": add %s (%u)", LOG_ENTRY_ARGS (i, entry), + vtable->platform_route_to_string (&entry->route.rx), (guint) expected_metric); + } } else if (entry->effective_metric != expected_metric) { g_hash_table_add (changed_metrics, GUINT_TO_POINTER (entry->effective_metric)); g_hash_table_add (changed_metrics, GUINT_TO_POINTER (expected_metric)); - _LOGD (vtable->addr_family, LOG_ENTRY_FMT": resync metric %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), vtable->platform_route_to_string (&entry->route), (guint) entry->effective_metric, (guint) expected_metric); + _LOGD (vtable->addr_family, LOG_ENTRY_FMT": resync metric %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), + vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric, + (guint) expected_metric); } entry->effective_metric = expected_metric; @@ -372,14 +376,14 @@ _entry_at_idx_update (const VTableIP *vtable, NMDefaultRouteManager *self, guint entry = g_ptr_array_index (entries, entry_idx); g_assert ( !old_entry - || (entry->source.pointer == old_entry->source.pointer && entry->route.ifindex == old_entry->route.ifindex)); + || (entry->source.pointer == old_entry->source.pointer && entry->route.rx.ifindex == old_entry->route.rx.ifindex)); if (!entry->synced) { - entry->effective_metric = entry->route.metric; + 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), + vtable->platform_route_to_string (&entry->route.rx), entry->never_default ? " (never-default)" : (entry->synced ? "" : " (not synced)")); } @@ -401,7 +405,9 @@ _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), (guint) entry->effective_metric, entry->synced ? "" : ", not synced"); + _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"); /* Remove the entry from the list (but don't free it yet) */ g_ptr_array_index (entries, entry_idx) = NULL; @@ -421,11 +427,7 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, Entry *entry; guint entry_idx; const NMPlatformIPRoute *default_route = NULL; - union { - NMPlatformIPRoute vx; - NMPlatformIP4Route v4; - NMPlatformIP6Route v6; - } rt; + NMPlatformIPXRoute rt; int ip_ifindex; GPtrArray *entries; NMDevice *device = NULL; @@ -460,11 +462,11 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, entry = _entry_find_by_source (entries, source, &entry_idx); if ( entry - && entry->route.ifindex != ip_ifindex) { + && entry->route.rx.ifindex != ip_ifindex) { /* Strange... the ifindex changed... Remove the device and start again. */ _LOGD (vtable->addr_family, "ifindex of "LOG_ENTRY_FMT" changed: %d -> %d", LOG_ENTRY_ARGS (entry_idx, entry), - entry->route.ifindex, ip_ifindex); + entry->route.rx.ifindex, ip_ifindex); g_object_freeze_notify (G_OBJECT (self)); _entry_at_idx_remove (vtable, self, entry_idx, FALSE); @@ -487,19 +489,19 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, if ( connection && nm_vpn_connection_get_vpn_state (vpn) == NM_VPN_CONNECTION_STATE_ACTIVATED) { + memset (&rt, 0, sizeof (rt)); if (VTABLE_IS_IP4) { NMIP4Config *vpn_config; vpn_config = nm_vpn_connection_get_ip4_config (vpn); if (vpn_config) { never_default = nm_ip4_config_get_never_default (vpn_config); - memset (&rt.v4, 0, sizeof (rt.v4)); - rt.v4.ifindex = ip_ifindex; - rt.v4.source = NM_IP_CONFIG_SOURCE_VPN; - rt.v4.gateway = nm_vpn_connection_get_ip4_internal_gateway (vpn); - rt.v4.metric = nm_vpn_connection_get_ip4_route_metric (vpn); - rt.v4.mss = nm_ip4_config_get_mss (vpn_config); - default_route = &rt.vx; + rt.r4.ifindex = ip_ifindex; + rt.r4.source = NM_IP_CONFIG_SOURCE_VPN; + rt.r4.gateway = nm_vpn_connection_get_ip4_internal_gateway (vpn); + rt.r4.metric = nm_vpn_connection_get_ip4_route_metric (vpn); + rt.r4.mss = nm_ip4_config_get_mss (vpn_config); + default_route = &rt.rx; } } else { NMIP6Config *vpn_config; @@ -509,13 +511,12 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, const struct in6_addr *int_gw = nm_vpn_connection_get_ip6_internal_gateway (vpn); never_default = nm_ip6_config_get_never_default (vpn_config); - memset (&rt.v6, 0, sizeof (rt.v6)); - rt.v6.ifindex = ip_ifindex; - rt.v6.source = NM_IP_CONFIG_SOURCE_VPN; - rt.v6.gateway = int_gw ? *int_gw : in6addr_any; - rt.v6.metric = nm_vpn_connection_get_ip6_route_metric (vpn); - rt.v6.mss = nm_ip6_config_get_mss (vpn_config); - default_route = &rt.vx; + rt.r6.ifindex = ip_ifindex; + rt.r6.source = NM_IP_CONFIG_SOURCE_VPN; + rt.r6.gateway = int_gw ? *int_gw : in6addr_any; + rt.r6.metric = nm_vpn_connection_get_ip6_route_metric (vpn); + rt.r6.mss = nm_ip6_config_get_mss (vpn_config); + default_route = &rt.rx; } } } @@ -535,15 +536,15 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, entry->source.object = g_object_ref (source); if (VTABLE_IS_IP4) - entry->route4 = *((const NMPlatformIP4Route *) default_route); + entry->route.r4 = *((const NMPlatformIP4Route *) default_route); else - entry->route6 = *((const NMPlatformIP6Route *) default_route); + entry->route.r6 = *((const NMPlatformIP6Route *) default_route); /* only use normalized metrics */ - entry->route.metric = vtable->route_metric_normalize (entry->route.metric); - entry->route.ifindex = ip_ifindex; + entry->route.rx.metric = vtable->route_metric_normalize (entry->route.rx.metric); + entry->route.rx.ifindex = ip_ifindex; entry->never_default = never_default; - entry->effective_metric = entry->route.metric; + entry->effective_metric = entry->route.rx.metric; entry->synced = synced; g_ptr_array_add (entries, entry); @@ -554,12 +555,12 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, new_entry = *entry; if (VTABLE_IS_IP4) - new_entry.route4 = *((const NMPlatformIP4Route *) default_route); + new_entry.route.r4 = *((const NMPlatformIP4Route *) default_route); else - new_entry.route6 = *((const NMPlatformIP6Route *) default_route); + new_entry.route.r6 = *((const NMPlatformIP6Route *) default_route); /* only use normalized metrics */ - new_entry.route.metric = vtable->route_metric_normalize (new_entry.route.metric); - new_entry.route.ifindex = ip_ifindex; + new_entry.route.rx.metric = vtable->route_metric_normalize (new_entry.route.rx.metric); + new_entry.route.rx.ifindex = ip_ifindex; new_entry.never_default = never_default; new_entry.synced = synced; From 825885d5b12300ada94826e75de254702b730e30 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 10 Nov 2014 15:11:53 +0100 Subject: [PATCH 03/13] policy: minor fix when having multiple default routes with metric MAXUINT32 The case of having a metric MAXUINT32 is special, because in face of multiple default routes with the same metric, NMDefaultRouteManager cannot reduce the effective metric (because there is no lower priority value). This case works already correct, just when adding such a default route, ensure that we add it to the *first* entry. Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 08fbdadb31..9ebf138e92 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -161,7 +161,8 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g if (e->synced) { g_assert (!entry || metric == G_MAXUINT32); - entry = e; + if (!entry) + entry = e; } else has_unsynced_entry = TRUE; } From 393f213c3bad9935e25652dade02237578562f17 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 11 Nov 2014 10:14:30 +0100 Subject: [PATCH 04/13] policy: remove unused @do_sync argument from NMDefaultRouteManager functions Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 9ebf138e92..cc2c6daee7 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -293,7 +293,7 @@ _sort_entries_cmp (gconstpointer a, gconstpointer b, gpointer user_data) } static void -_resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *changed_entry, const Entry *old_entry, gboolean do_sync) +_resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *changed_entry, const Entry *old_entry) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); Entry *entry; @@ -354,18 +354,16 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c last_metric = expected_metric; } - if (do_sync) { - g_hash_table_iter_init (&iter, changed_metrics); - while (g_hash_table_iter_next (&iter, &ptr, NULL)) - _platform_route_sync_add (vtable, self, GPOINTER_TO_UINT (ptr)); - _platform_route_sync_flush (vtable, self); - } + g_hash_table_iter_init (&iter, changed_metrics); + while (g_hash_table_iter_next (&iter, &ptr, NULL)) + _platform_route_sync_add (vtable, self, GPOINTER_TO_UINT (ptr)); + _platform_route_sync_flush (vtable, self); g_hash_table_unref (changed_metrics); } static void -_entry_at_idx_update (const VTableIP *vtable, NMDefaultRouteManager *self, guint entry_idx, const Entry *old_entry, gboolean do_sync) +_entry_at_idx_update (const VTableIP *vtable, NMDefaultRouteManager *self, guint entry_idx, const Entry *old_entry) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); Entry *entry; @@ -390,11 +388,11 @@ _entry_at_idx_update (const VTableIP *vtable, NMDefaultRouteManager *self, guint g_ptr_array_sort_with_data (entries, _sort_entries_cmp, NULL); - _resync_all (vtable, self, entry, old_entry, do_sync); + _resync_all (vtable, self, entry, old_entry); } static void -_entry_at_idx_remove (const VTableIP *vtable, NMDefaultRouteManager *self, guint entry_idx, gboolean do_sync) +_entry_at_idx_remove (const VTableIP *vtable, NMDefaultRouteManager *self, guint entry_idx) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); Entry *entry; @@ -414,7 +412,7 @@ _entry_at_idx_remove (const VTableIP *vtable, NMDefaultRouteManager *self, guint g_ptr_array_index (entries, entry_idx) = NULL; g_ptr_array_remove_index (entries, entry_idx); - _resync_all (vtable, self, NULL, entry, do_sync); + _resync_all (vtable, self, NULL, entry); _entry_free (entry); } @@ -470,7 +468,7 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, entry->route.rx.ifindex, ip_ifindex); g_object_freeze_notify (G_OBJECT (self)); - _entry_at_idx_remove (vtable, self, entry_idx, FALSE); + _entry_at_idx_remove (vtable, self, entry_idx); g_assert (!_entry_find_by_source (entries, source, NULL)); _ipx_update_default_route (vtable, self, source); g_object_thaw_notify (G_OBJECT (self)); @@ -549,7 +547,7 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, entry->synced = synced; g_ptr_array_add (entries, entry); - _entry_at_idx_update (vtable, self, entries->len - 1, NULL, TRUE); + _entry_at_idx_update (vtable, self, entries->len - 1, NULL); } else if (default_route) { /* update */ Entry old_entry, new_entry; @@ -570,10 +568,10 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, old_entry = *entry; *entry = new_entry; - _entry_at_idx_update (vtable, self, entry_idx, &old_entry, TRUE); + _entry_at_idx_update (vtable, self, entry_idx, &old_entry); } else { /* delete */ - _entry_at_idx_remove (vtable, self, entry_idx, TRUE); + _entry_at_idx_remove (vtable, self, entry_idx); } } @@ -603,7 +601,7 @@ _ipx_remove_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); if (_entry_find_by_source (vtable->get_entries (priv), source, &entry_idx)) - _entry_at_idx_remove (vtable, self, entry_idx, TRUE); + _entry_at_idx_remove (vtable, self, entry_idx); } void From 462456f2554163f15037305c3b5320028476bd36 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 12 Nov 2014 11:22:01 +0100 Subject: [PATCH 05/13] policy: remove redundant remove_default_route() functions from NMDefaultRouteManager When calling update_default_route(), NMDefaultRouteManager will look at the source, and determine whether it has a default route or not. For example for device sources, this means calling nm_device_get_ip4_default_route(). If the source indicates that it has no default route, the effect of calling update_default_route() is the same as calling remove_default_route() (hence, remove() can be replaced by update()). If the source however still indicates a default route, the behavior would be different. This case would be an undesired inconsistancy, because source and NMDefaultRouteManager would disagree of whether the source has a default route. Source must always properly indicate whether it has a default route or not, hence this situation does not arise. Hence it is always better to call update(). Signed-off-by: Thomas Haller --- src/devices/nm-device.c | 4 ++-- src/nm-default-route-manager.c | 29 ----------------------------- src/nm-default-route-manager.h | 3 --- src/vpn-manager/nm-vpn-connection.c | 9 ++------- 4 files changed, 4 insertions(+), 41 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 864a7d57bd..a533048c62 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -6935,8 +6935,8 @@ _cleanup_generic_post (NMDevice *self, gboolean deconfigure) priv->default_route.v4_has = FALSE; priv->default_route.v6_has = FALSE; - nm_default_route_manager_ip4_remove_default_route (nm_default_route_manager_get (), self); - nm_default_route_manager_ip6_remove_default_route (nm_default_route_manager_get (), self); + 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); /* Clean up IP configs; this does not actually deconfigure the * interface; the caller must flush routes and addresses explicitly. diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index cc2c6daee7..0cd7c1b582 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -589,35 +589,6 @@ nm_default_route_manager_ip6_update_default_route (NMDefaultRouteManager *self, /***********************************************************************************/ -static void -_ipx_remove_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, gpointer source) -{ - NMDefaultRouteManagerPrivate *priv; - guint entry_idx; - - g_return_if_fail (NM_IS_DEFAULT_ROUTE_MANAGER (self)); - g_return_if_fail (NM_IS_DEVICE (source) || NM_IS_VPN_CONNECTION (source)); - - priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); - - if (_entry_find_by_source (vtable->get_entries (priv), source, &entry_idx)) - _entry_at_idx_remove (vtable, self, entry_idx); -} - -void -nm_default_route_manager_ip4_remove_default_route (NMDefaultRouteManager *self, gpointer source) -{ - _ipx_remove_default_route (&vtable_ip4, self, source); -} - -void -nm_default_route_manager_ip6_remove_default_route (NMDefaultRouteManager *self, gpointer source) -{ - _ipx_remove_default_route (&vtable_ip6, self, source); -} - -/***********************************************************************************/ - static gboolean _ipx_connection_has_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, NMConnection *connection) { diff --git a/src/nm-default-route-manager.h b/src/nm-default-route-manager.h index 88fb59f0c9..d8e422735e 100644 --- a/src/nm-default-route-manager.h +++ b/src/nm-default-route-manager.h @@ -51,9 +51,6 @@ NMDefaultRouteManager *nm_default_route_manager_get (void); void nm_default_route_manager_ip4_update_default_route (NMDefaultRouteManager *manager, gpointer source); void nm_default_route_manager_ip6_update_default_route (NMDefaultRouteManager *manager, gpointer source); -void nm_default_route_manager_ip4_remove_default_route (NMDefaultRouteManager *manager, gpointer source); -void nm_default_route_manager_ip6_remove_default_route (NMDefaultRouteManager *manager, gpointer source); - gboolean nm_default_route_manager_ip4_connection_has_default_route (NMDefaultRouteManager *manager, NMConnection *connection); gboolean nm_default_route_manager_ip6_connection_has_default_route (NMDefaultRouteManager *manager, NMConnection *connection); diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c index 682e1dc5b7..be1ce60f93 100644 --- a/src/vpn-manager/nm-vpn-connection.c +++ b/src/vpn-manager/nm-vpn-connection.c @@ -237,9 +237,6 @@ vpn_cleanup (NMVpnConnection *connection, NMDevice *parent_dev) nm_platform_address_flush (priv->ip_ifindex); } - nm_default_route_manager_ip4_remove_default_route (nm_default_route_manager_get (), connection); - nm_default_route_manager_ip6_remove_default_route (nm_default_route_manager_get (), connection); - nm_device_set_vpn4_config (parent_dev, NULL); nm_device_set_vpn6_config (parent_dev, NULL); @@ -327,10 +324,8 @@ _set_vpn_state (NMVpnConnection *connection, dispatcher_cleanup (connection); - if (vpn_state >= STATE_DISCONNECTED && vpn_state <= STATE_FAILED) { - nm_default_route_manager_ip4_remove_default_route (nm_default_route_manager_get (), connection); - nm_default_route_manager_ip6_remove_default_route (nm_default_route_manager_get (), connection); - } + nm_default_route_manager_ip4_update_default_route (nm_default_route_manager_get (), connection); + nm_default_route_manager_ip6_update_default_route (nm_default_route_manager_get (), connection); /* The connection gets destroyed by the VPN manager when it enters the * disconnected/failed state, but we need to keep it around for a bit From 57dd4a125b93587d982c8f0d7e9955217997e2f2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 11 Nov 2014 10:24:31 +0100 Subject: [PATCH 06/13] device: only add default route when having any addresses This fixes the failure to add a default route because no addresses are configured yet. Signed-off-by: Thomas Haller --- src/devices/nm-device.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index a533048c62..b505f3ed74 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -238,6 +238,7 @@ typedef struct { IpState ip4_state; NMIP4Config * dev_ip4_config; /* Config from DHCP, PPP, LLv4, etc */ NMIP4Config * ext_ip4_config; /* Stuff added outside NM */ + gboolean ext_ip4_config_had_any_addresses; NMIP4Config * wwan_ip4_config; /* WWAN configuration */ struct { gboolean v4_has; @@ -273,6 +274,7 @@ typedef struct { NMIP6Config * vpn6_config; /* routes added by a VPN which uses this device */ NMIP6Config * wwan_ip6_config; NMIP6Config * ext_ip6_config; /* Stuff added outside NM */ + gboolean ext_ip6_config_had_any_addresses; gboolean nm_ipv6ll; /* TRUE if NM handles the device's IPv6LL address */ NMRDisc * rdisc; @@ -2808,7 +2810,12 @@ ip4_config_merge_and_apply (NMDevice *self, if (assumed) priv->default_route.v4_has = _device_get_default_route_from_platform (self, AF_INET, (NMPlatformIPRoute *) route); - else { + else 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 + * that we already have addresses, or if we are about to commit any addresses. + * Otherwise adding a default route will fail, because NMDefaultRouteManager does not add any + * addresses for the route. */ gateway = nm_ip4_config_get_gateway (composite); if ( gateway || nm_device_get_device_type (self) == NM_DEVICE_TYPE_MODEM) { @@ -3357,7 +3364,12 @@ ip6_config_merge_and_apply (NMDevice *self, if (assumed) priv->default_route.v6_has = _device_get_default_route_from_platform (self, AF_INET6, (NMPlatformIPRoute *) route); - else { + else 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 + * that we already have addresses, or if we are about to commit any addresses. + * Otherwise adding a default route will fail, because NMDefaultRouteManager does not add any + * addresses for the route. */ gateway = nm_ip6_config_get_gateway (composite); if (gateway) { memset (route, 0, sizeof (*route)); @@ -6336,7 +6348,8 @@ update_ip_config (NMDevice *self, gboolean initial) /* IPv4 */ g_clear_object (&priv->ext_ip4_config); priv->ext_ip4_config = nm_ip4_config_capture (ifindex, capture_resolv_conf); - + priv->ext_ip4_config_had_any_addresses = ( priv->ext_ip4_config + && nm_ip4_config_get_num_addresses (priv->ext_ip4_config) > 0); if (priv->ext_ip4_config) { if (initial) { g_clear_object (&priv->dev_ip4_config); @@ -6355,6 +6368,8 @@ update_ip_config (NMDevice *self, gboolean initial) /* IPv6 */ g_clear_object (&priv->ext_ip6_config); priv->ext_ip6_config = nm_ip6_config_capture (ifindex, capture_resolv_conf, NM_SETTING_IP6_CONFIG_PRIVACY_UNKNOWN); + priv->ext_ip6_config_had_any_addresses = ( priv->ext_ip6_config + && nm_ip6_config_get_num_addresses (priv->ext_ip6_config) > 0); if (priv->ext_ip6_config) { /* Check this before modifying ext_ip6_config */ @@ -6954,6 +6969,9 @@ _cleanup_generic_post (NMDevice *self, gboolean deconfigure) g_clear_object (&priv->wwan_ip6_config); g_clear_object (&priv->ip6_config); + priv->ext_ip4_config_had_any_addresses = FALSE; + priv->ext_ip6_config_had_any_addresses = FALSE; + clear_act_request (self); /* Clear legacy IPv4 address property */ From 6d4bb29781065f24efb8b055865f27b7e34f183a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 10 Nov 2014 15:05:58 +0100 Subject: [PATCH 07/13] policy: minor refactoring in NMDefaultRouteManager to access routes generically Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 0cd7c1b582..0ca7166172 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -100,6 +100,7 @@ typedef struct { int addr_family; GPtrArray *(*get_entries) (NMDefaultRouteManagerPrivate *priv); const char *(*platform_route_to_string) (const NMPlatformIPRoute *route); + GArray *(*platform_route_get_all) (int ifindex, NMPlatformGetRouteMode mode); gboolean (*platform_route_delete_default) (int ifindex, guint32 metric); guint32 (*route_metric_normalize) (guint32 metric); } VTableIP; @@ -108,6 +109,15 @@ static const VTableIP vtable_ip4, vtable_ip6; #define VTABLE_IS_IP4 (vtable->addr_family == AF_INET) +static NMPlatformIPRoute * +_vt_route_index (const VTableIP *vtable, GArray *routes, guint index) +{ + if (VTABLE_IS_IP4) + return (NMPlatformIPRoute *) &g_array_index (routes, NMPlatformIP4Route, index); + else + return (NMPlatformIPRoute *) &g_array_index (routes, NMPlatformIP6Route, index); +} + static void _entry_free (Entry *entry) { @@ -206,20 +216,14 @@ _platform_route_sync_flush (const VTableIP *vtable, NMDefaultRouteManager *self) guint i, j; /* prune all other default routes from this device. */ - if (VTABLE_IS_IP4) - routes = nm_platform_ip4_route_get_all (0, NM_PLATFORM_GET_ROUTE_MODE_ONLY_DEFAULT); - else - routes = nm_platform_ip6_route_get_all (0, NM_PLATFORM_GET_ROUTE_MODE_ONLY_DEFAULT); + routes = vtable->platform_route_get_all (0, NM_PLATFORM_GET_ROUTE_MODE_ONLY_DEFAULT); for (i = 0; i < routes->len; i++) { const NMPlatformIPRoute *route; gboolean has_ifindex_synced = FALSE; Entry *entry = NULL; - if (VTABLE_IS_IP4) - route = (const NMPlatformIPRoute *) &g_array_index (routes, NMPlatformIP4Route, i); - else - route = (const NMPlatformIPRoute *) &g_array_index (routes, NMPlatformIP6Route, i); + route = _vt_route_index (vtable, routes, i); /* look at all entires and see if the route for this ifindex pair is * a known entry. */ @@ -902,6 +906,7 @@ static const VTableIP vtable_ip4 = { .addr_family = AF_INET, .get_entries = _v4_get_entries, .platform_route_to_string = (const char *(*)(const NMPlatformIPRoute *)) nm_platform_ip4_route_to_string, + .platform_route_get_all = nm_platform_ip4_route_get_all, .platform_route_delete_default = _v4_platform_route_delete_default, .route_metric_normalize = _v4_route_metric_normalize, }; @@ -910,6 +915,7 @@ static const VTableIP vtable_ip6 = { .addr_family = AF_INET6, .get_entries = _v6_get_entries, .platform_route_to_string = (const char *(*)(const NMPlatformIPRoute *)) nm_platform_ip6_route_to_string, + .platform_route_get_all = nm_platform_ip6_route_get_all, .platform_route_delete_default = _v6_platform_route_delete_default, .route_metric_normalize = nm_utils_ip6_route_metric_normalize, }; From b4b67c47f1f8f69e8221140cd30890f4e2bc7565 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 10 Nov 2014 13:27:33 +0100 Subject: [PATCH 08/13] policy: consider additional assumed routes when synchronizing the default route Don't only consider the best route of assumed devices when syncing the route metrics. This fixes the following scenario: Have em1 assumed, with two default routes (metric 20 and 21). When activating em2, NMDefaultRouteManager would have determined 21 as the effective metric, thus replacing the assumed route of em1. Since we don't want to touch assumed interfaces, it is wrong to replace their default routes. Instead, keep track of all the assumed default routes and consider their metrics when choosing effective_metric. Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 105 +++++++++++++++++++++++++++++++-- 1 file changed, 99 insertions(+), 6 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index 0ca7166172..d3c08dee62 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -153,7 +153,7 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); GPtrArray *entries = vtable->get_entries (priv); guint i; - gboolean has_unsynced_entry = FALSE; + Entry *entry_unsynced = NULL; Entry *entry = NULL; gboolean success; @@ -174,11 +174,15 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g if (!entry) entry = e; } else - has_unsynced_entry = TRUE; + entry_unsynced = e; } - /* For synced entries, we expect that the metric is chosen uniquely. */ - g_assert (!entry || !has_unsynced_entry || metric == G_MAXUINT32); + /* We don't expect to have an unsynced *and* a synced entry for the same metric. + * Unless, (a) their metric is G_MAXUINT32, in which case we could not find an unused effective metric, + * or (b) if we have an unsynced and a synced entry for the same ifindex. + * The latter case happens for example when activating an openvpn connection (synced) and + * assuming the corresponding tun0 interface (unsynced). */ + g_assert (!entry || !entry_unsynced || (entry->route.rx.ifindex == entry_unsynced->route.rx.ifindex) || metric == G_MAXUINT32); /* we only add the route, if we have an (to be synced) entry for it. */ if (!entry) @@ -296,21 +300,68 @@ _sort_entries_cmp (gconstpointer a, gconstpointer b, gpointer user_data) return 0; } +static GHashTable * +_get_assumed_interface_metrics (const VTableIP *vtable, NMDefaultRouteManager *self, GArray *routes) +{ + NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + GPtrArray *entries; + guint i, j; + GHashTable *result; + + /* create a list of all metrics that are currently assigned on an interface + * that is *not* already covered by one of our synced entries. + * IOW, returns the metrics that are in use by assumed interfaces + * that we want to preserve. */ + + entries = vtable->get_entries (priv); + + result = g_hash_table_new (NULL, NULL); + + for (i = 0; i < routes->len; i++) { + gboolean ifindex_has_synced_entry = FALSE; + const NMPlatformIPRoute *route; + + route = _vt_route_index (vtable, routes, i); + + for (j = 0; j < entries->len; j++) { + Entry *e = g_ptr_array_index (entries, j); + + if ( !e->never_default + && e->synced + && e->route.rx.ifindex == route->ifindex) { + ifindex_has_synced_entry = TRUE; + break; + } + } + + if (!ifindex_has_synced_entry) + g_hash_table_add (result, GUINT_TO_POINTER (vtable->route_metric_normalize (route->metric))); + } + + return result; +} + static void _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *changed_entry, const Entry *old_entry) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); Entry *entry; - guint i; + guint i, j; gint64 last_metric = -1; guint32 expected_metric; GPtrArray *entries; GHashTableIter iter; gpointer ptr; GHashTable *changed_metrics = g_hash_table_new (NULL, NULL); + GHashTable *assumed_metrics; + GArray *routes; entries = vtable->get_entries (priv); + routes = vtable->platform_route_get_all (0, NM_PLATFORM_GET_ROUTE_MODE_ONLY_DEFAULT); + + assumed_metrics = _get_assumed_interface_metrics (vtable, self, routes); + if (old_entry && old_entry->synced) { /* The old version obviously changed. */ g_hash_table_add (changed_metrics, GUINT_TO_POINTER (old_entry->effective_metric)); @@ -326,7 +377,24 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c continue; if (!entry->synced) { - last_metric = MAX (last_metric, (gint64) entry->effective_metric); + gboolean has_synced_entry = FALSE; + + /* A non synced entry is completely ignored, if we have + * a synced entry for the same if index. + * Otherwise the metric of the entry is still remembered as + * last_metric to avoid reusing it. */ + for (j = 0; j < entries->len; j++) { + const Entry *e = g_ptr_array_index (entries, j); + + if ( !e->never_default + && e->synced + && e->route.rx.ifindex == entry->route.rx.ifindex) { + has_synced_entry = TRUE; + break; + } + } + if (!has_synced_entry) + last_metric = MAX (last_metric, (gint64) entry->effective_metric); continue; } @@ -334,6 +402,28 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c if ((gint64) expected_metric <= last_metric) expected_metric = last_metric == G_MAXUINT32 ? G_MAXUINT32 : last_metric + 1; + while ( expected_metric < G_MAXUINT32 + && g_hash_table_contains (assumed_metrics, GUINT_TO_POINTER (expected_metric))) { + gboolean has_metric_for_ifindex = FALSE; + + /* Check if there are assumed devices that have default routes with this metric. + * If there are any, we have to pick another effective_metric. */ + + /* However, if there is a matching route (ifindex+metric) for our current entry, we are done. */ + for (j = 0; j < routes->len; j++) { + const NMPlatformIPRoute *r = _vt_route_index (vtable, routes, i); + + if ( r->metric == expected_metric + && r->ifindex == entry->route.rx.ifindex) { + has_metric_for_ifindex = TRUE; + break; + } + } + if (has_metric_for_ifindex) + break; + expected_metric++; + } + if (changed_entry == entry) { /* for the changed entry, the previous metric was either old_entry->effective_metric, * or none. Hence, we only have to remember what is going to change. */ @@ -358,12 +448,15 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c last_metric = expected_metric; } + g_array_free (routes, TRUE); + g_hash_table_iter_init (&iter, changed_metrics); while (g_hash_table_iter_next (&iter, &ptr, NULL)) _platform_route_sync_add (vtable, self, GPOINTER_TO_UINT (ptr)); _platform_route_sync_flush (vtable, self); g_hash_table_unref (changed_metrics); + g_hash_table_unref (assumed_metrics); } static void From 16b0ddb66dbc07a14ae85c0e554450d791bf8605 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 10 Nov 2014 14:09:55 +0100 Subject: [PATCH 09/13] policy: resync routes on platform change events Monitor default routes from platform, and resync the default routes on changes. For one, this fixes the following use-case: have an assumed device em1 with two routes of metric 20 and 21. Activate em2, which will get effective metric 22. When externally removing route em1/20, em2 would resync the effective metric to 20. This is correct and already worked before. However, when deleting em1/21, nothing happened. With this change, em2 would resync to metric 21 to fill the gap. However this commit has much bigger effects: whenever the user externally adds a default route to an interface for which NM manages an default route, NM will delete it. Also, when deleting the default route (managed by NM), NM would readd it. Effectivly, the user can no longer mess with the default route on interfaces for which it manages the default route. If the connection is configured never-default, the user still can add default routes and NM will not touch them. Obviously, this has no effect for assumed devices either and the user can externally add and remove default routes as he wishes. https://bugzilla.gnome.org/show_bug.cgi?id=735512 Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 253 +++++++++++++++++++++++++++++++-- 1 file changed, 241 insertions(+), 12 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index d3c08dee62..ec1d07c741 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -37,6 +37,13 @@ typedef struct { GPtrArray *entries_ip4; GPtrArray *entries_ip6; + struct { + guint guard; + guint backoff_wait_time_ms; + guint idle_handle; + gboolean has_v4_changes; + gboolean has_v6_changes; + } resync; } NMDefaultRouteManagerPrivate; #define NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NM_TYPE_DEFAULT_ROUTE_MANAGER, NMDefaultRouteManagerPrivate)) @@ -51,7 +58,7 @@ static NMDefaultRouteManager *_instance; guint64 __domain = __addr_family == AF_INET ? LOGD_IP4 : LOGD_IP6; \ \ if (nm_logging_enabled ((level), (__domain))) { \ - char __ch = __addr_family == AF_INET ? '4' : '6'; \ + char __ch = __addr_family == AF_INET ? '4' : (__addr_family == AF_INET6 ? '6' : '-'); \ char __prefix[30] = "default-route"; \ \ if ((self) != _instance) \ @@ -78,6 +85,10 @@ static NMDefaultRouteManager *_instance; /***********************************************************************************/ +static void _resync_idle_cancel (NMDefaultRouteManager *self); + +/***********************************************************************************/ + typedef struct { union { void *pointer; @@ -118,6 +129,34 @@ _vt_route_index (const VTableIP *vtable, GArray *routes, guint index) return (NMPlatformIPRoute *) &g_array_index (routes, NMPlatformIP6Route, index); } +static gboolean +_vt_routes_has_entry (const VTableIP *vtable, GArray *routes, const Entry *entry) +{ + guint i; + NMPlatformIPXRoute route = entry->route; + + route.rx.metric = entry->effective_metric; + + if (VTABLE_IS_IP4) { + for (i = 0; i < routes->len; i++) { + NMPlatformIP4Route *r = &g_array_index (routes, NMPlatformIP4Route, i); + + route.rx.source = r->source; + if (nm_platform_ip4_route_cmp (r, &route.r4) == 0) + return TRUE; + } + } else { + for (i = 0; i < routes->len; i++) { + NMPlatformIP6Route *r = &g_array_index (routes, NMPlatformIP6Route, i); + + route.rx.source = r->source; + if (nm_platform_ip6_route_cmp (r, &route.r6) == 0) + return TRUE; + } + } + return FALSE; +} + static void _entry_free (Entry *entry) { @@ -147,7 +186,7 @@ _entry_find_by_source (GPtrArray *entries, gpointer source, guint *out_idx) return NULL; } -static void +static gboolean _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, guint32 metric) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); @@ -186,7 +225,7 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g /* we only add the route, if we have an (to be synced) entry for it. */ if (!entry) - return; + return FALSE; if (VTABLE_IS_IP4) { success = nm_platform_ip4_route_add (entry->route.rx.ifindex, @@ -209,15 +248,17 @@ _platform_route_sync_add (const VTableIP *vtable, NMDefaultRouteManager *self, g _LOGW (vtable->addr_family, "failed to add default route %s with effective metric %u", vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric); } + return TRUE; } -static void +static gboolean _platform_route_sync_flush (const VTableIP *vtable, NMDefaultRouteManager *self) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); GPtrArray *entries = vtable->get_entries (priv); GArray *routes; guint i, j; + gboolean changed = FALSE; /* prune all other default routes from this device. */ routes = vtable->platform_route_get_all (0, NM_PLATFORM_GET_ROUTE_MODE_ONLY_DEFAULT); @@ -252,10 +293,13 @@ _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 (has_ifindex_synced && !entry) { vtable->platform_route_delete_default (route->ifindex, route->metric); + changed = TRUE; + } } g_array_free (routes, TRUE); + return changed; } static int @@ -341,8 +385,8 @@ _get_assumed_interface_metrics (const VTableIP *vtable, NMDefaultRouteManager *s return result; } -static void -_resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *changed_entry, const Entry *old_entry) +static gboolean +_resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *changed_entry, const Entry *old_entry, gboolean external_change) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); Entry *entry; @@ -355,6 +399,19 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c GHashTable *changed_metrics = g_hash_table_new (NULL, NULL); GHashTable *assumed_metrics; GArray *routes; + gboolean changed = FALSE; + + g_assert (priv->resync.guard == 0); + priv->resync.guard++; + + if (!external_change) { + if (VTABLE_IS_IP4) + priv->resync.has_v4_changes = FALSE; + else + priv->resync.has_v6_changes = FALSE; + if (!priv->resync.has_v4_changes && !priv->resync.has_v6_changes) + _resync_idle_cancel (self); + } entries = vtable->get_entries (priv); @@ -442,9 +499,19 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c _LOGD (vtable->addr_family, LOG_ENTRY_FMT": resync metric %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric, (guint) expected_metric); + } else { + if (!_vt_routes_has_entry (vtable, routes, entry)) { + g_hash_table_add (changed_metrics, GUINT_TO_POINTER (entry->effective_metric)); + _LOGD (vtable->addr_family, LOG_ENTRY_FMT": readd route %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), + vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric, + (guint) entry->effective_metric); + } } - entry->effective_metric = expected_metric; + if (entry->effective_metric != expected_metric) { + entry->effective_metric = expected_metric; + changed = TRUE; + } last_metric = expected_metric; } @@ -452,11 +519,14 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c g_hash_table_iter_init (&iter, changed_metrics); while (g_hash_table_iter_next (&iter, &ptr, NULL)) - _platform_route_sync_add (vtable, self, GPOINTER_TO_UINT (ptr)); - _platform_route_sync_flush (vtable, self); + changed |= _platform_route_sync_add (vtable, self, GPOINTER_TO_UINT (ptr)); + changed |= _platform_route_sync_flush (vtable, self); g_hash_table_unref (changed_metrics); g_hash_table_unref (assumed_metrics); + + priv->resync.guard--; + return changed; } static void @@ -485,7 +555,7 @@ _entry_at_idx_update (const VTableIP *vtable, NMDefaultRouteManager *self, guint g_ptr_array_sort_with_data (entries, _sort_entries_cmp, NULL); - _resync_all (vtable, self, entry, old_entry); + _resync_all (vtable, self, entry, old_entry, FALSE); } static void @@ -509,7 +579,7 @@ _entry_at_idx_remove (const VTableIP *vtable, NMDefaultRouteManager *self, guint g_ptr_array_index (entries, entry_idx) = NULL; g_ptr_array_remove_index (entries, entry_idx); - _resync_all (vtable, self, NULL, entry); + _resync_all (vtable, self, NULL, entry, FALSE); _entry_free (entry); } @@ -1027,13 +1097,168 @@ nm_default_route_manager_get () /***********************************************************************************/ +static gboolean +_resync_idle_now (NMDefaultRouteManager *self) +{ + gboolean has_v4_changes, has_v6_changes; + gboolean changed = FALSE; + + NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + + has_v4_changes = priv->resync.has_v4_changes; + has_v6_changes = priv->resync.has_v6_changes; + + _LOGD (0, "resync: sync now (%u) (IPv4 changes: %s, IPv6 changes: %s)", priv->resync.idle_handle, + has_v4_changes ? "yes" : "no", has_v6_changes ? "yes" : "no"); + + priv->resync.has_v4_changes = FALSE; + priv->resync.has_v6_changes = FALSE; + priv->resync.idle_handle = 0; + priv->resync.backoff_wait_time_ms = + priv->resync.backoff_wait_time_ms == 0 + ? 100 + : priv->resync.backoff_wait_time_ms * 2; + + if (has_v4_changes) + changed |= _resync_all (&vtable_ip4, self, NULL, NULL, TRUE); + + if (has_v6_changes) + changed |= _resync_all (&vtable_ip6, self, NULL, NULL, TRUE); + + if (!changed) { + /* Nothing changed: reset the backoff wait time */ + _resync_idle_cancel (self); + } + + return G_SOURCE_REMOVE; +} + +static void +_resync_idle_cancel (NMDefaultRouteManager *self) +{ + NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + + if (priv->resync.idle_handle) { + _LOGD (0, "resync: cancelled (%u)", priv->resync.idle_handle); + g_source_remove (priv->resync.idle_handle); + priv->resync.idle_handle = 0; + } + priv->resync.backoff_wait_time_ms = 0; + priv->resync.has_v4_changes = FALSE; + priv->resync.has_v6_changes = FALSE; +} + +static void +_resync_idle_reschedule (NMDefaultRouteManager *self) +{ + NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + + /* since we react on external changes and readd/remove default routes for + * the interfaces we manage, there could be the erronous situation where two applications + * fight over a certain default route. + * Avoid this, by increasingly wait longer to touch the system (backoff wait time). */ + + if (priv->resync.backoff_wait_time_ms == 0) { + /* for scheduling idle, always reschedule (to process all other events first) */ + if (priv->resync.idle_handle) + g_source_remove (priv->resync.idle_handle); + else + _LOGD (0, "resync: schedule on idle"); + priv->resync.idle_handle = g_idle_add ((GSourceFunc) _resync_idle_now, self); + } else if (!priv->resync.idle_handle) { + priv->resync.idle_handle = g_timeout_add (priv->resync.backoff_wait_time_ms, (GSourceFunc) _resync_idle_now, self); + _LOGD (0, "resync: schedule in %u.%03u seconds (%u)", priv->resync.backoff_wait_time_ms/1000, + priv->resync.backoff_wait_time_ms%1000, priv->resync.idle_handle); + } +} + +static void +_platform_ipx_route_changed_cb (const VTableIP *vtable, + NMDefaultRouteManager *self, + const NMPlatformIPRoute *route) +{ + NMDefaultRouteManagerPrivate *priv; + + if (route && !NM_PLATFORM_IP_ROUTE_IS_DEFAULT (route)) { + /* we only care about address changes or changes of default route. */ + return; + } + + priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + + if (priv->resync.guard) { + /* callbacks while executing _resync_all() are ignored. */ + return; + } + + if (VTABLE_IS_IP4) + priv->resync.has_v4_changes = TRUE; + else + priv->resync.has_v6_changes = TRUE; + + _resync_idle_reschedule (self); +} + +static void +_platform_ip4_address_changed_cb (NMPlatform *platform, + int ifindex, + gpointer platform_object, + NMPlatformSignalChangeType change_type, + NMPlatformReason reason, + NMDefaultRouteManager *self) +{ + _platform_ipx_route_changed_cb (&vtable_ip4, self, NULL); +} + +static void +_platform_ip6_address_changed_cb (NMPlatform *platform, + int ifindex, + gpointer platform_object, + NMPlatformSignalChangeType change_type, + NMPlatformReason reason, + NMDefaultRouteManager *self) +{ + _platform_ipx_route_changed_cb (&vtable_ip6, self, NULL); +} + +static void +_platform_ip4_route_changed_cb (NMPlatform *platform, + int ifindex, + gpointer platform_object, + NMPlatformSignalChangeType change_type, + NMPlatformReason reason, + NMDefaultRouteManager *self) +{ + _platform_ipx_route_changed_cb (&vtable_ip4, self, platform_object); +} + +static void +_platform_ip6_route_changed_cb (NMPlatform *platform, + int ifindex, + gpointer platform_object, + NMPlatformSignalChangeType change_type, + NMPlatformReason reason, + NMDefaultRouteManager *self) +{ + _platform_ipx_route_changed_cb (&vtable_ip6, self, platform_object); +} + +/***********************************************************************************/ + static void nm_default_route_manager_init (NMDefaultRouteManager *self) { NMDefaultRouteManagerPrivate *priv = NM_DEFAULT_ROUTE_MANAGER_GET_PRIVATE (self); + NMPlatform *platform; priv->entries_ip4 = g_ptr_array_new_full (0, (GDestroyNotify) _entry_free); priv->entries_ip6 = g_ptr_array_new_full (0, (GDestroyNotify) _entry_free); + + platform = nm_platform_get (); + g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP4_ADDRESS_CHANGED, G_CALLBACK (_platform_ip4_address_changed_cb), self); + g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ADDRESS_CHANGED, G_CALLBACK (_platform_ip6_address_changed_cb), self); + g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP4_ROUTE_CHANGED, G_CALLBACK (_platform_ip4_route_changed_cb), self); + g_signal_connect (platform, NM_PLATFORM_SIGNAL_IP6_ROUTE_CHANGED, G_CALLBACK (_platform_ip6_route_changed_cb), self); } static void @@ -1051,6 +1276,10 @@ dispose (GObject *object) priv->entries_ip6 = NULL; } + _resync_idle_cancel (self); + + g_signal_handlers_disconnect_by_data (nm_platform_get (), self); + G_OBJECT_CLASS (nm_default_route_manager_parent_class)->dispose (object); } From 1f5f576c339052ef97160afb6d4388ee4c2425d2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 11 Nov 2014 12:29:46 +0100 Subject: [PATCH 10/13] policy: pick up externally configured default routes for managed interfaces The previous commit made NM enforce the default route on interfaces for which NM manages a default route. For interfaces that are configured never-default, NM will now pick up any externally configured default route, as if it was managed by NM. This is important, because NMDefaultRouteManager needs a notion of which is the best device. Without this change, it was agnostic to default routes on managed, never-default interfaces. Signed-off-by: Thomas Haller --- src/devices/nm-device.c | 46 +++++++++++++++++++++++----------- src/devices/nm-device.h | 4 +-- src/nm-default-route-manager.c | 7 +++--- 3 files changed, 38 insertions(+), 19 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index b505f3ed74..2a93f3e0dc 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -242,8 +242,10 @@ typedef struct { NMIP4Config * wwan_ip4_config; /* WWAN configuration */ struct { gboolean v4_has; + gboolean v4_is_assumed; NMPlatformIP4Route v4; gboolean v6_has; + gboolean v6_is_assumed; NMPlatformIP6Route v6; } default_route; @@ -726,7 +728,7 @@ nm_device_get_ip6_route_metric (NMDevice *self) } const NMPlatformIP4Route * -nm_device_get_ip4_default_route (NMDevice *self) +nm_device_get_ip4_default_route (NMDevice *self, gboolean *out_is_assumed) { NMDevicePrivate *priv; @@ -734,11 +736,14 @@ nm_device_get_ip4_default_route (NMDevice *self) priv = NM_DEVICE_GET_PRIVATE (self); + if (out_is_assumed) + *out_is_assumed = priv->default_route.v4_has && priv->default_route.v4_is_assumed; + return priv->default_route.v4_has ? &priv->default_route.v4 : NULL; } const NMPlatformIP6Route * -nm_device_get_ip6_default_route (NMDevice *self) +nm_device_get_ip6_default_route (NMDevice *self, gboolean *out_is_assumed) { NMDevicePrivate *priv; @@ -746,6 +751,9 @@ nm_device_get_ip6_default_route (NMDevice *self) priv = NM_DEVICE_GET_PRIVATE (self); + if (out_is_assumed) + *out_is_assumed = priv->default_route.v6_has && priv->default_route.v6_is_assumed; + return priv->default_route.v6_has ? &priv->default_route.v6 : NULL; } @@ -2786,6 +2794,7 @@ ip4_config_merge_and_apply (NMDevice *self, priv->default_route.v4_has = FALSE; if (connection) { gboolean assumed = nm_device_uses_assumed_connection (self); + NMPlatformIP4Route *route = &priv->default_route.v4; if (!nm_settings_connection_get_nm_generated_assumed (NM_SETTINGS_CONNECTION (connection))) { nm_ip4_config_merge_setting (composite, @@ -2804,14 +2813,12 @@ ip4_config_merge_and_apply (NMDevice *self, * NMDefaultRouteManager eventually configures (because the it might * tweak the effective metric). */ - if (nm_default_route_manager_ip4_connection_has_default_route (nm_default_route_manager_get (), connection)) { + if ( !assumed + && nm_default_route_manager_ip4_connection_has_default_route (nm_default_route_manager_get (), connection)) { guint32 gateway = 0; - NMPlatformIP4Route *route = &priv->default_route.v4; - if (assumed) - priv->default_route.v4_has = _device_get_default_route_from_platform (self, AF_INET, (NMPlatformIPRoute *) route); - else if ( (!commit && priv->ext_ip4_config_had_any_addresses) - || ( commit && nm_ip4_config_get_num_addresses (composite))) { + 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 * that we already have addresses, or if we are about to commit any addresses. * Otherwise adding a default route will fail, because NMDefaultRouteManager does not add any @@ -2825,6 +2832,7 @@ 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) @@ -2839,6 +2847,11 @@ ip4_config_merge_and_apply (NMDevice *self, } } } + } else { + /* 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; } } @@ -3340,6 +3353,7 @@ ip6_config_merge_and_apply (NMDevice *self, priv->default_route.v6_has = FALSE; if (connection) { gboolean assumed = nm_device_uses_assumed_connection (self); + NMPlatformIP6Route *route = &priv->default_route.v6; if (!nm_settings_connection_get_nm_generated_assumed (NM_SETTINGS_CONNECTION (connection))) { nm_ip6_config_merge_setting (composite, @@ -3358,14 +3372,12 @@ ip6_config_merge_and_apply (NMDevice *self, * NMDefaultRouteManager eventually configures (because the it might * tweak the effective metric). */ - if (nm_default_route_manager_ip6_connection_has_default_route (nm_default_route_manager_get (), connection)) { + if ( !assumed + && nm_default_route_manager_ip6_connection_has_default_route (nm_default_route_manager_get (), connection)) { const struct in6_addr *gateway = NULL; - NMPlatformIP6Route *route = &priv->default_route.v6; - if (assumed) - priv->default_route.v6_has = _device_get_default_route_from_platform (self, AF_INET6, (NMPlatformIPRoute *) route); - else if ( (!commit && priv->ext_ip6_config_had_any_addresses) - || ( commit && nm_ip6_config_get_num_addresses (composite))) { + 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 * that we already have addresses, or if we are about to commit any addresses. * Otherwise adding a default route will fail, because NMDefaultRouteManager does not add any @@ -3378,6 +3390,7 @@ 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) @@ -3392,6 +3405,11 @@ ip6_config_merge_and_apply (NMDevice *self, } } } + } else { + /* 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; } } diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 840a19c08b..64687f7cbe 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -362,8 +362,8 @@ gboolean nm_device_owns_iface (NMDevice *device, const char *iface); NMConnection *nm_device_new_default_connection (NMDevice *self); -const NMPlatformIP4Route *nm_device_get_ip4_default_route (NMDevice *self); -const NMPlatformIP6Route *nm_device_get_ip6_default_route (NMDevice *self); +const NMPlatformIP4Route *nm_device_get_ip4_default_route (NMDevice *self, gboolean *out_is_assumed); +const NMPlatformIP6Route *nm_device_get_ip6_default_route (NMDevice *self, gboolean *out_is_assumed); void nm_device_spawn_iface_helper (NMDevice *self); diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index ec1d07c741..e5aa9b1eee 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -600,6 +600,7 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, NMVpnConnection *vpn = NULL; gboolean never_default = FALSE; gboolean synced; + gboolean is_assumed = FALSE; g_return_if_fail (NM_IS_DEFAULT_ROUTE_MANAGER (self)); if (NM_IS_DEVICE (source)) @@ -646,9 +647,9 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, if (ip_ifindex > 0) { if (device) { if (VTABLE_IS_IP4) - default_route = (const NMPlatformIPRoute *) nm_device_get_ip4_default_route (device); + 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); + default_route = (const NMPlatformIPRoute *) nm_device_get_ip6_default_route (device, &is_assumed); } else { NMConnection *connection = nm_active_connection_get_connection ((NMActiveConnection *) vpn); @@ -692,7 +693,7 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, /* if the source is never_default or the device uses an assumed connection, * we don't sync the route. */ - synced = !never_default && (!device || !nm_device_uses_assumed_connection (device)); + synced = !never_default && !is_assumed; if (!entry && !default_route) /* nothing to do */; From 815e67a61f95a327b8bc55b6f7a8c8806a7fa8e0 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 14 Nov 2014 13:28:18 +0100 Subject: [PATCH 11/13] policy: sort default routes by metrics before adding them It's better to add the more important routes first. Otherwise there might be a short time when a lower priority route has precedence. Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 43 ++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index e5aa9b1eee..d0aa3f4bfe 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -385,6 +385,17 @@ _get_assumed_interface_metrics (const VTableIP *vtable, NMDefaultRouteManager *s return result; } +static int +_sort_metrics_ascending_fcn (gconstpointer a, gconstpointer b) +{ + guint32 m_a = *((guint32 *) a); + guint32 m_b = *((guint32 *) b); + + if (m_a < m_b) + return -1; + return m_a == m_b ? 0 : 1; +} + static gboolean _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *changed_entry, const Entry *old_entry, gboolean external_change) { @@ -394,9 +405,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c gint64 last_metric = -1; guint32 expected_metric; GPtrArray *entries; - GHashTableIter iter; - gpointer ptr; - GHashTable *changed_metrics = g_hash_table_new (NULL, NULL); + GArray *changed_metrics = g_array_new (FALSE, FALSE, sizeof (guint32)); GHashTable *assumed_metrics; GArray *routes; gboolean changed = FALSE; @@ -421,7 +430,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c if (old_entry && old_entry->synced) { /* The old version obviously changed. */ - g_hash_table_add (changed_metrics, GUINT_TO_POINTER (old_entry->effective_metric)); + g_array_append_val (changed_metrics, old_entry->effective_metric); } /* first iterate over all entries and adjust the effective metrics. */ @@ -484,7 +493,7 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c if (changed_entry == entry) { /* for the changed entry, the previous metric was either old_entry->effective_metric, * or none. Hence, we only have to remember what is going to change. */ - g_hash_table_add (changed_metrics, GUINT_TO_POINTER (expected_metric)); + g_array_append_val (changed_metrics, expected_metric); if (old_entry) { _LOGD (vtable->addr_family, LOG_ENTRY_FMT": update %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), vtable->platform_route_to_string (&entry->route.rx), (guint) old_entry->effective_metric, @@ -494,14 +503,14 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c vtable->platform_route_to_string (&entry->route.rx), (guint) expected_metric); } } else if (entry->effective_metric != expected_metric) { - g_hash_table_add (changed_metrics, GUINT_TO_POINTER (entry->effective_metric)); - g_hash_table_add (changed_metrics, GUINT_TO_POINTER (expected_metric)); + g_array_append_val (changed_metrics, entry->effective_metric); + g_array_append_val (changed_metrics, expected_metric); _LOGD (vtable->addr_family, LOG_ENTRY_FMT": resync metric %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric, (guint) expected_metric); } else { if (!_vt_routes_has_entry (vtable, routes, entry)) { - g_hash_table_add (changed_metrics, GUINT_TO_POINTER (entry->effective_metric)); + g_array_append_val (changed_metrics, entry->effective_metric); _LOGD (vtable->addr_family, LOG_ENTRY_FMT": readd route %s (%u -> %u)", LOG_ENTRY_ARGS (i, entry), vtable->platform_route_to_string (&entry->route.rx), (guint) entry->effective_metric, (guint) entry->effective_metric); @@ -517,12 +526,22 @@ _resync_all (const VTableIP *vtable, NMDefaultRouteManager *self, const Entry *c g_array_free (routes, TRUE); - g_hash_table_iter_init (&iter, changed_metrics); - while (g_hash_table_iter_next (&iter, &ptr, NULL)) - changed |= _platform_route_sync_add (vtable, self, GPOINTER_TO_UINT (ptr)); + g_array_sort (changed_metrics, _sort_metrics_ascending_fcn); + last_metric = -1; + for (j = 0; j < changed_metrics->len; j++) { + expected_metric = g_array_index (changed_metrics, guint32, j); + + if (last_metric == (gint64) expected_metric) { + /* skip duplicates. */ + continue; + } + changed |= _platform_route_sync_add (vtable, self, expected_metric); + last_metric = expected_metric; + } + changed |= _platform_route_sync_flush (vtable, self); - g_hash_table_unref (changed_metrics); + g_array_free (changed_metrics, TRUE); g_hash_table_unref (assumed_metrics); priv->resync.guard--; From 308a5e7953c74869ab385338a4a3d0500811c1a5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Nov 2014 16:29:05 +0100 Subject: [PATCH 12/13] 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 --- src/devices/nm-device.c | 17 +++-- src/nm-default-route-manager.c | 132 ++++++++++++++++++++++++--------- 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 From a0f81f266bac2a65385ef0b252c461ac152e85b9 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 19 Nov 2014 20:54:37 +0100 Subject: [PATCH 13/13] policy: enforce absence of default route on never-default VPN connections Signed-off-by: Thomas Haller --- src/nm-default-route-manager.c | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/src/nm-default-route-manager.c b/src/nm-default-route-manager.c index c006cff4df..4dec5853dd 100644 --- a/src/nm-default-route-manager.c +++ b/src/nm-default-route-manager.c @@ -119,14 +119,10 @@ typedef struct { * (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. + * Also, for VPN sources in addition we track them so that a never-default + * VPN connection can be choosen by get_best_config() to receive the DNS configuration. * - * (!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). + * (!synced && never_default): this combination makes no sense. */ gboolean synced; gboolean never_default; @@ -302,10 +298,6 @@ _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 - && !NM_IS_DEVICE (e->source.object)) - continue; - if ( e->route.rx.ifindex == route->ifindex && e->synced) { has_ifindex_synced = TRUE; @@ -400,10 +392,6 @@ _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 - && !NM_IS_DEVICE (e->source.object)) - continue; - if ( e->synced && e->route.rx.ifindex == route->ifindex) { ifindex_has_synced_entry = TRUE; @@ -765,7 +753,7 @@ _ipx_update_default_route (const VTableIP *vtable, NMDefaultRouteManager *self, } } } - synced = default_route && !never_default; + synced = TRUE; } } g_assert (!default_route || default_route->plen == 0);