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

1 files 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))