From f8e5e07355e23b6d59b1b1c9cd2387c6b40b214b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2023 09:44:59 +0200 Subject: [PATCH 1/6] 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 --- .../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 4009574472..c3cba7c693 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 @@ -5420,6 +5420,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 0172747481..bac5e93ed2 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 @@ -8410,21 +8410,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; @@ -8434,6 +8434,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(); @@ -8449,14 +8450,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, @@ -8466,7 +8474,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); @@ -8495,13 +8503,32 @@ test_write_infiniband(gconstpointer test_data) nmtst_assert_connection_verifies(connection); - _writer_new_connection(connection, TEST_SCRATCH_DIR, &testfile); + 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); - reread = _connection_from_file(testfile, NULL, TYPE_INFINIBAND, NULL); - - 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 From ea18e66ef657b55eca941dca3de4949b950e656b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2023 10:44:58 +0200 Subject: [PATCH 2/6] libnm/docs: clarify behavior of infiniband.p-key property --- src/libnm-core-impl/nm-setting-infiniband.c | 19 ++++++++++++++++--- src/libnmc-setting/settings-docs.h.in | 2 +- .../gen-metadata-nm-settings-nmcli.xml.in | 2 +- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/libnm-core-impl/nm-setting-infiniband.c b/src/libnm-core-impl/nm-setting-infiniband.c index 410f1f0687..7ba5720619 100644 --- a/src/libnm-core-impl/nm-setting-infiniband.c +++ b/src/libnm-core-impl/nm-setting-infiniband.c @@ -449,9 +449,20 @@ nm_setting_infiniband_class_init(NMSettingInfinibandClass *klass) * NMSettingInfiniband:p-key: * * The InfiniBand P_Key to use for this device. A value of -1 means to use - * the default P_Key (aka "the P_Key at index 0"). Otherwise, it is a 16-bit - * unsigned integer, whose high bit is set if it is a "full membership" - * P_Key. + * the default P_Key (aka "the P_Key at index 0"). Otherwise, it is a + * 16-bit unsigned integer, whose high bit 0x8000 is set if it is a "full + * membership" P_Key. The values 0 and 0x8000 are not allowed. + * + * With the p-key set, the interface name is always "$parent.$p_key". + * Setting "connection.interface-name" to another name is not supported. + * + * Note that kernel will internally always set the full membership bit, + * although the interface name does not reflect that. Thus, not setting + * the high bit is probably not useful. + * + * If the profile is stored in ifcfg-rh format, then the full membership + * bit is automatically added. To get consistent behavior, it is + * best to only use p-key values with the full membership bit set. **/ /* ---ifcfg-rh--- * property: p-key @@ -460,6 +471,8 @@ nm_setting_infiniband_class_init(NMSettingInfinibandClass *klass) * description: InfiniBand P_Key. The value can be a hex number prefixed with "0x" * or a decimal number. * When PKEY_ID is specified, PHYSDEV and DEVICE also must be specified. + * Note that ifcfg-rh format will always automatically set the full membership + * bit 0x8000. Other p-key cannot be stored. * example: PKEY=yes PKEY_ID=2 PHYSDEV=mlx4_ib0 DEVICE=mlx4_ib0.8002 * ---end--- */ diff --git a/src/libnmc-setting/settings-docs.h.in b/src/libnmc-setting/settings-docs.h.in index 212b2b6522..61a04022c1 100644 --- a/src/libnmc-setting/settings-docs.h.in +++ b/src/libnmc-setting/settings-docs.h.in @@ -155,7 +155,7 @@ #define DESCRIBE_DOC_NM_SETTING_GSM_USERNAME N_("The username used to authenticate with the network, if required. Many providers do not require a username, or accept any username. But if a username is required, it is specified here.") #define DESCRIBE_DOC_NM_SETTING_INFINIBAND_MAC_ADDRESS N_("If specified, this connection will only apply to the IPoIB device whose permanent MAC address matches. This property does not change the MAC address of the device (i.e. MAC spoofing).") #define DESCRIBE_DOC_NM_SETTING_INFINIBAND_MTU N_("If non-zero, only transmit packets of the specified size or smaller, breaking larger packets up into multiple frames.") -#define DESCRIBE_DOC_NM_SETTING_INFINIBAND_P_KEY N_("The InfiniBand P_Key to use for this device. A value of -1 means to use the default P_Key (aka \"the P_Key at index 0\"). Otherwise, it is a 16-bit unsigned integer, whose high bit is set if it is a \"full membership\" P_Key.") +#define DESCRIBE_DOC_NM_SETTING_INFINIBAND_P_KEY N_("The InfiniBand P_Key to use for this device. A value of -1 means to use the default P_Key (aka \"the P_Key at index 0\"). Otherwise, it is a 16-bit unsigned integer, whose high bit 0x8000 is set if it is a \"full membership\" P_Key. The values 0 and 0x8000 are not allowed. With the p-key set, the interface name is always \"$parent.$p_key\". Setting \"connection.interface-name\" to another name is not supported. Note that kernel will internally always set the full membership bit, although the interface name does not reflect that. Thus, not setting the high bit is probably not useful. If the profile is stored in ifcfg-rh format, then the full membership bit is automatically added. To get consistent behavior, it is best to only use p-key values with the full membership bit set.") #define DESCRIBE_DOC_NM_SETTING_INFINIBAND_PARENT N_("The interface name of the parent device of this device. Normally NULL, but if the \"p_key\" property is set, then you must specify the base device by setting either this property or \"mac-address\".") #define DESCRIBE_DOC_NM_SETTING_INFINIBAND_TRANSPORT_MODE N_("The IP-over-InfiniBand transport mode. Either \"datagram\" or \"connected\".") #define DESCRIBE_DOC_NM_SETTING_IP4_CONFIG_ADDRESSES N_("A list of IPv4 addresses and their prefix length. Multiple addresses can be separated by comma. For example \"192.168.1.5/24, 10.1.0.5/24\". The addresses are listed in decreasing priority, meaning the first address will be the primary address.") diff --git a/src/nmcli/gen-metadata-nm-settings-nmcli.xml.in b/src/nmcli/gen-metadata-nm-settings-nmcli.xml.in index 59bf453515..8b9d165130 100644 --- a/src/nmcli/gen-metadata-nm-settings-nmcli.xml.in +++ b/src/nmcli/gen-metadata-nm-settings-nmcli.xml.in @@ -612,7 +612,7 @@ nmcli-description="The IP-over-InfiniBand transport mode. Either "datagram" or "connected"." /> + nmcli-description="The InfiniBand P_Key to use for this device. A value of -1 means to use the default P_Key (aka "the P_Key at index 0"). Otherwise, it is a 16-bit unsigned integer, whose high bit 0x8000 is set if it is a "full membership" P_Key. The values 0 and 0x8000 are not allowed. With the p-key set, the interface name is always "$parent.$p_key". Setting "connection.interface-name" to another name is not supported. Note that kernel will internally always set the full membership bit, although the interface name does not reflect that. Thus, not setting the high bit is probably not useful. If the profile is stored in ifcfg-rh format, then the full membership bit is automatically added. To get consistent behavior, it is best to only use p-key values with the full membership bit set." /> From 4610fd67e6e795131a358b292ec3fc1ba2a2250f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2023 17:32:19 +0200 Subject: [PATCH 3/6] libnm: normalize interface-name for infiniband profiles NetworkManager does not support changing the interface name for infiniband interfaces. Consequently, we verify that "connection.interface-name" is either unset or set to the expected "$parent.$p_key". Anything else wouldn't work anyway and is rejected as invalid configuration. That brings problems however. Rejecting invalid configuration seems fine at first: $ nmcli --offline connection add type infiniband infiniband.parent ib0 infiniband.p-key 0x8010 connection.interface-name xxx Error: Error writing connection: connection.interface-name: interface name of software infiniband device must be 'ib0.8010' or unset (instead it is 'xxx') However, when we modify the p-key, we also get an error message: $ nmcli --offline connection add type infiniband infiniband.parent ib0 infiniband.p-key 0x8010 connection.interface-name ib0.8010 | nmcli --offline connection modify infiniband.p-key 5 Error: Error writing connection: connection.interface-name: interface name of software infiniband device must be 'ib0.0005' or unset (instead it is 'ib0.8010') It's worse, because ifcfg-rh reader will mangle the PKEY_ID with |=0x8000 to set the full membership flag. That means, if you add a profile like $ nmcli --offline connection add type infiniband infiniband.parent ib0 infiniband.p-key 0x0010 connection.interface-name ib0.0010 it gets written to ifcfg-rh file. Then upon reload it's invalid (as the interface name mismatches). There are multiple solutions for this. For example, ifcfg-rh reader could also mangle the connection.interface-name, so that the overall result is valid. Or we could just not validate at all, and accept any bogus interface-name. With this patch instead we will just normalize the invalid configuration to make it right. $ nmcli --offline connection add type infiniband infiniband.parent ib0 infiniband.p-key 0x8010 connection.interface-name ib0.8010 | nmcli --offline connection modify infiniband.p-key 5 ... The downside is that this happens silently, so a user doesn't notice that configuration is ignored: $ nmcli --offline connection add type infiniband infiniband.parent ib0 infiniband.p-key 0x8010 connection.interface-name foo ... interface-name=ib0.8010 This approach still seems preferable, because setting "connection.interface-name" for infiniband profiles makes little sense, so what we care here is to avoid problems. --- src/libnm-core-impl/nm-connection.c | 39 ++++++++++++++++----- src/libnm-core-impl/nm-setting-infiniband.c | 14 ++++---- src/libnm-core-impl/tests/test-general.c | 30 +++++++++++++--- 3 files changed, 63 insertions(+), 20 deletions(-) diff --git a/src/libnm-core-impl/nm-connection.c b/src/libnm-core-impl/nm-connection.c index c3a51b6e0f..6ffc750ac9 100644 --- a/src/libnm-core-impl/nm-connection.c +++ b/src/libnm-core-impl/nm-connection.c @@ -1340,18 +1340,41 @@ _normalize_ip_config(NMConnection *self, GHashTable *parameters) } static gboolean -_normalize_infiniband_mtu(NMConnection *self) +_normalize_infiniband(NMConnection *self) { NMSettingInfiniband *s_infini = nm_connection_get_setting_infiniband(self); + gboolean changed = FALSE; + const char *interface_name; + int p_key; - if (!s_infini || nm_setting_infiniband_get_mtu(s_infini) <= NM_INFINIBAND_MAX_MTU - || !NM_IN_STRSET(nm_setting_infiniband_get_transport_mode(s_infini), - "datagram", - "connected")) + if (!s_infini) return FALSE; - g_object_set(s_infini, NM_SETTING_INFINIBAND_MTU, (guint) NM_INFINIBAND_MAX_MTU, NULL); - return TRUE; + if (nm_setting_infiniband_get_mtu(s_infini) > NM_INFINIBAND_MAX_MTU) { + if (NM_IN_STRSET(nm_setting_infiniband_get_transport_mode(s_infini), + "datagram", + "connected")) { + g_object_set(s_infini, NM_SETTING_INFINIBAND_MTU, (guint) NM_INFINIBAND_MAX_MTU, NULL); + changed = TRUE; + } + } + + if ((p_key = nm_setting_infiniband_get_p_key(s_infini)) != -1 + && (interface_name = nm_connection_get_interface_name(self))) { + const char *virtual_iface_name; + + virtual_iface_name = nm_setting_infiniband_get_virtual_interface_name(s_infini); + + if (!nm_streq0(interface_name, virtual_iface_name)) { + g_object_set(nm_connection_get_setting_connection(self), + NM_SETTING_CONNECTION_INTERFACE_NAME, + virtual_iface_name, + NULL); + changed = TRUE; + } + } + + return changed; } static gboolean @@ -2017,7 +2040,7 @@ _connection_normalize(NMConnection *connection, was_modified |= _normalize_invalid_slave_port_settings(connection); was_modified |= _normalize_ip_config(connection, parameters); was_modified |= _normalize_ethernet_link_neg(connection); - was_modified |= _normalize_infiniband_mtu(connection); + was_modified |= _normalize_infiniband(connection); was_modified |= _normalize_bond_mode(connection); was_modified |= _normalize_bond_options(connection); was_modified |= _normalize_wireless_mac_address_randomization(connection); diff --git a/src/libnm-core-impl/nm-setting-infiniband.c b/src/libnm-core-impl/nm-setting-infiniband.c index 7ba5720619..f5b5bb3d1f 100644 --- a/src/libnm-core-impl/nm-setting-infiniband.c +++ b/src/libnm-core-impl/nm-setting-infiniband.c @@ -181,8 +181,8 @@ nm_setting_infiniband_get_virtual_interface_name(NMSettingInfiniband *setting) static gboolean verify(NMSetting *setting, NMConnection *connection, GError **error) { - NMSettingConnection *s_con = NULL; - NMSettingInfinibandPrivate *priv = NM_SETTING_INFINIBAND_GET_PRIVATE(setting); + NMSettingConnection *s_con; + NMSettingInfinibandPrivate *priv = NM_SETTING_INFINIBAND_GET_PRIVATE(setting); if (priv->mac_address && !nm_utils_hwaddr_valid(priv->mac_address, INFINIBAND_ALEN)) { g_set_error_literal(error, @@ -251,8 +251,10 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) } } - if (connection) - s_con = nm_connection_get_setting_connection(connection); + /* *** errors above here should be always fatal, below NORMALIZABLE_ERROR *** */ + + s_con = connection ? nm_connection_get_setting_connection(connection) : NULL; + if (s_con) { const char *interface_name = nm_setting_connection_get_interface_name(s_con); @@ -287,13 +289,11 @@ verify(NMSetting *setting, NMConnection *connection, GError **error) "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_INTERFACE_NAME); - return FALSE; + return NM_SETTING_VERIFY_NORMALIZABLE_ERROR; } } } - /* *** errors above here should be always fatal, below NORMALIZABLE_ERROR *** */ - if (priv->mtu > NM_INFINIBAND_MAX_MTU) { /* Traditionally, MTU for "datagram" mode was limited to 2044 * and for "connected" mode it was 65520. diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index d3ff202780..78fb7e1f6e 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -6164,16 +6164,17 @@ test_connection_normalize_slave_type_2(void) } static void -test_connection_normalize_infiniband_mtu(void) +test_connection_normalize_infiniband(void) { gs_unref_object NMConnection *con = NULL; NMSettingInfiniband *s_infini; + NMSettingConnection *s_con; guint mtu_regular = nmtst_rand_select(2044, 2045, 65520); - con = nmtst_create_minimal_connection("test_connection_normalize_infiniband_mtu", + con = nmtst_create_minimal_connection("test_connection_normalize_infiniband", NULL, NM_SETTING_INFINIBAND_SETTING_NAME, - NULL); + &s_con); s_infini = nm_connection_get_setting_infiniband(con); g_object_set(s_infini, NM_SETTING_INFINIBAND_TRANSPORT_MODE, "connected", NULL); @@ -6221,6 +6222,25 @@ test_connection_normalize_infiniband_mtu(void) NM_CONNECTION_ERROR_INVALID_PROPERTY); nmtst_connection_normalize(con); g_assert_cmpint(65520, ==, nm_setting_infiniband_get_mtu(s_infini)); + + g_object_set(s_infini, + NM_SETTING_INFINIBAND_PARENT, + "foo", + NM_SETTING_INFINIBAND_P_KEY, + 0x005c, + NULL); + nmtst_assert_connection_verifies_without_normalization(con); + + g_object_set(s_con, NM_SETTING_CONNECTION_INTERFACE_NAME, "foo.005c", NULL); + nmtst_assert_connection_verifies_without_normalization(con); + + g_object_set(s_con, NM_SETTING_CONNECTION_INTERFACE_NAME, "foo", NULL); + nmtst_assert_connection_verifies_after_normalization(con, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY); + + nmtst_connection_normalize(con); + g_assert_cmpstr(nm_connection_get_interface_name(con), ==, "foo.005c"); } static void @@ -11519,8 +11539,8 @@ main(int argc, char **argv) test_connection_normalize_slave_type_1); g_test_add_func("/core/general/test_connection_normalize_slave_type_2", test_connection_normalize_slave_type_2); - g_test_add_func("/core/general/test_connection_normalize_infiniband_mtu", - test_connection_normalize_infiniband_mtu); + g_test_add_func("/core/general/test_connection_normalize_infiniband", + test_connection_normalize_infiniband); g_test_add_func("/core/general/test_connection_normalize_gateway_never_default", test_connection_normalize_gateway_never_default); g_test_add_func("/core/general/test_connection_normalize_may_fail", From fa05d1c1695aacd2d7144a71795463a1f793288a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2023 21:33:57 +0200 Subject: [PATCH 4/6] libnm: add nm_setting_infiniband_create_virtual_interface_name() helper --- src/libnm-core-impl/nm-setting-infiniband.c | 9 ++++++++- src/libnm-core-intern/nm-core-internal.h | 2 ++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/libnm-core-impl/nm-setting-infiniband.c b/src/libnm-core-impl/nm-setting-infiniband.c index f5b5bb3d1f..10e44b905c 100644 --- a/src/libnm-core-impl/nm-setting-infiniband.c +++ b/src/libnm-core-impl/nm-setting-infiniband.c @@ -144,6 +144,12 @@ nm_setting_infiniband_get_parent(NMSettingInfiniband *setting) return NM_SETTING_INFINIBAND_GET_PRIVATE(setting)->parent; } +char * +nm_setting_infiniband_create_virtual_interface_name(const char *parent, int p_key) +{ + return g_strdup_printf("%s.%04x", parent, p_key); +} + /** * nm_setting_infiniband_get_virtual_interface_name: * @setting: the #NMSettingInfiniband @@ -172,7 +178,8 @@ nm_setting_infiniband_get_virtual_interface_name(NMSettingInfiniband *setting) priv->virtual_iface_name_p_key = priv->p_key; priv->virtual_iface_name_parent_length = len; g_free(priv->virtual_iface_name); - priv->virtual_iface_name = g_strdup_printf("%s.%04x", priv->parent, priv->p_key); + priv->virtual_iface_name = + nm_setting_infiniband_create_virtual_interface_name(priv->parent, priv->p_key); } return priv->virtual_iface_name; diff --git a/src/libnm-core-intern/nm-core-internal.h b/src/libnm-core-intern/nm-core-internal.h index dbb5a7fa59..d2143fe87d 100644 --- a/src/libnm-core-intern/nm-core-internal.h +++ b/src/libnm-core-intern/nm-core-internal.h @@ -323,6 +323,8 @@ typedef gpointer (*NMUtilsCopyFunc)(gpointer); const char ** _nm_ip_address_get_attribute_names(const NMIPAddress *addr, gboolean sorted, guint *out_length); +char *nm_setting_infiniband_create_virtual_interface_name(const char *parent, int p_key); + #define NM_SETTING_WIRED_S390_OPTION_MAX_LEN 200u void _nm_setting_wired_clear_s390_options(NMSettingWired *setting); From 1009f1f11f991e41f856f2616c0972652f812a85 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2023 21:38:27 +0200 Subject: [PATCH 5/6] libnm: truncate too long interface name in nm_setting_infiniband_create_virtual_interface_name() This is the same what kernel does, when the parent name is so long that it would result in a too long overall name. We need that the result is still a valid interface name. --- src/libnm-core-impl/nm-setting-infiniband.c | 8 +++++- src/libnm-core-impl/tests/test-general.c | 32 +++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/libnm-core-impl/nm-setting-infiniband.c b/src/libnm-core-impl/nm-setting-infiniband.c index 10e44b905c..605def1fcd 100644 --- a/src/libnm-core-impl/nm-setting-infiniband.c +++ b/src/libnm-core-impl/nm-setting-infiniband.c @@ -10,6 +10,7 @@ #include #include +#include "libnm-platform/nmp-base.h" #include "nm-utils.h" #include "nm-utils-private.h" #include "nm-setting-private.h" @@ -147,7 +148,12 @@ nm_setting_infiniband_get_parent(NMSettingInfiniband *setting) char * nm_setting_infiniband_create_virtual_interface_name(const char *parent, int p_key) { - return g_strdup_printf("%s.%04x", parent, p_key); + char *s; + + s = g_strdup_printf("%s.%04x", parent, (guint) p_key); + if (strlen(s) >= NMP_IFNAMSIZ) + s[NMP_IFNAMSIZ - 1] = '\0'; + return s; } /** diff --git a/src/libnm-core-impl/tests/test-general.c b/src/libnm-core-impl/tests/test-general.c index 78fb7e1f6e..360e05e2a8 100644 --- a/src/libnm-core-impl/tests/test-general.c +++ b/src/libnm-core-impl/tests/test-general.c @@ -6241,6 +6241,38 @@ test_connection_normalize_infiniband(void) nmtst_connection_normalize(con); g_assert_cmpstr(nm_connection_get_interface_name(con), ==, "foo.005c"); + + g_object_set(s_infini, + NM_SETTING_INFINIBAND_PARENT, + "x234567890123", + NM_SETTING_INFINIBAND_P_KEY, + 0x005c, + NULL); + nmtst_assert_connection_verifies_after_normalization(con, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY); + + nmtst_connection_normalize(con); + g_assert_cmpstr(nm_connection_get_interface_name(con), ==, "x234567890123.0"); + +#define iface_name(parent, p_key, expected) \ + G_STMT_START \ + { \ + gs_free char *_s = nm_setting_infiniband_create_virtual_interface_name((parent), (p_key)); \ + \ + g_assert(nm_utils_ifname_valid_kernel(_s, NULL)); \ + g_assert_cmpstr(_s, ==, (expected)); \ + } \ + G_STMT_END + + iface_name("foo", 15, "foo.000f"); + iface_name("x23456789012345", 15, "x23456789012345"); + iface_name("x2345678901234", 15, "x2345678901234."); + iface_name("x234567890123", 15, "x234567890123.0"); + iface_name("x23456789012", 15, "x23456789012.00"); + iface_name("x2345678901", 15, "x2345678901.000"); + iface_name("x234567890", 15, "x234567890.000f"); + iface_name("x23456789", 15, "x23456789.000f"); } static void From 82f5bff882a58226c22df1b735d4b434af883102 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 24 May 2023 21:34:00 +0200 Subject: [PATCH 6/6] ifcfg-rh: adjust infiniband p-key for later normalization when writing to file --- .../plugins/ifcfg-rh/nms-ifcfg-rh-writer.c | 36 +++++++++++++++---- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c index acee4b215e..18ae69eaae 100644 --- a/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c +++ b/src/core/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c @@ -1023,7 +1023,10 @@ write_wireless_setting(NMConnection *connection, } static gboolean -write_infiniband_setting(NMConnection *connection, shvarFile *ifcfg, GError **error) +write_infiniband_setting(NMConnection *connection, + shvarFile *ifcfg, + char **out_interface_name, + GError **error) { NMSettingInfiniband *s_infiniband; const char *mac, *transport_mode, *parent; @@ -1051,12 +1054,28 @@ write_infiniband_setting(NMConnection *connection, shvarFile *ifcfg, GError **er p_key = nm_setting_infiniband_get_p_key(s_infiniband); if (p_key != -1) { + /* The reader normalizes KKEY_ID with |=0x8000. Also do that when + * writing the profile so that what we write, is consistent with what + * we would read. */ + p_key |= 0x8000; + svSetValueStr(ifcfg, "PKEY", "yes"); svSetValueInt64(ifcfg, "PKEY_ID", p_key); parent = nm_setting_infiniband_get_parent(s_infiniband); - if (parent) - svSetValueStr(ifcfg, "PHYSDEV", parent); + svSetValueStr(ifcfg, "PHYSDEV", parent); + + if (parent && nm_connection_get_interface_name(connection)) { + /* The connection.interface-name depends on the p-key. Also, + * nm_connection_normalize() will automatically adjust the + * interface-name to match the p-key. + * + * As we patched the p-key above, also anticipate that change, and + * don't write a DEVICE= to the file, which would we normalize + * differently, when reading it back. */ + *out_interface_name = + nm_setting_infiniband_create_virtual_interface_name(parent, p_key); + } } svSetValueStr(ifcfg, "TYPE", TYPE_INFINIBAND); @@ -2095,7 +2114,7 @@ write_dcb_setting(NMConnection *connection, shvarFile *ifcfg, GError **error) } static void -write_connection_setting(NMSettingConnection *s_con, shvarFile *ifcfg) +write_connection_setting(NMSettingConnection *s_con, shvarFile *ifcfg, const char *interface_name) { guint32 n, i; nm_auto_free_gstring GString *str = NULL; @@ -2112,7 +2131,9 @@ write_connection_setting(NMSettingConnection *s_con, shvarFile *ifcfg) svSetValueStr(ifcfg, "NAME", nm_setting_connection_get_id(s_con)); svSetValueStr(ifcfg, "UUID", nm_setting_connection_get_uuid(s_con)); svSetValueStr(ifcfg, "STABLE_ID", nm_setting_connection_get_stable_id(s_con)); - svSetValueStr(ifcfg, "DEVICE", nm_setting_connection_get_interface_name(s_con)); + svSetValueStr(ifcfg, + "DEVICE", + interface_name ?: nm_setting_connection_get_interface_name(s_con)); svSetValueBoolean(ifcfg, "ONBOOT", nm_setting_connection_get_autoconnect(s_con)); vint = nm_setting_connection_get_autoconnect_priority(s_con); @@ -3313,6 +3334,7 @@ do_write_construct(NMConnection *connection, nm_auto_shvar_file_close shvarFile *route_content_svformat = NULL; nm_auto_free_gstring GString *route_content = NULL; nm_auto_free_gstring GString *route6_content = NULL; + gs_free char *interface_name = NULL; nm_assert(NM_IS_CONNECTION(connection)); nm_assert(_nm_connection_verify(connection, NULL) == NM_SETTING_VERIFY_SUCCESS); @@ -3418,7 +3440,7 @@ do_write_construct(NMConnection *connection, if (!write_wireless_setting(connection, ifcfg, secrets, &no_8021x, error)) return FALSE; } else if (!strcmp(type, NM_SETTING_INFINIBAND_SETTING_NAME)) { - if (!write_infiniband_setting(connection, ifcfg, error)) + if (!write_infiniband_setting(connection, ifcfg, &interface_name, error)) return FALSE; } else if (!strcmp(type, NM_SETTING_BOND_SETTING_NAME)) { if (!write_bond_setting(connection, ifcfg, &wired, error)) @@ -3523,7 +3545,7 @@ do_write_construct(NMConnection *connection, write_ip_routing_rules(connection, ifcfg, route_ignore); - write_connection_setting(s_con, ifcfg); + write_connection_setting(s_con, ifcfg, interface_name); NM_SET_OUT(out_ifcfg, g_steal_pointer(&ifcfg)); NM_SET_OUT(out_blobs, g_steal_pointer(&blobs));