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

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