From dc0ac73780bdb1da9d9c33f3595cb02721e20761 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jul 2021 11:09:40 +0200 Subject: [PATCH 1/4] platform: add is-external flag to NMPlatformIPRoute We will need to track whether a route is externally added or not. We maybe could use rt_source for that, but instead add a boolean flag. --- src/libnm-platform/nm-platform.c | 18 ++++++++++++++---- src/libnm-platform/nm-platform.h | 8 ++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 089d0d42c1..7c9594e2ea 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -6524,6 +6524,7 @@ nm_platform_ip4_route_to_string(const NMPlatformIP4Route *route, char *buf, gsiz "%s" /* initcwnd */ "%s" /* initrwnd */ "%s" /* mtu */ + "%s" /* is_external */ "", nm_net_aux_rtnl_rtntype_n2a_maybe_buf(nm_platform_route_type_uncoerce(route->type_coerced), str_type), @@ -6579,7 +6580,8 @@ nm_platform_ip4_route_to_string(const NMPlatformIP4Route *route, char *buf, gsiz " mtu %s%" G_GUINT32_FORMAT, route->lock_mtu ? "lock " : "", route->mtu) - : ""); + : "", + route->is_external ? " (E)" : ""); return buf; } @@ -6649,6 +6651,7 @@ nm_platform_ip6_route_to_string(const NMPlatformIP6Route *route, char *buf, gsiz "%s" /* initrwnd */ "%s" /* mtu */ "%s" /* pref */ + "%s" /* is_external */ "", nm_net_aux_rtnl_rtntype_n2a_maybe_buf(nm_platform_route_type_uncoerce(route->type_coerced), str_type), @@ -6708,7 +6711,8 @@ nm_platform_ip6_route_to_string(const NMPlatformIP6Route *route, char *buf, gsiz str_pref, " pref %s", nm_icmpv6_router_pref_to_string(route->rt_pref, str_pref2, sizeof(str_pref2))) - : ""); + : "", + route->is_external ? " (E)" : ""); return buf; } @@ -8005,7 +8009,8 @@ nm_platform_ip4_route_hash_update(const NMPlatformIP4Route *obj, obj->lock_cwnd, obj->lock_initcwnd, obj->lock_initrwnd, - obj->lock_mtu)); + obj->lock_mtu, + obj->is_external)); break; } } @@ -8095,6 +8100,8 @@ nm_platform_ip4_route_cmp(const NMPlatformIP4Route *a, NM_CMP_FIELD(a, b, initcwnd); NM_CMP_FIELD(a, b, initrwnd); NM_CMP_FIELD(a, b, mtu); + if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_FULL) + NM_CMP_FIELD_UNSAFE(a, b, is_external); break; } return 0; @@ -8186,7 +8193,8 @@ nm_platform_ip6_route_hash_update(const NMPlatformIP6Route *obj, obj->lock_cwnd, obj->lock_initcwnd, obj->lock_initrwnd, - obj->lock_mtu), + obj->lock_mtu, + obj->is_external), obj->window, obj->cwnd, obj->initcwnd, @@ -8269,6 +8277,8 @@ nm_platform_ip6_route_cmp(const NMPlatformIP6Route *a, NM_CMP_DIRECT(_route_pref_normalize(a->rt_pref), _route_pref_normalize(b->rt_pref)); else NM_CMP_FIELD(a, b, rt_pref); + if (cmp_type == NM_PLATFORM_IP_ROUTE_CMP_TYPE_FULL) + NM_CMP_FIELD_UNSAFE(a, b, is_external); break; } return 0; diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index 6fe723cba4..5b7a73e887 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -462,6 +462,14 @@ typedef union { * the "table_coerced" field is ignored (unlike for the metric). */ \ bool table_any : 1; \ \ + /* This route is tracked as external route, that is not a route that NetworkManager + * actively wants to add, but a route that was added externally. In some cases, such + * a route should be ignored. + * + * Note that unlike most other fields here, this flag only exists inside NetworkManager + * and is not reflected on netlink. */ \ + bool is_external : 1; \ + \ /* rtnh_flags * * Routes with rtm_flags RTM_F_CLONED are hidden by platform and From a6649ef87bbf12a556bdcecc32bbb7a5407ee70e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jul 2021 14:02:01 +0200 Subject: [PATCH 2/4] core: preserve "is_external" route flag during _nm_ip_config_add_obj() --- src/core/nm-ip4-config.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/core/nm-ip4-config.c b/src/core/nm-ip4-config.c index 47f0ee3a80..e7e90c4330 100644 --- a/src/core/nm-ip4-config.c +++ b/src/core/nm-ip4-config.c @@ -163,6 +163,11 @@ _nm_ip_config_add_obj(NMDedupMultiIndex * multi_idx, obj_new_stackinit.ip_route.rt_source = obj_old->ip_route.rt_source; modified = TRUE; } + if (!obj_new->ip_route.is_external && obj_old->ip_route.is_external) { + obj_new = nmp_object_stackinit_obj(&obj_new_stackinit, obj_new); + obj_new_stackinit.ip_route.is_external = FALSE; + modified = TRUE; + } break; default: nm_assert_not_reached(); From 1f1c7b82fd41caa7f92bfb86cbfd636bd8a09b22 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jul 2021 11:45:41 +0200 Subject: [PATCH 3/4] platform: mark routes in NMPlatform cache as "external" --- src/core/platform/nm-fake-platform.c | 3 + src/core/platform/tests/test-route.c | 104 +++++++++++++------------ src/libnm-platform/nm-linux-platform.c | 1 + 3 files changed, 60 insertions(+), 48 deletions(-) diff --git a/src/core/platform/nm-fake-platform.c b/src/core/platform/nm-fake-platform.c index 0108b581bf..b773df9a43 100644 --- a/src/core/platform/nm-fake-platform.c +++ b/src/core/platform/nm-fake-platform.c @@ -1117,6 +1117,9 @@ ip_route_add(NMPlatform * platform, : NMP_OBJECT_TYPE_IP6_ROUTE, (const NMPlatformObject *) route); r = NMP_OBJECT_CAST_IP_ROUTE(obj); + + r->is_external = TRUE; + nm_platform_ip_route_normalize(addr_family, r); switch (addr_family) { diff --git a/src/core/platform/tests/test-route.c b/src/core/platform/tests/test-route.c index 2b9b2f8f67..4b1db7fad7 100644 --- a/src/core/platform/tests/test-route.c +++ b/src/core/platform/tests/test-route.c @@ -332,30 +332,33 @@ test_ip4_route(void) /* Test route listing */ routes = nmtstp_ip4_route_get_all(NM_PLATFORM_GET, ifindex); memset(rts, 0, sizeof(rts)); - rts[0].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); - rts[0].network = gateway; - rts[0].plen = 32; - rts[0].ifindex = ifindex; - rts[0].gateway = INADDR_ANY; - rts[0].metric = metric; - rts[0].mss = mss; - rts[0].scope_inv = nm_platform_route_scope_inv(RT_SCOPE_LINK); - rts[1].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); - rts[1].network = network; - rts[1].plen = plen; - rts[1].ifindex = ifindex; - rts[1].gateway = gateway; - rts[1].metric = metric; - rts[1].mss = mss; - rts[1].scope_inv = nm_platform_route_scope_inv(RT_SCOPE_UNIVERSE); - rts[2].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); - rts[2].network = 0; - rts[2].plen = 0; - rts[2].ifindex = ifindex; - rts[2].gateway = gateway; - rts[2].metric = metric; - rts[2].mss = mss; - rts[2].scope_inv = nm_platform_route_scope_inv(RT_SCOPE_UNIVERSE); + rts[0].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); + rts[0].network = gateway; + rts[0].plen = 32; + rts[0].ifindex = ifindex; + rts[0].gateway = INADDR_ANY; + rts[0].metric = metric; + rts[0].mss = mss; + rts[0].is_external = TRUE; + rts[0].scope_inv = nm_platform_route_scope_inv(RT_SCOPE_LINK); + rts[1].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); + rts[1].network = network; + rts[1].plen = plen; + rts[1].ifindex = ifindex; + rts[1].gateway = gateway; + rts[1].metric = metric; + rts[1].mss = mss; + rts[1].scope_inv = nm_platform_route_scope_inv(RT_SCOPE_UNIVERSE); + rts[1].is_external = TRUE; + rts[2].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); + rts[2].network = 0; + rts[2].plen = 0; + rts[2].ifindex = ifindex; + rts[2].gateway = gateway; + rts[2].metric = metric; + rts[2].mss = mss; + rts[2].scope_inv = nm_platform_route_scope_inv(RT_SCOPE_UNIVERSE); + rts[2].is_external = TRUE; g_assert_cmpint(routes->len, ==, 3); nmtst_platform_ip4_routes_equal_aptr((const NMPObject *const *) routes->pdata, rts, @@ -489,30 +492,33 @@ test_ip6_route(void) /* Test route listing */ routes = nmtstp_ip6_route_get_all(NM_PLATFORM_GET, ifindex); memset(rts, 0, sizeof(rts)); - rts[0].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); - rts[0].network = gateway; - rts[0].plen = 128; - rts[0].ifindex = ifindex; - rts[0].gateway = in6addr_any; - rts[0].pref_src = in6addr_any; - rts[0].metric = metric; - rts[0].mss = mss; - rts[1].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); - rts[1].network = network; - rts[1].plen = plen; - rts[1].ifindex = ifindex; - rts[1].gateway = gateway; - rts[1].pref_src = pref_src; - rts[1].metric = metric; - rts[1].mss = mss; - rts[2].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); - rts[2].network = in6addr_any; - rts[2].plen = 0; - rts[2].ifindex = ifindex; - rts[2].gateway = gateway; - rts[2].pref_src = in6addr_any; - rts[2].metric = metric; - rts[2].mss = mss; + rts[0].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); + rts[0].network = gateway; + rts[0].plen = 128; + rts[0].ifindex = ifindex; + rts[0].gateway = in6addr_any; + rts[0].pref_src = in6addr_any; + rts[0].metric = metric; + rts[0].mss = mss; + rts[0].is_external = TRUE; + rts[1].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); + rts[1].network = network; + rts[1].plen = plen; + rts[1].ifindex = ifindex; + rts[1].gateway = gateway; + rts[1].pref_src = pref_src; + rts[1].metric = metric; + rts[1].mss = mss; + rts[1].is_external = TRUE; + rts[2].rt_source = nmp_utils_ip_config_source_round_trip_rtprot(NM_IP_CONFIG_SOURCE_USER); + rts[2].network = in6addr_any; + rts[2].plen = 0; + rts[2].ifindex = ifindex; + rts[2].gateway = gateway; + rts[2].pref_src = in6addr_any; + rts[2].metric = metric; + rts[2].mss = mss; + rts[2].is_external = TRUE; g_assert_cmpint(routes->len, ==, 3); nmtst_platform_ip6_routes_equal_aptr((const NMPObject *const *) routes->pdata, rts, @@ -709,6 +715,7 @@ test_ip4_route_options(gconstpointer test_data) for (i = 0; i < rts_n; i++) { rts_cmp[i] = rts_add[i]; nm_platform_ip_route_normalize(AF_INET, NM_PLATFORM_IP_ROUTE_CAST(&rts_cmp[i])); + rts_cmp[i].is_external = TRUE; } routes = nmtstp_ip4_route_get_all(NM_PLATFORM_GET, IFINDEX); @@ -880,6 +887,7 @@ test_ip6_route_options(gconstpointer test_data) for (i = 0; i < rts_n; i++) { rts_cmp[i] = rts_add[i]; nm_platform_ip_route_normalize(AF_INET6, NM_PLATFORM_IP_ROUTE_CAST(&rts_cmp[i])); + rts_cmp[i].is_external = TRUE; } routes = nmtstp_ip6_route_get_all(NM_PLATFORM_GET, IFINDEX); diff --git a/src/libnm-platform/nm-linux-platform.c b/src/libnm-platform/nm-linux-platform.c index 0801fae29d..a2a82278ce 100644 --- a/src/libnm-platform/nm-linux-platform.c +++ b/src/libnm-platform/nm-linux-platform.c @@ -3519,6 +3519,7 @@ rta_multipath_done:; obj = nmp_object_new(is_v4 ? NMP_OBJECT_TYPE_IP4_ROUTE : NMP_OBJECT_TYPE_IP6_ROUTE, NULL); + obj->ip_route.is_external = TRUE; obj->ip_route.type_coerced = nm_platform_route_type_coerce(rtm->rtm_type); obj->ip_route.table_coerced = nm_platform_route_table_coerce( tb[RTA_TABLE] ? nla_get_u32(tb[RTA_TABLE]) : (guint32) rtm->rtm_table); From 13d749942ff42638d87b0e624bc901c8a83e3365 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jul 2021 11:53:31 +0200 Subject: [PATCH 4/4] platform: don't add routes that are tracked as external routes Due to something that really should be fixed, NetworkManager merges the routes that it wants to configure, with the routes that are configured externally. This includes a subtract and merge dance, which is wrong. Anyway. If we are in nm_platform_ip_route_sync(), then we never want to actively configure a route, that we only have in the list because it is (or was) present on the interface. Otherwise we have a problem. Note that we make a plan which routes/addresses to add/remove before starting. So, if we start with an IPv4 address configured in kernel, then there is also a corresponding local route. We would track that local route as external. During sync, we first remove the IP address, and kernel automatically also removes the local route. However, as we already made the plan to keep that route, NetworkManager would wrongly configure it again. This should fix that bug. It is anyway wrong to even try to explicitly configure a route, that is purely in the list as being external. https://bugzilla.redhat.com/show_bug.cgi?id=1979192#c11 --- src/libnm-platform/nm-platform.c | 44 ++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 7c9594e2ea..6c0d0015d6 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -4489,6 +4489,20 @@ nm_platform_ip_route_sync(NMPlatform *self, conf_o = routes->pdata[i]; + if (NMP_OBJECT_CAST_IP_ROUTE(conf_o)->is_external) { + /* This route is added externally. We don't have our own agenda to + * add it, so skip. */ + continue; + } + + /* User space cannot add IPv6 routes with metric 0. However, kernel can, and we might track such + * routes in @route as they are present external. As we already skipped external routes above, + * we don't expect a user's choice to add such a route (it won't work anyway). */ + nm_assert( + IS_IPv4 + || nm_platform_ip6_route_get_effective_metric(NMP_OBJECT_CAST_IP6_ROUTE(conf_o)) + != 0); + #define VTABLE_IS_DEVICE_ROUTE(vt, o) \ (vt->is_ip4 ? (NMP_OBJECT_CAST_IP4_ROUTE(o)->gateway == 0) \ : IN6_IS_ADDR_UNSPECIFIED(&NMP_OBJECT_CAST_IP6_ROUTE(o)->gateway)) @@ -4505,7 +4519,7 @@ nm_platform_ip_route_sync(NMPlatform *self, routes_idx = g_hash_table_new((GHashFunc) nmp_object_id_hash, (GEqualFunc) nmp_object_id_equal); } - if (!g_hash_table_insert(routes_idx, (gpointer) conf_o, (gpointer) conf_o)) { + if (!g_hash_table_add(routes_idx, (gpointer) conf_o)) { _LOG3D("route-sync: skip adding duplicate route %s", nmp_object_to_string(conf_o, NMP_OBJECT_TO_STRING_PUBLIC, @@ -4514,14 +4528,6 @@ nm_platform_ip_route_sync(NMPlatform *self, continue; } - if (!IS_IPv4 - && nm_platform_ip6_route_get_effective_metric(NMP_OBJECT_CAST_IP6_ROUTE(conf_o)) - == 0) { - /* User space cannot add routes with metric 0. However, kernel can, and we might track such - * routes in @route as they are present external. Skip them silently. */ - continue; - } - plat_entry = nm_platform_lookup_entry(self, NMP_CACHE_ID_TYPE_OBJECT_TYPE, conf_o); if (plat_entry) { const NMPObject *plat_o; @@ -4684,6 +4690,24 @@ sync_route_add: } if (routes_prune) { + if (routes) { + for (i = 0; i < routes->len; i++) { + conf_o = routes->pdata[i]; + + if (NMP_OBJECT_CAST_IP_ROUTE(conf_o)->is_external) { + /* this is only to catch the case where an external route is + * both in @routes and @routes_prune list. In that case, + * @routes should win and we should not remove the address. */ + if (!routes_idx) { + routes_idx = g_hash_table_new((GHashFunc) nmp_object_id_hash, + (GEqualFunc) nmp_object_id_equal); + } + g_hash_table_add(routes_idx, (gpointer) conf_o); + continue; + } + } + } + for (i = 0; i < routes_prune->len; i++) { const NMPObject *prune_o; @@ -4694,7 +4718,7 @@ sync_route_add: || (!NM_IS_IPv4(addr_family) && NMP_OBJECT_GET_TYPE(prune_o) == NMP_OBJECT_TYPE_IP6_ROUTE)); - if (routes_idx && g_hash_table_lookup(routes_idx, prune_o)) + if (nm_g_hash_table_lookup(routes_idx, prune_o)) continue; if (!nm_platform_lookup_entry(self, NMP_CACHE_ID_TYPE_OBJECT_TYPE, prune_o))