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: 9fcbc6b37d ('device: make attach_port() asynchronous')

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

Resolves: https://issues.redhat.com/browse/RHEL-58026
This commit is contained in:
Beniamino Galvani
2024-09-23 17:28:03 +02:00
parent 0567cfe367
commit a8329587c8

View File

@@ -126,12 +126,18 @@ typedef enum _nm_packed {
ADDR_METHOD_STATE_FAILED, ADDR_METHOD_STATE_FAILED,
} AddrMethodState; } AddrMethodState;
typedef enum {
PORT_STATE_NOT_ATTACHED,
PORT_STATE_ATTACHED,
PORT_STATE_ATTACHING,
} PortState;
typedef struct { typedef struct {
CList lst_port; CList lst_port;
NMDevice *port; NMDevice *port;
GCancellable *cancellable; GCancellable *cancellable;
gulong watch_id; gulong watch_id;
bool port_is_attached; PortState port_state;
bool configure; bool configure;
} PortInfo; } PortInfo;
@@ -6715,7 +6721,7 @@ attach_port_done(NMDevice *self, NMDevice *port, gboolean success)
if (!info) if (!info)
return; return;
info->port_is_attached = success; info->port_state = (success ? PORT_STATE_ATTACHED : PORT_STATE_NOT_ATTACHED);
nm_device_port_notify_attach_as_port(info->port, success); nm_device_port_notify_attach_as_port(info->port, success);
@@ -6778,7 +6784,7 @@ nm_device_controller_attach_port(NMDevice *self, NMDevice *port, NMConnection *c
if (!info) if (!info)
return; return;
if (info->port_is_attached) if (info->port_state == PORT_STATE_ATTACHED)
success = TRUE; success = TRUE;
else { else {
configure = (info->configure && connection != NULL); configure = (info->configure && connection != NULL);
@@ -6787,6 +6793,7 @@ nm_device_controller_attach_port(NMDevice *self, NMDevice *port, NMConnection *c
nm_clear_g_cancellable(&info->cancellable); nm_clear_g_cancellable(&info->cancellable);
info->cancellable = g_cancellable_new(); info->cancellable = g_cancellable_new();
info->port_state = PORT_STATE_ATTACHING;
success = NM_DEVICE_GET_CLASS(self)->attach_port(self, success = NM_DEVICE_GET_CLASS(self)->attach_port(self,
port, port,
connection, connection,
@@ -6841,6 +6848,7 @@ nm_device_controller_release_port(NMDevice *self,
PortInfo *info; PortInfo *info;
gs_unref_object NMDevice *self_free = NULL; gs_unref_object NMDevice *self_free = NULL;
gs_unref_object NMDevice *port_free = NULL; gs_unref_object NMDevice *port_free = NULL;
const char *port_state_str;
g_return_if_fail(NM_DEVICE(self)); g_return_if_fail(NM_DEVICE(self));
g_return_if_fail(NM_DEVICE(port)); g_return_if_fail(NM_DEVICE(port));
@@ -6852,11 +6860,20 @@ nm_device_controller_release_port(NMDevice *self,
info = find_port_info(self, port); info = find_port_info(self, port);
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, _LOGT(LOGD_CORE,
"controller: release one port " NM_HASH_OBFUSCATE_PTR_FMT "/%s %s%s", "controller: release one port " NM_HASH_OBFUSCATE_PTR_FMT "/%s %s%s",
NM_HASH_OBFUSCATE_PTR(port), NM_HASH_OBFUSCATE_PTR(port),
nm_device_get_iface(port), nm_device_get_iface(port),
!info ? "(not registered)" : (info->port_is_attached ? "(attached)" : "(not attached)"), !info ? "(not registered)" : port_state_str,
release_type == RELEASE_PORT_TYPE_CONFIG_FORCE release_type == RELEASE_PORT_TYPE_CONFIG_FORCE
? " (force-configure)" ? " (force-configure)"
: (release_type == RELEASE_PORT_TYPE_CONFIG ? " (configure)" : "(no-config)")); : (release_type == RELEASE_PORT_TYPE_CONFIG ? " (configure)" : "(no-config)"));
@@ -6872,7 +6889,7 @@ nm_device_controller_release_port(NMDevice *self,
nm_clear_g_cancellable(&info->cancellable); nm_clear_g_cancellable(&info->cancellable);
/* first, let subclasses handle the release ... */ /* first, let subclasses handle the release ... */
if (info->port_is_attached || nm_device_managed_type_is_external(port) if (info->port_state != PORT_STATE_NOT_ATTACHED || nm_device_managed_type_is_external(port)
|| release_type >= RELEASE_PORT_TYPE_CONFIG_FORCE) { || release_type >= RELEASE_PORT_TYPE_CONFIG_FORCE) {
NMTernary ret; NMTernary ret;