merge: branch 'ih/rt-leftover'

l3cfg: remove routes added by NM on reapply

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/2080

src/core/nm-l3cfg.c

@@ -3919,15 +3919,18 @@ _l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd)
                  * table sync mode to FULL, to clear the DNS routes added
                  * previously. */
                 self->priv.dns_route_added_x[IS_IPv4] = FALSE;
-                nm_l3_config_data_set_route_table_sync(l3cd,
+                nm_l3_config_data_set_route_table_sync(
-                                                       addr_family,
+                    l3cd,
-                                                       NM_IP_ROUTE_TABLE_SYNC_MODE_FULL);
+                    addr_family,
+                    NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL);
             }
             continue;
         }
 
         nameservers = nm_l3_config_data_get_nameservers(l3cd, addr_family, &len);
-        nm_l3_config_data_set_route_table_sync(l3cd, addr_family, NM_IP_ROUTE_TABLE_SYNC_MODE_FULL);
+        nm_l3_config_data_set_route_table_sync(l3cd,
+                                               addr_family,
+                                               NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL);
 
         for (i = 0; i < len; i++) {
             nm_auto_nmpobj NMPObject *obj = NULL;
@@ -3977,9 +3980,10 @@ _l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd)
                       nm_platform_ip4_route_to_string(&route_new.r4, route_buf, sizeof(route_buf)));
 
                 nm_l3_config_data_add_route_4(l3cd, &route_new.r4);
-                nm_l3_config_data_set_route_table_sync(l3cd,
+                nm_l3_config_data_set_route_table_sync(
-                                                       AF_INET,
+                    l3cd,
-                                                       NM_IP_ROUTE_TABLE_SYNC_MODE_FULL);
+                    AF_INET,
+                    NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL);
                 route_added = TRUE;
             } else {
                 route_new.r6 = (NMPlatformIP6Route) {
@@ -5129,7 +5133,7 @@ _l3_commit_one(NML3Cfg              *self,
     }
 
     if (route_table_sync == NM_IP_ROUTE_TABLE_SYNC_MODE_NONE)
-        route_table_sync = NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN;
+        route_table_sync = NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN_AND_NM_ROUTES;
 
     if (any_dirty)
         _obj_states_track_prune_dirty(self, TRUE);
@@ -5158,6 +5162,8 @@ _l3_commit_one(NML3Cfg              *self,
         }
 
         if (c_list_is_empty(&self->priv.p->blocked_lst_head_x[IS_IPv4])) {
+            gs_unref_ptrarray GPtrArray *routes_old = NULL;
+
             addresses_prune =
                 nm_platform_ip_address_get_prune_list(self->priv.platform,
                                                       addr_family,
@@ -5165,10 +5171,28 @@ _l3_commit_one(NML3Cfg              *self,
                                                       nm_g_array_data(ipv6_temp_addrs_keep),
                                                       nm_g_array_len(ipv6_temp_addrs_keep));
 
+            if (route_table_sync == NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN_AND_NM_ROUTES) {
+                GHashTableIter h_iter;
+                ObjStateData  *obj_state;
+
+                /* Get list of all the routes that were configured by us */
+                routes_old = g_ptr_array_new_with_free_func((GDestroyNotify) nmp_object_unref);
+                g_hash_table_iter_init(&h_iter, self->priv.p->obj_state_hash);
+                while (g_hash_table_iter_next(&h_iter, (gpointer *) &obj_state, NULL)) {
+                    if (NMP_OBJECT_GET_TYPE(obj_state->obj) == NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4)
+                        && obj_state->os_nm_configured)
+                        g_ptr_array_add(routes_old, (gpointer) nmp_object_ref(obj_state->obj));
+                }
+
+                nm_platform_route_objs_sort(routes_old, NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY);
+            }
+
             routes_prune = nm_platform_ip_route_get_prune_list(self->priv.platform,
                                                                addr_family,
                                                                self->priv.ifindex,
-                                                               route_table_sync);
+                                                               route_table_sync,
+                                                               routes_old);
+
             _obj_state_zombie_lst_prune_all(self, addr_family);
         }
     } else {

src/core/nm-manager.c

@@ -462,21 +462,24 @@ static GVariant *
 _version_info_get(void)
 {
     const guint32 arr[] = {
+        /* The array contains as first element NM_VERSION, which can be
+         * used to numerically compare the version (see also NM_ENCODE_VERSION,
+         * nm_utils_version(), nm_encode_version() and nm_decode_version(). */
         NM_VERSION,
-    };
 
-    /* The array contains as first element NM_VERSION, which can be
+        /* The following elements of the array are a bitfield of capabilities.
-     * used to numerically compare the version (see also NM_ENCODE_VERSION,
+         * These capabilities should only depend on compile-time abilities
-     * nm_utils_version(), nm_encode_version() and nm_decode_version().
+         * (unlike NM_MANAGER_CAPABILITIES, NMCapability). The supported values
-     *
+         * are from NMVersionInfoCapability enum. This way to expose capabilities
-     * The following elements of the array are a bitfield of capabilities.
+         * is more cumbersome but more efficient compared to NM_MANAGER_CAPABILITIES.
-     * These capabilities should only depend on compile-time abilities
+         * As such, it is cheap to add capabilities for something, where you would
-     * (unlike NM_MANAGER_CAPABILITIES, NMCapability). The supported values
+         * avoid it as NM_MANAGER_CAPABILITIES due to the overhead.
-     * are from NMVersionInfoCapability enum. This way to expose capabilities
+         *
-     * is more cumbersome but more efficient compared to NM_MANAGER_CAPABILITIES.
+         * Each of the array's elements has 32 bits. This means that capabilities
-     * As such, it is cheap to add capabilities for something, where you would
+         * with index 0-31 goes to element #1, with index 32-63 to element #2,
-     * avoid it as NM_MANAGER_CAPABILITIES due to the overhead.
+         * with index 64-95 to element #3 and so on. */
-     */
+        1 << NM_VERSION_INFO_CAPABILITY_SYNC_ROUTE_WITH_TABLE,
+    };
 
     return nm_g_variant_new_au(arr, G_N_ELEMENTS(arr));
 }

src/libnm-client-aux-extern/nm-libnm-aux.c

@@ -169,14 +169,11 @@ nmc_client_has_version_info_capability(NMClient *nmc, NMVersionInfoCapability ca
     len--;
     ver++;
 
-    idx = (gsize) capability;
+    idx    = (gsize) capability;
-    if (idx >= G_MAXSIZE - 31u)
+    idx_hi = idx / 32u;
-        return FALSE;
+    idx_lo = idx % 32u;
 
-    idx_hi = ((idx + 31u) / 32u);
+    if (idx_hi >= len)
-    idx_lo = (idx % 32u);
-
-    if (idx_hi > len)
         return FALSE;
 
     return NM_FLAGS_ANY(ver[idx_hi], (1ull << idx_lo));

src/libnm-client-impl/nm-client.c

@@ -6316,7 +6316,7 @@ nm_client_get_capabilities(NMClient *client, gsize *length)
  *
  * If available, the first element in the array is NM_VERSION which
  * encodes the daemon version as "(major << 16 | minor << 8 | micro)".
- * The following elements are a bitfield of %NMVersionInfoCapabilities
+ * The following elements are a bitfield of %NMVersionInfoCapability
  * that indicate that the daemon supports a certain capability.
  *
  * Returns: (transfer none) (array length=length): the
@@ -8313,7 +8313,7 @@ nm_client_class_init(NMClientClass *client_class)
      * Expose version info and capabilities of NetworkManager. If non-empty,
      * the first element is NM_VERSION, which encodes the version of the
      * daemon as "(major << 16 | minor << 8 | micro)". The following elements
-     * is a bitfields of %NMVersionInfoCapabilities. If a bit is set, then
+     * is a bitfields of %NMVersionInfoCapability. If a bit is set, then
      * the running NetworkManager has the respective capability.
      *
      * Since: 1.42

src/libnm-core-public/nm-dbus-interface.h

@@ -94,16 +94,19 @@
 
 /**
  * NMVersionInfoCapability:
- * %_NM_VERSION_INFO_CAPABILITY_UNUSED: a dummy capability. It has no meaning,
+ * @NM_VERSION_INFO_CAPABILITY_SYNC_ROUTE_WITH_TABLE: Contains the fix to a bug that
- *   don't use it.
+ *   caused that routes in table other than main were not removed on reapply nor
+ *   on connection down.
+ *   https://issues.redhat.com/browse/RHEL-66262
+ *   https://issues.redhat.com/browse/RHEL-67324
  *
- * Currently no enum values are defined. These capabilities are exposed
+ * The numeric values represent the bit index of the capability. These capabilities
- * on D-Bus in the "VersionInfo" bit field.
+ * can be queried in the "VersionInfo" D-Bus property.
  *
  * Since: 1.42
  */
 typedef enum {
-    _NM_VERSION_INFO_CAPABILITY_UNUSED = 0x7FFFFFFFu,
+    NM_VERSION_INFO_CAPABILITY_SYNC_ROUTE_WITH_TABLE = 0,
 } NMVersionInfoCapability;
 
 /**

src/libnm-platform/nm-platform.c

@@ -61,6 +61,8 @@ G_STATIC_ASSERT(sizeof(((NMPlatformLink *) NULL)->l_address.data) == _NM_UTILS_H
 G_STATIC_ASSERT(sizeof(((NMPlatformLink *) NULL)->l_perm_address.data) == _NM_UTILS_HWADDR_LEN_MAX);
 G_STATIC_ASSERT(sizeof(((NMPlatformLink *) NULL)->l_broadcast.data) == _NM_UTILS_HWADDR_LEN_MAX);
 
+static int _route_objs_cmp_values(gconstpointer a, gconstpointer b, gpointer user_data);
+
 static const char *
 _nmp_link_port_data_to_string(NMPortKind                    port_kind,
                               const NMPlatformLinkPortData *port_data,
@@ -4904,11 +4906,24 @@ nm_platform_ip_address_get_prune_list(NMPlatform            *self,
     return result;
 }
 
+static gboolean
+_route_obj_find_bsearch(GPtrArray *sorted_routes_objs, const NMPObject *route_obj)
+{
+    gssize pos =
+        nm_ptrarray_find_bsearch((gconstpointer *) sorted_routes_objs->pdata,
+                                 sorted_routes_objs->len,
+                                 route_obj,
+                                 _route_objs_cmp_values,
+                                 GINT_TO_POINTER((int) NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY));
+    return pos >= 0;
+}
+
 GPtrArray *
 nm_platform_ip_route_get_prune_list(NMPlatform            *self,
                                     int                    addr_family,
                                     int                    ifindex,
-                                    NMIPRouteTableSyncMode route_table_sync)
+                                    NMIPRouteTableSyncMode route_table_sync,
+                                    GPtrArray             *sorted_old_routes_objs)
 {
     NMPLookup                    lookup;
     GPtrArray                   *routes_prune = NULL;
@@ -4922,10 +4937,21 @@ nm_platform_ip_route_get_prune_list(NMPlatform            *self,
     nm_assert(NM_IN_SET(addr_family, AF_INET, AF_INET6));
     nm_assert(NM_IN_SET(route_table_sync,
                         NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN,
-                        NM_IP_ROUTE_TABLE_SYNC_MODE_FULL,
+                        NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL,
+                        NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN_AND_NM_ROUTES,
                         NM_IP_ROUTE_TABLE_SYNC_MODE_ALL,
                         NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE));
 
+    if (route_table_sync == NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN_AND_NM_ROUTES) {
+        nm_assert(sorted_old_routes_objs);
+        nm_assert(nm_utils_ptrarray_is_sorted(
+            (gconstpointer *) sorted_old_routes_objs->pdata,
+            sorted_old_routes_objs->len,
+            FALSE,
+            _route_objs_cmp_values,
+            GINT_TO_POINTER((int) NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY)));
+    }
+
     nmp_lookup_init_object_by_ifindex(&lookup,
                                       NMP_OBJECT_TYPE_IP_ROUTE(NM_IS_IPv4(addr_family)),
                                       ifindex);
@@ -4947,7 +4973,12 @@ nm_platform_ip_route_get_prune_list(NMPlatform            *self,
             if (!nm_platform_route_table_is_main(nm_platform_ip_route_get_effective_table(&rt->rx)))
                 continue;
             break;
-        case NM_IP_ROUTE_TABLE_SYNC_MODE_FULL:
+        case NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN_AND_NM_ROUTES:
+            if (!nm_platform_route_table_is_main(nm_platform_ip_route_get_effective_table(&rt->rx))
+                && !_route_obj_find_bsearch(sorted_old_routes_objs, obj))
+                continue;
+            break;
+        case NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL:
             if (nm_platform_ip_route_get_effective_table(&rt->rx) == RT_TABLE_LOCAL)
                 continue;
             break;
@@ -5316,7 +5347,8 @@ nm_platform_ip_route_flush(NMPlatform *self, int addr_family, int ifindex)
         routes_prune = nm_platform_ip_route_get_prune_list(self,
                                                            AF_INET,
                                                            ifindex,
-                                                           NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE);
+                                                           NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE,
+                                                           NULL);
         success &= nm_platform_ip_route_sync(self, AF_INET, ifindex, NULL, routes_prune, NULL);
     }
     if (NM_IN_SET(addr_family, AF_UNSPEC, AF_INET6)) {
@@ -5325,7 +5357,8 @@ nm_platform_ip_route_flush(NMPlatform *self, int addr_family, int ifindex)
         routes_prune = nm_platform_ip_route_get_prune_list(self,
                                                            AF_INET6,
                                                            ifindex,
-                                                           NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE);
+                                                           NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE,
+                                                           NULL);
         success &= nm_platform_ip_route_sync(self, AF_INET6, ifindex, NULL, routes_prune, NULL);
     }
     return success;
@@ -8833,6 +8866,45 @@ nm_platform_lnk_wireguard_cmp(const NMPlatformLnkWireGuard *a, const NMPlatformL
     return 0;
 }
 
+static int
+_route_objs_cmp_values(gconstpointer a, gconstpointer b, gpointer user_data)
+{
+    const NMPObject         *a_obj    = a;
+    const NMPObject         *b_obj    = b;
+    NMPlatformIPRouteCmpType cmp_type = GPOINTER_TO_INT(user_data);
+
+    nm_assert(a_obj && b_obj);
+    nm_assert(NMP_OBJECT_CAST_IP_ROUTE(a_obj) && NMP_OBJECT_CAST_IP_ROUTE(b_obj));
+
+    if (NMP_OBJECT_GET_ADDR_FAMILY(a_obj) != NMP_OBJECT_GET_ADDR_FAMILY(b_obj)) {
+        return NMP_OBJECT_GET_ADDR_FAMILY(a_obj) == AF_INET ? 1 : -1;
+    } else if (NMP_OBJECT_GET_ADDR_FAMILY(a_obj) == AF_INET) {
+        return nm_platform_ip4_route_cmp(NMP_OBJECT_CAST_IP4_ROUTE(a_obj),
+                                         NMP_OBJECT_CAST_IP4_ROUTE(b_obj),
+                                         cmp_type);
+    } else {
+        return nm_platform_ip6_route_cmp(NMP_OBJECT_CAST_IP6_ROUTE(a_obj),
+                                         NMP_OBJECT_CAST_IP6_ROUTE(b_obj),
+                                         cmp_type);
+    }
+}
+
+static int
+_route_objs_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
+{
+    nm_assert(a && b);
+
+    return _route_objs_cmp_values(*((const NMPObject **) a), *((const NMPObject **) b), user_data);
+}
+
+void
+nm_platform_route_objs_sort(GPtrArray *routes_objs, NMPlatformIPRouteCmpType cmp_type)
+{
+    nm_assert(routes_objs);
+
+    g_ptr_array_sort_with_data(routes_objs, _route_objs_cmp, GINT_TO_POINTER((int) cmp_type));
+}
+
 void
 nm_platform_ip4_rt_nexthop_hash_update(const NMPlatformIP4RtNextHop *obj,
                                        gboolean                      for_id,

src/libnm-platform/nm-platform.h

@@ -2419,7 +2419,8 @@ int nm_platform_ip6_route_add(NMPlatform *self, NMPNlmFlags flags, const NMPlatf
 GPtrArray *nm_platform_ip_route_get_prune_list(NMPlatform            *self,
                                                int                    addr_family,
                                                int                    ifindex,
-                                               NMIPRouteTableSyncMode route_table_sync);
+                                               NMIPRouteTableSyncMode route_table_sync,
+                                               GPtrArray             *old_routes_objs);
 
 gboolean nm_platform_ip_route_sync(NMPlatform *self,
                                    int         addr_family,
@@ -2528,6 +2529,8 @@ int nm_platform_lnk_wireguard_cmp(const NMPlatformLnkWireGuard *a, const NMPlatf
 
 GHashTable *nm_platform_ip4_address_addr_to_hash(NMPlatform *self, int ifindex);
 
+void nm_platform_route_objs_sort(GPtrArray *routes_objs, NMPlatformIPRouteCmpType cmp_type);
+
 int nm_platform_ip4_route_cmp(const NMPlatformIP4Route *a,
                               const NMPlatformIP4Route *b,
                               NMPlatformIPRouteCmpType  cmp_type);

src/libnm-platform/nmp-base.h

@@ -212,8 +212,11 @@ nmp_object_type_to_flags(NMPObjectType obj_type)
  * @NM_IP_ROUTE_TABLE_SYNC_MODE_NONE: indicate an invalid setting.
  * @NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN: only the main table is synced. For all
  *   other tables, NM won't delete any extra routes.
- * @NM_IP_ROUTE_TABLE_SYNC_MODE_FULL: NM will sync all tables, except the
+ * @NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN_AND_NM_ROUTES: only the main table is synced,
- *   local table (255).
+ *   plus individual routes in other tables added by NM, leaving routes that
+ *   were not added by NM untouched.
+ * @NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL: NM will sync all tables, except
+ *   the local table (255).
  * @NM_IP_ROUTE_TABLE_SYNC_MODE_ALL: NM will sync all tables, including the
  *   local table (255).
  * @NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE: NM will sync all tables (including
@@ -223,7 +226,8 @@ nmp_object_type_to_flags(NMPObjectType obj_type)
 typedef enum {
     NM_IP_ROUTE_TABLE_SYNC_MODE_NONE,
     NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN,
-    NM_IP_ROUTE_TABLE_SYNC_MODE_FULL,
+    NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN_AND_NM_ROUTES,
+    NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL,
     NM_IP_ROUTE_TABLE_SYNC_MODE_ALL,
     NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE,
 } NMIPRouteTableSyncMode;