device: delay capturing permanent MAC address until UDEV is settled

The permanent MAC address of an NMDevice shall not change as
long as the device is realized. That is, we read it only once
and don't change it afterwards.

There are two issues that this commit tries to mitigate:

(1) users are advised to use UDEV to rename interfaces. As we lookup
  the permenent MAC address using ethtool (which uses the interface
  name), there is a race where we could read the permanent MAC
  address using the wrong interface name. We should wait until
  UDEV finished initializing the device and until the interface
  name is stable (see rh#1388286).
  This commit still cannot avoid the race of ethtool entirely. It only
  tries to avoid ethtool until UDEV has done its work. That is, until we
  expect the interface name no longer to change.

(2) some device types, don't have a permanent MAC address so we fall
  back to use the currently set address (fake). Again, users are advised
  to use UDEV to configure the MAC addresses on such software devices.
  Thus, we should not get the fake MAC address until UDEV initialized
  the device.

This patch actually doesn't solve the problem at all yet.
The reason is that a regular caller of nm_device_get_permanent_hw_address() can
not afford to wait until UDEV settled. Thus, any user who requests the
permanent MAC address before the link is initialized, runs into the
problems above.

In a next step, we shall revisit such calls to nm_device_get_permanent_hw_address()
and delay them until the link is initialized.

4 files changed, 76 insertions, 44 deletions

diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c
index 61d4c069f4..6a1f8034f7 100644
--- a/src/devices/nm-device-ethernet.c
+++ b/src/devices/nm-device-ethernet.c
@@ -1372,7 +1372,7 @@ complete_connection (NMDevice *device,
 		nm_connection_add_setting (connection, NM_SETTING (s_wired));
 	}
 
-	perm_hw_addr = nm_device_get_permanent_hw_address_full (device, &perm_hw_addr_is_fake);
+	perm_hw_addr = nm_device_get_permanent_hw_address_full (device, TRUE, &perm_hw_addr_is_fake);
 	if (perm_hw_addr && !perm_hw_addr_is_fake) {
 		setting_mac = nm_setting_wired_get_mac_address (s_wired);
 		if (setting_mac) {
@@ -1489,7 +1489,7 @@ update_connection (NMDevice *device, NMConnection *connection)
 	/* If the device reports a permanent address, use that for the MAC address
 	 * and the current MAC, if different, is the cloned MAC.
 	 */
-	perm_hw_addr = nm_device_get_permanent_hw_address_full (device, &perm_hw_addr_is_fake);
+	perm_hw_addr = nm_device_get_permanent_hw_address_full (device, TRUE, &perm_hw_addr_is_fake);
 	if (perm_hw_addr && !perm_hw_addr_is_fake) {
 		g_object_set (s_wired, NM_SETTING_WIRED_MAC_ADDRESS, perm_hw_addr, NULL);
 
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index fa69e723de..1a76f8c82d 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -1862,7 +1862,7 @@ device_link_changed (NMDevice *self)
 	had_hw_addr = (priv->hw_addr != NULL);
 	nm_device_update_hw_address (self);
 	got_hw_addr = (!had_hw_addr && priv->hw_addr);
-	nm_device_update_permanent_hw_address (self);
+	nm_device_update_permanent_hw_address (self, FALSE);
 
 	if (info.name[0] && strcmp (priv->iface, info.name) != 0) {
 		_LOGI (LOGD_DEVICE, "interface index %d renamed iface from '%s' to '%s'",
@@ -2329,7 +2329,7 @@ realize_start_setup (NMDevice *self, const NMPlatformLink *plink)
 
 	nm_device_update_hw_address (self);
 	nm_device_update_initial_hw_address (self);
-	nm_device_update_permanent_hw_address (self);
+	nm_device_update_permanent_hw_address (self, FALSE);
 
 	/* Note: initial hardware address must be read before calling get_ignore_carrier() */
 	config = nm_config_get ();
@@ -11693,12 +11693,14 @@ nm_device_update_initial_hw_address (NMDevice *self)
 }
 
 void
-nm_device_update_permanent_hw_address (NMDevice *self)
+nm_device_update_permanent_hw_address (NMDevice *self, gboolean force_freeze)
 {
 	NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self);
 	guint8 buf[NM_UTILS_HWADDR_LEN_MAX];
 	size_t len = 0;
 	gboolean success_read;
+	int ifindex;
+	const NMPlatformLink *pllink;
 
 	if (priv->hw_addr_perm) {
 		/* the permanent hardware address is only read once and not
@@ -11709,31 +11711,59 @@ nm_device_update_permanent_hw_address (NMDevice *self)
 		return;
 	}
 
-	if (priv->ifindex <= 0)
+	ifindex = priv->ifindex;
+	if (ifindex <= 0)
 		return;
 
-	if (!priv->hw_addr_len) {
-		nm_device_update_hw_address (self);
-		if (!priv->hw_addr_len)
+	/* the user is advised to configure stable MAC addresses for software devices via
+	 * UDEV. Thus, check whether the link is fully initalized. */
+	pllink = nm_platform_link_get (NM_PLATFORM_GET, ifindex);
+	if (   !pllink
+	    || !pllink->initialized) {
+		if (!force_freeze) {
+			/* we can afford to wait. Back off and leave the permanent MAC address
+			 * undecided for now. */
 			return;
+		}
+		/* try to refresh the link just to give UDEV a bit more time... */
+		nm_platform_link_refresh (NM_PLATFORM_GET, ifindex);
+		/* maybe the MAC address changed... */
+		nm_device_update_hw_address (self);
+	} else if (!priv->hw_addr_len)
+		nm_device_update_hw_address (self);
+
+	if (!priv->hw_addr_len) {
+		/* we need the current MAC address because we require the permanent MAC address
+		 * to have the same length as the current address.
+		 *
+		 * Abort if there is no current MAC address. */
+		return;
+	}
+
+	success_read = nm_platform_link_get_permanent_address (NM_PLATFORM_GET, ifindex, buf, &len);
+	if (success_read && priv->hw_addr_len == len) {
+		priv->hw_addr_perm_fake = FALSE;
+		priv->hw_addr_perm = nm_utils_hwaddr_ntoa (buf, len);
+		_LOGD (LOGD_DEVICE, "hw-addr: read permanent MAC address '%s'",
+		       priv->hw_addr_perm);
+		goto notify_and_out;
 	}
 
-	success_read = nm_platform_link_get_permanent_address (NM_PLATFORM_GET, priv->ifindex, buf, &len);
-	if (!success_read || len != priv->hw_addr_len) {
+	/* we failed to read a permanent MAC address, thus we use a fake address,
+	 * that is the current MAC address of the device.
+	 *
+	 * Note that the permanet MAC address of a NMDevice instance does not change
+	 * after being set once. Thus, we use now a fake address and stick to that
+	 * (until we unrealize the device). */
+	priv->hw_addr_perm_fake = TRUE;
+
+	/* We also persist our choice of the fake address to the device state
+	 * file to use the same address on restart of NetworkManager.
+	 * First, try to reload the address from the state file. */
+	{
 		gs_free NMConfigDeviceStateData *dev_state = NULL;
 
-		/* we failed to read a permanent MAC address, thus we use a fake address,
-		 * that is the current MAC address of the device.
-		 *
-		 * Note that the permanet MAC address of a NMDevice instance does not change
-		 * after being set once. Thus, we use now a fake address and stick to that
-		 * (until we unrealize the device). */
-		priv->hw_addr_perm_fake = TRUE;
-
-		/* We also persist our choice of the fake address to the device state
-		 * file to use the same address on restart of NetworkManager.
-		 * First, try to reload the address from the state file. */
-		dev_state = nm_config_device_state_load (nm_config_get (), priv->ifindex);
+		dev_state = nm_config_device_state_load (nm_config_get (), ifindex);
 		if (dev_state) {
 			if (   dev_state
 			    && dev_state->perm_hw_addr_fake
@@ -11746,25 +11776,19 @@ nm_device_update_permanent_hw_address (NMDevice *self)
 				       dev_state->perm_hw_addr_fake,
 				       priv->hw_addr);
 				priv->hw_addr_perm = nm_utils_hwaddr_ntoa (buf, priv->hw_addr_len);
-				goto out;
+				goto notify_and_out;
 			}
 		}
-
-		_LOGD (LOGD_PLATFORM | LOGD_ETHER, "hw-addr: %s (use current: %s)",
-		       success_read
-		           ? "read HW addr length of permanent MAC address differs"
-		           : "unable to read permanent MAC address",
-		       priv->hw_addr);
-		priv->hw_addr_perm = g_strdup (priv->hw_addr);
-		goto out;
 	}
 
-	priv->hw_addr_perm_fake = FALSE;
-	priv->hw_addr_perm = nm_utils_hwaddr_ntoa (buf, len);
-	_LOGD (LOGD_DEVICE, "hw-addr: read permanent MAC address '%s'",
-	       priv->hw_addr_perm);
+	_LOGD (LOGD_PLATFORM | LOGD_ETHER, "hw-addr: %s (use current: %s)",
+	       success_read
+	           ? "read HW addr length of permanent MAC address differs"
+	           : "unable to read permanent MAC address",
+	       priv->hw_addr);
+	priv->hw_addr_perm = g_strdup (priv->hw_addr);
 
-out:
+notify_and_out:
 	_notify (self, PROP_PERM_HW_ADDRESS);
 }
 
@@ -12099,13 +12123,22 @@ nm_device_hw_addr_reset (NMDevice *self, const char *detail)
 }
 
 const char *
-nm_device_get_permanent_hw_address_full (NMDevice *self, gboolean *out_is_fake)
+nm_device_get_permanent_hw_address_full (NMDevice *self, gboolean force_freeze, gboolean *out_is_fake)
 {
 	NMDevicePrivate *priv;
 
 	g_return_val_if_fail (NM_IS_DEVICE (self), NULL);
 
 	priv = NM_DEVICE_GET_PRIVATE (self);
+
+	if (   !priv->hw_addr_perm
+	    && force_freeze) {
+		/* somebody requests a permanent MAC address, but we don't have it set
+		 * yet. We cannot delay it any longer and try to get it without waiting
+		 * for UDEV. */
+		nm_device_update_permanent_hw_address (self, TRUE);
+	}
+
 	NM_SET_OUT (out_is_fake, priv->hw_addr_perm && priv->hw_addr_perm_fake);
 	return priv->hw_addr_perm;
 }
@@ -12113,9 +12146,7 @@ nm_device_get_permanent_hw_address_full (NMDevice *self, gboolean *out_is_fake)
 const char *
 nm_device_get_permanent_hw_address (NMDevice *self)
 {
-	g_return_val_if_fail (NM_IS_DEVICE (self), NULL);
-
-	return NM_DEVICE_GET_PRIVATE (self)->hw_addr_perm;
+	return nm_device_get_permanent_hw_address_full (self, TRUE, NULL);
 }
 
 const char *
@@ -12655,7 +12686,7 @@ get_property (GObject *object, guint prop_id,
 		const char *perm_hw_addr;
 		gboolean perm_hw_addr_is_fake;
 
-		perm_hw_addr = nm_device_get_permanent_hw_address_full (self, &perm_hw_addr_is_fake);
+		perm_hw_addr = nm_device_get_permanent_hw_address_full (self, FALSE, &perm_hw_addr_is_fake);
 		/* this property is exposed on D-Bus for NMDeviceEthernet and NMDeviceWifi. */
 		g_value_set_string (value, perm_hw_addr && !perm_hw_addr_is_fake ? perm_hw_addr : NULL);
 		break;
diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h
index 9d00fc6e10..5ad64adcb0 100644
--- a/src/devices/nm-device.h
+++ b/src/devices/nm-device.h
@@ -364,6 +364,7 @@ guint32         nm_device_get_ip6_route_metric  (NMDevice *dev);
 const char *    nm_device_get_hw_address        (NMDevice *dev);
 const char *    nm_device_get_permanent_hw_address (NMDevice *self);
 const char *    nm_device_get_permanent_hw_address_full (NMDevice *self,
+                                                         gboolean force_freeze,
                                                          gboolean *out_is_fake);
 const char *    nm_device_get_initial_hw_address (NMDevice *dev);
 
@@ -587,7 +588,7 @@ void nm_device_reactivate_ip6_config (NMDevice *device,
 
 gboolean nm_device_update_hw_address (NMDevice *self);
 void nm_device_update_initial_hw_address (NMDevice *self);
-void nm_device_update_permanent_hw_address (NMDevice *self);
+void nm_device_update_permanent_hw_address (NMDevice *self, gboolean force_freeze);
 void nm_device_update_dynamic_ip_setup (NMDevice *self);
 
 #endif /* __NETWORKMANAGER_DEVICE_H__ */
diff --git a/src/nm-manager.c b/src/nm-manager.c
index 34b6b2d3cd..df4d7a78e5 100644
--- a/src/nm-manager.c
+++ b/src/nm-manager.c
@@ -4666,7 +4666,7 @@ nm_manager_write_device_state (NMManager *self)
 				uuid = nm_connection_get_uuid (settings_connection);
 		}
 
-		perm_hw_addr_fake = nm_device_get_permanent_hw_address_full (device, &perm_hw_addr_is_fake);
+		perm_hw_addr_fake = nm_device_get_permanent_hw_address_full (device, FALSE, &perm_hw_addr_is_fake);
 		if (perm_hw_addr_fake && !perm_hw_addr_is_fake)
 			perm_hw_addr_fake = NULL;