From a49aede4b80e8b3c63f8c8b144de8e68da04e217 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 30 Sep 2022 22:18:12 +0200 Subject: [PATCH 1/4] checkpoint: move a log message a little lower It's happier there. --- src/core/nm-checkpoint.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/nm-checkpoint.c b/src/core/nm-checkpoint.c index 1566733289..cd0e17fa87 100644 --- a/src/core/nm-checkpoint.c +++ b/src/core/nm-checkpoint.c @@ -416,10 +416,10 @@ activate: } } else { /* The device was initially disconnected, deactivate any existing connection */ - _LOGD("rollback: disconnecting device"); if (nm_device_get_state(device) > NM_DEVICE_STATE_DISCONNECTED && nm_device_get_state(device) < NM_DEVICE_STATE_DEACTIVATING) { + _LOGD("rollback: disconnecting device"); nm_device_state_changed(device, NM_DEVICE_STATE_DEACTIVATING, NM_DEVICE_STATE_REASON_USER_REQUESTED); From bf60fd5acc22ac73a911796d2f9bc33e46b1ba5d Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 30 Sep 2022 17:41:26 +0200 Subject: [PATCH 2/4] device: fix recheck slave logic Since commit a1de6810df46 ('device: don't ignore external slave removals') we don't leave device_recheck_slave_status() on un-eslaving (that is plink->master = 0) early enough. This results in hooking of NM_MANAGER_DEVICE_IFINDEX_CHANGED even when we're not actually waiting for any master device to come up, accompanied by a messed up log entry: device[3fa7cfc200be4e84] (portXc): enslaved to unknown device 0 (??) We also log nonsense when we see any device's link being removed: device[a9a4b65bde851bcf] (br0): ifindex: set ifindex 0 (old-l3cfg: 05c6a4409f84d9d2) device[45d34e95fb71cce0] (portXa): master br0 with ifindex 0 appeared We don't do further damage afterwards, so this is purely a cosmetic annoyance. --- src/core/devices/nm-device.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index 3a7009e6c2..d4d21a408a 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -6573,27 +6573,30 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } - if (master && NM_DEVICE_GET_CLASS(master)->attach_port) { - nm_device_master_add_slave(master, self, FALSE); + if (master) { + if (NM_DEVICE_GET_CLASS(master)->attach_port) { + nm_device_master_add_slave(master, self, FALSE); + } else { + _LOGD(LOGD_DEVICE, + "enslaved to non-master-type device %s; ignoring", + nm_device_get_iface(master)); + } goto out; } - if (master) { - _LOGD(LOGD_DEVICE, - "enslaved to non-master-type device %s; ignoring", - nm_device_get_iface(master)); - } else { + if (plink->master) { _LOGD(LOGD_DEVICE, "enslaved to unknown device %d (%s%s%s)", plink->master, NM_PRINT_FMT_QUOTED(plink_master, "\"", plink_master->name, "\"", "??")); + if (!priv->ifindex_changed_id) { + priv->ifindex_changed_id = g_signal_connect(nm_device_get_manager(self), + NM_MANAGER_DEVICE_IFINDEX_CHANGED, + G_CALLBACK(device_ifindex_changed_cb), + self); + } } - if (!priv->ifindex_changed_id) { - priv->ifindex_changed_id = g_signal_connect(nm_device_get_manager(self), - NM_MANAGER_DEVICE_IFINDEX_CHANGED, - G_CALLBACK(device_ifindex_changed_cb), - self); - } + return; out: From 455dbfce6e2ccfae0f0a03d9df016e6a2eb0fe2a Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 30 Sep 2022 17:49:32 +0200 Subject: [PATCH 3/4] device: assert we're not waiting on a nil master If we're notified of a master appearing, make sure there's actually an ifindex we're waiting for. Triger an assertion failure if that is not the case, cause that's pretty messed up. --- src/core/devices/nm-device.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index d4d21a408a..ec8ea32831 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -6608,6 +6608,8 @@ device_ifindex_changed_cb(NMManager *manager, NMDevice *device_changed, NMDevice { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); + g_return_if_fail(priv->master_ifindex > 0); + if (priv->master_ifindex != nm_device_get_ifindex(device_changed)) return; From dc254f90e2b306700a0b81f7194e9b0438c62f4c Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Fri, 30 Sep 2022 22:40:03 +0200 Subject: [PATCH 4/4] bond,bridge,team: use uuid for con.master when generating connection If we're generating a connection for an externally configured slave, refer the master by the UUID instead of the device name. This doesn't matter most of the time. However, on a checkpoint restore we need to make sure that a connection that is unambiguously the original master is up. Otherwise it could happen that a different connection was activated on the same master device and the slaves being restored don't agree on which master connection to bring up. I can't think of any thing that would rely on this but I've been wrong about more serious things before. Fixes-test: @libnm_snapshot_reattach_unmanaged_ports_to_bridge https://bugzilla.redhat.com/show_bug.cgi?id=2125615 --- src/core/devices/nm-device-bond.c | 9 +++++---- src/core/devices/nm-device-bridge.c | 9 +++++---- src/core/devices/team/nm-device-team.c | 9 +++++---- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/core/devices/nm-device-bond.c b/src/core/devices/nm-device-bond.c index dc5e1d5c7e..2c8d43872d 100644 --- a/src/core/devices/nm-device-bond.c +++ b/src/core/devices/nm-device-bond.c @@ -224,9 +224,10 @@ controller_update_port_connection(NMDevice *self, GError **error) { NMSettingBondPort *s_port; - int ifindex_port = nm_device_get_ifindex(port); - uint queue_id = NM_BOND_PORT_QUEUE_ID_DEF; - gs_free char *queue_id_str = NULL; + int ifindex_port = nm_device_get_ifindex(port); + NMConnection *applied_connection = nm_device_get_applied_connection(self); + uint queue_id = NM_BOND_PORT_QUEUE_ID_DEF; + gs_free char *queue_id_str = NULL; g_return_val_if_fail(ifindex_port > 0, FALSE); @@ -243,7 +244,7 @@ controller_update_port_connection(NMDevice *self, g_object_set(nm_connection_get_setting_connection(connection), NM_SETTING_CONNECTION_MASTER, - nm_device_get_iface(self), + nm_connection_get_uuid(applied_connection), NM_SETTING_CONNECTION_SLAVE_TYPE, NM_SETTING_BOND_SETTING_NAME, NULL); diff --git a/src/core/devices/nm-device-bridge.c b/src/core/devices/nm-device-bridge.c index 31cf361e8e..d8f1337058 100644 --- a/src/core/devices/nm-device-bridge.c +++ b/src/core/devices/nm-device-bridge.c @@ -679,9 +679,10 @@ master_update_slave_connection(NMDevice *device, NMDeviceBridge *self = NM_DEVICE_BRIDGE(device); NMSettingConnection *s_con; NMSettingBridgePort *s_port; - int ifindex_slave = nm_device_get_ifindex(slave); - const char *iface = nm_device_get_iface(device); - const Option *option; + int ifindex_slave = nm_device_get_ifindex(slave); + NMConnection *applied_connection = nm_device_get_applied_connection(device); + + const Option *option; g_return_val_if_fail(ifindex_slave > 0, FALSE); @@ -717,7 +718,7 @@ master_update_slave_connection(NMDevice *device, g_object_set(s_con, NM_SETTING_CONNECTION_MASTER, - iface, + nm_connection_get_uuid(applied_connection), NM_SETTING_CONNECTION_SLAVE_TYPE, NM_SETTING_BRIDGE_SETTING_NAME, NULL); diff --git a/src/core/devices/team/nm-device-team.c b/src/core/devices/team/nm-device-team.c index d90b766bdb..49ce953df1 100644 --- a/src/core/devices/team/nm-device-team.c +++ b/src/core/devices/team/nm-device-team.c @@ -258,9 +258,10 @@ master_update_slave_connection(NMDevice *device, gs_free_error GError *connect_error = NULL; int err = 0; struct teamdctl *tdc; - const char *team_port_config = NULL; - const char *iface = nm_device_get_iface(device); - const char *iface_slave = nm_device_get_iface(slave); + const char *team_port_config = NULL; + const char *iface = nm_device_get_iface(device); + const char *iface_slave = nm_device_get_iface(slave); + NMConnection *applied_connection = nm_device_get_applied_connection(device); tdc = _tdc_connect_new(self, iface, &connect_error); if (!tdc) { @@ -299,7 +300,7 @@ master_update_slave_connection(NMDevice *device, g_object_set(nm_connection_get_setting_connection(connection), NM_SETTING_CONNECTION_MASTER, - iface, + nm_connection_get_uuid(applied_connection), NM_SETTING_CONNECTION_SLAVE_TYPE, NM_SETTING_TEAM_SETTING_NAME, NULL);