From 95709ab6dcc2c8b021de5d1e2a12b4d63041d684 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 28 Jun 2019 17:31:40 +0200 Subject: [PATCH] device: fix crash releasing destroyed slave I encountered this on a WIP branch, but I think it can happen under regular conditions. I think there is no error condition here, and we should do nothing if we have no ifindex. [1561653068.2192] platform: signal: link removed: 1699: test1p mtu 1500 master 1698 arp 1 veth* init addrgenmode none addr D6:14:45:97:06:75 brd FF:FF:FF:FF:FF:FF driver veth rx:0,0 tx:38,5606 ... [1561653068.2617] device (test1): state change: activated -> unmanaged (reason 'unmanaged', sys-iface-state: 'removed') ... [1561653068.2635] device[0x564058c73750] (test1p): sys-iface-state: external -> removed [1561653068.2635] device[0x564058c73750] (test1p): unrealize (ifindex 1699) [1561653068.2636] device[0x564058c73750] (test1p): parent: clear [1561653068.2636] device[0x564058b98eb0] (vethbr): mtu: commit-mtu... [1561653068.2639] device[0x564058c73750] (test1p): unmanaged: flags set to [platform-init,!sleeping,!by-type,!user-explicit,!user-settings,!user-udev,!is-slave=0x10/0x1479/unmanaged/unrealized], set-unmanaged [platform-init=0x10]) [1561653068.2639] device[0x564058c73750] (test1p): unmanaged: flags set to [platform-init,!sleeping,!user-settings=0x10/0x51/unmanaged/unrealized], forget [parent,by-type,user-explicit,user-udev,external-down,is-slave=0x1c2c]) [1561653068.2639] device (test1p): state change: activated -> unmanaged (reason 'unmanaged', sys-iface-state: 'removed') [1561653068.2640] device[0x564058c73750] (test1p): deactivating device (reason 'unmanaged') [3] [1561653068.2640] device[0x564058c73750] (test1p): ip4-state: set to 0 (none) [1561653068.2640] device[0x564058c73750] (test1p): ip6-state: set to 0 (none) [1561653068.2640] device[0x564058c73750] (test1p): remove_pending_action (0): 'dhcp6' not pending (expected) [1561653068.2640] device[0x564058c73750] (test1p): remove_pending_action (0): 'autoconf6' not pending (expected) [1561653068.2640] rules-manager: sync [1561653068.2640] device[0x564058c73750] (test1p): set metered value 0 [1561653068.2641] device[0x564058c73750] (test1p): ip4-config: update (commit=1, new-config=(nil)) [1561653068.2641] device[0x564058c73750] (test1p): ip6-config: update (commit=1, new-config=(nil)) [1561653068.2644] device[0x564058b98eb0] (vethbr): slave test1p state change 100 (activated) -> 10 (unmanaged) [1561653068.2644] device[0x564058b98eb0] (vethbr): master: release one slave 0x564058c73750/test1p ((src/platform/nm-platform.c:2002)): assertion '' failed backtrace: ... #3 0x0000564057fb713e _nm_g_return_if_fail_warning (NetworkManager) #4 0x000056405808b37c release_slave (NetworkManager) #5 0x0000564058079aef nm_device_master_release_one_slave (NetworkManager) #6 0x00005640580844d7 slave_state_changed (NetworkManager) #7 0x00007efc24833fae ffi_call_unix64 (libffi.so.6) #8 0x00007efc2483396f ffi_call (libffi.so.6) #9 0x00007efc29b836e5 g_cclosure_marshal_generic (libgobject-2.0.so.0) #10 0x00007efc29b82c1d g_closure_invoke (libgobject-2.0.so.0) #11 0x00007efc29b96173 signal_emit_unlocked_R (libgobject-2.0.so.0) #12 0x00007efc29b9f29a g_signal_emit_valist (libgobject-2.0.so.0) #13 0x00007efc29b9f893 g_signal_emit (libgobject-2.0.so.0) #14 0x000056405807ab20 _set_state_full (NetworkManager) #15 0x000056405807d803 nm_device_unrealize (NetworkManager) #16 0x0000564057f6072c _platform_link_cb_idle (NetworkManager) #17 0x00007efc296a01db g_idle_dispatch (libglib-2.0.so.0) ... (cherry picked from commit 5b98f2fb0174c760396f3d2093c49d2756b23e9f) --- src/devices/nm-device-bond.c | 38 ++++++++++++++++++++----------- src/devices/nm-device-bridge.c | 10 +++++++- src/devices/team/nm-device-team.c | 13 +++++++++-- 3 files changed, 45 insertions(+), 16 deletions(-) diff --git a/src/devices/nm-device-bond.c b/src/devices/nm-device-bond.c index 790f00d44a..ef8fe1a519 100644 --- a/src/devices/nm-device-bond.c +++ b/src/devices/nm-device-bond.c @@ -413,6 +413,7 @@ release_slave (NMDevice *device, NMDeviceBond *self = NM_DEVICE_BOND (device); gboolean success; gs_free char *address = NULL; + int ifindex_slave; int ifindex; ifindex = nm_device_get_ifindex (device); @@ -420,22 +421,29 @@ release_slave (NMDevice *device, || !nm_platform_link_get (nm_device_get_platform (device), ifindex)) configure = FALSE; + ifindex_slave = nm_device_get_ip_ifindex (slave); + + if (ifindex_slave <= 0) + _LOGD (LOGD_TEAM, "bond slave %s is already released", nm_device_get_ip_iface (slave)); + if (configure) { /* When the last slave is released the bond MAC will be set to a random * value by kernel; remember the current one and restore it afterwards. */ address = g_strdup (nm_device_get_hw_address (device)); - success = nm_platform_link_release (nm_device_get_platform (device), - nm_device_get_ip_ifindex (device), - nm_device_get_ip_ifindex (slave)); + if (ifindex_slave > 0) { + success = nm_platform_link_release (nm_device_get_platform (device), + nm_device_get_ip_ifindex (device), + ifindex_slave); - if (success) { - _LOGI (LOGD_BOND, "released bond slave %s", - nm_device_get_ip_iface (slave)); - } else { - _LOGW (LOGD_BOND, "failed to release bond slave %s", - nm_device_get_ip_iface (slave)); + if (success) { + _LOGI (LOGD_BOND, "released bond slave %s", + nm_device_get_ip_iface (slave)); + } else { + _LOGW (LOGD_BOND, "failed to release bond slave %s", + nm_device_get_ip_iface (slave)); + } } nm_platform_process_events (nm_device_get_platform (device)); @@ -446,11 +454,15 @@ release_slave (NMDevice *device, * IFF_UP), so we must bring it back up here to ensure carrier changes and * other state is noticed by the now-released slave. */ - if (!nm_device_bring_up (slave, TRUE, NULL)) - _LOGW (LOGD_BOND, "released bond slave could not be brought up."); + if (ifindex_slave > 0) { + if (!nm_device_bring_up (slave, TRUE, NULL)) + _LOGW (LOGD_BOND, "released bond slave could not be brought up."); + } } else { - _LOGI (LOGD_BOND, "bond slave %s was released", - nm_device_get_ip_iface (slave)); + if (ifindex_slave > 0) { + _LOGI (LOGD_BOND, "bond slave %s was released", + nm_device_get_ip_iface (slave)); + } } } diff --git a/src/devices/nm-device-bridge.c b/src/devices/nm-device-bridge.c index e81bcd5d26..79e48e678b 100644 --- a/src/devices/nm-device-bridge.c +++ b/src/devices/nm-device-bridge.c @@ -625,6 +625,7 @@ release_slave (NMDevice *device, { NMDeviceBridge *self = NM_DEVICE_BRIDGE (device); gboolean success; + int ifindex_slave; int ifindex; ifindex = nm_device_get_ifindex (device); @@ -632,10 +633,17 @@ release_slave (NMDevice *device, || !nm_platform_link_get (nm_device_get_platform (device), ifindex)) configure = FALSE; + ifindex_slave = nm_device_get_ip_ifindex (slave); + + if (ifindex_slave <= 0) { + _LOGD (LOGD_TEAM, "bond slave %s is already released", nm_device_get_ip_iface (slave)); + return; + } + if (configure) { success = nm_platform_link_release (nm_device_get_platform (device), nm_device_get_ip_ifindex (device), - nm_device_get_ip_ifindex (slave)); + ifindex_slave); if (success) { _LOGI (LOGD_BRIDGE, "detached bridge port %s", diff --git a/src/devices/team/nm-device-team.c b/src/devices/team/nm-device-team.c index 7afd186064..876b48f821 100644 --- a/src/devices/team/nm-device-team.c +++ b/src/devices/team/nm-device-team.c @@ -774,6 +774,7 @@ release_slave (NMDevice *device, NMDeviceTeam *self = NM_DEVICE_TEAM (device); NMDeviceTeamPrivate *priv = NM_DEVICE_TEAM_GET_PRIVATE (self); gboolean success; + int ifindex_slave; int ifindex; ifindex = nm_device_get_ifindex (device); @@ -781,10 +782,17 @@ release_slave (NMDevice *device, || !nm_platform_link_get (nm_device_get_platform (device), ifindex)) configure = FALSE; + ifindex_slave = nm_device_get_ip_ifindex (slave); + + if (ifindex_slave <= 0) { + _LOGD (LOGD_TEAM, "team port %s is already released", nm_device_get_ip_iface (slave)); + return; + } + if (configure) { success = nm_platform_link_release (nm_device_get_platform (device), nm_device_get_ip_ifindex (device), - nm_device_get_ip_ifindex (slave)); + ifindex_slave); if (success) _LOGI (LOGD_TEAM, "released team port %s", nm_device_get_ip_iface (slave)); @@ -795,9 +803,10 @@ release_slave (NMDevice *device, * IFF_UP), so we must bring it back up here to ensure carrier changes and * other state is noticed by the now-released port. */ - if (!nm_device_bring_up (slave, TRUE, NULL)) + if (!nm_device_bring_up (slave, TRUE, NULL)) { _LOGW (LOGD_TEAM, "released team port %s could not be brought up", nm_device_get_ip_iface (slave)); + } nm_clear_g_source (&priv->teamd_read_timeout); priv->teamd_read_timeout = g_timeout_add_seconds (5,