From b445b1f8fe4eff23f59b7e7efaba137e53a6b70e Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 29 Nov 2018 11:31:50 +0100 Subject: [PATCH 1/2] platform: add nm_platform_link_get_ifi_flags() helper Add helper nm_platform_link_get_ifi_flags() to access the ifi-flags. This replaces the internal API _link_get_flags() and makes it public. However, the return value also allows to distinguish between errors and valid flags. Also, consider non-visible links. These are links that are in netlink, but not visible in udev. The ifi-flags are inherrently netlink specific, so it seems wrong to pretend that the link doesn't exist. --- src/platform/nm-platform.c | 38 +++++++++++++++++++++++++++++--------- src/platform/nm-platform.h | 1 + 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 494e2ddaf..f0a80cb8b 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -1217,13 +1217,29 @@ nm_platform_link_refresh (NMPlatform *self, int ifindex) return TRUE; } -static guint -_link_get_flags (NMPlatform *self, int ifindex) +int +nm_platform_link_get_ifi_flags (NMPlatform *self, + int ifindex, + guint requested_flags) { const NMPlatformLink *pllink; - pllink = nm_platform_link_get (self, ifindex); - return pllink ? pllink->n_ifi_flags : IFF_NOARP; + _CHECK_SELF (self, klass, -EINVAL); + + if (ifindex <= 0) + return -EINVAL; + + /* include invisible links (only in netlink, not udev). */ + pllink = NMP_OBJECT_CAST_LINK (nm_platform_link_get_obj (self, ifindex, FALSE)); + if (!pllink) + return -ENODEV; + + /* Errors are signaled as negative values. That means, you cannot request + * the most significant bit (2^31) with this API. Assert against that. */ + nm_assert ((int) requested_flags >= 0); + nm_assert (requested_flags < (guint) G_MAXINT); + + return (int) (pllink->n_ifi_flags & requested_flags); } /** @@ -1236,9 +1252,7 @@ _link_get_flags (NMPlatform *self, int ifindex) gboolean nm_platform_link_is_up (NMPlatform *self, int ifindex) { - _CHECK_SELF (self, klass, FALSE); - - return NM_FLAGS_HAS (_link_get_flags (self, ifindex), IFF_UP); + return nm_platform_link_get_ifi_flags (self, ifindex, IFF_UP) == IFF_UP; } /** @@ -1269,9 +1283,15 @@ nm_platform_link_is_connected (NMPlatform *self, int ifindex) gboolean nm_platform_link_uses_arp (NMPlatform *self, int ifindex) { - _CHECK_SELF (self, klass, FALSE); + int f; - return !NM_FLAGS_HAS (_link_get_flags (self, ifindex), IFF_NOARP); + f = nm_platform_link_get_ifi_flags (self, ifindex, IFF_NOARP); + + if (f < 0) + return FALSE; + if (f == IFF_NOARP) + return FALSE; + return TRUE; } /** diff --git a/src/platform/nm-platform.h b/src/platform/nm-platform.h index 2a18d8adf..dff5102d8 100644 --- a/src/platform/nm-platform.h +++ b/src/platform/nm-platform.h @@ -1166,6 +1166,7 @@ int nm_platform_link_get_ifindex (NMPlatform *self, const char *name); const char *nm_platform_link_get_name (NMPlatform *self, int ifindex); NMLinkType nm_platform_link_get_type (NMPlatform *self, int ifindex); gboolean nm_platform_link_is_software (NMPlatform *self, int ifindex); +int nm_platform_link_get_ifi_flags (NMPlatform *self, int ifindex, guint requested_flags); gboolean nm_platform_link_is_up (NMPlatform *self, int ifindex); gboolean nm_platform_link_is_connected (NMPlatform *self, int ifindex); gboolean nm_platform_link_uses_arp (NMPlatform *self, int ifindex); From e206a3473249be4c92c5d71214a33e90db301127 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 23 Nov 2018 17:05:34 +0100 Subject: [PATCH 2/2] device: avoid taking down link to change MAC address 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 --- src/devices/nm-device.c | 78 +++++++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ff082b3d8..d879b43c9 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,30 @@ _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_is_up (nm_device_get_platform (self), + nm_device_get_ip_ifindex (self)); + _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 +15473,40 @@ 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); - } } - } 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 (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_is_up (nm_device_get_platform (self), + nm_device_get_ip_ifindex (self)); + + _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)" : ""); + } } - 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; }