From cec06138f173d0dd969dba5acdb3cde4a92884a4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Mar 2020 08:00:46 +0100 Subject: [PATCH 1/4] shared: add NM_SWAP() macro --- shared/nm-glib-aux/nm-macros-internal.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/shared/nm-glib-aux/nm-macros-internal.h b/shared/nm-glib-aux/nm-macros-internal.h index d54bb3ddda..758a4ec9aa 100644 --- a/shared/nm-glib-aux/nm-macros-internal.h +++ b/shared/nm-glib-aux/nm-macros-internal.h @@ -757,6 +757,17 @@ NM_G_ERROR_MSG (GError *error) /*****************************************************************************/ +#define NM_SWAP(a, b) \ + G_STMT_START { \ + typeof (a) _tmp; \ + \ + _tmp = (a); \ + (a) = (b); \ + (b) = _tmp; \ + } G_STMT_END + +/*****************************************************************************/ + static inline gboolean _NM_IN_STRSET_streq (const char *x, const char *s) { From 6f9a478b7ded9f767e6f421d582f189c08c33103 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 23 Mar 2020 08:04:33 +0100 Subject: [PATCH 2/4] tests: replace NMTST_SWAP() by new NM_SWAP() macro NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses a temporary variable with typeof(). I think the latter is preferable. Also, the macro is essentially doing the same thing. --- libnm-core/tests/test-general.c | 6 +++--- shared/nm-glib-aux/tests/test-shared-general.c | 2 +- shared/nm-utils/nm-test-utils.h | 13 ++----------- src/platform/tests/test-route.c | 4 ++-- src/tests/test-core.c | 2 +- src/tests/test-ip6-config.c | 2 +- 6 files changed, 10 insertions(+), 19 deletions(-) diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index e64f1b2b44..53847b72a3 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -3493,7 +3493,7 @@ test_setting_compare_addresses (void) nm_ip_address_unref (a); if (nmtst_get_rand_uint32 () % 2) - NMTST_SWAP (s1, s2); + NM_SWAP (s1, s2); success = nm_setting_compare (s1, s2, NM_SETTING_COMPARE_FLAG_EXACT); g_assert (!success); @@ -3525,7 +3525,7 @@ test_setting_compare_routes (void) nm_ip_route_unref (r); if (nmtst_get_rand_uint32 () % 2) - NMTST_SWAP (s1, s2); + NM_SWAP (s1, s2); success = nm_setting_compare (s1, s2, NM_SETTING_COMPARE_FLAG_EXACT); g_assert (!success); @@ -6820,7 +6820,7 @@ _team_config_equal_check (const char *conf1, gboolean is_same; if (nmtst_get_rand_bool ()) - NMTST_SWAP (conf1, conf2); + NM_SWAP (conf1, conf2); if (!nm_streq0 (conf1, conf2)) { _team_config_equal_check (conf1, conf1, port_config, TRUE); diff --git a/shared/nm-glib-aux/tests/test-shared-general.c b/shared/nm-glib-aux/tests/test-shared-general.c index 28aff95c02..54a96ecb5b 100644 --- a/shared/nm-glib-aux/tests/test-shared-general.c +++ b/shared/nm-glib-aux/tests/test-shared-general.c @@ -296,7 +296,7 @@ _strv_cmp_fuzz_input (const char *const*in, if (nmtst_get_rand_bool ()) { /* randomly swap the original and the clone. That means, out_s1 is either * the input argument (as-is) or the sementically equal clone. */ - NMTST_SWAP (*out_s1, *out_s2); + NM_SWAP (*out_s1, *out_s2); } if (nmtst_get_rand_bool ()) { /* randomly make s1 and s2 the same. This is for testing that diff --git a/shared/nm-utils/nm-test-utils.h b/shared/nm-utils/nm-test-utils.h index adf2ba1b35..881fdcf89c 100644 --- a/shared/nm-utils/nm-test-utils.h +++ b/shared/nm-utils/nm-test-utils.h @@ -1272,15 +1272,6 @@ nmtst_uuid_generate (void) #endif -#define NMTST_SWAP(x, y) \ - G_STMT_START { \ - char __nmtst_swap_temp[sizeof((x)) == sizeof((y)) ? (signed) sizeof((x)) : -1]; \ - \ - memcpy(__nmtst_swap_temp, &(y), sizeof (__nmtst_swap_temp)); \ - memcpy(&(y), &(x), sizeof (__nmtst_swap_temp)); \ - memcpy(&(x), __nmtst_swap_temp, sizeof (__nmtst_swap_temp)); \ - } G_STMT_END - #define nmtst_assert_str_has_substr(str, substr) \ G_STMT_START { \ const char *__str = (str); \ @@ -2087,14 +2078,14 @@ nmtst_assert_setting_is_equal (gconstpointer /* const NMSetting * */ a, g_assert (NM_IS_SETTING (b)); if (NM_FLAGS_HAS (r, 0x4)) - NMTST_SWAP (a, b); + NM_SWAP (a, b); g_assert (nm_setting_compare ((NMSetting *) a, (NMSetting *) b, flags)); if (NM_FLAGS_HAS (r, 0x8)) - NMTST_SWAP (a, b); + NM_SWAP (a, b); g_assert (nm_setting_diff ((NMSetting *) a, (NMSetting *) b, diff --git a/src/platform/tests/test-route.c b/src/platform/tests/test-route.c index 707313b4ca..f074ca69cf 100644 --- a/src/platform/tests/test-route.c +++ b/src/platform/tests/test-route.c @@ -1239,7 +1239,7 @@ again_uid_range: rr->uid_range.end = nmtst_rand_select (0u, uids.uid, uids.euid); if (rr->uid_range_has) { if (rr->uid_range.end < rr->uid_range.start) - NMTST_SWAP (rr->uid_range.start, rr->uid_range.end); + NM_SWAP (rr->uid_range.start, rr->uid_range.end); if ( rr->uid_range.start == ((guint32) -1) || rr->uid_range.end == ((guint32) -1)) goto again_uid_range; @@ -1259,7 +1259,7 @@ again_uid_range: range->start = nmtst_rand_select (1u, 0xFFFEu, ((p ) % 0xFFFEu) + 1); range->end = nmtst_rand_select (1u, 0xFFFEu, ((p >> 16) % 0xFFFEu) + 1, range->start); if (range->end < range->start) - NMTST_SWAP (range->start, range->end); + NM_SWAP (range->start, range->end); } } } diff --git a/src/tests/test-core.c b/src/tests/test-core.c index 590f7c36a5..b90a8fac69 100644 --- a/src/tests/test-core.c +++ b/src/tests/test-core.c @@ -1019,7 +1019,7 @@ _test_connection_sort_autoconnect_priority_one (NMConnection **list, gboolean sh if (shuffle) { for (i = count - 1; i > 0; i--) { j = g_rand_int (nmtst_get_rand ()) % (i + 1); - NMTST_SWAP (connections->pdata[i], connections->pdata[j]); + NM_SWAP (connections->pdata[i], connections->pdata[j]); } } diff --git a/src/tests/test-ip6-config.c b/src/tests/test-ip6-config.c index 401d1eee06..efc329832d 100644 --- a/src/tests/test-ip6-config.c +++ b/src/tests/test-ip6-config.c @@ -246,7 +246,7 @@ test_nm_ip6_config_addresses_sort_check (NMIP6Config *config, NMSettingIP6Config for (i = 0; i < addr_count; i++) { int j = g_rand_int_range (nmtst_get_rand (), i, addr_count); - NMTST_SWAP (idx[i], idx[j]); + NM_SWAP (idx[i], idx[j]); nm_ip6_config_add_address (copy, _nmtst_ip6_config_get_address (config, idx[i])); } From c878c48efbc5bedf3beaa7148d84acd31b50abe2 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Mar 2020 18:03:03 +0100 Subject: [PATCH 3/4] platform: sort IPv6 addresses from platform during sync There is no guarantee that addresses returned from the cache are in scope-priority order. Sort them. --- src/platform/nm-platform.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 699c8902fa..c19a6e2da5 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -3842,12 +3842,15 @@ ip6_address_scope_priority (const struct in6_addr *addr) } static int -ip6_address_scope_cmp (gconstpointer a, gconstpointer b) +ip6_address_scope_cmp (gconstpointer a, gconstpointer b, gpointer increasing) { const NMPlatformIP6Address *x = NMP_OBJECT_CAST_IP6_ADDRESS (*(const void **) a); const NMPlatformIP6Address *y = NMP_OBJECT_CAST_IP6_ADDRESS (*(const void **) b); + int ret; - return ip6_address_scope_priority (&x->address) - ip6_address_scope_priority (&y->address); + ret = ip6_address_scope_priority (&x->address) - ip6_address_scope_priority (&y->address); + + return GPOINTER_TO_INT (increasing) ? ret : -ret; } /** @@ -3888,7 +3891,7 @@ nm_platform_ip6_address_sync (NMPlatform *self, * apply the same sorting to known addresses, so that we don't try to * unnecessary change the order of addresses with different scopes. */ if (known_addresses) - g_ptr_array_sort (known_addresses, ip6_address_scope_cmp); + g_ptr_array_sort_with_data (known_addresses, ip6_address_scope_cmp, GINT_TO_POINTER (TRUE)); if (!_addr_array_clean_expired (AF_INET6, ifindex, known_addresses, now, &known_addresses_idx)) known_addresses = NULL; @@ -3904,6 +3907,8 @@ nm_platform_ip6_address_sync (NMPlatform *self, if (plat_addresses) { guint known_addresses_len; + 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 From 0118ad5125a414e775890a09c60c6557d2530dc6 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 18 Mar 2020 21:54:22 +0100 Subject: [PATCH 4/4] platform: improve IPv6 address synchronization When we must synchronize IPv6 addresses, we compare the order of addresses to set with what is currently set on platform. Starting from addresses with lower priority, when a mismatch is found we remove it from platform and also remove all following addresses, so that we can re-add them in the right order. Since kernel keeps addresses internally sorted by scope, we should consider each scope separately in order to avoid unnecessary address deletions. For example, if we want to configure addresses fe80::1/64,2000::1/64 and we currently have on platform 2000::1/64, it's not necessary to remove the existing address; we can just add the link-local one. Co-authored-by: Thomas Haller https://bugzilla.redhat.com/show_bug.cgi?id=1814557 --- src/platform/nm-platform.c | 95 +++++++++++++++++++++++++------------- 1 file changed, 63 insertions(+), 32 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index c19a6e2da5..9bbc37453c 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -3831,26 +3831,39 @@ delete_and_next2: return TRUE; } -static guint -ip6_address_scope_priority (const struct in6_addr *addr) +typedef enum { + IP6_ADDR_SCOPE_LOOPBACK, + IP6_ADDR_SCOPE_LINKLOCAL, + IP6_ADDR_SCOPE_SITELOCAL, + IP6_ADDR_SCOPE_OTHER, +} IP6AddrScope; + +static IP6AddrScope +ip6_address_scope (const NMPlatformIP6Address *a) { - if (IN6_IS_ADDR_LINKLOCAL (addr)) - return 1; - if (IN6_IS_ADDR_SITELOCAL (addr)) - return 2; - return 3; + if (IN6_IS_ADDR_LOOPBACK (&a->address)) + return IP6_ADDR_SCOPE_LOOPBACK; + if (IN6_IS_ADDR_LINKLOCAL (&a->address)) + return IP6_ADDR_SCOPE_LINKLOCAL; + if (IN6_IS_ADDR_SITELOCAL (&a->address)) + return IP6_ADDR_SCOPE_SITELOCAL; + return IP6_ADDR_SCOPE_OTHER; } static int -ip6_address_scope_cmp (gconstpointer a, gconstpointer b, gpointer increasing) +ip6_address_scope_cmp (gconstpointer p_a, gconstpointer p_b, gpointer increasing) { - const NMPlatformIP6Address *x = NMP_OBJECT_CAST_IP6_ADDRESS (*(const void **) a); - const NMPlatformIP6Address *y = NMP_OBJECT_CAST_IP6_ADDRESS (*(const void **) b); - int ret; + const NMPlatformIP6Address *a; + const NMPlatformIP6Address *b; - ret = ip6_address_scope_priority (&x->address) - ip6_address_scope_priority (&y->address); + if (!increasing) + NM_SWAP (p_a, p_b); - return GPOINTER_TO_INT (increasing) ? ret : -ret; + a = NMP_OBJECT_CAST_IP6_ADDRESS (*(const NMPObject *const*) p_a); + b = NMP_OBJECT_CAST_IP6_ADDRESS (*(const NMPObject *const*) p_b); + + NM_CMP_DIRECT (ip6_address_scope (a), ip6_address_scope (b)); + return 0; } /** @@ -3906,6 +3919,8 @@ nm_platform_ip6_address_sync (NMPlatform *self, if (plat_addresses) { guint known_addresses_len; + IP6AddrScope cur_scope; + gboolean delete_remaining_addrs; g_ptr_array_sort_with_data (plat_addresses, ip6_address_scope_cmp, GINT_TO_POINTER (FALSE)); @@ -3959,38 +3974,54 @@ clear_and_next: * selection will choose addresses in the order as they are reported by kernel. * Note that the order in @plat_addresses of the remaining matches is highest * priority first. - * We need to compare this to the order in @known_addresses (which has lowest - * priority first). + * We need to compare this to the order of addresses with same scope in + * @known_addresses (which has lowest priority first). * * If we find a first discrepancy, we need to delete all remaining addresses - * from that point on, because below we must re-add all the addresses in the - * right order to get their priority right. */ + * with 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; i_plat = plat_addresses->len; i_know = 0; while (i_plat > 0) { const NMPlatformIP6Address *plat_addr = NMP_OBJECT_CAST_IP6_ADDRESS (plat_addresses->pdata[--i_plat]); + IP6AddrScope plat_scope; if (!plat_addr) continue; - for (; i_know < known_addresses_len; i_know++) { - const NMPlatformIP6Address *know_addr = NMP_OBJECT_CAST_IP6_ADDRESS (known_addresses->pdata[i_know]); + plat_scope = ip6_address_scope (plat_addr); + if (cur_scope != plat_scope) { + nm_assert (cur_scope < plat_scope); + delete_remaining_addrs = FALSE; + cur_scope = plat_scope; + } - if (!know_addr) - continue; + if (!delete_remaining_addrs) { + delete_remaining_addrs = TRUE; + for (; i_know < known_addresses_len; i_know++) { + const NMPlatformIP6Address *know_addr = NMP_OBJECT_CAST_IP6_ADDRESS (known_addresses->pdata[i_know]); + IP6AddrScope know_scope; - if (IN6_ARE_ADDR_EQUAL (&plat_addr->address, &know_addr->address)) { - /* we have a match. Mark address as handled. */ - i_know++; - goto next_plat; + if (!know_addr) + continue; + + know_scope = ip6_address_scope (know_addr); + if (know_scope < plat_scope) + continue; + + 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; + } + + /* plat_address has no match. Now delete_remaining_addrs is TRUE and we will + * delete all the remaining addresses with cur_scope. */ + break; } - - /* all remainging addresses need to be removed as well, so that we can - * re-add them in the correct order. Signal that, by setting @i_know - * so that the next @i_plat iteration, we won't enter the loop and - * delete the address right away */ - i_know = known_addresses_len; - break; } nm_platform_ip6_address_delete (self, ifindex, plat_addr->address, plat_addr->plen);