From 6ddfee37516156882c8f322b0488f60cbe87bca3 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 19 Jan 2021 18:38:57 +0100 Subject: [PATCH 1/7] device: make 'nm_device_master_release_slaves' internal API https://bugzilla.redhat.com/show_bug.cgi?id=1870691 Signed-off-by: Antonio Cardace --- src/devices/nm-device-private.h | 2 ++ src/devices/nm-device.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device-private.h b/src/devices/nm-device-private.h index b165a549a8..8675a699a3 100644 --- a/src/devices/nm-device-private.h +++ b/src/devices/nm-device-private.h @@ -127,6 +127,8 @@ void nm_device_recheck_available_connections(NMDevice *device); void nm_device_master_check_slave_physical_port(NMDevice *self, NMDevice *slave, NMLogDomain log_domain); +void nm_device_master_release_slaves(NMDevice *self); + void nm_device_set_carrier(NMDevice *self, gboolean carrier); void nm_device_queue_recheck_assume(NMDevice *device); diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 574f8c6476..a45b6b9e04 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -6488,7 +6488,7 @@ nm_device_master_check_slave_physical_port(NMDevice *self, NMDevice *slave, NMLo } /* release all slaves */ -static void +void nm_device_master_release_slaves(NMDevice *self) { NMDevicePrivate * priv = NM_DEVICE_GET_PRIVATE(self); From 91766a6910cde49bdcca0bcdc902583b1009378f Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Thu, 21 Jan 2021 16:49:46 +0100 Subject: [PATCH 2/7] device: make devices 'external' when going to 'unmanaged' state This is the default state for new devices hence they should return back to this state when unmanaged. Signed-off-by: Antonio Cardace --- src/devices/nm-device.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index a45b6b9e04..8c5b9c52e6 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -16390,9 +16390,9 @@ _set_state_full(NMDevice *self, NMDeviceState state, NMDeviceStateReason reason, nm_device_hw_addr_reset(self, "unmanage"); set_nm_ipv6ll(self, FALSE); restore_ip6_properties(self); - break; } } + nm_device_sys_iface_state_set(self, NM_DEVICE_SYS_IFACE_STATE_EXTERNAL); break; case NM_DEVICE_STATE_UNAVAILABLE: if (old_state == NM_DEVICE_STATE_UNMANAGED) { From a4d37612c84de0718fd9629e1a4d06277d149c5a Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 19 Jan 2021 18:42:41 +0100 Subject: [PATCH 3/7] device: take over unmanaged devices when explicitely deactivated https://bugzilla.redhat.com/show_bug.cgi?id=1870691 Signed-off-by: Antonio Cardace --- src/devices/nm-device.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 8c5b9c52e6..e3f55240d0 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -13272,6 +13272,12 @@ nm_device_disconnect_active_connection(NMActiveConnection * active, if (NM_ACTIVE_CONNECTION(priv->act_request.obj) == active) { if (priv->state < NM_DEVICE_STATE_DEACTIVATING) { + /* When the user actively deactivates a profile, we set + * the sys-iface-state to managed so that we deconfigure/cleanup the interface. + * But for external connections that go down otherwise, we don't want to touch the interface. */ + if (nm_device_sys_iface_state_is_external(self)) + nm_device_sys_iface_state_set(self, NM_DEVICE_SYS_IFACE_STATE_MANAGED); + nm_device_state_changed(self, NM_DEVICE_STATE_DEACTIVATING, device_reason); } else { /* @active is the current ac of @self, but it's going down already. From d2d74f99a90cdcab5e0bdd518bf3b076d2cf48fa Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 19 Jan 2021 18:49:50 +0100 Subject: [PATCH 4/7] bond: release slaves prior to changing mode Bonds need to release all enslaved devices when the bond mode is to be changed. Also release slaves when they're external interfaces as it means the master it's now managed by NetworkManager. https://bugzilla.redhat.com/show_bug.cgi?id=1870691 Signed-off-by: Antonio Cardace --- src/devices/nm-device-bond.c | 8 ++++++++ src/devices/nm-device.c | 2 +- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index cf1abd33cf..5671a42ef5 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -331,6 +331,7 @@ apply_bonding_config(NMDeviceBond *self) NMSettingBond *s_bond; NMBondMode mode; const char * mode_str; + gs_free char * device_bond_mode = NULL; s_bond = nm_device_get_applied_setting(device, NM_TYPE_SETTING_BOND); g_return_val_if_fail(s_bond, FALSE); @@ -342,6 +343,13 @@ apply_bonding_config(NMDeviceBond *self) /* Set mode first, as some other options (e.g. arp_interval) are valid * only for certain modes. */ + device_bond_mode = nm_platform_sysctl_master_get_option(nm_device_get_platform(device), + nm_device_get_ifindex(device), + NM_SETTING_BOND_OPTION_MODE); + /* Need to release all slaves before we can change bond mode */ + if (!nm_streq0(device_bond_mode, mode_str)) + nm_device_master_release_slaves(device); + set_bond_attr_or_default(device, s_bond, NM_SETTING_BOND_OPTION_MODE); set_bond_arp_ip_targets(device, s_bond); diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index e3f55240d0..378fc20c33 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -4769,7 +4769,7 @@ nm_device_master_release_one_slave(NMDevice * self, nm_assert(slave == info->slave); /* first, let subclasses handle the release ... */ - if (info->slave_is_enslaved || force) + if (info->slave_is_enslaved || nm_device_sys_iface_state_is_external(slave) || force) NM_DEVICE_GET_CLASS(self)->release_slave(self, slave, configure); /* raise notifications about the release, including clearing is_enslaved. */ From 84dc705159bfba3d9676dfb727182c9ad99c7756 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Wed, 20 Jan 2021 19:25:51 +0100 Subject: [PATCH 5/7] device: add 'master_ifindex' field to NMDevice https://bugzilla.redhat.com/show_bug.cgi?id=1870691 Signed-off-by: Antonio Cardace --- src/devices/nm-device.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 378fc20c33..372cfa492b 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -642,6 +642,7 @@ typedef struct _NMDevicePrivate { /* master interface for bridge/bond/team slave */ NMDevice *master; gulong master_ready_id; + int master_ifindex; /* slave management */ CList slaves; /* list of SlaveInfo */ @@ -5116,6 +5117,8 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) return; } + priv->master_ifindex = plink->master; + if (priv->master) { if (plink->master > 0 && plink->master == nm_device_get_ifindex(priv->master)) { /* call add-slave again. We expect @self already to be added to @@ -6197,6 +6200,8 @@ nm_device_unrealize(NMDevice *self, gboolean remove_resources, GError **error) if (nm_clear_g_free(&priv->ip_iface_)) _notify(self, PROP_IP_IFACE); + priv->master_ifindex = 0; + _set_mtu(self, 0); if (priv->driver_version) { From 374cc9612686e36d58208aed8505846f3506d9d8 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 19 Jan 2021 18:57:58 +0100 Subject: [PATCH 6/7] core: add 'device-ifindex-changed' signal NMManager now emits a 'device-ifindex-changed' whenever a device ifindex gets changed. https://bugzilla.redhat.com/show_bug.cgi?id=1870691 Signed-off-by: Antonio Cardace --- src/devices/nm-device.c | 2 ++ src/nm-manager.c | 18 ++++++++++++++++++ src/nm-manager.h | 2 ++ 3 files changed, 22 insertions(+) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 372cfa492b..3b79cd85be 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2885,6 +2885,8 @@ _set_ifindex(NMDevice *self, int ifindex, gboolean is_ip_ifindex) if (!is_ip_ifindex) _notify(self, PROP_IFINDEX); + if (priv->manager) + nm_manager_emit_device_ifindex_changed(priv->manager, self); return TRUE; } diff --git a/src/nm-manager.c b/src/nm-manager.c index 550ac5e6ce..5e5f4e7ff2 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -102,6 +102,7 @@ enum { ACTIVE_CONNECTION_ADDED, ACTIVE_CONNECTION_REMOVED, CONFIGURE_QUIT, + DEVICE_IFINDEX_CHANGED, LAST_SIGNAL }; @@ -7622,6 +7623,12 @@ nm_manager_set_capability(NMManager *self, NMCapability cap) _notify(self, PROP_CAPABILITIES); } +void +nm_manager_emit_device_ifindex_changed(NMManager *self, NMDevice *device) +{ + g_signal_emit(self, signals[DEVICE_IFINDEX_CHANGED], 0, device); +} + /*****************************************************************************/ NM_DEFINE_SINGLETON_REGISTER(NMManager); @@ -8675,4 +8682,15 @@ nm_manager_class_init(NMManagerClass *manager_class) NULL, G_TYPE_NONE, 0); + + signals[DEVICE_IFINDEX_CHANGED] = g_signal_new(NM_MANAGER_DEVICE_IFINDEX_CHANGED, + G_OBJECT_CLASS_TYPE(object_class), + G_SIGNAL_RUN_FIRST, + 0, + NULL, + NULL, + NULL, + G_TYPE_NONE, + 1, + NM_TYPE_DEVICE); } diff --git a/src/nm-manager.h b/src/nm-manager.h index e3f71b8055..7a8b3a1e03 100644 --- a/src/nm-manager.h +++ b/src/nm-manager.h @@ -50,6 +50,7 @@ /* Signals */ #define NM_MANAGER_DEVICE_ADDED "device-added" #define NM_MANAGER_DEVICE_REMOVED "device-removed" +#define NM_MANAGER_DEVICE_IFINDEX_CHANGED "device-ifindex-changed" #define NM_MANAGER_USER_PERMISSIONS_CHANGED "user-permissions-changed" #define NM_MANAGER_ACTIVE_CONNECTION_ADDED "active-connection-added" @@ -180,6 +181,7 @@ gboolean nm_manager_deactivate_connection(NMManager * manager, GError ** error); void nm_manager_set_capability(NMManager *self, NMCapability cap); +void nm_manager_emit_device_ifindex_changed(NMManager *self, NMDevice *device); NMDevice *nm_manager_get_device(NMManager *self, const char *ifname, NMDeviceType device_type); gboolean nm_manager_remove_device(NMManager *self, const char *ifname, NMDeviceType device_type); From 136081957dfd7da799c351398bf14397a3d3e1a6 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Tue, 19 Jan 2021 19:06:38 +0100 Subject: [PATCH 7/7] device: fix bond-slave creation race-condition In some cases the enslavement event of a device gets processed by nm-platform before the master NMDevice is created, in such cases the enslavement information is simply lost by NM, to prevent this let NMDevice connect to the 'device-ifindex-changed' signal to be notified of when its master NMDevice appears so that eventually the NMDevices will be consistent with the kernel network state. https://bugzilla.redhat.com/show_bug.cgi?id=1870691 Signed-off-by: Antonio Cardace --- src/devices/nm-device.c | 49 +++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 3b79cd85be..c7c459f499 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -442,6 +442,7 @@ typedef struct _NMDevicePrivate { guint carrier_defer_id; guint carrier_wait_id; gulong config_changed_id; + gulong ifindex_changed_id; guint32 mtu; guint32 ip6_mtu; guint32 mtu_initial; @@ -762,6 +763,9 @@ static void concheck_update_state(NMDevice * self, static void sriov_op_cb(GError *error, gpointer user_data); +static void device_ifindex_changed_cb(NMManager *manager, NMDevice *device_changed, NMDevice *self); +static gboolean device_link_changed(NMDevice *self); + /*****************************************************************************/ static NM_UTILS_LOOKUP_STR_DEFINE( @@ -5107,7 +5111,7 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) g_return_if_fail(plink); if (plink->master <= 0) - return; + goto out; master = nm_manager_get_device_by_ifindex(NM_MANAGER_GET, plink->master); plink_master = nm_platform_link_get(nm_device_get_platform(self), plink->master); @@ -5116,7 +5120,7 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) if (master == NULL && plink_master && nm_streq0(plink_master->name, "ovs-system") && plink_master->type == NM_LINK_TYPE_OPENVSWITCH) { _LOGD(LOGD_DEVICE, "the device claimed by openvswitch"); - return; + goto out; } priv->master_ifindex = plink->master; @@ -5126,7 +5130,7 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) /* call add-slave again. We expect @self already to be added to * the master, but this also triggers a recheck-assume. */ nm_device_master_add_slave(priv->master, self, FALSE); - return; + goto out; } nm_device_master_release_one_slave(priv->master, @@ -5136,18 +5140,47 @@ device_recheck_slave_status(NMDevice *self, const NMPlatformLink *plink) NM_DEVICE_STATE_REASON_CONNECTION_ASSUMED); } - if (master && NM_DEVICE_GET_CLASS(master)->enslave_slave) + if (master && NM_DEVICE_GET_CLASS(master)->enslave_slave) { nm_device_master_add_slave(master, self, FALSE); - else if (master) { - _LOGI(LOGD_DEVICE, + goto out; + } + + if (master) { + _LOGD(LOGD_DEVICE, "enslaved to non-master-type device %s; ignoring", nm_device_get_iface(master)); } else { - _LOGW(LOGD_DEVICE, + _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); + } + return; + +out: + nm_clear_g_signal_handler(nm_device_get_manager(self), &priv->ifindex_changed_id); +} + +static void +device_ifindex_changed_cb(NMManager *manager, NMDevice *device_changed, NMDevice *self) +{ + NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE(self); + + if (priv->master_ifindex != nm_device_get_ifindex(device_changed)) + return; + + _LOGD(LOGD_DEVICE, + "master %s with ifindex %d appeared", + nm_device_get_iface(device_changed), + nm_device_get_ifindex(device_changed)); + if (!priv->device_link_changed_id) + priv->device_link_changed_id = g_idle_add((GSourceFunc) device_link_changed, self); } static void @@ -6244,6 +6277,7 @@ nm_device_unrealize(NMDevice *self, gboolean remove_resources, GError **error) _notify(self, PROP_CAPABILITIES); nm_clear_g_signal_handler(nm_config_get(), &priv->config_changed_id); + nm_clear_g_signal_handler(priv->manager, &priv->ifindex_changed_id); priv->real = FALSE; _notify(self, PROP_REAL); @@ -18262,6 +18296,7 @@ dispose(GObject *object) arp_cleanup(self); nm_clear_g_signal_handler(nm_config_get(), &priv->config_changed_id); + nm_clear_g_signal_handler(priv->manager, &priv->ifindex_changed_id); dispatcher_cleanup(self);