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 a4fe16a426.

Affected versions were 1.42.2+ and 1.40.2+.

See-also: 05333c3602/f/rdma.ifup-ib (_75)

https://bugzilla.redhat.com/show_bug.cgi?id=2209164

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;

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, ==, 0x800c);
-    g_assert_cmpint(pkey, ==, 12);
 
     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);
+    const int                     TEST_IDX    = GPOINTER_TO_INT(test_data);
-    nmtst_auto_unlinkfile char   *testfile   = NULL;
+    nmtst_auto_unlinkfile char   *testfile    = NULL;
-    gs_unref_object NMConnection *connection = NULL;
+    gs_unref_object NMConnection *connection  = NULL;
-    gs_unref_object NMConnection *reread     = 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))
+    if (NM_IN_SET(TEST_IDX, 1, 2))
-        interface_name = "ib0.000c";
+        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);
+    _writer_new_connection_reread(connection,
-
+                                  TEST_SCRATCH_DIR,
-    nmtst_assert_connection_equals(connection, TRUE, reread, FALSE);
+                                  &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