From 83c32e9f172382dcf153c640af715e70a7245bee Mon Sep 17 00:00:00 2001
From: Beniamino Galvani <bgalvani@redhat.com>
Date: Mon, 23 Sep 2024 17:28:03 +0200
Subject: [PATCH] device: fix bug when deactivating port connections
 asynchronously

When the attach_port()/detach_port() methods do not return immediately
(currently, only for OVS ports), the following situation can arise:

 - nm_device_controller_attach_port() starts the attachment by sending
   the command to ovsdb. Note that here we don't set
   `PortInfo->port_is_attached` to TRUE yet; that happens only after
   the asynchronous command returns;

 - the activation of the port gets interrupted because the connection
   is deleted;

 - the port device enters the deactivating state, triggering function
   port_state_changed()

 - the function calls nm_device_controller_release_port() which checks
   whether the port is already attached; since
   `PortInfo->port_is_attached` is not set yet, it assumes the port
   doesn't need to be detached;

 - in the meantime, the ovsdb operation succeeds. As a consequence,
   the kernel link is created even if the connection no longer exists.

Fix this by turning `port_is_attached` into a tri-state variable that
also tracks when the port is attaching. When it is, we need to perform
an explicit detach during deactivation.

Fixes: 9fcbc6b37dec ('device: make attach_port() asynchronous')

https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/2043

Resolves: https://issues.redhat.com/browse/RHEL-58026
(cherry picked from commit a8329587c8bdd53e2bc4513a4e82529727cfa5ef)
(cherry picked from commit d809ca6db24b5145fcc1857b962afb7ae17d07a5)
(cherry picked from commit ca6ca684b21235f706b02cee42075f2ee3cb1795)
---
 src/core/devices/nm-device.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c
index 575dd2a8ff..a7b5e903cd 100644
--- a/src/core/devices/nm-device.c
+++ b/src/core/devices/nm-device.c
@@ -123,12 +123,18 @@ typedef enum _nm_packed {
     ADDR_METHOD_STATE_FAILED,
 } AddrMethodState;
 
+typedef enum {
+    PORT_STATE_NOT_ATTACHED,
+    PORT_STATE_ATTACHED,
+    PORT_STATE_ATTACHING,
+} PortState;
+
 typedef struct {
     CList         lst_slave;
     NMDevice     *slave;
     GCancellable *cancellable;
     gulong        watch_id;
-    bool          slave_is_enslaved;
+    PortState     port_state;
     bool          configure;
 } SlaveInfo;
 
@@ -6597,7 +6603,7 @@ attach_port_done(NMDevice *self, NMDevice *slave, gboolean success)
     if (!info)
         return;
 
-    info->slave_is_enslaved = success;
+    info->port_state = (success ? PORT_STATE_ATTACHED : PORT_STATE_NOT_ATTACHED);
 
     nm_device_slave_notify_enslave(info->slave, success);
 
@@ -6660,7 +6666,7 @@ nm_device_master_enslave_slave(NMDevice *self, NMDevice *slave, NMConnection *co
     if (!info)
         return;
 
-    if (info->slave_is_enslaved)
+    if (info->port_state == PORT_STATE_ATTACHED)
         success = TRUE;
     else {
         configure = (info->configure && connection != NULL);
@@ -6669,6 +6675,7 @@ nm_device_master_enslave_slave(NMDevice *self, NMDevice *slave, NMConnection *co
 
         nm_clear_g_cancellable(&info->cancellable);
         info->cancellable = g_cancellable_new();
+        info->port_state  = PORT_STATE_ATTACHING;
         success           = NM_DEVICE_GET_CLASS(self)->attach_port(self,
                                                          slave,
                                                          connection,
@@ -6723,6 +6730,7 @@ nm_device_master_release_slave(NMDevice           *self,
     SlaveInfo                *info;
     gs_unref_object NMDevice *self_free  = NULL;
     gs_unref_object NMDevice *slave_free = NULL;
+    const char               *port_state_str;
 
     g_return_if_fail(NM_DEVICE(self));
     g_return_if_fail(NM_DEVICE(slave));
@@ -6734,11 +6742,20 @@ nm_device_master_release_slave(NMDevice           *self,
 
     info = find_slave_info(self, slave);
 
+    if (info->port_state == PORT_STATE_ATTACHED)
+        port_state_str = "(attached)";
+    else if (info->port_state == PORT_STATE_NOT_ATTACHED)
+        port_state_str = "(not attached)";
+    else {
+        nm_assert(info->port_state == PORT_STATE_ATTACHING);
+        port_state_str = "(attaching)";
+    }
+
     _LOGT(LOGD_CORE,
           "master: release one slave " NM_HASH_OBFUSCATE_PTR_FMT "/%s %s%s",
           NM_HASH_OBFUSCATE_PTR(slave),
           nm_device_get_iface(slave),
-          !info ? "(not registered)" : (info->slave_is_enslaved ? "(enslaved)" : "(not enslaved)"),
+          !info ? "(not registered)" : port_state_str,
           release_type == RELEASE_SLAVE_TYPE_CONFIG_FORCE
               ? " (force-configure)"
               : (release_type == RELEASE_SLAVE_TYPE_CONFIG ? " (configure)" : "(no-config)"));
@@ -6754,7 +6771,7 @@ nm_device_master_release_slave(NMDevice           *self,
     nm_clear_g_cancellable(&info->cancellable);
 
     /* first, let subclasses handle the release ... */
-    if (info->slave_is_enslaved || nm_device_sys_iface_state_is_external(slave)
+    if (info->port_state != PORT_STATE_NOT_ATTACHED || nm_device_sys_iface_state_is_external(slave)
         || release_type >= RELEASE_SLAVE_TYPE_CONFIG_FORCE) {
         NMTernary ret;