From d72a292005c75ad42d283879e0177da7079b021d Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Wed, 2 Feb 2022 09:39:23 +0100 Subject: [PATCH] device: fix assuming connections when platform-init arrives late When the NM_UNMANAGED_PLATFORM_INIT flag is cleared last in device_link_changed(), a recheck-assume is scheduled and then the device goes immediately to UNAVAILABLE. During the state transition, addresses and routes are removed from the interface. Then, recheck-assume finds that the device can be assumed but it's too late since the device was already deconfigured. This is a problem as the whole point of assuming a device is to activate a connection while leaving the device untouched. In the NMCI "dracut_NM_vlan_over_bridge and dracut_NM_vlan_over_bond" test, NM in real root tries to assume a vlan device that was activated in initrd. When the interface gets deconfigured in UNAVAILABLE, the connection to the NFS server breaks and the rootfs becomes inaccessible. The fix to this problem is to delay state transitions in device_link_changed() to a idle handler, so that recheck-assume can run before. Fixes-test: @dracut_NM_vlan_over_bridge Fixes-test: @dracut_NM_vlan_over_bond https://bugzilla.redhat.com/show_bug.cgi?id=2047302 --- src/core/devices/nm-device.c | 47 ++++++++++++++++++++++++------------ src/core/devices/nm-device.h | 2 +- src/core/nm-manager.c | 4 +-- 3 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/core/devices/nm-device.c b/src/core/devices/nm-device.c index bcf744f89..d30c57c84 100644 --- a/src/core/devices/nm-device.c +++ b/src/core/devices/nm-device.c @@ -825,7 +825,8 @@ static void _dev_ipac6_start(NMDevice *self); static void _dev_ipac6_set_state(NMDevice *self, NMDeviceIPState state); -static void _dev_unamanged_check_external_down(NMDevice *self, gboolean only_if_unmanaged); +static void +_dev_unmanaged_check_external_down(NMDevice *self, gboolean only_if_unmanaged, gboolean now); static void _dev_ipshared4_start(NMDevice *self); static void _dev_ipshared4_spawn_dnsmasq(NMDevice *self); @@ -3914,7 +3915,7 @@ _dev_l3_cfg_notify_cb(NML3Cfg *l3cfg, const NML3ConfigNotifyData *notify_data, N nmp_object_type_to_flags(NMP_OBJECT_TYPE_LINK) | nmp_object_type_to_flags(NMP_OBJECT_TYPE_IP4_ADDRESS) | nmp_object_type_to_flags(NMP_OBJECT_TYPE_IP6_ADDRESS))) - _dev_unamanged_check_external_down(self, TRUE); + _dev_unmanaged_check_external_down(self, TRUE, TRUE); if (NM_FLAGS_ANY(notify_data->platform_change_on_idle.obj_type_flags, nmp_object_type_to_flags(NMP_OBJECT_TYPE_IP4_ADDRESS) @@ -6047,7 +6048,7 @@ _dev_unmanaged_is_external_down(NMDevice *self, gboolean consider_can) } static void -_dev_unamanged_check_external_down(NMDevice *self, gboolean only_if_unmanaged) +_dev_unmanaged_check_external_down(NMDevice *self, gboolean only_if_unmanaged, gboolean now) { NMUnmanFlagOp ext_flags; @@ -6067,10 +6068,17 @@ _dev_unamanged_check_external_down(NMDevice *self, gboolean only_if_unmanaged) nm_device_queue_recheck_assume(self); } - nm_device_set_unmanaged_by_flags(self, - NM_UNMANAGED_EXTERNAL_DOWN, - ext_flags, - NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + if (now) { + nm_device_set_unmanaged_by_flags(self, + NM_UNMANAGED_EXTERNAL_DOWN, + ext_flags, + NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + } else { + nm_device_set_unmanaged_by_flags_queue(self, + NM_UNMANAGED_EXTERNAL_DOWN, + ext_flags, + NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); + } } void @@ -6522,11 +6530,16 @@ device_link_changed(gpointer user_data) } } + /* The assume check should happen before the device transitions to + * UNAVAILABLE, because in UNAVAILABLE we already clean up the IP + * configuration. Therefore, this function should never trigger a + * sync state transition. + */ nm_device_queue_recheck_assume(self); - nm_device_set_unmanaged_by_flags(self, NM_UNMANAGED_PLATFORM_INIT, FALSE, reason); + nm_device_set_unmanaged_by_flags_queue(self, NM_UNMANAGED_PLATFORM_INIT, FALSE, reason); } - _dev_unamanged_check_external_down(self, FALSE); + _dev_unmanaged_check_external_down(self, FALSE, FALSE); device_recheck_slave_status(self, pllink); @@ -6545,7 +6558,7 @@ device_link_changed(gpointer user_data) } if (update_unmanaged_specs) - nm_device_set_unmanaged_by_user_settings(self); + nm_device_set_unmanaged_by_user_settings(self, FALSE); if (got_hw_addr && !priv->up && nm_device_get_state(self) == NM_DEVICE_STATE_UNAVAILABLE) { /* @@ -14127,7 +14140,7 @@ nm_device_check_unrealized_device_managed(NMDevice *self) } void -nm_device_set_unmanaged_by_user_settings(NMDevice *self) +nm_device_set_unmanaged_by_user_settings(NMDevice *self, gboolean now) { gboolean unmanaged; @@ -14149,11 +14162,13 @@ nm_device_set_unmanaged_by_user_settings(NMDevice *self) self, nm_settings_get_unmanaged_specs(NM_DEVICE_GET_PRIVATE(self)->settings)); - nm_device_set_unmanaged_by_flags(self, - NM_UNMANAGED_USER_SETTINGS, - !!unmanaged, - unmanaged ? NM_DEVICE_STATE_REASON_NOW_UNMANAGED - : NM_DEVICE_STATE_REASON_NOW_MANAGED); + _set_unmanaged_flags(self, + NM_UNMANAGED_USER_SETTINGS, + !!unmanaged, + TRUE, + now, + unmanaged ? NM_DEVICE_STATE_REASON_NOW_UNMANAGED + : NM_DEVICE_STATE_REASON_NOW_MANAGED); } void diff --git a/src/core/devices/nm-device.h b/src/core/devices/nm-device.h index 3acd0cd21..cfcd4ade6 100644 --- a/src/core/devices/nm-device.h +++ b/src/core/devices/nm-device.h @@ -621,7 +621,7 @@ void nm_device_set_unmanaged_by_flags_queue(NMDevice *self, NMUnmanagedFlags flags, NMUnmanFlagOp set_op, NMDeviceStateReason reason); -void nm_device_set_unmanaged_by_user_settings(NMDevice *self); +void nm_device_set_unmanaged_by_user_settings(NMDevice *self, gboolean now); void nm_device_set_unmanaged_by_user_udev(NMDevice *self); void nm_device_set_unmanaged_by_user_conf(NMDevice *self); void nm_device_set_unmanaged_by_quitting(NMDevice *device); diff --git a/src/core/nm-manager.c b/src/core/nm-manager.c index 7714e4a2b..b440b2245 100644 --- a/src/core/nm-manager.c +++ b/src/core/nm-manager.c @@ -2266,7 +2266,7 @@ system_unmanaged_devices_changed_cb(NMSettings *settings, GParamSpec *pspec, gpo NMDevice *device; c_list_for_each_entry (device, &priv->devices_lst_head, devices_lst) - nm_device_set_unmanaged_by_user_settings(device); + nm_device_set_unmanaged_by_user_settings(device, TRUE); } static void @@ -3304,7 +3304,7 @@ add_device(NMManager *self, NMDevice *device, GError **error) type_desc = nm_device_get_type_desc(device); g_assert(type_desc); - nm_device_set_unmanaged_by_user_settings(device); + nm_device_set_unmanaged_by_user_settings(device, TRUE); nm_device_set_unmanaged_flags(device, NM_UNMANAGED_SLEEPING, manager_sleeping(self));