From 11d8c41898e43236476dd5f5f26736e2a3fdbea5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Apr 2017 18:47:18 +0200 Subject: [PATCH 1/5] platform: cleanup possibly non-zero host part for route operations Platform's add/remove operations accept a "network" argument. Kernel requires that the host part (based on plen) is all zero. For NetworkManager we are more resilient to user configuration. Cleanup the input argument already before calling _nl_msg_new_route(). Note that we use the same "network" argument to construct a obj_id instance and to find the route in the cache (do_add_addrroute()). Without cleaning the host part, the added object cannot be found and the add-route command seemingly fails. --- src/platform/nm-linux-platform.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 7369aaf045..c3af263040 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -2416,7 +2416,6 @@ _nl_msg_new_route (int nlmsg_type, .rtm_dst_len = plen, .rtm_src_len = src ? src_plen : 0, }; - NMIPAddr network_clean; gsize addr_len; @@ -2433,8 +2432,7 @@ _nl_msg_new_route (int nlmsg_type, addr_len = family == AF_INET ? sizeof (in_addr_t) : sizeof (struct in6_addr); - nm_utils_ipx_address_clear_host_address (family, &network_clean, network, plen); - NLA_PUT (msg, RTA_DST, addr_len, &network_clean); + NLA_PUT (msg, RTA_DST, addr_len, network); if (src) NLA_PUT (msg, RTA_SRC, addr_len, src); @@ -6030,6 +6028,8 @@ ip4_route_delete (NMPlatform *platform, int ifindex, in_addr_t network, guint8 p nm_auto_nlmsg struct nl_msg *nlmsg = NULL; NMPObject obj_id; + network = nm_utils_ip4_address_clear_host_address (network, plen); + nmp_object_stackinit_id_ip4_route (&obj_id, ifindex, network, plen, metric); if (metric == 0) { @@ -6096,6 +6096,8 @@ ip6_route_delete (NMPlatform *platform, int ifindex, struct in6_addr network, gu metric = nm_utils_ip6_route_metric_normalize (metric); + nm_utils_ip6_address_clear_host_address (&network, &network, plen); + nlmsg = _nl_msg_new_route (RTM_DELROUTE, 0, AF_INET6, From 57b0dce083a1991530e14735588d873e441f26fa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Apr 2017 18:58:22 +0200 Subject: [PATCH 2/5] platform: only consider net part of routes for route cache's ID Routes with a non-zero host part are not allowed by kernel and don't really exist. We didn't reject such routes in users configuration, so various part of NM allow such routes. NM should silently strip the host part. Extend the cache's route ID to clear the host part too. Note that NM's handling of routes is fundamentally flawed, as for kernels routes don't have an "id" (or rather: all properties of a route are part of it's ID, not only the family,ifindex, network/plen and metric tuple (see related bug rh#1337855). --- src/platform/nmp-object.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/platform/nmp-object.c b/src/platform/nmp-object.c index 719b2c4d80..b27d9e33b6 100644 --- a/src/platform/nmp-object.c +++ b/src/platform/nmp-object.c @@ -805,12 +805,17 @@ _vt_cmd_plobj_id_equal (ip4_route, NMPlatformIP4Route, obj1->ifindex == obj2->ifindex && obj1->plen == obj2->plen && obj1->metric == obj2->metric - && obj1->network == obj2->network); + && nm_utils_ip4_address_clear_host_address (obj1->network, obj1->plen) == nm_utils_ip4_address_clear_host_address (obj2->network, obj2->plen)); _vt_cmd_plobj_id_equal (ip6_route, NMPlatformIP6Route, obj1->ifindex == obj2->ifindex && obj1->plen == obj2->plen && obj1->metric == obj2->metric - && IN6_ARE_ADDR_EQUAL( &obj1->network, &obj2->network)); + && ({ + struct in6_addr n1, n2; + + IN6_ARE_ADDR_EQUAL(nm_utils_ip6_address_clear_host_address (&n1, &obj1->network, obj1->plen), + nm_utils_ip6_address_clear_host_address (&n2, &obj2->network, obj2->plen)); + })); guint nmp_object_id_hash (const NMPObject *obj) @@ -864,14 +869,17 @@ _vt_cmd_plobj_id_hash (ip4_route, NMPlatformIP4Route, { hash = hash + ((guint) obj->ifindex); hash = hash * 33 + ((guint) obj->plen); hash = hash * 33 + ((guint) obj->metric); - hash = hash * 33 + ((guint) obj->network); + hash = hash * 33 + ((guint) nm_utils_ip4_address_clear_host_address (obj->network, obj->plen)); }) _vt_cmd_plobj_id_hash (ip6_route, NMPlatformIP6Route, { hash = (guint) 3999787007u; hash = hash + ((guint) obj->ifindex); hash = hash * 33 + ((guint) obj->plen); hash = hash * 33 + ((guint) obj->metric); - hash = hash * 33 + _id_hash_ip6_addr (&obj->network); + hash = hash * 33 + ({ + struct in6_addr n1; + _id_hash_ip6_addr (nm_utils_ip6_address_clear_host_address (&n1, &obj->network, obj->plen)); + }); }) gboolean From 034b7fb51c1d16a4002d2902c60aac05e946bb4f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Apr 2017 19:15:02 +0200 Subject: [PATCH 3/5] src: only compare network parts of routes in nm_utils_match_connection() Kernel requires that routes have a host part of zero. For NetworkManager configuration we allow non-zero host parts (but ignore them). Fix route_compare() to ignore the host part. This has only effect during assuming connections. That means, on restart NM would fail to match a connection with static routes if it has a non-zero host part. So, the impact is rather small. --- src/NetworkManagerUtils.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/NetworkManagerUtils.c b/src/NetworkManagerUtils.c index fbca3b82f3..779dc8aaeb 100644 --- a/src/NetworkManagerUtils.c +++ b/src/NetworkManagerUtils.c @@ -342,18 +342,19 @@ static int route_compare (NMIPRoute *route1, NMIPRoute *route2, gint64 default_metric) { gint64 r, metric1, metric2; + int family; + guint plen; + NMIPAddr a1 = { 0 }, a2 = { 0 }; - r = g_strcmp0 (nm_ip_route_get_dest (route1), nm_ip_route_get_dest (route2)); - if (r) - return r; - - r = nm_ip_route_get_prefix (route1) - nm_ip_route_get_prefix (route2); + family = nm_ip_route_get_family (route1); + r = family - nm_ip_route_get_family (route2); if (r) return r > 0 ? 1 : -1; - r = g_strcmp0 (nm_ip_route_get_next_hop (route1), nm_ip_route_get_next_hop (route2)); + plen = nm_ip_route_get_prefix (route1); + r = plen - nm_ip_route_get_prefix (route2); if (r) - return r; + return r > 0 ? 1 : -1; metric1 = nm_ip_route_get_metric (route1) == -1 ? default_metric : nm_ip_route_get_metric (route1); metric2 = nm_ip_route_get_metric (route2) == -1 ? default_metric : nm_ip_route_get_metric (route2); @@ -362,9 +363,18 @@ route_compare (NMIPRoute *route1, NMIPRoute *route2, gint64 default_metric) if (r) return r > 0 ? 1 : -1; - r = nm_ip_route_get_family (route1) - nm_ip_route_get_family (route2); + r = g_strcmp0 (nm_ip_route_get_next_hop (route1), nm_ip_route_get_next_hop (route2)); if (r) - return r > 0 ? 1 : -1; + return r; + + /* NMIPRoute validates family and dest. inet_pton() is not expected to fail. */ + inet_pton (family, nm_ip_route_get_dest (route1), &a1); + inet_pton (family, nm_ip_route_get_dest (route2), &a2); + nm_utils_ipx_address_clear_host_address (family, &a1, &a1, plen); + nm_utils_ipx_address_clear_host_address (family, &a2, &a2, plen); + r = memcmp (&a1, &a2, sizeof (a1)); + if (r) + return r; return 0; } From 5c54b7a31e401d48568cc50827bca3c4043da20c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Apr 2017 22:10:20 +0200 Subject: [PATCH 4/5] route-manager: normalize host part of tracked routes in _vx_route_sync() The input list of routes is allowed to contain non-normalized routes, that is, routes which host part is non-zero. Such routes are rejected by kernel, but NM should transparently allow them (by normalizing the host part). The ID comparison function route_id_cmp() already properly ignored the (possibly non-zero) host part. However, in the internal list we also should make sure not to track such routes. We achive that by normalizing the host part to zero. Note that below we check whether the tracked route is idential to the route configured at platform. If we don't normalize the host part, the comparison will always indicate that the route is not yet configured, and thus we will re-sync the route every time. --- src/nm-route-manager.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/nm-route-manager.c b/src/nm-route-manager.c index 4a7ef71449..d97d868af4 100644 --- a/src/nm-route-manager.c +++ b/src/nm-route-manager.c @@ -536,6 +536,8 @@ _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); + nm_utils_ipx_address_clear_host_address (vtable->vt->addr_family, cur_ipx_route->rx.network_ptr, + cur_ipx_route->rx.network_ptr, cur_ipx_route->rx.plen); 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, NULL, 0)); @@ -631,6 +633,8 @@ _vx_route_sync (const VTableIP *vtable, NMRouteManager *self, int ifindex, const ipx_route = VTABLE_ROUTE_INDEX (vtable, ipx_routes->entries, ipx_routes->entries->len - 1); ipx_route->rx.ifindex = ifindex; ipx_route->rx.metric = vtable->vt->metric_normalize (ipx_route->rx.metric); + nm_utils_ipx_address_clear_host_address (vtable->vt->addr_family, ipx_route->rx.network_ptr, + ipx_route->rx.network_ptr, ipx_route->rx.plen); g_array_index (ipx_routes->effective_metrics_reverse, gint64, j++) = -1; From b78562570ac49ddd31e20336206399613d5604ef Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Apr 2017 22:34:32 +0200 Subject: [PATCH 5/5] core: ignore host part when comparing routes for route-manager --- src/nm-route-manager.c | 2 +- src/platform/nm-platform.c | 25 +++++++++++++++++++------ src/platform/nm-platform.h | 18 +++++++++++++++--- src/tests/test-route-manager.c | 2 +- 4 files changed, 36 insertions(+), 11 deletions(-) diff --git a/src/nm-route-manager.c b/src/nm-route-manager.c index d97d868af4..76c672ee7c 100644 --- a/src/nm-route-manager.c +++ b/src/nm-route-manager.c @@ -377,7 +377,7 @@ _route_equals_ignoring_ifindex (const VTableIP *vtable, const NMPlatformIPXRoute r2_backup.rx.metric = (guint32) r2_metric; r2 = &r2_backup; } - return vtable->vt->route_cmp (r1, r2) == 0; + return vtable->vt->route_cmp (r1, r2, FALSE) == 0; } static NMPlatformIPXRoute * diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 6eb4a1f807..bb81749d73 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -4291,11 +4291,16 @@ nm_platform_ip6_address_cmp (const NMPlatformIP6Address *a, const NMPlatformIP6A } int -nm_platform_ip4_route_cmp (const NMPlatformIP4Route *a, const NMPlatformIP4Route *b) +nm_platform_ip4_route_cmp_full (const NMPlatformIP4Route *a, const NMPlatformIP4Route *b, gboolean consider_host_part) { _CMP_SELF (a, b); _CMP_FIELD (a, b, ifindex); - _CMP_FIELD (a, b, network); + if (consider_host_part) + _CMP_FIELD (a, b, network); + else { + _CMP_DIRECT (nm_utils_ip4_address_clear_host_address (a->network, a->plen), + nm_utils_ip4_address_clear_host_address (b->network, b->plen)); + } _CMP_FIELD (a, b, plen); _CMP_FIELD (a, b, metric); _CMP_FIELD (a, b, gateway); @@ -4319,11 +4324,19 @@ nm_platform_ip4_route_cmp (const NMPlatformIP4Route *a, const NMPlatformIP4Route } int -nm_platform_ip6_route_cmp (const NMPlatformIP6Route *a, const NMPlatformIP6Route *b) +nm_platform_ip6_route_cmp_full (const NMPlatformIP6Route *a, const NMPlatformIP6Route *b, gboolean consider_host_part) { _CMP_SELF (a, b); _CMP_FIELD (a, b, ifindex); - _CMP_FIELD_MEMCMP (a, b, network); + if (consider_host_part) + _CMP_FIELD_MEMCMP (a, b, network); + else { + struct in6_addr n1, n2; + + nm_utils_ip6_address_clear_host_address (&n1, &a->network, a->plen); + nm_utils_ip6_address_clear_host_address (&n2, &b->network, b->plen); + _CMP_DIRECT_MEMCMP (&n1, &n2, sizeof (struct in6_addr)); + } _CMP_FIELD (a, b, plen); _CMP_FIELD (a, b, metric); _CMP_FIELD_MEMCMP (a, b, gateway); @@ -4541,7 +4554,7 @@ const NMPlatformVTableRoute nm_platform_vtable_route_v4 = { .is_ip4 = TRUE, .addr_family = AF_INET, .sizeof_route = sizeof (NMPlatformIP4Route), - .route_cmp = (int (*) (const NMPlatformIPXRoute *a, const NMPlatformIPXRoute *b)) nm_platform_ip4_route_cmp, + .route_cmp = (int (*) (const NMPlatformIPXRoute *a, const NMPlatformIPXRoute *b, gboolean consider_host_part)) nm_platform_ip4_route_cmp_full, .route_to_string = (const char *(*) (const NMPlatformIPXRoute *route, char *buf, gsize len)) nm_platform_ip4_route_to_string, .route_get_all = nm_platform_ip4_route_get_all, .route_add = _vtr_v4_route_add, @@ -4554,7 +4567,7 @@ const NMPlatformVTableRoute nm_platform_vtable_route_v6 = { .is_ip4 = FALSE, .addr_family = AF_INET6, .sizeof_route = sizeof (NMPlatformIP6Route), - .route_cmp = (int (*) (const NMPlatformIPXRoute *a, const NMPlatformIPXRoute *b)) nm_platform_ip6_route_cmp, + .route_cmp = (int (*) (const NMPlatformIPXRoute *a, const NMPlatformIPXRoute *b, gboolean consider_host_part)) nm_platform_ip6_route_cmp_full, .route_to_string = (const char *(*) (const NMPlatformIPXRoute *route, char *buf, gsize len)) nm_platform_ip6_route_to_string, .route_get_all = nm_platform_ip6_route_get_all, .route_add = _vtr_v6_route_add, diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 5eb46b42f2..468e889188 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -392,7 +392,7 @@ typedef struct { gboolean is_ip4; int addr_family; gsize sizeof_route; - int (*route_cmp) (const NMPlatformIPXRoute *a, const NMPlatformIPXRoute *b); + int (*route_cmp) (const NMPlatformIPXRoute *a, const NMPlatformIPXRoute *b, gboolean consider_host_part); const char *(*route_to_string) (const NMPlatformIPXRoute *route, char *buf, gsize len); GArray *(*route_get_all) (NMPlatform *self, int ifindex, NMPlatformGetRouteFlags flags); gboolean (*route_add) (NMPlatform *self, int ifindex, const NMPlatformIPXRoute *route, gint64 metric); @@ -1019,8 +1019,20 @@ int nm_platform_lnk_vlan_cmp (const NMPlatformLnkVlan *a, const NMPlatformLnkVla int nm_platform_lnk_vxlan_cmp (const NMPlatformLnkVxlan *a, const NMPlatformLnkVxlan *b); int nm_platform_ip4_address_cmp (const NMPlatformIP4Address *a, const NMPlatformIP4Address *b); int nm_platform_ip6_address_cmp (const NMPlatformIP6Address *a, const NMPlatformIP6Address *b); -int nm_platform_ip4_route_cmp (const NMPlatformIP4Route *a, const NMPlatformIP4Route *b); -int nm_platform_ip6_route_cmp (const NMPlatformIP6Route *a, const NMPlatformIP6Route *b); +int nm_platform_ip4_route_cmp_full (const NMPlatformIP4Route *a, const NMPlatformIP4Route *b, gboolean consider_host_part); +int nm_platform_ip6_route_cmp_full (const NMPlatformIP6Route *a, const NMPlatformIP6Route *b, gboolean consider_host_part); + +static inline int +nm_platform_ip4_route_cmp (const NMPlatformIP4Route *a, const NMPlatformIP4Route *b) +{ + return nm_platform_ip4_route_cmp_full (a, b, TRUE); +} + +static inline int +nm_platform_ip6_route_cmp (const NMPlatformIP6Route *a, const NMPlatformIP6Route *b) +{ + return nm_platform_ip6_route_cmp_full (a, b, TRUE); +} gboolean nm_platform_check_support_kernel_extended_ifa_flags (NMPlatform *self); gboolean nm_platform_check_support_user_ipv6ll (NMPlatform *self); diff --git a/src/tests/test-route-manager.c b/src/tests/test-route-manager.c index ba7834e9ea..d534075753 100644 --- a/src/tests/test-route-manager.c +++ b/src/tests/test-route-manager.c @@ -806,7 +806,7 @@ _assert_route_check (const NMPlatformVTableRoute *vtable, gboolean has, const NM c.r6 = route->r6; c.rx.rt_source = nmp_utils_ip_config_source_round_trip_rtprot (c.rx.rt_source); } - if (!r || vtable->route_cmp (r, &c) != 0) { + if (!r || vtable->route_cmp (r, &c, TRUE) != 0) { g_error ("Invalid route. Expect %s, has %s", vtable->route_to_string (&c, NULL, 0), vtable->route_to_string (r, buf, sizeof (buf)));