From b9ce5ae9d752eed1bb6dbc21065ee1746dff498a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 22 Jun 2020 17:04:06 +0200 Subject: ppp: fix taking control of link generated by kernel NetworkManager can't control the name of the PPP interface name created by pppd; so it has to wait for the interface to appear and then rename it. This happens in nm_device_take_over_link() called by nm-device-ppp.c:ppp_ifindex_set() when pppd tells NM the ifindex of the interface that was created. However, sometimes the initial interface name is already correct, for example when the connection.interface-name is ppp0 and this is the first PPP interface created. When this happens, nm_device_update_from_platform_link() is called on the NMDevicePPP and it sets the device ifindex. Later, when pppd notifies NM, nm_device_take_over_link() fails because the ifindex is already set: nm_device_take_over_link: assertion 'priv->ifindex <= 0' failed Make nm_device_take_over_link() more robust to cope with this situation. https://bugzilla.redhat.com/show_bug.cgi?id=1849386 --- src/devices/nm-device-ppp.c | 6 ++++- src/devices/nm-device-private.h | 2 +- src/devices/nm-device.c | 51 +++++++++++++++++++++++++++++++++-------- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/src/devices/nm-device-ppp.c b/src/devices/nm-device-ppp.c index 52784143a1..cbc871419a 100644 --- a/src/devices/nm-device-ppp.c +++ b/src/devices/nm-device-ppp.c @@ -71,9 +71,13 @@ ppp_ifindex_set (NMPPPManager *ppp_manager, gpointer user_data) { NMDevice *device = NM_DEVICE (user_data); + NMDevicePpp *self = NM_DEVICE_PPP (device); gs_free char *old_name = NULL; + gs_free_error GError *error = NULL; - if (!nm_device_take_over_link (device, ifindex, &old_name)) { + if (!nm_device_take_over_link (device, ifindex, &old_name, &error)) { + _LOGW (LOGD_DEVICE | LOGD_PPP, "could not take control of link %d: %s", + ifindex, error->message); nm_device_state_changed (device, NM_DEVICE_STATE_FAILED, NM_DEVICE_STATE_REASON_IP_CONFIG_UNAVAILABLE); return; diff --git a/src/devices/nm-device-private.h b/src/devices/nm-device-private.h index 4e260da2f3..cd72f3d712 100644 --- a/src/devices/nm-device-private.h +++ b/src/devices/nm-device-private.h @@ -62,7 +62,7 @@ gboolean nm_device_bring_up (NMDevice *self, gboolean wait, gboolean *no_firmwar void nm_device_take_down (NMDevice *self, gboolean block); -gboolean nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name); +gboolean nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name, GError **error); gboolean nm_device_hw_addr_set (NMDevice *device, const char *addr, diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index c46952de57..0c1ae60a55 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -1746,25 +1746,51 @@ nm_device_get_iface (NMDevice *self) return NM_DEVICE_GET_PRIVATE (self)->iface; } +/** + * nm_device_take_over_link: + * @self: the #NMDevice + * @ifindex: a ifindex + * @old_name: (transfer full): on return, the name of the old link, if + * the link was renamed + * @error: location to store error, or %NULL + * + * Given an existing link, move it under the control of a device. In + * particular, the link will be renamed to match the device name. If the + * link was renamed, the old name is returned in @old_name. + * + * Returns: %TRUE if the device took control of the link, %FALSE otherwise + */ gboolean -nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name) +nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name, GError **error) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); const NMPlatformLink *plink; NMPlatform *platform; - gboolean up, success = TRUE; - gs_free char *name = NULL; - - g_return_val_if_fail (priv->ifindex <= 0, FALSE); + nm_assert (ifindex > 0); NM_SET_OUT (old_name, NULL); + if ( priv->ifindex > 0 + && priv->ifindex != ifindex) { + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + "the device already has ifindex %d", + priv->ifindex); + return FALSE; + } + platform = nm_device_get_platform (self); plink = nm_platform_link_get (platform, ifindex); - if (!plink) + if (!plink) { + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + "link %d not found", ifindex); return FALSE; + } if (!nm_streq (plink->name, nm_device_get_iface (self))) { + gboolean up; + gboolean success; + gs_free char *name = NULL; + up = NM_FLAGS_HAS (plink->n_ifi_flags, IFF_UP); name = g_strdup (plink->name); @@ -1775,16 +1801,21 @@ nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name) if (up) nm_platform_link_set_up (platform, ifindex, NULL); - if (success) - NM_SET_OUT (old_name, g_steal_pointer (&name)); + if (!success) { + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + "failure renaming link %d", ifindex); + return FALSE; + } + + NM_SET_OUT (old_name, g_steal_pointer (&name)); } - if (success) { + if (priv->ifindex != ifindex) { priv->ifindex = ifindex; _notify (self, PROP_IFINDEX); } - return success; + return TRUE; } int -- cgit v1.2.1