From ba950cedee5c4a92d9fb7e00f58b9f97a151b9e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 26 Oct 2016 14:44:02 +0200 Subject: [PATCH 01/10] shared: add nm_assert_se() macro We usually don't build NM with g_assert() disabled (G_DISABLE_ASSERT). But even if we would, there is no assertion macro that always evaluates the condition for possible side effects. I think that is a useful thing to have. --- shared/nm-utils/nm-macros-internal.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shared/nm-utils/nm-macros-internal.h b/shared/nm-utils/nm-macros-internal.h index 26b69bc138..658b3b92a4 100644 --- a/shared/nm-utils/nm-macros-internal.h +++ b/shared/nm-utils/nm-macros-internal.h @@ -305,9 +305,11 @@ nm_strdup_not_empty (const char *str) #if NM_MORE_ASSERTS #define nm_assert(cond) G_STMT_START { g_assert (cond); } G_STMT_END +#define nm_assert_se(cond) G_STMT_START { if (G_LIKELY (cond)) { ; } else { g_assert (FALSE && (cond)); } } G_STMT_END #define nm_assert_not_reached() G_STMT_START { g_assert_not_reached (); } G_STMT_END #else #define nm_assert(cond) G_STMT_START { if (FALSE) { if (cond) { } } } G_STMT_END +#define nm_assert_se(cond) G_STMT_START { if (G_LIKELY (cond)) { ; } } G_STMT_END #define nm_assert_not_reached() G_STMT_START { ; } G_STMT_END #endif From 5e208e59b3e54fa01c53ec007d80798043dadab7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Oct 2016 10:26:08 +0200 Subject: [PATCH 02/10] config: remove unused error variable from nm_config_device_state_load() --- src/nm-config.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/nm-config.c b/src/nm-config.c index 458265b507..92ef0468cd 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -1943,14 +1943,13 @@ nm_config_device_state_load (NMConfig *self, NMConfigDeviceStateData *device_state; char path[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR) + 60]; gs_unref_keyfile GKeyFile *kf = NULL; - gs_free_error GError *error = NULL; g_return_val_if_fail (ifindex > 0, NULL); nm_sprintf_buf (path, "%s/%d", NM_CONFIG_DEVICE_STATE_DIR, ifindex); kf = nm_config_create_keyfile (); - if (!g_key_file_load_from_file (kf, path, G_KEY_FILE_NONE, &error)) + if (!g_key_file_load_from_file (kf, path, G_KEY_FILE_NONE, NULL)) g_clear_pointer (&kf, g_key_file_unref); device_state = _config_device_state_data_new (ifindex, kf); From e5fe5a4c033875679adbea3cae50108daef43eb3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Oct 2016 17:51:07 +0200 Subject: [PATCH 03/10] libnm-core/utils: update hwaddr utilities _nm_utils_hwaddr_length() did a validation of the string and returned the length of the address. In all cases where we were interested in that, we also either want to validate the address, get the address in binary form, or canonicalize the address. We can avoid these duplicate checks, by using _nm_utils_hwaddr_aton() which both does the parsing and returning the length. --- libnm-core/nm-core-internal.h | 5 +- libnm-core/nm-utils.c | 309 +++++++++++++++++++--------------- src/devices/nm-device.c | 46 ++--- src/nm-core-utils.c | 7 +- 4 files changed, 206 insertions(+), 161 deletions(-) diff --git a/libnm-core/nm-core-internal.h b/libnm-core/nm-core-internal.h index 0ea59e4f08..8fba357167 100644 --- a/libnm-core/nm-core-internal.h +++ b/libnm-core/nm-core-internal.h @@ -124,7 +124,10 @@ guint32 _nm_setting_get_setting_priority (NMSetting *setting); gboolean _nm_setting_get_property (NMSetting *setting, const char *name, GValue *value); -guint _nm_utils_hwaddr_length (const char *asc); +#define NM_UTILS_HWADDR_LEN_MAX_STR (NM_UTILS_HWADDR_LEN_MAX * 3) + +guint8 *_nm_utils_hwaddr_aton (const char *asc, gpointer buffer, gsize buffer_length, gsize *out_length); +const char *nm_utils_hwaddr_ntoa_buf (gconstpointer addr, gsize addr_len, gboolean upper_case, char *buf, gsize buf_len); char *_nm_utils_bin2str (gconstpointer addr, gsize length, gboolean upper_case); diff --git a/libnm-core/nm-utils.c b/libnm-core/nm-utils.c index d963323e22..4be430af6b 100644 --- a/libnm-core/nm-utils.c +++ b/libnm-core/nm-utils.c @@ -2995,17 +2995,64 @@ nm_utils_wifi_strength_bars (guint8 strength) gsize nm_utils_hwaddr_len (int type) { - g_return_val_if_fail (type == ARPHRD_ETHER || type == ARPHRD_INFINIBAND, 0); - if (type == ARPHRD_ETHER) return ETH_ALEN; else if (type == ARPHRD_INFINIBAND) return INFINIBAND_ALEN; - g_assert_not_reached (); + g_return_val_if_reached (0); } -#define HEXVAL(c) ((c) <= '9' ? (c) - '0' : ((c) & 0x4F) - 'A' + 10) +static guint8 * +hwaddr_aton (const char *asc, guint8 *buffer, gsize buffer_length, gsize *out_len) +{ + const char *in = asc; + guint8 *out = buffer; + guint8 delimiter = '\0'; + + nm_assert (asc); + nm_assert (buffer); + nm_assert (buffer_length); + nm_assert (out_len); + + while (TRUE) { + const guint8 d1 = in[0]; + guint8 d2; + + if (!g_ascii_isxdigit (d1)) + return NULL; + +#define HEXVAL(c) ((c) <= '9' ? (c) - '0' : ((c) & 0x4F) - ('A' - 10)) + + /* If there's no leading zero (ie "aa:b:cc") then fake it */ + d2 = in[1]; + if (d2 && g_ascii_isxdigit (d2)) { + *out++ = (HEXVAL (d1) << 4) + HEXVAL (d2); + d2 = in[2]; + in += 3; + } else { + /* Fake leading zero */ + *out++ = HEXVAL (d1); + in += 2; + } + + if (!d2) + break; + if (--buffer_length == 0) + return NULL; + + if (d2 != delimiter) { + if ( delimiter == '\0' + && (d2 == ':' || d2 == '-')) + delimiter = d2; + else + return NULL; + } + } + + *out_len = out - buffer; + return buffer; +} /** * nm_utils_hwaddr_atoba: @@ -3022,18 +3069,52 @@ GByteArray * nm_utils_hwaddr_atoba (const char *asc, gsize length) { GByteArray *ba; + gsize l; - g_return_val_if_fail (asc != NULL, NULL); + g_return_val_if_fail (asc, NULL); g_return_val_if_fail (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX, NULL); ba = g_byte_array_sized_new (length); g_byte_array_set_size (ba, length); - if (!nm_utils_hwaddr_aton (asc, ba->data, length)) { - g_byte_array_unref (ba); - return NULL; - } + if (!hwaddr_aton (asc, ba->data, length, &l)) + goto fail; + if (length != l) + goto fail; return ba; +fail: + g_byte_array_unref (ba); + return NULL; +} + +/** + * _nm_utils_hwaddr_aton: + * @asc: the ASCII representation of a hardware address + * @buffer: buffer to store the result into. Must have + * at least a size of @buffer_length. + * @buffer_length: the length of the input buffer @buffer. + * The result must fit into that buffer, otherwise + * the function fails and returns %NULL. + * @out_length: the output length in case of success. + * + * Parses @asc and converts it to binary form in @buffer. + * Bytes in @asc can be sepatared by colons (:), or hyphens (-), but not mixed. + * + * It is like nm_utils_hwaddr_aton(), but contrary to that it + * can parse addresses of any length. That is, you don't need + * to know the length before-hand. + * + * Return value: @buffer, or %NULL if @asc couldn't be parsed. + */ +guint8 * +_nm_utils_hwaddr_aton (const char *asc, gpointer buffer, gsize buffer_length, gsize *out_length) +{ + g_return_val_if_fail (asc, NULL); + g_return_val_if_fail (buffer, NULL); + g_return_val_if_fail (buffer_length > 0, NULL); + g_return_val_if_fail (out_length, NULL); + + return hwaddr_aton (asc, buffer, buffer_length, out_length); } /** @@ -3052,72 +3133,55 @@ nm_utils_hwaddr_atoba (const char *asc, gsize length) guint8 * nm_utils_hwaddr_aton (const char *asc, gpointer buffer, gsize length) { - const char *in = asc; - guint8 *out = (guint8 *)buffer; - char delimiter = '\0'; + gsize l; - g_return_val_if_fail (asc != NULL, NULL); - g_return_val_if_fail (buffer != NULL, NULL); + g_return_val_if_fail (asc, NULL); + g_return_val_if_fail (buffer, NULL); g_return_val_if_fail (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX, NULL); - while (length && *in) { - guint8 d1 = in[0], d2 = in[1]; + if (!hwaddr_aton (asc, buffer, length, &l)) + return NULL; + if (length != l) + return NULL; + return buffer; +} - if (!g_ascii_isxdigit (d1)) - return NULL; +static void +_bin2str_buf (gconstpointer addr, gsize length, gboolean upper_case, char *out) +{ + const guint8 *in = addr; + const char *LOOKUP = upper_case ? "0123456789ABCDEF" : "0123456789abcdef"; - /* If there's no leading zero (ie "aa:b:cc") then fake it */ - if (d2 && g_ascii_isxdigit (d2)) { - *out++ = (HEXVAL (d1) << 4) + HEXVAL (d2); - in += 2; - } else { - /* Fake leading zero */ - *out++ = (HEXVAL ('0') << 4) + HEXVAL (d1); - in += 1; - } + nm_assert (addr); + nm_assert (out); + nm_assert (length > 0); + /* @out must contain at least @length*3 bytes */ + + for (;;) { + const guint8 v = *in++; + + *out++ = LOOKUP[v >> 4]; + *out++ = LOOKUP[v & 0x0F]; length--; - if (*in) { - if (delimiter == '\0') { - if (*in == ':' || *in == '-') - delimiter = *in; - else - return NULL; - } else { - if (*in != delimiter) - return NULL; - } - in++; - } + if (!length) + break; + *out++ = ':'; } - if (length == 0 && !*in) - return buffer; - else - return NULL; + *out = 0; } static char * _bin2str (gconstpointer addr, gsize length, gboolean upper_case) { - const guint8 *in = addr; - char *out, *result; - const char *LOOKUP = upper_case ? "0123456789ABCDEF" : "0123456789abcdef"; + char *result; - g_return_val_if_fail (addr != NULL, g_strdup ("")); - g_return_val_if_fail (length > 0, g_strdup ("")); + nm_assert (addr); + nm_assert (length > 0); - result = out = g_malloc (length * 3); - while (length--) { - guint8 v = *in++; - - *out++ = LOOKUP[v >> 4]; - *out++ = LOOKUP[v & 0x0F]; - if (length) - *out++ = ':'; - } - - *out = 0; + result = g_malloc (length * 3); + _bin2str_buf (addr, length, upper_case, result); return result; } @@ -3133,9 +3197,25 @@ _bin2str (gconstpointer addr, gsize length, gboolean upper_case) char * nm_utils_hwaddr_ntoa (gconstpointer addr, gsize length) { + g_return_val_if_fail (addr, g_strdup ("")); + g_return_val_if_fail (length > 0, g_strdup ("")); + return _bin2str (addr, length, TRUE); } +const char * +nm_utils_hwaddr_ntoa_buf (gconstpointer addr, gsize addr_len, gboolean upper_case, char *buf, gsize buf_len) +{ + g_return_val_if_fail (addr, NULL); + g_return_val_if_fail (addr_len > 0, NULL); + g_return_val_if_fail (buf, NULL); + if (buf_len < addr_len * 3) + g_return_val_if_reached (NULL); + + _bin2str_buf (addr, addr_len, TRUE, buf); + return buf; +} + /** * _nm_utils_bin2str: * @addr: (type guint8) (array length=length): a binary hardware address @@ -3149,51 +3229,12 @@ nm_utils_hwaddr_ntoa (gconstpointer addr, gsize length) char * _nm_utils_bin2str (gconstpointer addr, gsize length, gboolean upper_case) { + g_return_val_if_fail (addr, g_strdup ("")); + g_return_val_if_fail (length > 0, g_strdup ("")); + return _bin2str (addr, length, upper_case); } -static int -hwaddr_binary_len (const char *asc) -{ - int octets = 1; - - if (!*asc) - return 0; - - for (; *asc; asc++) { - if (*asc == ':' || *asc == '-') - octets++; - } - return octets; -} - -/** - * _nm_utils_hwaddr_length: - * @asc: the ASCII representation of the hardware address - * - * Validates that @asc is a valid representation of a hardware - * address up to (including) %NM_UTILS_HWADDR_LEN_MAX bytes. - * - * Returns: binary length of the hardware address @asc or - * 0 on error. - */ -guint -_nm_utils_hwaddr_length (const char *asc) -{ - int l; - - if (!asc) - return 0; - - l = hwaddr_binary_len (asc); - if (l <= 0 || l > NM_UTILS_HWADDR_LEN_MAX) - return 0; - - if (!nm_utils_hwaddr_valid (asc, l)) - return 0; - return l; -} - /** * nm_utils_hwaddr_valid: * @asc: the ASCII representation of a hardware address @@ -3210,17 +3251,18 @@ gboolean nm_utils_hwaddr_valid (const char *asc, gssize length) { guint8 buf[NM_UTILS_HWADDR_LEN_MAX]; + gsize l; g_return_val_if_fail (asc != NULL, FALSE); - g_return_val_if_fail (length == -1 || (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX), FALSE); - if (length == -1) { - length = hwaddr_binary_len (asc); - if (length == 0 || length > NM_UTILS_HWADDR_LEN_MAX) + if (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX) { + if (!hwaddr_aton (asc, buf, length, &l)) return FALSE; - } - - return nm_utils_hwaddr_aton (asc, buf, length) != NULL; + return length == l; + } else if (length == -1) { + return !!hwaddr_aton (asc, buf, sizeof (buf), &l); + } else + g_return_val_if_reached (FALSE); } /** @@ -3240,20 +3282,23 @@ char * nm_utils_hwaddr_canonical (const char *asc, gssize length) { guint8 buf[NM_UTILS_HWADDR_LEN_MAX]; + gsize l; - g_return_val_if_fail (asc != NULL, NULL); + g_return_val_if_fail (asc, NULL); g_return_val_if_fail (length == -1 || (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX), NULL); - if (length == -1) { - length = hwaddr_binary_len (asc); - if (length == 0 || length > NM_UTILS_HWADDR_LEN_MAX) + if (length > 0 && length <= NM_UTILS_HWADDR_LEN_MAX) { + if (!hwaddr_aton (asc, buf, length, &l)) return NULL; - } + if (l != length) + return NULL; + } else if (length == -1) { + if (!hwaddr_aton (asc, buf, NM_UTILS_HWADDR_LEN_MAX, &l)) + return NULL; + } else + g_return_val_if_reached (NULL); - if (nm_utils_hwaddr_aton (asc, buf, length) == NULL) - return NULL; - - return nm_utils_hwaddr_ntoa (buf, length); + return nm_utils_hwaddr_ntoa (buf, l); } /* This is used to possibly canonicalize values passed to MAC address property @@ -3317,17 +3362,17 @@ nm_utils_hwaddr_matches (gconstpointer hwaddr1, gssize hwaddr2_len) { guint8 buf1[NM_UTILS_HWADDR_LEN_MAX], buf2[NM_UTILS_HWADDR_LEN_MAX]; + gsize l; if (hwaddr1_len == -1) { g_return_val_if_fail (hwaddr1 != NULL, FALSE); - hwaddr1_len = hwaddr_binary_len (hwaddr1); - if (hwaddr1_len == 0 || hwaddr1_len > NM_UTILS_HWADDR_LEN_MAX) + if (!hwaddr_aton (hwaddr1, buf1, sizeof (buf1), &l)) { + g_return_val_if_fail ((hwaddr2_len == -1 && hwaddr2) || (hwaddr2_len > 0 && hwaddr2_len <= NM_UTILS_HWADDR_LEN_MAX), FALSE); return FALSE; - if (!nm_utils_hwaddr_aton (hwaddr1, buf1, hwaddr1_len)) - return FALSE; - + } hwaddr1 = buf1; + hwaddr1_len = l; } else { g_return_val_if_fail (hwaddr1_len > 0 && hwaddr1_len <= NM_UTILS_HWADDR_LEN_MAX, FALSE); @@ -3340,23 +3385,24 @@ nm_utils_hwaddr_matches (gconstpointer hwaddr1, if (hwaddr2_len == -1) { g_return_val_if_fail (hwaddr2 != NULL, FALSE); - if (!nm_utils_hwaddr_aton (hwaddr2, buf2, hwaddr1_len)) + if (!hwaddr_aton (hwaddr2, buf2, sizeof (buf2), &l)) + return FALSE; + if (l != hwaddr1_len) return FALSE; - hwaddr2 = buf2; hwaddr2_len = hwaddr1_len; } else { g_return_val_if_fail (hwaddr2_len > 0 && hwaddr2_len <= NM_UTILS_HWADDR_LEN_MAX, FALSE); + if (hwaddr2_len != hwaddr1_len) + return FALSE; + if (!hwaddr2) { memset (buf2, 0, hwaddr2_len); hwaddr2 = buf2; } } - if (hwaddr1_len != hwaddr2_len) - return FALSE; - if (hwaddr1_len == INFINIBAND_ALEN) { hwaddr1 = (guint8 *)hwaddr1 + INFINIBAND_ALEN - 8; hwaddr2 = (guint8 *)hwaddr2 + INFINIBAND_ALEN - 8; @@ -3372,16 +3418,11 @@ static GVariant * _nm_utils_hwaddr_to_dbus_impl (const char *str) { guint8 buf[NM_UTILS_HWADDR_LEN_MAX]; - int len; + gsize len; if (!str) return NULL; - - len = _nm_utils_hwaddr_length (str); - if (len == 0) - return NULL; - - if (!nm_utils_hwaddr_aton (str, buf, len)) + if (!hwaddr_aton (str, buf, sizeof (buf), &len)) return NULL; return g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, buf, len, 1); diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 60c60f62e7..d7868f6502 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -11806,25 +11806,25 @@ nm_device_hw_addr_is_explict (NMDevice *self) } static gboolean -_hw_addr_matches (NMDevice *self, const char *addr) +_hw_addr_matches (NMDevice *self, const guint8 *addr, gsize addr_len) { const char *cur_addr; cur_addr = nm_device_get_hw_address (self); - return cur_addr && nm_utils_hwaddr_matches (cur_addr, -1, addr, -1); + return cur_addr && nm_utils_hwaddr_matches (addr, addr_len, cur_addr, -1); } static gboolean _hw_addr_set (NMDevice *self, - const char *addr, - const char *operation, - const char *detail) + const char *const addr, + const char *const operation, + const char *const detail) { NMDevicePrivate *priv; gboolean success = FALSE; NMPlatformError plerr; guint8 addr_bytes[NM_UTILS_HWADDR_LEN_MAX]; - guint hw_addr_len; + gsize addr_len; gboolean was_up; nm_assert (NM_IS_DEVICE (self)); @@ -11833,18 +11833,20 @@ _hw_addr_set (NMDevice *self, priv = NM_DEVICE_GET_PRIVATE (self); + if (!_nm_utils_hwaddr_aton (addr, addr_bytes, sizeof (addr_bytes), &addr_len)) + g_return_val_if_reached (FALSE); + /* Do nothing if current MAC is same */ - if (_hw_addr_matches (self, addr)) { + if (_hw_addr_matches (self, addr_bytes, addr_len)) { _LOGT (LOGD_DEVICE, "set-hw-addr: no MAC address change needed (%s)", addr); return TRUE; } - hw_addr_len = priv->hw_addr_len; - if (!hw_addr_len) - hw_addr_len = _nm_utils_hwaddr_length (addr); - if ( !hw_addr_len - || !nm_utils_hwaddr_aton (addr, addr_bytes, hw_addr_len)) - g_return_val_if_reached (FALSE); + if (priv->hw_addr_len != addr_len) { + if (priv->hw_addr_len) + g_return_val_if_reached (FALSE); + priv->hw_addr_len = addr_len; + } _LOGT (LOGD_DEVICE, "set-hw-addr: setting MAC address to '%s' (%s, %s)...", addr, operation, detail); @@ -11854,12 +11856,12 @@ _hw_addr_set (NMDevice *self, nm_device_take_down (self, FALSE); } - plerr = nm_platform_link_set_address (NM_PLATFORM_GET, nm_device_get_ip_ifindex (self), addr_bytes, hw_addr_len); + plerr = nm_platform_link_set_address (NM_PLATFORM_GET, nm_device_get_ip_ifindex (self), addr_bytes, addr_len); success = (plerr == NM_PLATFORM_ERROR_SUCCESS); if (success) { /* MAC address succesfully changed; update the current MAC to match */ nm_device_update_hw_address (self); - if (_hw_addr_matches (self, addr)) { + 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 { @@ -11889,7 +11891,7 @@ _hw_addr_set (NMDevice *self, goto handle_fail; if (!nm_device_update_hw_address (self)) goto handle_wait; - if (!_hw_addr_matches (self, addr)) + if (!_hw_addr_matches (self, addr_bytes, addr_len)) goto handle_fail; break; @@ -12016,9 +12018,8 @@ nm_device_hw_addr_set_cloned (NMDevice *self, NMConnection *connection, gboolean addr = hw_addr_generated; } else { /* this must be a valid address. Otherwise, we shouldn't come here. */ - if (_nm_utils_hwaddr_length (addr) <= 0) { + if (!nm_utils_hwaddr_valid (addr, -1)) g_return_val_if_reached (FALSE); - } priv->hw_addr_type = HW_ADDR_TYPE_EXPLICIT; } @@ -12217,13 +12218,16 @@ constructor (GType type, } if (priv->hw_addr_perm) { - priv->hw_addr_len = _nm_utils_hwaddr_length (priv->hw_addr_perm); - if (!priv->hw_addr_len) { + guint8 buf[NM_UTILS_HWADDR_LEN_MAX]; + gsize l; + + if (!_nm_utils_hwaddr_aton (priv->hw_addr_perm, buf, sizeof (buf), &l)) { g_clear_pointer (&priv->hw_addr_perm, g_free); g_return_val_if_reached (object); } - priv->hw_addr = g_strdup (priv->hw_addr_perm); + priv->hw_addr_len = l; + priv->hw_addr = nm_utils_hwaddr_ntoa (buf, l); _LOGT (LOGD_DEVICE, "hw-addr: has permanent hw-address '%s'", priv->hw_addr_perm); } diff --git a/src/nm-core-utils.c b/src/nm-core-utils.c index 984e3ec2ed..c48bf6cd45 100644 --- a/src/nm-core-utils.c +++ b/src/nm-core-utils.c @@ -1272,7 +1272,7 @@ nm_match_spec_hwaddr (const GSList *specs, const char *hwaddr) { const GSList *iter; NMMatchSpecMatchType match = NM_MATCH_SPEC_NO_MATCH; - guint hwaddr_len = 0; + gsize hwaddr_len = 0; guint8 hwaddr_bin[NM_UTILS_HWADDR_LEN_MAX]; nm_assert (nm_utils_hwaddr_valid (hwaddr, -1)); @@ -1297,11 +1297,8 @@ nm_match_spec_hwaddr (const GSList *specs, const char *hwaddr) continue; if (G_UNLIKELY (hwaddr_len == 0)) { - hwaddr_len = _nm_utils_hwaddr_length (hwaddr); - if (!hwaddr_len) + if (!_nm_utils_hwaddr_aton (hwaddr, hwaddr_bin, sizeof (hwaddr_bin), &hwaddr_len)) g_return_val_if_reached (NM_MATCH_SPEC_NO_MATCH); - if (!nm_utils_hwaddr_aton (hwaddr, hwaddr_bin, hwaddr_len)) - nm_assert_not_reached (); } if (nm_utils_hwaddr_matches (spec_str, -1, hwaddr_bin, hwaddr_len)) { From 5912b2f9a170893002b789fe37a7784eefac4e34 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Oct 2016 15:56:13 +0200 Subject: [PATCH 04/10] core: persist the fake permanent hardware address to the device's statefile On devices that have no real permanent hardware address (as returned by ethtool), we take the current MAC address of the device. Currently, NM is a bit flaky about whether to accept such fake permanent addresses for settings like keyfile.unmanaged-devices or the per- connection property ethernet.mac-address. Probably, we should allow using fake addresses there in general. However, that leads to problems because NetworkManager itself changes the current MAC address of such devices. For example when configuing keyfile.unmanaged-device=22:33:44:55:66:77 and later activating a connection with ethernet.cloned-mac-address=22:33:44:55:66:77 we have a strange situation after restart and the device becomes unmanaged. We are going to avoid that, by remembering the fake permanent address in the device state file. This only matters: - for devices that don't have a real permanent address (veth) - if the user or NetworkManager itself changed the MAC address of the device - after a restart of NetworkManager, without reboot. A reboot clears the device state for /var/run/NetworkManager. --- src/devices/nm-device.c | 57 ++++++++++++++++++++++++++++++++------- src/devices/nm-device.h | 2 ++ src/nm-config.c | 60 ++++++++++++++++++++++++++++++++--------- src/nm-config.h | 3 +++ src/nm-manager.c | 7 +++++ 5 files changed, 107 insertions(+), 22 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index d7868f6502..6cc2b4db07 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -11697,24 +11697,49 @@ nm_device_update_permanent_hw_address (NMDevice *self) success_read = nm_platform_link_get_permanent_address (NM_PLATFORM_GET, priv->ifindex, buf, &len); if (!success_read || len != priv->hw_addr_len) { - /* Fall back to current address. We use the fake address and keep it - * until the device unrealizes. + 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. * - * In some cases it might be necessary to know whether this is a "real" or - * a temporary address (fake). */ + * 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); + if ( dev_state + && dev_state->perm_hw_addr_fake + && nm_utils_hwaddr_aton (dev_state->perm_hw_addr_fake, buf, priv->hw_addr_len) + && !nm_utils_hwaddr_matches (buf, priv->hw_addr_len, priv->hw_addr, -1)) { + _LOGD (LOGD_PLATFORM | LOGD_ETHER, "hw-addr: %s (use from statefile: %s, current: %s)", + success_read + ? "read HW addr length of permanent MAC address differs" + : "unable to read permanent MAC address", + dev_state->perm_hw_addr_fake, + priv->hw_addr); + priv->hw_addr_perm = nm_utils_hwaddr_ntoa (buf, priv->hw_addr_len); + goto 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_fake = TRUE; priv->hw_addr_perm = g_strdup (priv->hw_addr); - } else { - 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 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); + +out: _notify (self, PROP_PERM_HW_ADDRESS); } @@ -12050,6 +12075,18 @@ nm_device_hw_addr_reset (NMDevice *self, const char *detail) return _hw_addr_set (self, addr, "reset", detail); } +const char * +nm_device_get_permanent_hw_address_full (NMDevice *self, gboolean *out_is_fake) +{ + NMDevicePrivate *priv; + + g_return_val_if_fail (NM_IS_DEVICE (self), NULL); + + priv = NM_DEVICE_GET_PRIVATE (self); + NM_SET_OUT (out_is_fake, priv->hw_addr_perm && priv->hw_addr_perm_fake); + return priv->hw_addr_perm; +} + const char * nm_device_get_permanent_hw_address (NMDevice *self, gboolean fallback_fake) { diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index ceee6c0b83..9977c7c55e 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -364,6 +364,8 @@ 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 *dev, gboolean fallback_fake); +const char * nm_device_get_permanent_hw_address_full (NMDevice *self, + gboolean *out_is_fake); const char * nm_device_get_initial_hw_address (NMDevice *dev); NMProxyConfig * nm_device_get_proxy_config (NMDevice *dev); diff --git a/src/nm-config.c b/src/nm-config.c index 92ef0468cd..7f22b54f74 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -1879,6 +1879,7 @@ _nm_config_state_set (NMConfig *self, #define DEVICE_RUN_STATE_KEYFILE_GROUP_DEVICE "device" #define DEVICE_RUN_STATE_KEYFILE_KEY_DEVICE_MANAGED "managed" +#define DEVICE_RUN_STATE_KEYFILE_KEY_DEVICE_PERM_HW_ADDR_FAKE "perm-hw-addr-fake" #define DEVICE_RUN_STATE_KEYFILE_KEY_DEVICE_CONNECTION_UUID "connection-uuid" static NMConfigDeviceStateData * @@ -1887,7 +1888,10 @@ _config_device_state_data_new (int ifindex, GKeyFile *kf) NMConfigDeviceStateData *device_state; NMConfigDeviceStateManagedType managed_type = NM_CONFIG_DEVICE_STATE_MANAGED_TYPE_UNKNOWN; gs_free char *connection_uuid = NULL; - gsize len_plus_1; + gs_free char *perm_hw_addr_fake = NULL; + gsize connection_uuid_len; + gsize perm_hw_addr_fake_len; + char *p; nm_assert (ifindex > 0); @@ -1908,21 +1912,42 @@ _config_device_state_data_new (int ifindex, GKeyFile *kf) DEVICE_RUN_STATE_KEYFILE_KEY_DEVICE_CONNECTION_UUID, NM_CONFIG_GET_VALUE_STRIP | NM_CONFIG_GET_VALUE_NO_EMPTY); } + + perm_hw_addr_fake = nm_config_keyfile_get_value (kf, + DEVICE_RUN_STATE_KEYFILE_GROUP_DEVICE, + DEVICE_RUN_STATE_KEYFILE_KEY_DEVICE_PERM_HW_ADDR_FAKE, + NM_CONFIG_GET_VALUE_STRIP | NM_CONFIG_GET_VALUE_NO_EMPTY); + if (perm_hw_addr_fake) { + char *normalized; + + normalized = nm_utils_hwaddr_canonical (perm_hw_addr_fake, -1); + g_free (perm_hw_addr_fake); + perm_hw_addr_fake = normalized; + } } - len_plus_1 = connection_uuid ? strlen (connection_uuid) + 1 : 0; + connection_uuid_len = connection_uuid ? strlen (connection_uuid) + 1 : 0; + perm_hw_addr_fake_len = perm_hw_addr_fake ? strlen (perm_hw_addr_fake) + 1 : 0; - device_state = g_malloc (sizeof (NMConfigDeviceStateData) + len_plus_1); + device_state = g_malloc (sizeof (NMConfigDeviceStateData) + + connection_uuid_len + + perm_hw_addr_fake_len); device_state->ifindex = ifindex; device_state->managed = managed_type; device_state->connection_uuid = NULL; - if (connection_uuid) { - char *device_state_data; + device_state->perm_hw_addr_fake = NULL; - device_state_data = (char *) (&device_state[1]); - memcpy (device_state_data, connection_uuid, len_plus_1); - device_state->connection_uuid = device_state_data; + p = (char *) (&device_state[1]); + if (connection_uuid) { + memcpy (p, connection_uuid, connection_uuid_len); + device_state->connection_uuid = p; + p += connection_uuid_len; + } + if (perm_hw_addr_fake) { + memcpy (p, perm_hw_addr_fake, perm_hw_addr_fake_len); + device_state->perm_hw_addr_fake = p; + p += perm_hw_addr_fake_len; } return device_state; @@ -1955,10 +1980,11 @@ nm_config_device_state_load (NMConfig *self, device_state = _config_device_state_data_new (ifindex, kf); if (kf) { - _LOGT ("device-state: read #%d (%s); managed=%d, connection-uuid=%s%s%s", + _LOGT ("device-state: read #%d (%s); managed=%d%s%s%s%s%s%s", ifindex, path, device_state->managed, - NM_PRINT_FMT_QUOTE_STRING (device_state->connection_uuid)); + NM_PRINT_FMT_QUOTED (device_state->connection_uuid, ", connection-uuid=", device_state->connection_uuid, "", ""), + NM_PRINT_FMT_QUOTED (device_state->perm_hw_addr_fake, ", perm-hw-addr-fake=", device_state->perm_hw_addr_fake, "", "")); } else { _LOGT ("device-state: read #%d (%s); no persistent state", ifindex, path); @@ -1971,6 +1997,7 @@ gboolean nm_config_device_state_write (NMConfig *self, int ifindex, gboolean managed, + const char *perm_hw_addr_fake, const char *connection_uuid) { char path[NM_STRLEN (NM_CONFIG_DEVICE_STATE_DIR) + 60]; @@ -1982,6 +2009,8 @@ nm_config_device_state_write (NMConfig *self, g_return_val_if_fail (!connection_uuid || *connection_uuid, FALSE); g_return_val_if_fail (managed || !connection_uuid, FALSE); + nm_assert (!perm_hw_addr_fake || nm_utils_hwaddr_valid (perm_hw_addr_fake, -1)); + nm_sprintf_buf (path, "%s/%d", NM_CONFIG_DEVICE_STATE_DIR, ifindex); kf = nm_config_create_keyfile (); @@ -1989,6 +2018,12 @@ nm_config_device_state_write (NMConfig *self, DEVICE_RUN_STATE_KEYFILE_GROUP_DEVICE, DEVICE_RUN_STATE_KEYFILE_KEY_DEVICE_MANAGED, !!managed); + if (perm_hw_addr_fake) { + g_key_file_set_string (kf, + DEVICE_RUN_STATE_KEYFILE_GROUP_DEVICE, + DEVICE_RUN_STATE_KEYFILE_KEY_DEVICE_PERM_HW_ADDR_FAKE, + perm_hw_addr_fake); + } if (connection_uuid) { g_key_file_set_string (kf, DEVICE_RUN_STATE_KEYFILE_GROUP_DEVICE, @@ -2001,10 +2036,11 @@ nm_config_device_state_write (NMConfig *self, g_error_free (local); return FALSE; } - _LOGT ("device-state: write #%d (%s); managed=%d, connection-uuid=%s%s%s", + _LOGT ("device-state: write #%d (%s); managed=%d%s%s%s%s%s%s", ifindex, path, (bool) managed, - NM_PRINT_FMT_QUOTE_STRING (connection_uuid)); + NM_PRINT_FMT_QUOTED (connection_uuid, ", connection-uuid=", connection_uuid, "", ""), + NM_PRINT_FMT_QUOTED (perm_hw_addr_fake, ", perm-hw-addr-fake=", perm_hw_addr_fake, "", "")); return TRUE; } diff --git a/src/nm-config.h b/src/nm-config.h index 0d3faa8832..326661e9f2 100644 --- a/src/nm-config.h +++ b/src/nm-config.h @@ -199,6 +199,8 @@ struct _NMConfigDeviceStateData { /* the UUID of the last settings-connection active * on the device. */ const char *connection_uuid; + + const char *perm_hw_addr_fake; }; NMConfigDeviceStateData *nm_config_device_state_load (NMConfig *self, @@ -206,6 +208,7 @@ NMConfigDeviceStateData *nm_config_device_state_load (NMConfig *self, gboolean nm_config_device_state_write (NMConfig *self, int ifindex, gboolean managed, + const char *perm_hw_addr_fake, const char *connection_uuid); void nm_config_device_state_prune_unseen (NMConfig *self, GHashTable *seen_ifindexes); diff --git a/src/nm-manager.c b/src/nm-manager.c index ed63b58605..dbc8083f64 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4674,6 +4674,8 @@ nm_manager_write_device_state (NMManager *self) gboolean managed; NMConnection *settings_connection; const char *uuid = NULL; + const char *perm_hw_addr_fake = NULL; + gboolean perm_hw_addr_is_fake; ifindex = nm_device_get_ip_ifindex (device); if (ifindex <= 0) @@ -4693,9 +4695,14 @@ 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); + if (perm_hw_addr_fake && !perm_hw_addr_is_fake) + perm_hw_addr_fake = NULL; + if (nm_config_device_state_write (priv->config, ifindex, managed, + perm_hw_addr_fake, uuid)) g_hash_table_add (seen_ifindexes, GINT_TO_POINTER (ifindex)); } From 416164aa29c45d8071c8767fed866e06ee0e9169 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 13 Oct 2016 18:52:12 +0200 Subject: [PATCH 05/10] device: treat fake permanent MAC address mostly like a real one Now that we persist the fake permanent address across restart of NetworkManager, we want to consider fake addresses as good enough in most cases. --- src/devices/nm-device-ethernet.c | 15 +++++++++------ src/devices/nm-device-infiniband.c | 6 +++--- src/devices/nm-device-macvlan.c | 2 +- src/devices/nm-device-vlan.c | 2 +- src/devices/nm-device.c | 25 +++++++++++-------------- src/devices/nm-device.h | 3 +-- src/devices/wifi/nm-device-wifi.c | 4 ++-- src/nm-config.c | 2 +- src/nm-manager.c | 2 +- src/settings/nm-settings.c | 2 +- 10 files changed, 31 insertions(+), 32 deletions(-) diff --git a/src/devices/nm-device-ethernet.c b/src/devices/nm-device-ethernet.c index b99271afdd..61d4c069f4 100644 --- a/src/devices/nm-device-ethernet.c +++ b/src/devices/nm-device-ethernet.c @@ -394,7 +394,7 @@ check_connection_compatible (NMDevice *device, NMConnection *connection) if (!match_subchans (self, s_wired, &try_mac)) return FALSE; - perm_hw_addr = nm_device_get_permanent_hw_address (device, TRUE); + perm_hw_addr = nm_device_get_permanent_hw_address (device); mac = nm_setting_wired_get_mac_address (s_wired); if (perm_hw_addr) { if (try_mac && mac && !nm_utils_hwaddr_matches (mac, -1, perm_hw_addr, -1)) @@ -1344,6 +1344,7 @@ complete_connection (NMDevice *device, NMSettingPppoe *s_pppoe; const char *setting_mac; const char *perm_hw_addr; + gboolean perm_hw_addr_is_fake; s_pppoe = nm_connection_get_setting_pppoe (connection); @@ -1371,8 +1372,8 @@ complete_connection (NMDevice *device, nm_connection_add_setting (connection, NM_SETTING (s_wired)); } - perm_hw_addr = nm_device_get_permanent_hw_address (device, FALSE); - if (perm_hw_addr) { + perm_hw_addr = nm_device_get_permanent_hw_address_full (device, &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) { /* Make sure the setting MAC (if any) matches the device's permanent MAC */ @@ -1408,7 +1409,7 @@ new_default_connection (NMDevice *self) if (nm_config_get_no_auto_default_for_device (nm_config_get (), self)) return NULL; - perm_hw_addr = nm_device_get_permanent_hw_address (self, TRUE); + perm_hw_addr = nm_device_get_permanent_hw_address (self); if (!perm_hw_addr) return NULL; @@ -1468,7 +1469,8 @@ update_connection (NMDevice *device, NMConnection *connection) { NMDeviceEthernetPrivate *priv = NM_DEVICE_ETHERNET_GET_PRIVATE ((NMDeviceEthernet *) device); NMSettingWired *s_wired = nm_connection_get_setting_wired (connection); - const char *perm_hw_addr = nm_device_get_permanent_hw_address (device, FALSE); + gboolean perm_hw_addr_is_fake; + const char *perm_hw_addr; const char *mac = nm_device_get_hw_address (device); const char *mac_prop = NM_SETTING_WIRED_MAC_ADDRESS; GHashTableIter iter; @@ -1487,7 +1489,8 @@ 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. */ - if (perm_hw_addr) { + perm_hw_addr = nm_device_get_permanent_hw_address_full (device, &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); mac_prop = NULL; diff --git a/src/devices/nm-device-infiniband.c b/src/devices/nm-device-infiniband.c index fa0d591754..88bd7c0aef 100644 --- a/src/devices/nm-device-infiniband.c +++ b/src/devices/nm-device-infiniband.c @@ -160,7 +160,7 @@ check_connection_compatible (NMDevice *device, NMConnection *connection) mac = nm_setting_infiniband_get_mac_address (s_infiniband); if (mac) { - hw_addr = nm_device_get_permanent_hw_address (device, TRUE); + hw_addr = nm_device_get_permanent_hw_address (device); if ( !hw_addr || !nm_utils_hwaddr_matches (mac, -1, hw_addr, -1)) return FALSE; @@ -197,7 +197,7 @@ complete_connection (NMDevice *device, } setting_mac = nm_setting_infiniband_get_mac_address (s_infiniband); - hw_address = nm_device_get_permanent_hw_address (device, TRUE); + hw_address = nm_device_get_permanent_hw_address (device); if (setting_mac) { /* Make sure the setting MAC (if any) matches the device's MAC */ if (!nm_utils_hwaddr_matches (setting_mac, -1, hw_address, -1)) { @@ -223,7 +223,7 @@ static void update_connection (NMDevice *device, NMConnection *connection) { NMSettingInfiniband *s_infiniband = nm_connection_get_setting_infiniband (connection); - const char *mac = nm_device_get_permanent_hw_address (device, TRUE); + const char *mac = nm_device_get_permanent_hw_address (device); const char *transport_mode = "datagram"; int ifindex; diff --git a/src/devices/nm-device-macvlan.c b/src/devices/nm-device-macvlan.c index 9e32694ffd..7b240ec6ae 100644 --- a/src/devices/nm-device-macvlan.c +++ b/src/devices/nm-device-macvlan.c @@ -369,7 +369,7 @@ match_hwaddr (NMDevice *device, NMConnection *connection, gboolean fail_if_no_hw if (!priv->parent) return !fail_if_no_hwaddr; - parent_mac = nm_device_get_permanent_hw_address (priv->parent, FALSE); + parent_mac = nm_device_get_permanent_hw_address (priv->parent); return parent_mac && nm_utils_hwaddr_matches (setting_mac, -1, parent_mac, -1); } diff --git a/src/devices/nm-device-vlan.c b/src/devices/nm-device-vlan.c index 71364658e6..87a36130b8 100644 --- a/src/devices/nm-device-vlan.c +++ b/src/devices/nm-device-vlan.c @@ -384,7 +384,7 @@ match_hwaddr (NMDevice *device, NMConnection *connection, gboolean fail_if_no_hw if (!priv->parent) return !fail_if_no_hwaddr; - parent_mac = nm_device_get_permanent_hw_address (priv->parent, FALSE); + parent_mac = nm_device_get_permanent_hw_address (priv->parent); return parent_mac && nm_utils_hwaddr_matches (setting_mac, -1, parent_mac, -1); } diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 6cc2b4db07..ad665db288 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -11999,7 +11999,7 @@ nm_device_hw_addr_set_cloned (NMDevice *self, NMConnection *connection, gboolean } if (nm_streq (addr, NM_CLONED_MAC_PERMANENT)) { - addr = nm_device_get_permanent_hw_address (self, TRUE); + addr = nm_device_get_permanent_hw_address (self); if (!addr) return FALSE; priv->hw_addr_type = HW_ADDR_TYPE_PERMANENT; @@ -12088,19 +12088,11 @@ nm_device_get_permanent_hw_address_full (NMDevice *self, gboolean *out_is_fake) } const char * -nm_device_get_permanent_hw_address (NMDevice *self, gboolean fallback_fake) +nm_device_get_permanent_hw_address (NMDevice *self) { - NMDevicePrivate *priv; - g_return_val_if_fail (NM_IS_DEVICE (self), NULL); - priv = NM_DEVICE_GET_PRIVATE (self); - if (!priv->hw_addr_perm) - return NULL; - if ( priv->hw_addr_perm_fake - && !fallback_fake) - return NULL; - return priv->hw_addr_perm; + return NM_DEVICE_GET_PRIVATE (self)->hw_addr_perm; } const char * @@ -12157,7 +12149,7 @@ spec_match_list (NMDevice *self, const GSList *specs) } } - hw_addr_perm = nm_device_get_permanent_hw_address (self, TRUE); + hw_addr_perm = nm_device_get_permanent_hw_address (self); if (hw_addr_perm) { m = nm_match_spec_hwaddr (specs, hw_addr_perm); matched = MAX (matched, m); @@ -12636,10 +12628,15 @@ get_property (GObject *object, guint prop_id, case PROP_HW_ADDRESS: g_value_set_string (value, priv->hw_addr); break; - case PROP_PERM_HW_ADDRESS: + case PROP_PERM_HW_ADDRESS: { + 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); /* this property is exposed on D-Bus for NMDeviceEthernet and NMDeviceWifi. */ - g_value_set_string (value, nm_device_get_permanent_hw_address (self, FALSE)); + g_value_set_string (value, perm_hw_addr && !perm_hw_addr_is_fake ? perm_hw_addr : NULL); break; + } case PROP_HAS_PENDING_ACTION: g_value_set_boolean (value, nm_device_has_pending_action (self)); break; diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 9977c7c55e..9d00fc6e10 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -362,8 +362,7 @@ guint32 nm_device_get_ip4_route_metric (NMDevice *dev); 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 *dev, - gboolean fallback_fake); +const char * nm_device_get_permanent_hw_address (NMDevice *self); const char * nm_device_get_permanent_hw_address_full (NMDevice *self, gboolean *out_is_fake); const char * nm_device_get_initial_hw_address (NMDevice *dev); diff --git a/src/devices/wifi/nm-device-wifi.c b/src/devices/wifi/nm-device-wifi.c index 320d3a1c55..8381aaccb3 100644 --- a/src/devices/wifi/nm-device-wifi.c +++ b/src/devices/wifi/nm-device-wifi.c @@ -603,7 +603,7 @@ check_connection_compatible (NMDevice *device, NMConnection *connection) if (!s_wireless) return FALSE; - perm_hw_addr = nm_device_get_permanent_hw_address (device, FALSE); + perm_hw_addr = nm_device_get_permanent_hw_address (device); mac = nm_setting_wireless_get_mac_address (s_wireless); if (perm_hw_addr) { if (mac && !nm_utils_hwaddr_matches (mac, -1, perm_hw_addr, -1)) @@ -895,7 +895,7 @@ complete_connection (NMDevice *device, if (hidden) g_object_set (s_wifi, NM_SETTING_WIRELESS_HIDDEN, TRUE, NULL); - perm_hw_addr = nm_device_get_permanent_hw_address (device, FALSE); + perm_hw_addr = nm_device_get_permanent_hw_address (device); if (perm_hw_addr) { setting_mac = nm_setting_wireless_get_mac_address (s_wifi); if (setting_mac) { diff --git a/src/nm-config.c b/src/nm-config.c index 7f22b54f74..3f492026a1 100644 --- a/src/nm-config.c +++ b/src/nm-config.c @@ -400,7 +400,7 @@ nm_config_set_no_auto_default_for_device (NMConfig *self, NMDevice *device) priv = NM_CONFIG_GET_PRIVATE (self); - hw_address = nm_device_get_permanent_hw_address (device, TRUE); + hw_address = nm_device_get_permanent_hw_address (device); if (!hw_address) return; diff --git a/src/nm-manager.c b/src/nm-manager.c index dbc8083f64..4fbd45195e 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -621,7 +621,7 @@ find_device_by_permanent_hw_addr (NMManager *manager, const char *hwaddr) if (nm_utils_hwaddr_valid (hwaddr, -1)) { for (iter = NM_MANAGER_GET_PRIVATE (manager)->devices; iter; iter = iter->next) { - device_addr = nm_device_get_permanent_hw_address (NM_DEVICE (iter->data), FALSE); + device_addr = nm_device_get_permanent_hw_address (NM_DEVICE (iter->data)); if (device_addr && nm_utils_hwaddr_matches (hwaddr, -1, device_addr, -1)) return NM_DEVICE (iter->data); } diff --git a/src/settings/nm-settings.c b/src/settings/nm-settings.c index 385a917f62..ff7fea0152 100644 --- a/src/settings/nm-settings.c +++ b/src/settings/nm-settings.c @@ -1883,7 +1883,7 @@ have_connection_for_device (NMSettings *self, NMDevice *device) g_return_val_if_fail (NM_IS_SETTINGS (self), FALSE); - perm_hw_addr = nm_device_get_permanent_hw_address (device, FALSE); + perm_hw_addr = nm_device_get_permanent_hw_address (device); /* Find a wired connection locked to the given MAC address, if any */ g_hash_table_iter_init (&iter, priv->connections); From cbea1f9f23fac42626134b38cf94dd4ebca4091d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 26 Oct 2016 12:20:12 +0200 Subject: [PATCH 06/10] device: don't allow mutating the device's hardware address length We repeatedly call nm_device_update_hw_address() to reset the cached MAC address of the device. However, we don't allow changing the address length once it is set. Multiple entities (initial, current and permanent MAC address) are all checked to have the same address length. Changing the length would be a very strange thing (and probably indicate a bug somewhere else). Just don't allow that. --- src/devices/nm-device.c | 53 ++++++++++++++++++++++++++++------------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index ad665db288..90d0ea9cdc 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -227,7 +227,10 @@ typedef struct _NMDevicePrivate { char * iface; /* may change, could be renamed by user */ int ifindex; - guint hw_addr_len; + union { + const guint8 hw_addr_len; /* read-only */ + guint8 hw_addr_len_; + }; guint8 /*HwAddrType*/ hw_addr_type; bool real; @@ -2516,11 +2519,6 @@ nm_device_unrealize (NMDevice *self, gboolean remove_resources, GError **error) g_clear_pointer (&priv->udi, g_free); _notify (self, PROP_UDI); } - if (priv->hw_addr) { - priv->hw_addr_len = 0; - g_clear_pointer (&priv->hw_addr, g_free); - _notify (self, PROP_HW_ADDRESS); - } if (priv->physical_port_id) { g_clear_pointer (&priv->physical_port_id, g_free); _notify (self, PROP_PHYSICAL_PORT_ID); @@ -2529,9 +2527,12 @@ nm_device_unrealize (NMDevice *self, gboolean remove_resources, GError **error) nm_clear_g_source (&priv->stats.timeout_id); _stats_update_counters (self, 0, 0); + priv->hw_addr_len_ = 0; + if (nm_clear_g_free (&priv->hw_addr)) + _notify (self, PROP_HW_ADDRESS); priv->hw_addr_type = HW_ADDR_TYPE_UNSET; - g_clear_pointer (&priv->hw_addr_perm, g_free); - _notify (self, PROP_PERM_HW_ADDRESS); + if (nm_clear_g_free (&priv->hw_addr_perm)) + _notify (self, PROP_PERM_HW_ADDRESS); g_clear_pointer (&priv->hw_addr_initial, g_free); priv->capabilities = NM_DEVICE_CAP_NM_SUPPORTED; @@ -11587,11 +11588,17 @@ const char * nm_device_get_hw_address (NMDevice *self) { NMDevicePrivate *priv; + char buf[NM_UTILS_HWADDR_LEN_MAX]; + gsize l; g_return_val_if_fail (NM_IS_DEVICE (self), NULL); + priv = NM_DEVICE_GET_PRIVATE (self); - nm_assert ((!priv->hw_addr) ^ (priv->hw_addr_len > 0)); + nm_assert ( (!priv->hw_addr && priv->hw_addr_len == 0) + || ( priv->hw_addr + && _nm_utils_hwaddr_aton (priv->hw_addr, buf, sizeof (buf), &l) + && l == priv->hw_addr_len)); return priv->hw_addr; } @@ -11616,9 +11623,25 @@ nm_device_update_hw_address (NMDevice *self) hwaddrlen = 0; if (hwaddrlen) { - priv->hw_addr_len = hwaddrlen; + if ( priv->hw_addr_len + && priv->hw_addr_len != hwaddrlen) { + char s_buf[NM_UTILS_HWADDR_LEN_MAX_STR]; + + /* we cannot change the address length of a device once it is set (except + * unrealizing the device). + * + * The reason is that the permanent and initial MAC addresses also must have the + * same address length, so it's unclear what it would mean that the length changes. */ + _LOGD (LOGD_PLATFORM | LOGD_DEVICE, + "hw-addr: read a MAC address with differing length (%s vs. %s)", + priv->hw_addr, + nm_utils_hwaddr_ntoa_buf (hwaddr, hwaddrlen, TRUE, s_buf, sizeof (s_buf))); + return FALSE; + } + if (!priv->hw_addr || !nm_utils_hwaddr_matches (priv->hw_addr, -1, hwaddr, hwaddrlen)) { g_free (priv->hw_addr); + priv->hw_addr_len_ = hwaddrlen; priv->hw_addr = nm_utils_hwaddr_ntoa (hwaddr, hwaddrlen); _LOGD (LOGD_PLATFORM | LOGD_DEVICE, "hw-addr: hardware address now %s", priv->hw_addr); @@ -11867,11 +11890,9 @@ _hw_addr_set (NMDevice *self, return TRUE; } - if (priv->hw_addr_len != addr_len) { - if (priv->hw_addr_len) - g_return_val_if_reached (FALSE); - priv->hw_addr_len = addr_len; - } + if ( priv->hw_addr_len + && priv->hw_addr_len != addr_len) + g_return_val_if_reached (FALSE); _LOGT (LOGD_DEVICE, "set-hw-addr: setting MAC address to '%s' (%s, %s)...", addr, operation, detail); @@ -12255,7 +12276,7 @@ constructor (GType type, g_return_val_if_reached (object); } - priv->hw_addr_len = l; + priv->hw_addr_len_ = l; priv->hw_addr = nm_utils_hwaddr_ntoa (buf, l); _LOGT (LOGD_DEVICE, "hw-addr: has permanent hw-address '%s'", priv->hw_addr_perm); } From 7b7c653c4f812f0ede676e8e0a08aa750e9e30b5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 24 Oct 2016 12:50:17 +0200 Subject: [PATCH 07/10] 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. --- src/devices/nm-device-ethernet.c | 4 +- src/devices/nm-device.c | 111 ++++++++++++++++++++----------- src/devices/nm-device.h | 3 +- src/nm-manager.c | 2 +- 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 90d0ea9cdc..f88383d417 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 initialized. */ + 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, priv->ifindex, buf, &len); - if (!success_read || len != priv->hw_addr_len) { + 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; + } + + /* 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 && dev_state->perm_hw_addr_fake && nm_utils_hwaddr_aton (dev_state->perm_hw_addr_fake, buf, priv->hw_addr_len) @@ -11745,24 +11775,18 @@ 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); } @@ -12097,13 +12121,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; } @@ -12111,9 +12144,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 * @@ -12653,7 +12684,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 4fbd45195e..21387a182d 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -4695,7 +4695,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; From c0d249b733325046ca192d112d468b86aac54ba4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 25 Oct 2016 15:27:57 +0200 Subject: [PATCH 08/10] device: delay evaluating unmanaged-by-user-settings flags until link initialized Before the link is initialized, that is before UDEV completed initializing the device, we should not evaluate the user-settings unmanaged flags. The reason is, that evaluating it likely involves looking at the permanent MAC address, which might use the wrong fake MAC address (before UDEV set the right one). Also, it might use the wrong ifname to lookup the permanent MAC address via ethtool. --- src/devices/nm-device.c | 39 ++++++++++++++++++++++++++++++++------- src/devices/nm-device.h | 2 +- src/nm-manager.c | 15 +++++++-------- 3 files changed, 40 insertions(+), 16 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index f88383d417..590b88a494 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -1874,7 +1874,7 @@ device_link_changed (NMDevice *self) ip_ifname_changed = !priv->ip_iface; if (nm_device_get_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT)) - nm_device_set_unmanaged_by_user_settings (self, nm_settings_get_unmanaged_specs (priv->settings)); + nm_device_set_unmanaged_by_user_settings (self); else update_unmanaged_specs = TRUE; @@ -1953,7 +1953,7 @@ device_link_changed (NMDevice *self) } if (update_unmanaged_specs) - nm_device_set_unmanaged_by_user_settings (self, nm_settings_get_unmanaged_specs (priv->settings)); + nm_device_set_unmanaged_by_user_settings (self); if ( got_hw_addr && !priv->up @@ -9938,6 +9938,21 @@ _set_unmanaged_flags (NMDevice *self, allow_state_transition = FALSE; was_managed = allow_state_transition && nm_device_get_managed (self, FALSE); + if ( NM_FLAGS_HAS (priv->unmanaged_flags, NM_UNMANAGED_PLATFORM_INIT) + && NM_FLAGS_HAS (flags, NM_UNMANAGED_PLATFORM_INIT) + && NM_IN_SET (set_op, NM_UNMAN_FLAG_OP_SET_MANAGED)) { + /* we are clearing the platform-init flags. This triggers additional actions. */ + if (!NM_FLAGS_HAS (flags, NM_UNMANAGED_USER_SETTINGS)) { + gboolean unmanaged; + + unmanaged = nm_device_spec_match_list (self, + nm_settings_get_unmanaged_specs (NM_DEVICE_GET_PRIVATE (self)->settings)); + nm_device_set_unmanaged_flags (self, + NM_UNMANAGED_USER_SETTINGS, + !!unmanaged); + } + } + old_flags = priv->unmanaged_flags; old_mask = priv->unmanaged_mask; @@ -10052,20 +10067,30 @@ nm_device_set_unmanaged_by_flags_queue (NMDevice *self, } void -nm_device_set_unmanaged_by_user_settings (NMDevice *self, const GSList *unmanaged_specs) +nm_device_set_unmanaged_by_user_settings (NMDevice *self) { - NMDevicePrivate *priv; gboolean unmanaged; g_return_if_fail (NM_IS_DEVICE (self)); - priv = NM_DEVICE_GET_PRIVATE (self); + if (nm_device_get_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT)) { + /* the device is already unmanaged due to platform-init. + * + * We want to delay evaluating the device spec, because it will freeze + * the permanent MAC address. That should not be done, before the platform + * link is fully initialized (via UDEV). + * + * Note that when clearing NM_UNMANAGED_PLATFORM_INIT, we will re-evaluate + * whether the device is unmanaged by user-settings. */ + return; + } - unmanaged = nm_device_spec_match_list (self, unmanaged_specs); + unmanaged = nm_device_spec_match_list (self, + nm_settings_get_unmanaged_specs (NM_DEVICE_GET_PRIVATE (self)->settings)); nm_device_set_unmanaged_by_flags (self, NM_UNMANAGED_USER_SETTINGS, - unmanaged, + !!unmanaged, unmanaged ? NM_DEVICE_STATE_REASON_NOW_UNMANAGED : NM_DEVICE_STATE_REASON_NOW_MANAGED); diff --git a/src/devices/nm-device.h b/src/devices/nm-device.h index 5ad64adcb0..356331189c 100644 --- a/src/devices/nm-device.h +++ b/src/devices/nm-device.h @@ -508,7 +508,7 @@ void nm_device_set_unmanaged_by_flags_queue (NMDevice *self, NMUnmanagedFlags flags, NMUnmanFlagOp set_op, NMDeviceStateReason reason); -void nm_device_set_unmanaged_by_user_settings (NMDevice *self, const GSList *unmanaged_specs); +void nm_device_set_unmanaged_by_user_settings (NMDevice *self); void nm_device_set_unmanaged_by_user_udev (NMDevice *self); void nm_device_set_unmanaged_by_quitting (NMDevice *device); diff --git a/src/nm-manager.c b/src/nm-manager.c index 21387a182d..556ae32e9d 100644 --- a/src/nm-manager.c +++ b/src/nm-manager.c @@ -1331,11 +1331,10 @@ system_unmanaged_devices_changed_cb (NMSettings *settings, { NMManager *self = NM_MANAGER (user_data); NMManagerPrivate *priv = NM_MANAGER_GET_PRIVATE (self); - const GSList *unmanaged_specs, *iter; + const GSList *iter; - unmanaged_specs = nm_settings_get_unmanaged_specs (priv->settings); for (iter = priv->devices; iter; iter = g_slist_next (iter)) - nm_device_set_unmanaged_by_user_settings (NM_DEVICE (iter->data), unmanaged_specs); + nm_device_set_unmanaged_by_user_settings (NM_DEVICE (iter->data)); } static void @@ -2004,7 +2003,7 @@ add_device (NMManager *self, NMDevice *device, GError **error) type_desc = nm_device_get_type_desc (device); g_assert (type_desc); - nm_device_set_unmanaged_by_user_settings (device, nm_settings_get_unmanaged_specs (priv->settings)); + nm_device_set_unmanaged_by_user_settings (device); nm_device_set_unmanaged_flags (device, NM_UNMANAGED_SLEEPING, @@ -2880,15 +2879,15 @@ unmanaged_to_disconnected (NMDevice *device) if (nm_device_get_state (device) == NM_DEVICE_STATE_UNMANAGED) { nm_device_state_changed (device, - NM_DEVICE_STATE_UNAVAILABLE, - NM_DEVICE_STATE_REASON_USER_REQUESTED); + NM_DEVICE_STATE_UNAVAILABLE, + NM_DEVICE_STATE_REASON_USER_REQUESTED); } if ( nm_device_is_available (device, NM_DEVICE_CHECK_DEV_AVAILABLE_FOR_USER_REQUEST) && (nm_device_get_state (device) == NM_DEVICE_STATE_UNAVAILABLE)) { nm_device_state_changed (device, - NM_DEVICE_STATE_DISCONNECTED, - NM_DEVICE_STATE_REASON_USER_REQUESTED); + NM_DEVICE_STATE_DISCONNECTED, + NM_DEVICE_STATE_REASON_USER_REQUESTED); } } From 31ca7962f8f7d1993f0a363b9677c7cee89e7ee3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 26 Oct 2016 13:43:49 +0200 Subject: [PATCH 09/10] device: don't evaluate IP config changes until device is initialized The unmanaged flags PLATFORM_INIT indicates whether UDEV is done initializing the device. We should not handle IP config changes before that pointer. This avoids codepaths that require the permanent MAC address of the device. We should not freeze the permanent MAC address before UDEV initialized the device, for two reasons: - getting the permanent MAC address using ethtool is racy as UDEV might still rename the interface. - freezing a fake permanent MAC address should only happen after UDEV is done configuring the MAC address of software devices. #0 0x000055555568bc7a in nm_device_update_permanent_hw_address (self=self@entry=0x555555f0fb70 [NMDeviceVeth], force_freeze=force_freeze@entry=1) at src/devices/nm-device.c:11817 #1 0x000055555568c443 in nm_device_get_permanent_hw_address_full (self=self@entry=0x555555f0fb70 [NMDeviceVeth], force_freeze=force_freeze@entry=1, out_is_fake=out_is_fake@entry=0x0) at src/devices/nm-device.c:12227 #2 0x000055555568cb06 in nm_device_get_permanent_hw_address (self=self@entry=0x555555f0fb70 [NMDeviceVeth]) at src/devices/nm-device.c:12237 #3 0x000055555568cb50 in spec_match_list (self=0x555555f0fb70 [NMDeviceVeth], specs=0x555555a5c000 = {...}) at src/devices/nm-device.c:12294 #4 0x00005555556a4ee6 in spec_match_list (device=0x555555f0fb70 [NMDeviceVeth], specs=0x555555a5c000 = {...}) at src/devices/nm-device-ethernet.c:1461 #5 0x00005555556978db in nm_device_spec_match_list (self=self@entry=0x555555f0fb70 [NMDeviceVeth], specs=0x555555a5c000 = {...}) at src/devices/nm-device.c:12277 #6 0x000055555558e187 in _match_section_infos_lookup (match_section_infos=0x555555a5d500, keyfile=0x555555a46f80, property=property@entry=0x555555793123 "ipv4.route-metric", device=device@entry=0x555555f0fb70 [NMDeviceVeth], out_value=out_value@entry=0x7fffffffe018) at src/nm-config-data.c:1169 #7 0x00005555555922ca in nm_config_data_get_connection_default (self=0x555555a548c0 [NMConfigData], property=property@entry=0x555555793123 "ipv4.route-metric", device=device@entry=0x555555f0fb70 [NMDeviceVeth]) at src/nm-config-data.c:1234 #8 0x00005555556790cd in _get_ipx_route_metric (self=self@entry=0x555555f0fb70 [NMDeviceVeth], is_v4=is_v4@entry=1) at src/devices/nm-device.c:1142 #9 0x000055555567912e in nm_device_get_ip4_route_metric (self=self@entry=0x555555f0fb70 [NMDeviceVeth]) at src/devices/nm-device.c:1161 #10 0x000055555567da6c in ip4_config_merge_and_apply (self=self@entry=0x555555f0fb70 [NMDeviceVeth], config=config@entry=0x0, commit=commit@entry=0, out_reason=out_reason@entry=0x0) at src/devices/nm-device.c:4787 #11 0x000055555567e0fb in update_ip4_config (self=self@entry=0x555555f0fb70 [NMDeviceVeth], initial=initial@entry=0) at src/devices/nm-device.c:9532 #12 0x0000555555693acd in queued_ip4_config_change (user_data=0x555555f0fb70) at src/devices/nm-device.c:9651 #13 0x00007ffff4c966ba in g_main_context_dispatch (context=0x555555a46af0) at gmain.c:3154 #14 0x00007ffff4c966ba in g_main_context_dispatch (context=context@entry=0x555555a46af0) at gmain.c:3769 #15 0x00007ffff4c96a70 in g_main_context_iterate (context=0x555555a46af0, block=block@entry=1, dispatch=dispatch@entry=1, self=) at gmain.c:3840 #16 0x00007ffff4c96d92 in g_main_loop_run (loop=0x555555a47400) at gmain.c:4034 #17 0x000055555558372a in main (argc=, argv=) at src/main.c:411 --- src/devices/nm-device.c | 44 +++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 590b88a494..9fd8a1ed5b 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -233,7 +233,12 @@ typedef struct _NMDevicePrivate { }; guint8 /*HwAddrType*/ hw_addr_type; - bool real; + bool real:1; + + /* there was a IP config change, but no idle action was scheduled because device + * is still not platform-init */ + bool queued_ip4_config_pending:1; + bool queued_ip6_config_pending:1; char * ip_iface; int ip_ifindex; @@ -2323,9 +2328,8 @@ realize_start_setup (NMDevice *self, const NMPlatformLink *plink) _notify (self, PROP_UDI); } - /* trigger initial ip config change to initialize ip-config */ - priv->queued_ip4_config_id = g_idle_add (queued_ip4_config_change, self); - priv->queued_ip6_config_id = g_idle_add (queued_ip6_config_change, self); + priv->queued_ip4_config_pending = TRUE; + priv->queued_ip6_config_pending = TRUE; nm_device_update_hw_address (self); nm_device_update_initial_hw_address (self); @@ -7698,6 +7702,7 @@ _cleanup_ip4_pre (NMDevice *self, CleanupType cleanup_type) if (nm_clear_g_source (&priv->queued_ip4_config_id)) _LOGD (LOGD_DEVICE, "clearing queued IP4 config change"); + priv->queued_ip4_config_pending = FALSE; dhcp4_cleanup (self, cleanup_type, FALSE); arp_cleanup (self); @@ -7714,6 +7719,7 @@ _cleanup_ip6_pre (NMDevice *self, CleanupType cleanup_type) if (nm_clear_g_source (&priv->queued_ip6_config_id)) _LOGD (LOGD_DEVICE, "clearing queued IP6 config change"); + priv->queued_ip6_config_pending = FALSE; g_clear_object (&priv->dad6_ip6_config); dhcp6_cleanup (self, cleanup_type, FALSE); @@ -9409,6 +9415,7 @@ update_ip4_config (NMDevice *self, gboolean initial) && activation_source_is_scheduled (self, activate_stage5_ip4_config_commit, AF_INET)) { + priv->queued_ip4_config_pending = FALSE; priv->queued_ip4_config_id = g_idle_add (queued_ip4_config_change, self); _LOGT (LOGD_DEVICE, "IP4 update was postponed"); return; @@ -9499,6 +9506,7 @@ update_ip6_config (NMDevice *self, gboolean initial) && activation_source_is_scheduled (self, activate_stage5_ip6_config_commit, AF_INET6)) { + priv->queued_ip6_config_pending = FALSE; priv->queued_ip6_config_id = g_idle_add (queued_ip6_config_change, self); _LOGT (LOGD_DEVICE, "IP6 update was postponed"); return; @@ -9576,6 +9584,8 @@ queued_ip4_config_change (gpointer user_data) priv = NM_DEVICE_GET_PRIVATE (self); + nm_assert (!priv->queued_ip4_config_pending); + /* Wait for any queued state changes */ if (priv->queued_state.id) return TRUE; @@ -9600,6 +9610,8 @@ queued_ip6_config_change (gpointer user_data) priv = NM_DEVICE_GET_PRIVATE (self); + nm_assert (!priv->queued_ip4_config_pending); + /* Wait for any queued state changes */ if (priv->queued_state.id) return TRUE; @@ -9679,7 +9691,11 @@ device_ipx_changed (NMPlatform *platform, switch (obj_type) { case NMP_OBJECT_TYPE_IP4_ADDRESS: case NMP_OBJECT_TYPE_IP4_ROUTE: - if (!priv->queued_ip4_config_id) { + if (nm_device_get_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT)) { + priv->queued_ip4_config_pending = TRUE; + nm_assert_se (!nm_clear_g_source (&priv->queued_ip4_config_id)); + } else if (!priv->queued_ip4_config_id) { + priv->queued_ip4_config_pending = FALSE; priv->queued_ip4_config_id = g_idle_add (queued_ip4_config_change, self); _LOGD (LOGD_DEVICE, "queued IP4 config change"); } @@ -9696,7 +9712,11 @@ device_ipx_changed (NMPlatform *platform, } /* fallthrough */ case NMP_OBJECT_TYPE_IP6_ROUTE: - if (!priv->queued_ip6_config_id) { + if (nm_device_get_unmanaged_flags (self, NM_UNMANAGED_PLATFORM_INIT)) { + priv->queued_ip6_config_pending = TRUE; + nm_assert_se (!nm_clear_g_source (&priv->queued_ip6_config_id)); + } else if (!priv->queued_ip6_config_id) { + priv->queued_ip6_config_pending = FALSE; priv->queued_ip6_config_id = g_idle_add (queued_ip6_config_change, self); _LOGD (LOGD_DEVICE, "queued IP6 config change"); } @@ -9951,6 +9971,18 @@ _set_unmanaged_flags (NMDevice *self, NM_UNMANAGED_USER_SETTINGS, !!unmanaged); } + + if (priv->queued_ip4_config_pending) { + priv->queued_ip4_config_pending = FALSE; + nm_assert_se (!nm_clear_g_source (&priv->queued_ip4_config_id)); + priv->queued_ip4_config_id = g_idle_add (queued_ip4_config_change, self); + } + + if (priv->queued_ip6_config_pending) { + priv->queued_ip6_config_pending = FALSE; + nm_assert_se (!nm_clear_g_source (&priv->queued_ip6_config_id)); + priv->queued_ip6_config_id = g_idle_add (queued_ip6_config_change, self); + } } old_flags = priv->unmanaged_flags; From 0e0018c8016156b9a01028f1448e7baf76e14d4a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 28 Oct 2016 17:06:13 +0200 Subject: [PATCH 10/10] device: suppress log message in nm_device_update_hw_address() when no MAC address For example for tun devices we get a lot of (tun7): hw-addr: failed reading current MAC address warnings. Just be silent about it. We log when something changes, we don't need to log when we fail to obtain a MAC address. Thereby, refactor nm_device_update_hw_address() to return early. --- src/devices/nm-device.c | 83 ++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 46 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 9fd8a1ed5b..27c111964f 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -11666,7 +11666,6 @@ nm_device_update_hw_address (NMDevice *self) NMDevicePrivate *priv; const guint8 *hwaddr; gsize hwaddrlen = 0; - gboolean changed = FALSE; priv = NM_DEVICE_GET_PRIVATE (self); if (priv->ifindex <= 0) @@ -11679,54 +11678,46 @@ nm_device_update_hw_address (NMDevice *self) && nm_utils_hwaddr_matches (hwaddr, hwaddrlen, nm_ip_addr_zero.addr_eth, sizeof (nm_ip_addr_zero.addr_eth))) hwaddrlen = 0; - if (hwaddrlen) { - if ( priv->hw_addr_len - && priv->hw_addr_len != hwaddrlen) { - char s_buf[NM_UTILS_HWADDR_LEN_MAX_STR]; + if (!hwaddrlen) + return FALSE; - /* we cannot change the address length of a device once it is set (except - * unrealizing the device). - * - * The reason is that the permanent and initial MAC addresses also must have the - * same address length, so it's unclear what it would mean that the length changes. */ - _LOGD (LOGD_PLATFORM | LOGD_DEVICE, - "hw-addr: read a MAC address with differing length (%s vs. %s)", - priv->hw_addr, - nm_utils_hwaddr_ntoa_buf (hwaddr, hwaddrlen, TRUE, s_buf, sizeof (s_buf))); - return FALSE; - } + if ( priv->hw_addr_len + && priv->hw_addr_len != hwaddrlen) { + char s_buf[NM_UTILS_HWADDR_LEN_MAX_STR]; - if (!priv->hw_addr || !nm_utils_hwaddr_matches (priv->hw_addr, -1, hwaddr, hwaddrlen)) { - g_free (priv->hw_addr); - priv->hw_addr_len_ = hwaddrlen; - priv->hw_addr = nm_utils_hwaddr_ntoa (hwaddr, hwaddrlen); - - _LOGD (LOGD_PLATFORM | LOGD_DEVICE, "hw-addr: hardware address now %s", priv->hw_addr); - _notify (self, PROP_HW_ADDRESS); - - if ( !priv->hw_addr_initial - || ( priv->hw_addr_type == HW_ADDR_TYPE_UNSET - && priv->state < NM_DEVICE_STATE_PREPARE - && !nm_device_is_activating (self))) { - /* when we get a hw_addr the first time or while the device - * is not activated (with no explict hw address set), always - * update our inital hw-address as well. */ - nm_device_update_initial_hw_address (self); - } - changed = TRUE; - } - } else { - /* Invalid or no hardware address */ - if (priv->hw_addr_len != 0) { - _LOGD (LOGD_PLATFORM | LOGD_DEVICE, - "hw-addr: failed reading current MAC address (stay with %s)", - priv->hw_addr); - } else { - _LOGD (LOGD_PLATFORM | LOGD_DEVICE, - "hw-addr: failed reading current MAC address"); - } + /* we cannot change the address length of a device once it is set (except + * unrealizing the device). + * + * The reason is that the permanent and initial MAC addresses also must have the + * same address length, so it's unclear what it would mean that the length changes. */ + _LOGD (LOGD_PLATFORM | LOGD_DEVICE, + "hw-addr: read a MAC address with differing length (%s vs. %s)", + priv->hw_addr, + nm_utils_hwaddr_ntoa_buf (hwaddr, hwaddrlen, TRUE, s_buf, sizeof (s_buf))); + return FALSE; } - return changed; + + if ( priv->hw_addr + && nm_utils_hwaddr_matches (priv->hw_addr, -1, hwaddr, hwaddrlen)) + return FALSE; + + g_free (priv->hw_addr); + priv->hw_addr_len_ = hwaddrlen; + priv->hw_addr = nm_utils_hwaddr_ntoa (hwaddr, hwaddrlen); + + _LOGD (LOGD_PLATFORM | LOGD_DEVICE, "hw-addr: hardware address now %s", priv->hw_addr); + _notify (self, PROP_HW_ADDRESS); + + if ( !priv->hw_addr_initial + || ( priv->hw_addr_type == HW_ADDR_TYPE_UNSET + && priv->state < NM_DEVICE_STATE_PREPARE + && !nm_device_is_activating (self))) { + /* when we get a hw_addr the first time or while the device + * is not activated (with no explict hw address set), always + * update our inital hw-address as well. */ + nm_device_update_initial_hw_address (self); + } + return TRUE; } void