From 08884c53a9c3b7d4bcc63b9d4be7ccab3a521e17 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 Apr 2022 16:25:01 +0200 Subject: [PATCH 1/3] platform: re-configure one address at a time in nm_platform_ip_address_sync() Try to do one change at a time when reconfiguring addresses, to not remove several/all addresses at once. For IP addresses, kernel cares about the order in which they were added. This mostly affects source address selection, and the "secondary" flag for IPv4 addresses. The order is thus related to the priority of an address. There is no direct kernel API to change the order. Instead, we have to add them in the correct order. During a sync, if an address already exists in the wrong order, we need to remove it, and re-add it. Btw, with IPv4 addresses added first via netlink are the primary address, while with IPv6 it's reverse. Previously, we would first iterate over all addresses and remove those that had a conflicting order. This means, that we would potentially remove all addresses for a short while, before readding them. That seems problematic. Instead, first track all addresses that are in the wrong order. And in the step when we add/update the address, remove it. We now only remove and address shortly before re-adding it. This way the time for which the address on the interface is missing is shorter. More importantly, we will never remove all addresses at the same time. (cherry picked from commit a6fd6416345ccb397ae174dc39d66e9e7bfa7fe6) (cherry picked from commit a1835c2c0509a949974d9fd57caee037fc4a5d56) --- src/libnm-platform/nm-platform.c | 61 ++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 179a577bb6..e9318b5be7 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -3965,9 +3965,10 @@ nm_platform_ip_address_sync(NMPlatform *self, gint32 now = 0; const int IS_IPv4 = NM_IS_IPv4(addr_family); NMPLookup lookup; - const gboolean EXTRA_LOGGING = FALSE; - gs_unref_hashtable GHashTable *known_addresses_idx = NULL; - gs_unref_ptrarray GPtrArray *plat_addresses = NULL; + const gboolean EXTRA_LOGGING = FALSE; + gs_unref_hashtable GHashTable *known_addresses_idx = NULL; + gs_unref_hashtable GHashTable *plat_addrs_to_delete = NULL; + gs_unref_ptrarray GPtrArray *plat_addresses = NULL; gboolean success; guint i_plat; guint i_know; @@ -3976,6 +3977,19 @@ nm_platform_ip_address_sync(NMPlatform *self, _CHECK_SELF(self, klass, FALSE); +#define _plat_addrs_to_delete_ensure(ptr) \ + ({ \ + GHashTable **_ptr = (ptr); \ + \ + if (!*_ptr) { \ + *_ptr = g_hash_table_new_full((GHashFunc) nmp_object_id_hash, \ + (GEqualFunc) nmp_object_id_equal, \ + (GDestroyNotify) nmp_object_unref, \ + NULL); \ + } \ + *_ptr; \ + }) + /* Disabled. Enable this for printf debugging. */ if (EXTRA_LOGGING) { char sbuf[NM_UTILS_TO_STRING_BUFFER_SIZE]; @@ -4133,11 +4147,8 @@ nm_platform_ip_address_sync(NMPlatform *self, } plat_handled[i] = TRUE; - nm_platform_ip4_address_delete(self, - ifindex, - plat_address->address, - plat_address->plen, - plat_address->peer_address); + g_hash_table_add(_plat_addrs_to_delete_ensure(&plat_addrs_to_delete), + (gpointer) nmp_object_ref(plat_obj)); if (!ip4_addr_subnets_is_secondary(plat_obj, plat_subnets, @@ -4167,12 +4178,11 @@ nm_platform_ip_address_sync(NMPlatform *self, 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)); + continue; } + + g_hash_table_add(_plat_addrs_to_delete_ensure(&plat_addrs_to_delete), + (gpointer) nmp_object_ref(*o)); } } } @@ -4208,7 +4218,10 @@ nm_platform_ip_address_sync(NMPlatform *self, * @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. */ + * removing and readding it. Thus, we need to delete plat_addr. + * + * We don't just add this address to @plat_addrs_to_delete, because + * it's too different. Instead, delete and re-add below. */ nm_platform_ip_address_delete(self, AF_INET6, ifindex, @@ -4234,9 +4247,9 @@ nm_platform_ip_address_sync(NMPlatform *self, i_know = nm_g_ptr_array_len(known_addresses); while (i_plat > 0) { - const NMPlatformIP6Address *plat_addr = - NMP_OBJECT_CAST_IP6_ADDRESS(plat_addresses->pdata[--i_plat]); - IP6AddrScope plat_scope; + const NMPObject *plat_obj = plat_addresses->pdata[--i_plat]; + const NMPlatformIP6Address *plat_addr = NMP_OBJECT_CAST_IP6_ADDRESS(plat_obj); + IP6AddrScope plat_scope; if (!plat_addr) continue; @@ -4275,7 +4288,8 @@ nm_platform_ip_address_sync(NMPlatform *self, } } - nm_platform_ip6_address_delete(self, ifindex, plat_addr->address, plat_addr->plen); + g_hash_table_add(_plat_addrs_to_delete_ensure(&plat_addrs_to_delete), + (gpointer) nmp_object_ref(plat_obj)); next_plat:; } } @@ -4320,6 +4334,17 @@ next_plat:; nm_assert(lifetime > 0); plat_obj = nm_platform_ip_address_get(self, addr_family, ifindex, known_address); + + if (plat_obj && nm_g_hash_table_contains(plat_addrs_to_delete, plat_obj)) { + /* This address exists, but it had the wrong priority earlier. We + * cannot just update it, we need to remove it first. */ + nm_platform_ip_address_delete(self, + addr_family, + ifindex, + NMP_OBJECT_CAST_IP_ADDRESS(plat_obj)); + plat_obj = NULL; + } + if (plat_obj && nm_platform_vtable_address.vx[IS_IPv4].address_cmp( known_address, From 1188942c75c16e6ac33d6aef9eae1108685e3880 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 2 May 2022 13:08:29 +0200 Subject: [PATCH 2/3] platform: fix handling IPv6 address index in nm_platform_ip_address_sync() Fixes: 4a548423b91e ('core: change order/priority of static IPv6 addresses relative to autoconf6/DHCPv6') (cherry picked from commit b52941ac34c11fa2e155e94bac78e07cf5930c9e) (cherry picked from commit 169d74b2e42dfc0b49073988b5ad4d777f9aa510) --- src/libnm-platform/nm-platform.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index e9318b5be7..9b7c04fde5 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -4277,7 +4277,6 @@ nm_platform_ip_address_sync(NMPlatform *self, if (IN6_ARE_ADDR_EQUAL(&plat_addr->address, &know_addr->address)) { /* we have a match. Mark address as handled. */ - i_know++; delete_remaining_addrs = FALSE; goto next_plat; } From fdc4207ab1c2459c055cb9cffbc6926e06f1e1db Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 2 May 2022 13:10:24 +0200 Subject: [PATCH 3/3] platform: simplify loop for IPv6 addresses in nm_platform_ip_address_sync() (cherry picked from commit 9b930cd96275869bab573ab9bc60febb8d3371ce) (cherry picked from commit 555891fe8d2b3f6ad5ecf22017521f0f1262dca3) --- src/libnm-platform/nm-platform.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index 9b7c04fde5..0fadce0904 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -4262,7 +4262,6 @@ nm_platform_ip_address_sync(NMPlatform *self, } if (!delete_remaining_addrs) { - delete_remaining_addrs = TRUE; while (i_know > 0) { const NMPlatformIP6Address *know_addr = NMP_OBJECT_CAST_IP6_ADDRESS(known_addresses->pdata[--i_know]); @@ -4277,14 +4276,14 @@ nm_platform_ip_address_sync(NMPlatform *self, if (IN6_ARE_ADDR_EQUAL(&plat_addr->address, &know_addr->address)) { /* we have a match. Mark address as handled. */ - delete_remaining_addrs = FALSE; goto next_plat; } - /* plat_address has no match. Now delete_remaining_addrs is TRUE and we will - * delete all the remaining addresses with cur_scope. */ + /* "plat_address" has no match. "delete_remaining_addrs" will be set to TRUE and we will + * delete all the remaining addresses with "cur_scope". */ break; } + delete_remaining_addrs = TRUE; } g_hash_table_add(_plat_addrs_to_delete_ensure(&plat_addrs_to_delete),