From 1fe8166fc9fb93dc64992325e31e7611725aaeb2 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Thu, 2 Jun 2022 13:26:10 +0200 Subject: [PATCH] device: only deactivate when the master we've enslaved to goes away Sometimes weird things happen. Let dummy0 be an externally created device that has a master. We decide to activate a connection that has no master on it: active-connection[0x55ed7ba78400]: constructed (NMActRequest, version-id 4, type managed) device[0a458361f9fed8f5] (dummy0): sys-iface-state: external -> managed device[0a458361f9fed8f5] (dummy0): queue activation request waiting for currently active connection to disconnect device (dummy0): disconnecting for new activation request. device (dummy0): state change: activated -> deactivating (reason 'new-activation', sys-iface-state: 'managed') device (br0): master: release one slave 0a458361f9fed8f5/dummy0 (enslaved)(no-config) Note the "no-config" above. We'set priv->master = NULL, but didn't communicate the change to the platform. I believe this is not good. device (br0): bridge port dummy0 was detached device (dummy0): released from master device br0 active-connection[0x55ed7ba782e0]: set state deactivating (was activated) device (dummy0): ip4: set state none (was done, reason: ip-state-clear) device (dummy0): ip6: set state none (was done, reason: ip-state-clear) device (dummy0): state change: deactivating -> disconnected (reason 'new-activation', sys-iface-state: 'managed') platform: (dummy0) emit signal link-changed changed: 102: dummy0 mtu 1500 master 101 arp 1 dummy* init addrgenmode none addr EA:8D:DD:DF:1F:B7 brd FF:FF:FF:FF:FF:FF driver dummy rx:0,0 tx:39,4746 Now the platform sent us a new link, the "master" property is still set. device[0a458361f9fed8f5] (dummy0): queued link change for ifindex 102 device[0a458361f9fed8f5] (dummy0): deactivating device (reason 'new-activation') [60] device (dummy0): ip: set (combined) state none (was done, reason: ip-state-clear) config: device-state: write #102 (/run/NetworkManager/devices/102); managed=managed, perm-hw-addr-fake=EA:8D:DD:DF:1F:B7, route-metric-default=0-0 active-connection[0x55ed7ba782e0]: set state deactivated (was deactivating) active-connection[0x55ed7ba782e0]: check-master-ready: already signalled (state deactivated, master 0x55ed7ba781c0 is in state activated) device (dummy0): Activation: starting connection 'dummy1' (ec6fca51-84e6-4a5b-a297-f602252c9f69) device[0a458361f9fed8f5] (dummy0): activation-stage: schedule activate_stage1_device_prepare l3cfg[ae290b5c1f585d6c,ifindex=102]: emit signal (platform-change-on-idle, obj-type-flags=0x2a) device (br0): master: add one slave 0a458361f9fed8f5/dummy0 Amidst the new activation we're processing the netlink message we got. We set priv->master back, effectively nullifying the release above. device (dummy0): state change: disconnected -> prepare (reason 'none', sys-iface-state: 'managed') device[0a458361f9fed8f5] (dummy0): add_pending_action (2): 'in-state-change' active-connection[0x55ed7ba78400]: set state activating (was unknown) manager: NetworkManager state is now CONNECTING active-connection[0x55ed7ba78400]: check-master-ready: not signalling (state activating, no master) device[8fff58d61c7686ce] (br0): slave dummy0 state change 30 (disconnected) -> 40 (prepare) device[0a458361f9fed8f5] (dummy0): remove_pending_action (1): 'in-state-change' device (br0): master: release one slave 0a458361f9fed8f5/dummy0 (not enslaved) (force-configure) platform: (dummy0) link: releasing 102 from master 'br0' (101) device (br0): detached bridge port dummy0 Now stage1 cleans the device up, removing it from the master. device[0a458361f9fed8f5] (dummy0): Activation: connection 'dummy1' master deactivated device (dummy0): ip4: set state none (was pending, reason: ip-state-clear) device (dummy0): ip6: set state none (was pending, reason: ip-state-clear) device[0a458361f9fed8f5] (dummy0): add_pending_action (2): 'queued-state-change-deactivating' We decide to deal with this by enqueuing a deactivation. That is not great -- we shouldn't even have had this master! This patch takes the deactivation path only if we were willingly enslaved to the master in question. --- src/core/devices/nm-device.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 8a1c80f313..753e320066 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -7920,6 +7920,9 @@ nm_device_slave_notify_release(NMDevice *self, NMDeviceStateReason reason) g_return_if_fail(priv->master); + if (!priv->is_enslaved) + return; + if (priv->state > NM_DEVICE_STATE_DISCONNECTED && priv->state <= NM_DEVICE_STATE_ACTIVATED) { switch (nm_device_state_reason_check(reason)) { case NM_DEVICE_STATE_REASON_DEPENDENCY_FAILED: @@ -7949,14 +7952,12 @@ nm_device_slave_notify_release(NMDevice *self, NMDeviceStateReason reason) } else _LOGI(LOGD_DEVICE, "released from master device %s", nm_device_get_iface(priv->master)); - if (priv->is_enslaved) { - priv->is_enslaved = FALSE; + priv->is_enslaved = FALSE; - _notify(self, PROP_MASTER); + _notify(self, PROP_MASTER); - nm_clear_pointer(&NM_DEVICE_GET_PRIVATE(priv->master)->ports_variant, g_variant_unref); - nm_gobject_notify_together(priv->master, PROP_PORTS, PROP_SLAVES); - } + nm_clear_pointer(&NM_DEVICE_GET_PRIVATE(priv->master)->ports_variant, g_variant_unref); + nm_gobject_notify_together(priv->master, PROP_PORTS, PROP_SLAVES); } /**