From 95064b80257e5d0b969e7c1899fe2b17ecb46e09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Wed, 27 Nov 2024 13:53:02 +0100 Subject: [PATCH] l3cfg: remove routes added by NM on reapply By default, on reapply we were only syncing the main routes table. This causes that routes added by NM to other tables are not removed on reapply. This was done to preserve routes added externally, but routes added by NM itself should be removed. Add a new route table syncing mode "main + NM routes". This mode maintains the normal behaviour of syncing completely the main table, and for other tables removes only routes that were added by us, leaving the rest untouched. Use this mode by default, as this is what a user would expect on reapply. Note: this might not work if NM is restarted between the profile being modified and the reapply, because NM forgets what routes were added by itself because of the restart. This is a rare corner case, though. Use the D-Bus property "VersionInfo" to expose a capability flag indicating that this bug is fixed. It is the first capability that we expose in this way. However, it is convenient to do it this way as it's something that clients like nmstate needs to know, so they can decide whether a conn down is needed or not. It is not enough to decide that by version number because it might be fixed via a downstream patch in distros like RHEL. https://issues.redhat.com/browse/RHEL-67324 https://issues.redhat.com/browse/RHEL-66262 Fixes: e9c17fcc9b33 ('l3cfg: default to 'main' route table sync mode') (cherry picked from commit e330eb9c4a721d158641701cb48cd8094246d258) (cherry picked from commit 7f6e84b26eaaabcd046b5c547824fffcba706051) --- src/core/nm-l3cfg.c | 22 ++++++- src/core/nm-manager.c | 29 +++++---- src/libnm-core-public/nm-dbus-interface.h | 13 ++-- src/libnm-platform/nm-platform.c | 78 ++++++++++++++++++++++- src/libnm-platform/nm-platform.h | 5 +- src/libnm-platform/nmp-base.h | 4 ++ 6 files changed, 127 insertions(+), 24 deletions(-) diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c index 86c5de1197..7133b15a44 100644 --- a/src/core/nm-l3cfg.c +++ b/src/core/nm-l3cfg.c @@ -4999,7 +4999,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); @@ -5028,6 +5028,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, @@ -5035,10 +5037,26 @@ _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) { + NMDedupMultiIter iter; + const NMPObject *rt_obj; + + routes_old = g_ptr_array_new(); + nm_l3_config_data_iter_obj_for_each (&iter, + l3cd_old, + &rt_obj, + NMP_OBJECT_TYPE_IP_ROUTE(IS_IPv4)) + g_ptr_array_add(routes_old, (gpointer) rt_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 { diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index 0a7e7b2e4a..a673279712 100644 --- a/src/core/nm-manager.c +++ b/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 - * used to numerically compare the version (see also NM_ENCODE_VERSION, - * nm_utils_version(), nm_encode_version() and nm_decode_version(). - * - * The following elements of the array are a bitfield of capabilities. - * These capabilities should only depend on compile-time abilities - * (unlike NM_MANAGER_CAPABILITIES, NMCapability). The supported values - * are from NMVersionInfoCapability enum. This way to expose capabilities - * is more cumbersome but more efficient compared to NM_MANAGER_CAPABILITIES. - * As such, it is cheap to add capabilities for something, where you would - * avoid it as NM_MANAGER_CAPABILITIES due to the overhead. - */ + /* The following elements of the array are a bitfield of capabilities. + * These capabilities should only depend on compile-time abilities + * (unlike NM_MANAGER_CAPABILITIES, NMCapability). The supported values + * are from NMVersionInfoCapability enum. This way to expose capabilities + * is more cumbersome but more efficient compared to NM_MANAGER_CAPABILITIES. + * As such, it is cheap to add capabilities for something, where you would + * avoid it as NM_MANAGER_CAPABILITIES due to the overhead. + * + * Each of the array's elements has 32 bits. This means that capabilities + * with index 0-31 goes to element #1, with index 32-63 to element #2, + * 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)); } diff --git a/src/libnm-core-public/nm-dbus-interface.h b/src/libnm-core-public/nm-dbus-interface.h index 5eedd7da3a..9c737dbea5 100644 --- a/src/libnm-core-public/nm-dbus-interface.h +++ b/src/libnm-core-public/nm-dbus-interface.h @@ -93,16 +93,19 @@ /** * NMVersionInfoCapability: - * %_NM_VERSION_INFO_CAPABILITY_UNUSED: a dummy capability. It has no meaning, - * don't use it. + * @NM_VERSION_INFO_CAPABILITY_SYNC_ROUTE_WITH_TABLE: Contains the fix to a bug that + * 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 - * on D-Bus in the "VersionInfo" bit field. + * The numeric values represent the bit index of the capability. These capabilities + * 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; /** diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index ac2ecb421c..6523fb8a98 100644 --- a/src/libnm-platform/nm-platform.c +++ b/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, @@ -4872,11 +4874,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; @@ -4891,9 +4906,20 @@ nm_platform_ip_route_get_prune_list(NMPlatform *self, nm_assert(NM_IN_SET(route_table_sync, NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN, 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); @@ -4915,6 +4941,11 @@ 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_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; @@ -5284,7 +5315,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)) { @@ -5293,7 +5325,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; @@ -8767,6 +8800,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, diff --git a/src/libnm-platform/nm-platform.h b/src/libnm-platform/nm-platform.h index e33be81356..22bf0fdbec 100644 --- a/src/libnm-platform/nm-platform.h +++ b/src/libnm-platform/nm-platform.h @@ -2389,7 +2389,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, @@ -2495,6 +2496,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); diff --git a/src/libnm-platform/nmp-base.h b/src/libnm-platform/nmp-base.h index 9e2e1063a1..3784a78e9d 100644 --- a/src/libnm-platform/nmp-base.h +++ b/src/libnm-platform/nmp-base.h @@ -211,6 +211,9 @@ 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_MAIN_AND_NM_ROUTES: only the main table is synced, + * 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 @@ -222,6 +225,7 @@ 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_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,