From a6fd6416345ccb397ae174dc39d66e9e7bfa7fe6 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 27 Apr 2022 16:25:01 +0200 Subject: [PATCH] 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. --- 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 921782ec8..53a3dd4e6 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -4029,9 +4029,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; @@ -4040,6 +4041,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]; @@ -4200,11 +4214,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, @@ -4234,12 +4245,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)); } } } @@ -4275,7 +4285,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, @@ -4301,9 +4314,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; @@ -4342,7 +4355,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:; } } @@ -4387,6 +4401,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,