From cd4601802de52f0239b5b8c19bd90ed9ccb6a50c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 28 Mar 2022 21:13:09 +0200 Subject: [PATCH] platform: fix address order in nm_platform_ip_address_sync() In the past, nm_platform_ip_address_sync() only had the @known_addresses argument. We would figure out which addresses to delete and which to preserve, based on what addresses were known. That means, @known_addresses must have contained all the addresses we wanted to preserve, even the external ones. That approach was inherently racy. Instead, nowadays we have the addresses we want to configure (@known_addresses) and the addresses we want to delete (@prune_addresses). This started to change in commit dadfc3abd510 ('platform: allow injecting the list of addresses to prune'), but only commit 58287cbcc0c8 ('core: rework IP configuration in NetworkManager using layer 3 configuration') actually changed to pass separate @prune_addresses argument. However, the order of IP addresses matters and there is no sensible kernel API to configure the order (short of adding them in the right order), we still need to look at all the addresses, check their order, and possibly delete some. That is, we need to handle addresses we want to delete (@prune_addresses) but still look at all addresses in platform (@plat_addresses) to check their order. Now, first handle @prune_addresses. That's simple. These are just the addresses we want to delete. Second, get the list of all addresses in platform (@plat_addresses) and check the order. Note that if there is an external address that interferes with our desired order, we will leave it untouched. Thus, such external addresses might prevent us from getting the order as desired. But that's just how it is. Don't add addresses outside of NetworkManager to avoid that. Fixes: 58287cbcc0c8 ('core: rework IP configuration in NetworkManager using layer 3 configuration') (cherry picked from commit 80f8e23992b58aa0b6fd88de0d3973eea51691a4) (cherry picked from commit 4c3197b37790c6c89c7b3df0e92a26e1f8719a5a) --- src/libnm-platform/nm-platform.c | 207 +++++++++++++++++++------------ 1 file changed, 126 insertions(+), 81 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 922f412df7..61fcf459ab 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -3889,9 +3889,8 @@ ip6_address_scope_cmp(gconstpointer p_a, gconstpointer p_b, gpointer increasing) * If platform has such an address configured, it will be deleted * at the beginning of the sync. Note that the array will be modified * by the function. - * Note that the addresses must be properly sorted, by their priority. - * Create this list with nm_platform_ip_address_get_prune_list() which - * gets the sorting right. + * Addresses that are both contained in @known_addresses and @addresses_prune + * will be configured. * * A convenience function to synchronize addresses for a specific interface * with the least possible disturbance. It simply removes addresses that are @@ -3906,11 +3905,12 @@ nm_platform_ip_address_sync(NMPlatform *self, GPtrArray *known_addresses, GPtrArray *addresses_prune) { - const gint32 now = nm_utils_get_monotonic_timestamp_sec(); - const int IS_IPv4 = NM_IS_IPv4(addr_family); + const gint32 now = nm_utils_get_monotonic_timestamp_sec(); + const int IS_IPv4 = NM_IS_IPv4(addr_family); + NMPLookup lookup; gs_unref_hashtable GHashTable *known_addresses_idx = NULL; - GPtrArray *plat_addresses; - GHashTable *known_subnets = NULL; + gs_unref_ptrarray GPtrArray *plat_addresses = NULL; + GHashTable *known_subnets = NULL; guint i_plat; guint i_know; guint i; @@ -3918,6 +3918,9 @@ nm_platform_ip_address_sync(NMPlatform *self, _CHECK_SELF(self, klass, FALSE); + /* @known_addresses (IPv4) are in decreasing priority order (highest priority addresses first). + * @known_addresses (IPv6) are in increasing priority order (highest priority addresses last) (we will sort them by scope next). */ + /* The order we want to enforce is only among addresses with the same * scope, as the kernel keeps addresses sorted by scope. Therefore, * apply the same sorting to known addresses, so that we don't try to @@ -3936,51 +3939,92 @@ nm_platform_ip_address_sync(NMPlatform *self, &known_addresses_idx)) known_addresses = NULL; - /* @plat_addresses must be sorted in decreasing priority order (highest priority addresses first), contrary to - * @known_addresses which is in increasing priority order (lowest priority addresses first). */ - plat_addresses = addresses_prune; + if (nm_g_ptr_array_len(addresses_prune) > 0) { + /* First delete addresses that we should prune (and which are no longer tracked + * as @known_addresses. */ + for (i = 0; i < addresses_prune->len; i++) { + const NMPObject *prune_obj = addresses_prune->pdata[i]; + + nm_assert(NM_IN_SET(NMP_OBJECT_GET_TYPE(prune_obj), + NMP_OBJECT_TYPE_IP4_ADDRESS, + NMP_OBJECT_TYPE_IP6_ADDRESS)); + + if (nm_g_hash_table_contains(known_addresses_idx, prune_obj)) + continue; + + nm_platform_ip_address_delete(self, + addr_family, + ifindex, + NMP_OBJECT_CAST_IP_ADDRESS(prune_obj)); + } + } + + /* @plat_addresses for IPv6 must be sorted in decreasing priority order (highest priority addresses first). + * IPv4 are probably unsorted or sorted with lowest priority first, but their order doesn't matter because + * we check the "secondary" flag. */ + plat_addresses = nm_platform_lookup_clone( + self, + nmp_lookup_init_object(&lookup, NMP_OBJECT_TYPE_IP_ADDRESS(IS_IPv4), ifindex), + NULL, + NULL); if (nm_g_ptr_array_len(plat_addresses) > 0) { - /* Delete unknown addresses */ + /* Delete addresses that interfere with our intended order. */ if (IS_IPv4) { - GHashTable *plat_subnets; + gs_free bool *plat_handled_to_free = NULL; + bool *plat_handled = NULL; + GHashTable *plat_subnets; + + /* For IPv4, we only consider it a conflict for addresses in the same + * subnet. That's where kernel will assign a primary/secondary flag. + * For different subnets, we don't define the order. */ plat_subnets = ip4_addr_subnets_build_index(plat_addresses, TRUE, TRUE); for (i = 0; i < plat_addresses->len; i++) { - const NMPObject *plat_obj; + const NMPObject *plat_obj = plat_addresses->pdata[i]; + const NMPObject *known_obj; const NMPlatformIP4Address *plat_address; const GPtrArray *addr_list; + gboolean secondary; - plat_obj = plat_addresses->pdata[i]; - if (!plat_obj) { - /* Already deleted */ + if (plat_handled && plat_handled[i]) + continue; + + known_obj = nm_g_hash_table_lookup(known_addresses_idx, plat_obj); + + if (!known_obj) { + /* this address is added externally. Even if it's presence would mess + * with our desired order, we cannot delete it. Skip it. */ + if (!plat_handled) { + plat_handled = nm_malloc0_maybe_a(300, + sizeof(bool) * plat_addresses->len, + &plat_handled_to_free); + } + plat_handled[i] = TRUE; continue; } + if (!known_subnets) + known_subnets = ip4_addr_subnets_build_index(known_addresses, FALSE, FALSE); + plat_address = NMP_OBJECT_CAST_IP4_ADDRESS(plat_obj); - if (known_addresses) { - const NMPObject *o; - - o = g_hash_table_lookup(known_addresses_idx, plat_obj); - if (o) { - gboolean secondary; - - if (!known_subnets) - known_subnets = - ip4_addr_subnets_build_index(known_addresses, FALSE, FALSE); - - secondary = - ip4_addr_subnets_is_secondary(o, known_subnets, known_addresses, NULL); - if (secondary == NM_FLAGS_HAS(plat_address->n_ifa_flags, IFA_F_SECONDARY)) { - /* if we have an existing known-address, with matching secondary role, - * do not delete the platform-address. */ - continue; - } - } + secondary = + ip4_addr_subnets_is_secondary(known_obj, known_subnets, known_addresses, NULL); + if (secondary == NM_FLAGS_HAS(plat_address->n_ifa_flags, IFA_F_SECONDARY)) { + /* if we have an existing known-address, with matching secondary role, + * do not delete the platform-address. */ + continue; } + if (!plat_handled) { + plat_handled = nm_malloc0_maybe_a(300, + sizeof(bool) * plat_addresses->len, + &plat_handled_to_free); + } + plat_handled[i] = TRUE; + nm_platform_ip4_address_delete(self, ifindex, plat_address->address, @@ -3999,22 +4043,27 @@ nm_platform_ip_address_sync(NMPlatform *self, * addresses are deleted, so that we can start with a clean * slate and add addresses in the right order. */ for (j = 1; j < addr_list->len; j++) { - const NMPObject **o; + const NMPObject **o = ip4_addr_subnets_addr_list_get(addr_list, j); + guint o_idx; - o = ip4_addr_subnets_addr_list_get(addr_list, j); - nm_assert(o); + o_idx = (o - ((const NMPObject **) &plat_addresses->pdata[0])); - if (*o) { - const NMPlatformIP4Address *a; + nm_assert(o_idx < plat_addresses->len); + nm_assert(o == ((const NMPObject **) &plat_addresses->pdata[o_idx])); - a = NMP_OBJECT_CAST_IP4_ADDRESS(*o); - nm_platform_ip4_address_delete(self, - ifindex, - a->address, - a->plen, - a->peer_address); - nmp_object_unref(*o); - *o = NULL; + if (plat_handled[o_idx]) + continue; + + plat_handled[o_idx] = TRUE; + + if (!nm_g_hash_table_contains(known_addresses_idx, *o)) { + /* Again, this is an external address. We cannot delete + * it to fix the address order. Pass. */ + } else { + nm_platform_ip_address_delete(self, + AF_INET, + ifindex, + NMP_OBJECT_CAST_IP4_ADDRESS(*o)); } } } @@ -4025,48 +4074,44 @@ nm_platform_ip_address_sync(NMPlatform *self, IP6AddrScope cur_scope; gboolean delete_remaining_addrs; + /* For IPv6, we only compare addresses per-scope. Addresses in different + * scopes don't have a defined order. */ + g_ptr_array_sort_with_data(plat_addresses, ip6_address_scope_cmp, GINT_TO_POINTER(FALSE)); known_addresses_len = known_addresses ? known_addresses->len : 0; - /* First, compare every address whether it is still a "known address", that is, whether - * to keep it or to delete it. - * - * If we don't find a matching valid address in @known_addresses, we will delete - * plat_addr. - * - * Certain addresses, like temporary addresses, are ignored by this function - * if not run with full_sync. These addresses are usually not managed by NetworkManager - * directly, or at least, they are not managed via nm_platform_ip6_address_sync(). - * Only in full_sync mode, we really want to get rid of them (usually, when we take - * the interface down). - * - * Note that we mark handled addresses by setting it to %NULL in @plat_addresses array. */ + /* First, check that existing addresses have a matching plen as the ones + * we are about to configure (@known_addresses). If not, delete them. */ for (i_plat = 0; i_plat < plat_addresses->len; i_plat++) { - const NMPObject *plat_obj = plat_addresses->pdata[i_plat]; - const NMPObject *know_obj; - const NMPlatformIP6Address *plat_addr = NMP_OBJECT_CAST_IP6_ADDRESS(plat_obj); + const NMPObject *plat_obj = plat_addresses->pdata[i_plat]; + const NMPObject *known_obj; - if (known_addresses_idx) { - know_obj = g_hash_table_lookup(known_addresses_idx, plat_obj); - if (know_obj - && plat_addr->plen == NMP_OBJECT_CAST_IP6_ADDRESS(know_obj)->plen) { - /* technically, plen is not part of the ID for IPv6 addresses and thus - * @plat_addr is essentially the same address as @know_addr (regrading - * its identity, not its other attributes). - * However, we cannot modify an existing addresses' plen without - * removing and readding it. Thus, only keep plat_addr, if the plen - * matches. - * - * keep this one, and continue */ - continue; - } + known_obj = nm_g_hash_table_lookup(known_addresses_idx, plat_obj); + if (!known_obj) { + /* We don't know this address. It was added externally. Keep it configured. + * We also don't want to delete the address below, so mark it as handled + * by clearing the pointer. */ + nm_clear_pointer(&plat_addresses->pdata[i_plat], nmp_object_unref); + continue; } - nm_platform_ip6_address_delete(self, ifindex, plat_addr->address, plat_addr->plen); - nmp_object_unref(g_steal_pointer(&plat_addresses->pdata[i_plat])); + if (NMP_OBJECT_CAST_IP6_ADDRESS(plat_obj)->plen + != NMP_OBJECT_CAST_IP6_ADDRESS(known_obj)->plen) { + /* technically, plen is not part of the ID for IPv6 addresses and thus + * @plat_addr is essentially the same address as @know_addr (w.r.t. + * its identity, not its other attributes). + * However, we cannot modify an existing addresses' plen without + * removing and readding it. Thus, we need to delete plat_addr. */ + nm_platform_ip_address_delete(self, + AF_INET6, + ifindex, + NMP_OBJECT_CAST_IP6_ADDRESS(plat_obj)); + /* Mark address as handled. */ + nm_clear_pointer(&plat_addresses->pdata[i_plat], nmp_object_unref); + } } /* Next, we must preserve the priority of the routes. That is, source address @@ -4077,7 +4122,7 @@ nm_platform_ip_address_sync(NMPlatform *self, * @known_addresses (which has lowest priority first). * * If we find a first discrepancy, we need to delete all remaining addresses - * with same scope from that point on, because below we must re-add all the + * for same scope from that point on, because below we must re-add all the * addresses in the right order to get their priority right. */ cur_scope = IP6_ADDR_SCOPE_LOOPBACK; delete_remaining_addrs = FALSE;