From 0055a7dd744ee4c12fa5147ce4f5e9a6fccdb0f7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 Jul 2015 12:32:37 +0200 Subject: [PATCH 1/5] route-manager: fix trace logging statement --- src/nm-route-manager.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nm-route-manager.c b/src/nm-route-manager.c index 5afce85489..2a2ee23939 100644 --- a/src/nm-route-manager.c +++ b/src/nm-route-manager.c @@ -549,7 +549,7 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const g_assert (cur_plat_route->rx.ifindex == ifindex); - _LOGT (vtable->vt->addr_family, "%3d: platform rt #%u - %s", ifindex, i_ipx_routes, vtable->vt->route_to_string (cur_plat_route)); + _LOGT (vtable->vt->addr_family, "%3d: platform rt #%u - %s", ifindex, i_plat_routes, vtable->vt->route_to_string (cur_plat_route)); /* skip over @cur_ipx_route that are ordered before @cur_plat_route */ while ( cur_ipx_route From 635eea60cfb340cbefec67732467f9075e7dc6c2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 Jul 2015 11:42:59 +0200 Subject: [PATCH 2/5] route-manager: add compare function for route-destination --- src/nm-route-manager.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/src/nm-route-manager.c b/src/nm-route-manager.c index 2a2ee23939..fbccb2e827 100644 --- a/src/nm-route-manager.c +++ b/src/nm-route-manager.c @@ -75,6 +75,12 @@ NM_DEFINE_SINGLETON_GETTER (NMRouteManager, nm_route_manager_get, NM_TYPE_ROUTE_ typedef struct { const NMPlatformVTableRoute *vt; + /* a compare function for two routes that considers only the destination fields network/plen. + * It is a looser comparisong then @route_id_cmp(), that means that if @route_dest_cmp() + * returns non-zero, also @route_id_cmp() returns the same value. It also means, that + * sorting by @route_id_cmp() implicitly sorts by @route_dest_cmp() as well. */ + int (*route_dest_cmp) (const NMPlatformIPXRoute *r1, const NMPlatformIPXRoute *r2); + /* a compare function for two routes that considers only the fields network/plen,metric. */ int (*route_id_cmp) (const NMPlatformIPXRoute *r1, const NMPlatformIPXRoute *r2); } VTableIP; @@ -202,13 +208,22 @@ ASSERT_route_index_valid (const VTableIP *vtable, const GArray *entries, const R /*********************************************************************************************/ +static int +_v4_route_dest_cmp (const NMPlatformIP4Route *r1, const NMPlatformIP4Route *r2) +{ + CMP_AND_RETURN_INT (r1->plen, r2->plen); + CMP_AND_RETURN_INT (nm_utils_ip4_address_clear_host_address (r1->network, r1->plen), + nm_utils_ip4_address_clear_host_address (r2->network, r2->plen)); + return 0; +} + static int _v4_route_id_cmp (const NMPlatformIP4Route *r1, const NMPlatformIP4Route *r2) { CMP_AND_RETURN_INT (r1->plen, r2->plen); - CMP_AND_RETURN_INT (r1->metric, r2->metric); CMP_AND_RETURN_INT (nm_utils_ip4_address_clear_host_address (r1->network, r1->plen), nm_utils_ip4_address_clear_host_address (r2->network, r2->plen)); + CMP_AND_RETURN_INT (r1->metric, r2->metric); return 0; } @@ -216,10 +231,27 @@ static int _v6_route_id_cmp (const NMPlatformIP6Route *r1, const NMPlatformIP6Route *r2) { struct in6_addr n1, n2; + int c; CMP_AND_RETURN_INT (r1->plen, r2->plen); + + nm_utils_ip6_address_clear_host_address (&n1, &r1->network, r1->plen); + nm_utils_ip6_address_clear_host_address (&n2, &r2->network, r2->plen); + c = memcmp (&n1, &n2, sizeof (n1)); + if (c != 0) + return c; + CMP_AND_RETURN_INT (nm_utils_ip6_route_metric_normalize (r1->metric), nm_utils_ip6_route_metric_normalize (r2->metric)); + return 0; +} + +static int +_v6_route_dest_cmp (const NMPlatformIP6Route *r1, const NMPlatformIP6Route *r2) +{ + struct in6_addr n1, n2; + + CMP_AND_RETURN_INT (r1->plen, r2->plen); nm_utils_ip6_address_clear_host_address (&n1, &r1->network, r1->plen); nm_utils_ip6_address_clear_host_address (&n2, &r2->network, r2->plen); @@ -874,11 +906,13 @@ nm_route_manager_ip4_route_register_device_route_purge_list (NMRouteManager *sel static const VTableIP vtable_v4 = { .vt = &nm_platform_vtable_route_v4, + .route_dest_cmp = (int (*) (const NMPlatformIPXRoute *, const NMPlatformIPXRoute *)) _v4_route_dest_cmp, .route_id_cmp = (int (*) (const NMPlatformIPXRoute *, const NMPlatformIPXRoute *)) _v4_route_id_cmp, }; static const VTableIP vtable_v6 = { .vt = &nm_platform_vtable_route_v6, + .route_dest_cmp = (int (*) (const NMPlatformIPXRoute *, const NMPlatformIPXRoute *)) _v6_route_dest_cmp, .route_id_cmp = (int (*) (const NMPlatformIPXRoute *, const NMPlatformIPXRoute *)) _v6_route_id_cmp, }; From 09fdf58f4d79a3ca4536b16c8d9b17c19403dbe4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 Jul 2015 13:25:49 +0200 Subject: [PATCH 3/5] platform: add optional @metric argument to route_add() function Allow overwriting the route metric. --- src/nm-route-manager.c | 4 ++-- src/platform/nm-platform.c | 8 ++++---- src/platform/nm-platform.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/nm-route-manager.c b/src/nm-route-manager.c index fbccb2e827..18c0432c38 100644 --- a/src/nm-route-manager.c +++ b/src/nm-route-manager.c @@ -612,7 +612,7 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const * device routes, on the second the others (gateway routes). */ continue; } - vtable->vt->route_add (priv->platform, 0, rest_route); + vtable->vt->route_add (priv->platform, 0, rest_route, -1); } } g_array_unref (to_restore_routes); @@ -656,7 +656,7 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const || route_id_cmp_result != 0 || !_route_equals_ignoring_ifindex (vtable, cur_plat_route, cur_ipx_route)) { - if (!vtable->vt->route_add (priv->platform, ifindex, cur_ipx_route)) { + if (!vtable->vt->route_add (priv->platform, ifindex, cur_ipx_route, -1)) { if (cur_ipx_route->rx.source < NM_IP_CONFIG_SOURCE_USER) { _LOGD (vtable->vt->addr_family, "ignore error adding IPv%c route to kernel: %s", diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index ed0d3c4340..b534b273d2 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -2947,7 +2947,7 @@ log_ip6_route (NMPlatform *self, NMPObjectType obj_type, int ifindex, NMPlatform /******************************************************************/ static gboolean -_vtr_v4_route_add (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route) +_vtr_v4_route_add (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route, gint64 metric) { return nm_platform_ip4_route_add (self, ifindex > 0 ? ifindex : route->rx.ifindex, @@ -2956,12 +2956,12 @@ _vtr_v4_route_add (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *rout route->rx.plen, route->r4.gateway, route->r4.pref_src, - route->rx.metric, + metric >= 0 ? (guint32) metric : route->rx.metric, route->rx.mss); } static gboolean -_vtr_v6_route_add (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route) +_vtr_v6_route_add (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route, gint64 metric) { return nm_platform_ip6_route_add (self, ifindex > 0 ? ifindex : route->rx.ifindex, @@ -2969,7 +2969,7 @@ _vtr_v6_route_add (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *rout route->r6.network, route->rx.plen, route->r6.gateway, - route->rx.metric, + metric >= 0 ? (guint32) metric : route->rx.metric, route->rx.mss); } diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 9bfb22b1a7..1b5bfb1575 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -315,7 +315,7 @@ typedef struct { int (*route_cmp) (const NMPlatformIPXRoute *a, const NMPlatformIPXRoute *b); const char *(*route_to_string) (const NMPlatformIPXRoute *route); GArray *(*route_get_all) (NMPlatform *self, int ifindex, NMPlatformGetRouteFlags flags); - gboolean (*route_add) (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route); + gboolean (*route_add) (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route, gint64 metric); gboolean (*route_delete) (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route); gboolean (*route_delete_default) (NMPlatform *self, int ifindex, guint32 metric); guint32 (*metric_normalize) (guint32 metric); From 700bb965236636254d449506bb9cfe1dc4f3f4e6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 Jul 2015 18:04:08 +0200 Subject: [PATCH 4/5] route-manager: add optional @r2_metric argument to _route_equals_ignoring_ifindex() --- src/nm-route-manager.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/nm-route-manager.c b/src/nm-route-manager.c index 18c0432c38..a1d82c6b97 100644 --- a/src/nm-route-manager.c +++ b/src/nm-route-manager.c @@ -347,13 +347,16 @@ _route_index_reverse_idx (const VTableIP *vtable, const RouteIndex *index, guint /*********************************************************************************************/ static gboolean -_route_equals_ignoring_ifindex (const VTableIP *vtable, const NMPlatformIPXRoute *r1, const NMPlatformIPXRoute *r2) +_route_equals_ignoring_ifindex (const VTableIP *vtable, const NMPlatformIPXRoute *r1, const NMPlatformIPXRoute *r2, gint64 r2_metric) { NMPlatformIPXRoute r2_backup; - if (r1->rx.ifindex != r2->rx.ifindex) { + if ( r1->rx.ifindex != r2->rx.ifindex + || (r2_metric >= 0 && ((guint32) r2_metric) != r2->rx.metric)) { memcpy (&r2_backup, r2, vtable->vt->sizeof_route); r2_backup.rx.ifindex = r1->rx.ifindex; + if (r2_metric >= 0) + r2_backup.rx.metric = (guint32) r2_metric; r2 = &r2_backup; } return vtable->vt->route_cmp (r1, r2) == 0; @@ -513,7 +516,7 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const if ( cur_ipx_route && cur_known_route && route_id_cmp_result == 0) { - if (!_route_equals_ignoring_ifindex (vtable, cur_ipx_route, cur_known_route)) { + if (!_route_equals_ignoring_ifindex (vtable, cur_ipx_route, cur_known_route, -1)) { /* The routes match. Update the entry in place. As this is an exact match of primary * fields, this only updates possibly modified fields such as @gateway or @mss. * Modifiying @cur_ipx_route this way does not invalidate @ipx_routes->index. */ @@ -654,7 +657,7 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const * i.e. if @cur_plat_route is different from @cur_ipx_route. */ if ( !cur_plat_route || route_id_cmp_result != 0 - || !_route_equals_ignoring_ifindex (vtable, cur_plat_route, cur_ipx_route)) { + || !_route_equals_ignoring_ifindex (vtable, cur_plat_route, cur_ipx_route, -1)) { if (!vtable->vt->route_add (priv->platform, ifindex, cur_ipx_route, -1)) { if (cur_ipx_route->rx.source < NM_IP_CONFIG_SOURCE_USER) { From f5c087c8e96aae3b2906080e1b9ff1efdcd2bf39 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sun, 5 Jul 2015 10:04:03 +0200 Subject: [PATCH 5/5] route-manager: always add conflicting direct routes and bump the route-metric if necessary Kernel does not allow to add the same route (as determined by network/plen,metric) on two different interfaces (ifindex). In case of conflict, NMRouteManager used to ignore any but the firstly added route. On the other hand, we cannot add a gateway-route, if there is no direct route to the gateway. Hence, skipping duplicate routes can mean that we skip a direct route what was necessary to add another gateway-route, which then leads to a failure to add that route. This also applies to IPv4 device routes that since recently are managed by NMRouteManager. For example, say you connect two interfaces to the same IP subnet. The route-metric can conflict if the interfaces are of the same type or if the user explicitly configured a conflict. In case of conflicts, NMRouteManager would only configure the first appearing route and skip the shadowed route on the second interface. Now we cannot configure gateway-routes on the second interface because the gateway is unreachable. There are many scenarios where this issue can happen, especially with default-routes and user-configured-routes. For example with default-routes, ip4_config_merge_and_apply() would check if the default-gateway requires an explict route and possibly add it. But then NMRouteManager might not add the route because it is shadowed by a route on an other interface. This patch solves the issue by having NMRouteManager configure shadowed routes too, similar to what NMDefaultRouteManager does. It does that by searching for an unused, non-conflicting, higher metric for the route, i.e. bump the metric by 1 until we can add it without conflict. Also note that NMRouteManager still ensures that for conflicting routes the best route sticks to the interface that configured it first. That means if you later add the conflicting route on another interface, it will be added with higher metric and the data is still routed along the first interface. --- src/nm-route-manager.c | 228 +++++++++++++++++++++++++++------ src/tests/test-route-manager.c | 160 +++++++++++++++++++++-- 2 files changed, 340 insertions(+), 48 deletions(-) diff --git a/src/nm-route-manager.c b/src/nm-route-manager.c index a1d82c6b97..e4d18d3428 100644 --- a/src/nm-route-manager.c +++ b/src/nm-route-manager.c @@ -44,6 +44,13 @@ typedef struct { typedef struct { GArray *entries; RouteIndex *index; + + /* list of effective metrics. The indexes of the array correspond to @index, not @entries. */ + GArray *effective_metrics; + + /* this array contains the effective metrics but using the reversed index that corresponds + * to @entries, instead of @index. */ + GArray *effective_metrics_reverse; } RouteEntries; typedef struct { @@ -446,11 +453,14 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const RouteIndex *plat_routes_idx, *known_routes_idx; gboolean success = TRUE; guint i, i_type; - GArray *to_delete_indexes = NULL, *to_restore_routes = NULL; + GArray *to_delete_indexes = NULL; GPtrArray *to_add_routes = NULL; guint i_known_routes, i_plat_routes, i_ipx_routes; const NMPlatformIPXRoute *cur_known_route, *cur_plat_route; NMPlatformIPXRoute *cur_ipx_route; + gint64 *p_effective_metric = NULL; + gboolean ipx_routes_changed = FALSE; + gint64 *effective_metrics = NULL; nm_platform_process_events (priv->platform); @@ -462,6 +472,8 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const plat_routes_idx = _route_index_create (vtable, plat_routes); known_routes_idx = _route_index_create (vtable, known_routes); + effective_metrics = &g_array_index (ipx_routes->effective_metrics, gint64, 0); + ASSERT_route_index_valid (vtable, plat_routes, plat_routes_idx, TRUE); ASSERT_route_index_valid (vtable, known_routes, known_routes_idx, FALSE); @@ -472,7 +484,10 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const ifindex, i, vtable->vt->route_to_string (VTABLE_ROUTE_INDEX (vtable, known_routes, i))); } for (i = 0; i < ipx_routes->index->len; i++) - _LOGT (vtable->vt->addr_family, "%3d: STATE: has #%u - %s", ifindex, i, vtable->vt->route_to_string (ipx_routes->index->entries[i])); + _LOGT (vtable->vt->addr_family, "%3d: STATE: has #%u - %s (%lld)", + ifindex, i, + vtable->vt->route_to_string (ipx_routes->index->entries[i]), + (long long) g_array_index (ipx_routes->effective_metrics, gint64, i)); } /*************************************************************************** @@ -497,19 +512,6 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const to_delete_indexes = g_array_new (FALSE, FALSE, sizeof (guint)); g_array_append_val (to_delete_indexes, i_ipx_routes); - /* later we will delete @cur_ipx_route. See if @cur_ipx_route was shadowing another route, that - * we must restore. */ - if (i_ipx_routes + 1 < ipx_routes->index->len) { - const NMPlatformIPXRoute *next_route = ipx_routes->index->entries[i_ipx_routes + 1]; - - if (vtable->route_id_cmp (cur_ipx_route, next_route) == 0) { - if (!to_restore_routes) - to_restore_routes = g_array_new (FALSE, FALSE, vtable->vt->sizeof_route); - g_array_append_vals (to_restore_routes, next_route, 1); - g_assert (next_route->rx.ifindex != ifindex); - } - } - /* find the next @cur_ipx_route with matching ifindex. */ cur_ipx_route = _get_next_ipx_route (ipx_routes->index, FALSE, &i_ipx_routes, ifindex); } @@ -523,6 +525,7 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const memcpy (cur_ipx_route, cur_known_route, vtable->vt->sizeof_route); cur_ipx_route->rx.ifindex = ifindex; cur_ipx_route->rx.metric = vtable->vt->metric_normalize (cur_ipx_route->rx.metric); + ipx_routes_changed = TRUE; _LOGT (vtable->vt->addr_family, "%3d: STATE: update #%u - %s", ifindex, i_ipx_routes, vtable->vt->route_to_string (cur_ipx_route)); } } else if (cur_known_route) { @@ -551,9 +554,14 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const } g_array_sort (to_delete_indexes, (GCompareFunc) _sort_indexes_cmp); nm_utils_array_remove_at_indexes (ipx_routes->entries, &g_array_index (to_delete_indexes, guint, 0), to_delete_indexes->len); + nm_utils_array_remove_at_indexes (ipx_routes->effective_metrics_reverse, &g_array_index (to_delete_indexes, guint, 0), to_delete_indexes->len); g_array_unref (to_delete_indexes); } if (to_add_routes) { + guint j = ipx_routes->effective_metrics_reverse->len; + + g_array_set_size (ipx_routes->effective_metrics_reverse, j + to_add_routes->len); + for (i = 0; i < to_add_routes->len; i++) { NMPlatformIPXRoute *ipx_route; @@ -563,15 +571,100 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const ipx_route->rx.ifindex = ifindex; ipx_route->rx.metric = vtable->vt->metric_normalize (ipx_route->rx.metric); + g_array_index (ipx_routes->effective_metrics_reverse, gint64, j++) = -1; + _LOGT (vtable->vt->addr_family, "%3d: STATE: added #%u - %s", ifindex, ipx_routes->entries->len - 1, vtable->vt->route_to_string (ipx_route)); } g_ptr_array_unref (to_add_routes); } g_free (ipx_routes->index); ipx_routes->index = _route_index_create (vtable, ipx_routes->entries); + ipx_routes_changed = TRUE; ASSERT_route_index_valid (vtable, ipx_routes->entries, ipx_routes->index, TRUE); } + if (ipx_routes_changed) { + /*************************************************************************** + * Rebuild the list of effective metrics. In case of conflicting routes, + * we configure device routes with a bumped metric. We do this, because non-direct + * routes might require this direct route to reach the gateway (e.g. the default + * route). + * + * We determine the effective metrics only based on our internal list @ipx_routes + * and don't consider @plat_routes. That means, we might bump the metric of a route + * and thereby cause a conflict with an existing route on an unmanaged device (which + * causes the route on the unmanaged device to be replaced). + * Still, that is not much different then from messing with unmanaged routes when + * the effective and the intended metrics equal. The rules is: NM will leave routes + * on unmanged devices alone, unless they conflict with what NM wants to configure. + ***************************************************************************/ + + g_array_set_size (ipx_routes->effective_metrics, ipx_routes->entries->len); + effective_metrics = &g_array_index (ipx_routes->effective_metrics, gint64, 0); + + /* Completely regenerate the list of effective metrics by walking through + * ipx_routes->index and determining the effective metric. */ + + for (i_ipx_routes = 0; i_ipx_routes < ipx_routes->index->len; i_ipx_routes++) { + gint64 *p_effective_metric_before; + gboolean is_shadowed; + guint i_ipx_routes_before; + + cur_ipx_route = ipx_routes->index->entries[i_ipx_routes]; + p_effective_metric = &effective_metrics[i_ipx_routes]; + + is_shadowed = i_ipx_routes > 0 + && vtable->route_dest_cmp (cur_ipx_route, ipx_routes->index->entries[i_ipx_routes - 1]) == 0; + + if (!is_shadowed) { + /* the route is not shadowed, the effective metric is just as specified. */ + *p_effective_metric = cur_ipx_route->rx.metric; + goto next; + } + if (!VTABLE_IS_DEVICE_ROUTE (vtable, cur_ipx_route)) { + /* The route is not a device route. We want to add redundant device routes, because + * we might need the direct routes to the gateway. For non-direct routes, there is not much + * reason to do the metric increment. */ + *p_effective_metric = -1; + goto next; + } + + /* The current route might be shadowed by several other routes. Find the one with the highest metric, + * i.e. the one with an effecive metric set and in the index before the current index. */ + i_ipx_routes_before = i_ipx_routes; + while (TRUE) { + nm_assert (i_ipx_routes_before > 0); + + i_ipx_routes_before--; + + p_effective_metric_before = &effective_metrics[i_ipx_routes_before]; + + if (*p_effective_metric_before == -1) { + /* this route is also shadowed, continue search. */ + continue; + } + + if (*p_effective_metric_before < cur_ipx_route->rx.metric) { + /* the previous route has a lower metric. There is no conflict, + * just use the original metric. */ + *p_effective_metric = cur_ipx_route->rx.metric; + } else if (*p_effective_metric_before == G_MAXUINT32) { + /* we cannot bump the metric. Don't configure this route. */ + *p_effective_metric = -1; + } else { + /* bump the metric by one. */ + *p_effective_metric = *p_effective_metric_before + 1; + } + break; + } +next: + _LOGT (vtable->vt->addr_family, "%3d: new metric #%u - %s (%lld)", + ifindex, i_ipx_routes, + vtable->vt->route_to_string (cur_ipx_route), + (long long) *p_effective_metric); + } + } + /*************************************************************************** * Delete routes in platform, that no longer exist in @ipx_routes ***************************************************************************/ @@ -579,8 +672,10 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const /* iterate over @plat_routes and @ipx_routes */ cur_plat_route = _get_next_plat_route (plat_routes_idx, TRUE, &i_plat_routes); cur_ipx_route = _get_next_ipx_route (ipx_routes->index, TRUE, &i_ipx_routes, ifindex); + if (cur_ipx_route) + p_effective_metric = &effective_metrics[i_ipx_routes]; while (cur_plat_route) { - int route_id_cmp_result = 0; + int route_dest_cmp_result = 0; g_assert (cur_plat_route->rx.ifindex == ifindex); @@ -588,37 +683,79 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const /* skip over @cur_ipx_route that are ordered before @cur_plat_route */ while ( cur_ipx_route - && ((route_id_cmp_result = vtable->route_id_cmp (cur_ipx_route, cur_plat_route)) < 0)) { + && ((route_dest_cmp_result = vtable->route_dest_cmp (cur_ipx_route, cur_plat_route)) <= 0)) { + if ( route_dest_cmp_result == 0 + && *p_effective_metric != -1 + && *p_effective_metric >= cur_plat_route->rx.metric) { + break; + } cur_ipx_route = _get_next_ipx_route (ipx_routes->index, FALSE, &i_ipx_routes, ifindex); + if (cur_ipx_route) + p_effective_metric = &effective_metrics[i_ipx_routes]; } /* if @cur_ipx_route is not equal to @plat_route, the route must be deleted. */ - if (!(cur_ipx_route && route_id_cmp_result == 0)) + if ( !cur_ipx_route + || route_dest_cmp_result != 0 + || *p_effective_metric != cur_plat_route->rx.metric) vtable->vt->route_delete (priv->platform, ifindex, cur_plat_route); cur_plat_route = _get_next_plat_route (plat_routes_idx, FALSE, &i_plat_routes); } /*************************************************************************** - * Restore shadowed routes. These routes are on an other @ifindex, but were - * shadowed before. Unshadow them now. + * Restore shadowed routes. These routes are on an other @ifindex then what + * we are syncing now. But the current changes make it necessary to add those + * routes. + * + * Only add some routes that might be necessary. We don't delete any routes + * on other ifindexes here. I.e. we don't do a full sync, but only ~add~ routes + * that were shadowed previously, but should be now present with a different + * metric. **************************************************************************/ - if (to_restore_routes) { - for (i_type = 0; i_type < 2; i_type++) { - for (i = 0; i < to_restore_routes->len; i++) { - const NMPlatformIPXRoute *rest_route = VTABLE_ROUTE_INDEX (vtable, to_restore_routes, i); + if (ipx_routes_changed) { + /* @effective_metrics_reverse contains the list of assigned metrics from the last + * sync. Walk through it and see what changes there are (and possibly restore a + * shadowed route). + * Thereby also update @effective_metrics_reverse to be up-to-date again. */ + for (i_ipx_routes = 0; i_ipx_routes < ipx_routes->entries->len; i_ipx_routes++) { + guint i_ipx_routes_reverse; + gint64 *p_effective_metric_reversed; - if ( (i_type == 0 && !VTABLE_IS_DEVICE_ROUTE (vtable, rest_route)) - || (i_type == 1 && VTABLE_IS_DEVICE_ROUTE (vtable, rest_route))) { - /* Make two runs over the list of @to_restore_routes. On the first, only add - * device routes, on the second the others (gateway routes). */ - continue; - } - vtable->vt->route_add (priv->platform, 0, rest_route, -1); + p_effective_metric = &effective_metrics[i_ipx_routes]; + + i_ipx_routes_reverse = _route_index_reverse_idx (vtable, ipx_routes->index, i_ipx_routes, ipx_routes->entries); + p_effective_metric_reversed = &g_array_index (ipx_routes->effective_metrics_reverse, gint64, i_ipx_routes_reverse); + + if (*p_effective_metric_reversed == *p_effective_metric) { + /* The entry is up to date. No change, continue with the next one. */ + continue; } + *p_effective_metric_reversed = *p_effective_metric; + + if (*p_effective_metric == -1) { + /* the entry is shadowed. Nothing to do. */ + continue; + } + + cur_ipx_route = ipx_routes->index->entries[i_ipx_routes]; + if (cur_ipx_route->rx.ifindex == ifindex) { + /* @cur_ipx_route is on the current @ifindex. No need to special handling them + * because we are about to do a full sync of the ifindex. */ + continue; + } + + /* the effective metric from previous sync changed. While @cur_ipx_route is not on the + * ifindex we are about to sync, we still must add this route. Possibly it was shadowed + * before, and now we want to restore it. + * + * Note that we don't do a full sync on the other ifindex. Especially, we don't delete + * or add any further routes then this. That means there might be some stale routes + * (with a higher metric!). They will only be removed on the next sync of that other + * ifindex. */ + vtable->vt->route_add (priv->platform, 0, cur_ipx_route, *p_effective_metric); } - g_array_unref (to_restore_routes); } /*************************************************************************** @@ -633,7 +770,7 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const * we need to know whether a route is shadowed by another route, and that * requires to look at @ipx_routes. */ for (; cur_ipx_route; cur_ipx_route = _get_next_ipx_route (ipx_routes->index, FALSE, &i_ipx_routes, ifindex)) { - int route_id_cmp_result = -1; + int route_id_dest_result = -1; if ( (i_type == 0 && !VTABLE_IS_DEVICE_ROUTE (vtable, cur_ipx_route)) || (i_type == 1 && VTABLE_IS_DEVICE_ROUTE (vtable, cur_ipx_route))) { @@ -642,24 +779,29 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const continue; } - if ( i_ipx_routes > 0 - && vtable->route_id_cmp (cur_ipx_route, ipx_routes->index->entries[i_ipx_routes - 1]) == 0) { + p_effective_metric = &effective_metrics[i_ipx_routes]; + + if (*p_effective_metric == -1) { /* @cur_ipx_route is shadewed by another route. */ continue; } /* skip over @plat_routes that are ordered before our @cur_ipx_route. */ while ( cur_plat_route - && (route_id_cmp_result = vtable->route_id_cmp (cur_plat_route, cur_ipx_route)) < 0) + && (route_id_dest_result = vtable->route_dest_cmp (cur_plat_route, cur_ipx_route)) <= 0) { + if ( route_id_dest_result == 0 + && cur_plat_route->rx.metric >= *p_effective_metric) + break; cur_plat_route = _get_next_plat_route (plat_routes_idx, FALSE, &i_plat_routes); + } /* only add the route if we don't have an identical route in @plat_routes, * i.e. if @cur_plat_route is different from @cur_ipx_route. */ if ( !cur_plat_route - || route_id_cmp_result != 0 - || !_route_equals_ignoring_ifindex (vtable, cur_plat_route, cur_ipx_route, -1)) { + || route_id_dest_result != 0 + || !_route_equals_ignoring_ifindex (vtable, cur_plat_route, cur_ipx_route, *p_effective_metric)) { - if (!vtable->vt->route_add (priv->platform, ifindex, cur_ipx_route, -1)) { + if (!vtable->vt->route_add (priv->platform, ifindex, cur_ipx_route, *p_effective_metric)) { if (cur_ipx_route->rx.source < NM_IP_CONFIG_SOURCE_USER) { _LOGD (vtable->vt->addr_family, "ignore error adding IPv%c route to kernel: %s", @@ -930,6 +1072,10 @@ nm_route_manager_init (NMRouteManager *self) priv->ip4_routes.entries = g_array_new (FALSE, FALSE, sizeof (NMPlatformIP4Route)); priv->ip6_routes.entries = g_array_new (FALSE, FALSE, sizeof (NMPlatformIP6Route)); + priv->ip4_routes.effective_metrics = g_array_new (FALSE, FALSE, sizeof (gint64)); + priv->ip6_routes.effective_metrics = g_array_new (FALSE, FALSE, sizeof (gint64)); + priv->ip4_routes.effective_metrics_reverse = g_array_new (FALSE, FALSE, sizeof (gint64)); + priv->ip6_routes.effective_metrics_reverse = g_array_new (FALSE, FALSE, sizeof (gint64)); priv->ip4_routes.index = _route_index_create (&vtable_v4, priv->ip4_routes.entries); priv->ip6_routes.index = _route_index_create (&vtable_v6, priv->ip6_routes.entries); priv->ip4_device_routes.entries = g_hash_table_new_full ((GHashFunc) nmp_object_id_hash, @@ -959,6 +1105,10 @@ finalize (GObject *object) g_array_free (priv->ip4_routes.entries, TRUE); g_array_free (priv->ip6_routes.entries, TRUE); + g_array_free (priv->ip4_routes.effective_metrics, TRUE); + g_array_free (priv->ip6_routes.effective_metrics, TRUE); + g_array_free (priv->ip4_routes.effective_metrics_reverse, TRUE); + g_array_free (priv->ip6_routes.effective_metrics_reverse, TRUE); g_free (priv->ip4_routes.index); g_free (priv->ip6_routes.index); diff --git a/src/tests/test-route-manager.c b/src/tests/test-route-manager.c index d62ffea33d..5325b83fae 100644 --- a/src/tests/test-route-manager.c +++ b/src/tests/test-route-manager.c @@ -191,6 +191,26 @@ test_ip4 (test_fixture *fixture, gconstpointer user_data) .mss = 0, .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_LINK), }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = nmtst_inet4_from_string ("6.6.6.0"), + .plen = 24, + .ifindex = fixture->ifindex1, + .gateway = INADDR_ANY, + .metric = 21, + .mss = 0, + .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_LINK), + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = nmtst_inet4_from_string ("8.0.0.0"), + .plen = 8, + .ifindex = fixture->ifindex1, + .gateway = nmtst_inet4_from_string ("6.6.6.2"), + .metric = 22, + .mss = 0, + .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_UNIVERSE), + }, }; NMPlatformIP4Route state2[] = { @@ -224,6 +244,26 @@ test_ip4 (test_fixture *fixture, gconstpointer user_data) .mss = 0, .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_LINK), }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = nmtst_inet4_from_string ("6.6.6.0"), + .plen = 24, + .ifindex = fixture->ifindex1, + .gateway = INADDR_ANY, + .metric = 21, + .mss = 0, + .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_LINK), + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = nmtst_inet4_from_string ("8.0.0.0"), + .plen = 8, + .ifindex = fixture->ifindex1, + .gateway = nmtst_inet4_from_string ("6.6.6.2"), + .metric = 22, + .mss = 0, + .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_UNIVERSE), + }, }; NMPlatformIP4Route state3[] = { @@ -247,22 +287,44 @@ test_ip4 (test_fixture *fixture, gconstpointer user_data) .mss = 0, .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_LINK), }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = nmtst_inet4_from_string ("8.0.0.0"), + .plen = 8, + .ifindex = fixture->ifindex1, + .gateway = nmtst_inet4_from_string ("6.6.6.2"), + .metric = 22, + .mss = 0, + .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_UNIVERSE), + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = nmtst_inet4_from_string ("6.6.6.0"), + .plen = 24, + .ifindex = fixture->ifindex1, + .gateway = INADDR_ANY, + /* this is a ghost entry because we synced ifindex0 and restore the route + * with metric 20 (above). But we don't remove the metric 21. */ + .metric = 21, + .mss = 0, + .scope_inv = nm_platform_route_scope_inv (RT_SCOPE_LINK), + }, }; setup_dev0_ip4 (fixture->ifindex0, 1000, 21021); - g_test_expect_message ("NetworkManager", G_LOG_LEVEL_WARNING, "*failure adding ip4-route '*: 8.0.0.0/8 22': *"); setup_dev1_ip4 (fixture->ifindex1); g_test_assert_expected_messages (); - /* 6.6.6.0/24 on dev0 won over 6.6.6.0/24 on dev1 - * 7.0.0.0/8 routes did not clash - * 8.0.0.0/8 could not be added. */ + /* - 6.6.6.0/24 on dev0 won over 6.6.6.0/24 on dev1 + * - 6.6.6.0/24 on dev1 has metric bumped. + * - 7.0.0.0/8 route, metric 21021 added + * - 7.0.0.0/8 route, metric 22 added + * - 8.0.0.0/8 could be added. */ routes = ip4_routes (fixture); g_assert_cmpint (routes->len, ==, G_N_ELEMENTS (state1)); nmtst_platform_ip4_routes_equal ((NMPlatformIP4Route *) routes->data, state1, routes->len, TRUE); g_array_free (routes, TRUE); - g_test_expect_message ("NetworkManager", G_LOG_LEVEL_WARNING, "*failure adding ip4-route '*: 8.0.0.0/8 22': *"); setup_dev1_ip4 (fixture->ifindex1); g_test_assert_expected_messages (); @@ -278,7 +340,7 @@ test_ip4 (test_fixture *fixture, gconstpointer user_data) update_dev0_ip4 (fixture->ifindex0); - /* 7.0.0.0/8 on dev0 was updated for gateway removal*/ + /* minor changes in the routes. Quite similar to state1. */ routes = ip4_routes (fixture); g_assert_cmpint (routes->len, ==, G_N_ELEMENTS (state2)); nmtst_platform_ip4_routes_equal ((NMPlatformIP4Route *) routes->data, state2, routes->len, TRUE); @@ -287,8 +349,9 @@ test_ip4 (test_fixture *fixture, gconstpointer user_data) nm_route_manager_route_flush (nm_route_manager_get (), fixture->ifindex0); /* 6.6.6.0/24 is now on dev1 + * 6.6.6.0/24 is also still on dev1 with bumped metric 21. * 7.0.0.0/8 gone from dev0, still present on dev1 - * 8.0.0.0/8 is present on dev1 now that 6.6.6.0/24 is on dev1 too + * 8.0.0.0/8 is present on dev1 * No dev0 routes left. */ routes = ip4_routes (fixture); g_assert_cmpint (routes->len, ==, G_N_ELEMENTS (state3)); @@ -512,6 +575,33 @@ test_ip6 (test_fixture *fixture, gconstpointer user_data) .metric = 22, .mss = 0, }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:1337::"), + .plen = 48, + .ifindex = fixture->ifindex1, + .gateway = in6addr_any, + .metric = 1025, + .mss = 0, + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:8086::"), + .plen = 48, + .ifindex = fixture->ifindex1, + .gateway = in6addr_any, + .metric = 21, + .mss = 0, + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:d34d::"), + .plen = 64, + .ifindex = fixture->ifindex1, + .gateway = *nmtst_inet6_from_string ("2001:db8:8086::2"), + .metric = 20, + .mss = 0, + }, }; NMPlatformIP6Route state2[] = { @@ -551,6 +641,33 @@ test_ip6 (test_fixture *fixture, gconstpointer user_data) .metric = 22, .mss = 0, }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:1337::"), + .plen = 48, + .ifindex = fixture->ifindex1, + .gateway = in6addr_any, + .metric = 1025, + .mss = 0, + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:8086::"), + .plen = 48, + .ifindex = fixture->ifindex1, + .gateway = in6addr_any, + .metric = 21, + .mss = 0, + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:d34d::"), + .plen = 64, + .ifindex = fixture->ifindex1, + .gateway = *nmtst_inet6_from_string ("2001:db8:8086::2"), + .metric = 20, + .mss = 0, + }, }; NMPlatformIP6Route state3[] = { @@ -581,10 +698,36 @@ test_ip6 (test_fixture *fixture, gconstpointer user_data) .metric = 1024, .mss = 0, }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:1337::"), + .plen = 48, + .ifindex = fixture->ifindex1, + .gateway = in6addr_any, + .metric = 1025, + .mss = 0, + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:8086::"), + .plen = 48, + .ifindex = fixture->ifindex1, + .gateway = in6addr_any, + .metric = 21, + .mss = 0, + }, + { + .source = NM_IP_CONFIG_SOURCE_USER, + .network = *nmtst_inet6_from_string ("2001:db8:d34d::"), + .plen = 64, + .ifindex = fixture->ifindex1, + .gateway = *nmtst_inet6_from_string ("2001:db8:8086::2"), + .metric = 20, + .mss = 0, + }, }; setup_dev0_ip6 (fixture->ifindex0); - g_test_expect_message ("NetworkManager", G_LOG_LEVEL_WARNING, "*failure adding ip6-route '*: 2001:db8:d34d::/64 20': *"); setup_dev1_ip6 (fixture->ifindex1); g_test_assert_expected_messages (); @@ -598,7 +741,6 @@ test_ip6 (test_fixture *fixture, gconstpointer user_data) g_array_free (routes, TRUE); - g_test_expect_message ("NetworkManager", G_LOG_LEVEL_WARNING, "*failure adding ip6-route '*: 2001:db8:d34d::/64 20': *"); setup_dev1_ip6 (fixture->ifindex1); g_test_assert_expected_messages (); setup_dev0_ip6 (fixture->ifindex0);