From 6ee7be690e8e0f4ec86e6e2845295624c183fb5b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 20 Jul 2021 11:53:31 +0200 Subject: [PATCH] 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 (cherry picked from commit 13d749942ff42638d87b0e624bc901c8a83e3365) --- 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))