From a1b73d823f5ec30c240418137d62d183b6ff8ca7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2023 09:44:59 +0200 Subject: [PATCH 1/2] Revert "infiniband: avoid normalizing the p-key when reading from ifcfg" Historically, initscripts' ifup-ib would set the highest bit of PKEY_ID=. That changed and needs to be restored. Note that it probably makes little sense to ever configure p-keys without the highest bit set, because that flag indicates full membership and kernel will automatically add it. At least, kernel will add the flag for the p-key, but not for the automatically chosen interface name. Meaning, writing 0x00f0 to create_child sysctl, results in an interface "$parent.00f0", but `ip -d link` shows pkey 0x80f0. As NetworkManager otherwise supports p-keys without the highest bit set, and since that high bit is honored for the interface name, we cannot just always add the high bit. NetworkManager always assuming the highest bit is set, would change the interface names of existing configuration. With this revert, when a user configures a small p-key and the profile is stored in ifcfg-rh format, the settings backend will automatically mangle the profile and set 0x8000. That is different from when the profile is stored in keyfile format. Since using small p-keys is probably an odd case, we don't try to workaround that any other way (like that ifcfg format could represent the orignal value of the profile and not doing such mangling, or to add the high bit throughout NetworkManager to the p-key). It's an inconsistency, but given the existing behaviors it seems best to stick (revert) to it. This reverts commit a4fe16a426097eee263cb3ef831dcea468b1ca26. Affected versions were 1.42.2+ and 1.40.2+. See-also: https://src.fedoraproject.org/rpms/rdma/blob/05333c3602aa3c1d82a6363521bdd5a498eac6d0/f/rdma.ifup-ib#_75 https://bugzilla.redhat.com/show_bug.cgi?id=2209164 (cherry picked from commit f8e5e07355e23b6d59b1b1c9cd2387c6b40b214b) --- .../plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 18 ++++++ .../plugins/ifcfg-rh/tests/test-ifcfg-rh.c | 57 ++++++++++++++----- 2 files changed, 60 insertions(+), 15 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 02ba843201..533379c678 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -5387,6 +5387,24 @@ parse_infiniband_p_key(shvarFile *ifcfg, int *out_p_key, char **out_parent, GErr return FALSE; } + /* The highest bit 0x8000 indicates full membership, which kernel always + * automatically sets. + * + * NetworkManager supports p-keys without the high bit set. That affects + * the interface name (nmp_utils_new_infiniband_name()) and is what + * we write to "create_child"/"delete_child" sysctl. Kernel will honor + * such p-keys for the interface name, but for other purposes it adds the + * highest bit. That makes using p-keys without the highest bit odd. + * + * Historically, /etc/sysconfig/network-scripts/ifup-ib would always add "|=0x8000". + * The reader does that too. + * + * Note that this means ifcfg cannot handle p-keys without the highest bit set, + * and when trying to store that to ifcfg format, the profile will be mangled/modified + * by the ifcg plugin (unlike keyfile backend, which preserves the original p-key value). + */ + id |= 0x8000; + *out_p_key = id; *out_parent = g_steal_pointer(&physdev); return TRUE; diff --git a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c index d2ac2b29db..01eb24216a 100644 --- a/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c +++ b/src/core/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh.c @@ -8383,21 +8383,21 @@ test_read_ipoib(void) s_infiniband = nmtst_connection_assert_setting(connection, NM_TYPE_SETTING_INFINIBAND); pkey = nm_setting_infiniband_get_p_key(s_infiniband); - g_assert(pkey); - g_assert_cmpint(pkey, ==, 12); + g_assert_cmpint(pkey, ==, 0x800c); transport_mode = nm_setting_infiniband_get_transport_mode(s_infiniband); - g_assert(transport_mode); g_assert_cmpstr(transport_mode, ==, "connected"); } static void test_write_infiniband(gconstpointer test_data) { - const int TEST_IDX = GPOINTER_TO_INT(test_data); - nmtst_auto_unlinkfile char *testfile = NULL; - gs_unref_object NMConnection *connection = NULL; - gs_unref_object NMConnection *reread = NULL; + const int TEST_IDX = GPOINTER_TO_INT(test_data); + nmtst_auto_unlinkfile char *testfile = NULL; + gs_unref_object NMConnection *connection = NULL; + gs_unref_object NMConnection *expected = NULL; + gs_unref_object NMConnection *reread = NULL; + gboolean reread_same = FALSE; NMSettingConnection *s_con; NMSettingInfiniband *s_infiniband; NMSettingIPConfig *s_ip4; @@ -8407,6 +8407,7 @@ test_write_infiniband(gconstpointer test_data) NMIPAddress *addr; GError *error = NULL; const char *interface_name = NULL; + int p_key; connection = nm_simple_connection_new(); @@ -8422,14 +8423,21 @@ test_write_infiniband(gconstpointer test_data) NM_SETTING_INFINIBAND_SETTING_NAME, NULL); - if (NM_IN_SET(TEST_IDX, 1, 3)) - interface_name = "ib0.000c"; + if (NM_IN_SET(TEST_IDX, 1, 2)) + p_key = nmtst_get_rand_bool() ? 0x000c : 0x800c; + else + p_key = -1; + + if (NM_IN_SET(TEST_IDX, 1, 3)) { + if (p_key >= 0x8000) + interface_name = "ib0.800c"; + } g_object_set(s_con, NM_SETTING_CONNECTION_INTERFACE_NAME, interface_name, NULL); s_infiniband = _nm_connection_new_setting(connection, NM_TYPE_SETTING_INFINIBAND); g_object_set(s_infiniband, NM_SETTING_INFINIBAND_TRANSPORT_MODE, "connected", NULL); - if (NM_IN_SET(TEST_IDX, 1, 2)) { + if (p_key == -1) { g_object_set(s_infiniband, NM_SETTING_INFINIBAND_MAC_ADDRESS, mac, @@ -8439,7 +8447,7 @@ test_write_infiniband(gconstpointer test_data) } else { g_object_set(s_infiniband, NM_SETTING_INFINIBAND_P_KEY, - 12, + p_key, NM_SETTING_INFINIBAND_PARENT, "ib0", NULL); @@ -8468,13 +8476,32 @@ test_write_infiniband(gconstpointer test_data) nmtst_assert_connection_verifies(connection); - _writer_new_connection(connection, TEST_SCRATCH_DIR, &testfile); - - reread = _connection_from_file(testfile, NULL, TYPE_INFINIBAND, NULL); + if (p_key != -1 && p_key < 0x8000) { + expected = nm_simple_connection_new_clone(connection); + g_object_set(nm_connection_get_setting(expected, NM_TYPE_SETTING_INFINIBAND), + NM_SETTING_INFINIBAND_P_KEY, + (int) (p_key | 0x8000), + NULL); + } else + expected = g_object_ref(connection); - nmtst_assert_connection_equals(connection, TRUE, reread, FALSE); + _writer_new_connection_reread(connection, + TEST_SCRATCH_DIR, + &testfile, + NO_EXPECTED, + &reread, + &reread_same); + _assert_reread_same(expected, reread); + if (p_key == -1 || p_key > 0x8000) + g_assert(reread_same); + else + g_assert(!reread_same); g_assert_cmpstr(interface_name, ==, nm_connection_get_interface_name(reread)); + g_assert_cmpint(nm_setting_infiniband_get_p_key( + _nm_connection_get_setting(reread, NM_TYPE_SETTING_INFINIBAND)), + ==, + p_key == -1 ? -1 : (p_key | 0x8000)); } static void -- 2.39.3