device: avoid taking down link to change MAC addressth/device-set-mac-addr-no-down

A lot of drivers actually support changing the MAC address of a link
without taking it down.

Taking down a link is very bad, because kernel will remove routes
and IPv6 addresses.

For example, if the user used a dispatcher script to add routes,
these will be lost. Note that we may change the MAC address of a
device any time. For example, a VLAN device watches the parent's
MAC address and configures it (with a logging message "parent hardware
address changed to ...").

Try first whether we can change the MAC address without taking the
link down. Only if that fails, retry with taking it down first.

https://bugzilla.redhat.com/show_bug.cgi?id=1639274

2 files changed, 71 insertions, 25 deletions

diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index ff082b3d84..9f76a962b0 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -15385,7 +15385,8 @@ _hw_addr_set (NMDevice *self,
 	NMPlatformError plerr;
 	guint8 addr_bytes[NM_UTILS_HWADDR_LEN_MAX];
 	gsize addr_len;
-	gboolean was_up;
+	gboolean was_taken_down;
+	gboolean retry_down;
 
 	nm_assert (NM_IS_DEVICE (self));
 	nm_assert (addr);
@@ -15408,21 +15409,31 @@ _hw_addr_set (NMDevice *self,
 
 	_LOGT (LOGD_DEVICE, "set-hw-addr: setting MAC address to '%s' (%s, %s)...", addr, operation, detail);
 
-	was_up = nm_device_is_up (self);
-	if (was_up) {
-		/* Can't change MAC address while device is up */
-		nm_device_take_down (self, FALSE);
-	}
+	was_taken_down = FALSE;
 
+again:
 	plerr = nm_platform_link_set_address (nm_device_get_platform (self), nm_device_get_ip_ifindex (self), addr_bytes, addr_len);
 	success = (plerr == NM_PLATFORM_ERROR_SUCCESS);
-	if (success) {
+	if (!success) {
+		retry_down =    !was_taken_down
+		             && plerr != NM_PLATFORM_ERROR_NOT_FOUND
+		             && (nm_platform_link_get_ifi_flags (nm_device_get_platform (self),
+		                                                 nm_device_get_ip_ifindex (self),
+		                                                 IFF_UP) > 0);
+		_NMLOG (     retry_down
+		          || plerr == NM_PLATFORM_ERROR_NOT_FOUND
+		        ? LOGL_DEBUG
+		        : LOGL_WARN,
+		        LOGD_DEVICE,
+		        "set-hw-addr: failed to %s MAC address to %s (%s) (%s)%s",
+		        operation, addr, detail,
+		        nm_platform_error_to_string_a (plerr),
+		        retry_down ? " (retry with taking down)" : "");
+	} else {
 		/* MAC address successfully changed; update the current MAC to match */
 		nm_device_update_hw_address (self);
-		if (_hw_addr_matches (self, addr_bytes, addr_len)) {
-			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
-			       operation, addr, detail);
-		} else {
+
+		if (!_hw_addr_matches (self, addr_bytes, addr_len)) {
 			gint64 poll_end, now;
 
 			_LOGD (LOGD_DEVICE,
@@ -15463,24 +15474,41 @@ handle_fail:
 				success = FALSE;
 				break;
 			}
+		}
 
-			if (success) {
-				_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
-				       operation, addr, detail);
-			} else {
-				_LOGW (LOGD_DEVICE,
-				       "set-hw-addr: new MAC address %s not successfully %s (%s)",
-				       addr, operation, detail);
-			}
+		if (success) {
+			retry_down = FALSE;
+			_LOGI (LOGD_DEVICE, "set-hw-addr: %s MAC address to %s (%s)",
+			       operation, addr, detail);
+		} else {
+			retry_down =    !was_taken_down
+			             && (nm_platform_link_get_ifi_flags (nm_device_get_platform (self),
+			                                                 nm_device_get_ip_ifindex (self),
+			                                                 IFF_UP) > 0);
+
+			_NMLOG (  retry_down
+			        ? LOGL_DEBUG
+			        : LOGL_WARN,
+			        LOGD_DEVICE,
+			        "set-hw-addr: new MAC address %s not successfully %s (%s)%s",
+			        addr,
+			        operation,
+			        detail,
+			        retry_down ? " (retry with taking down)" : "");
 		}
-	} else {
-		_NMLOG (plerr == NM_PLATFORM_ERROR_NOT_FOUND ? LOGL_DEBUG : LOGL_WARN,
-		        LOGD_DEVICE, "set-hw-addr: failed to %s MAC address to %s (%s) (%s)",
-		        operation, addr, detail,
-		        nm_platform_error_to_string_a (plerr));
 	}
 
-	if (was_up) {
+	if (retry_down) {
+		/* changing the MAC address failed, but also the device was up (and we did not yet try to take
+		 * it down). Optimally, we change the MAC address without taking the device down, but some
+		 * devices don't like that. So, retry with taking the device down. */
+		retry_down = FALSE;
+		was_taken_down = TRUE;
+		nm_device_take_down (self, FALSE);
+		goto again;
+	}
+
+	if (was_taken_down) {
 		if (!nm_device_bring_up (self, TRUE, NULL))
 			return FALSE;
 	}
diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h
index 2a18d8adf2..acb2109c22 100644
--- a/src/platform/nm-platform.h
+++ b/src/platform/nm-platform.h
@@ -1135,6 +1135,24 @@ const NMPlatformLink *nm_platform_link_get (NMPlatform *self, int ifindex);
 const NMPlatformLink *nm_platform_link_get_by_ifname (NMPlatform *self, const char *ifname);
 const NMPlatformLink *nm_platform_link_get_by_address (NMPlatform *self, NMLinkType link_type, gconstpointer address, size_t length);
 
+static inline int
+nm_platform_link_get_ifi_flags (NMPlatform *self, int ifindex, guint requested_flags)
+{
+	const NMPlatformLink *l;
+
+	l = nm_platform_link_get (self, ifindex);
+	if (!l) {
+		/* non-existing link is signaled as negative value.
+		 *
+		 * Note, the return value is signed, but we return the requested-flags (which are
+		 * unsigned). This works all fine, unless you try to request flags > 2^31. Don't do that. */
+		return -ENODEV;
+	}
+	nm_assert ((int) requested_flags >= 0);
+	nm_assert (requested_flags < (guint) G_MAXINT);
+	return (int) (l->n_ifi_flags & requested_flags);
+}
+
 GPtrArray *nm_platform_link_get_all (NMPlatform *self, gboolean sort_by_name);
 NMPlatformError nm_platform_link_dummy_add (NMPlatform *self, const char *name, const NMPlatformLink **out_link);
 NMPlatformError nm_platform_link_bridge_add (NMPlatform *self, const char *name, const void *address, size_t address_len, const NMPlatformLink **out_link);