From b9ce5ae9d752eed1bb6dbc21065ee1746dff498a Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 22 Jun 2020 17:04:06 +0200 Subject: [PATCH 1/7] ppp: fix taking control of link generated by kernel NetworkManager can't control the name of the PPP interface name created by pppd; so it has to wait for the interface to appear and then rename it. This happens in nm_device_take_over_link() called by nm-device-ppp.c:ppp_ifindex_set() when pppd tells NM the ifindex of the interface that was created. However, sometimes the initial interface name is already correct, for example when the connection.interface-name is ppp0 and this is the first PPP interface created. When this happens, nm_device_update_from_platform_link() is called on the NMDevicePPP and it sets the device ifindex. Later, when pppd notifies NM, nm_device_take_over_link() fails because the ifindex is already set: nm_device_take_over_link: assertion 'priv->ifindex <= 0' failed Make nm_device_take_over_link() more robust to cope with this situation. https://bugzilla.redhat.com/show_bug.cgi?id=1849386 --- src/devices/nm-device-ppp.c | 6 +++- src/devices/nm-device-private.h | 2 +- src/devices/nm-device.c | 51 ++++++++++++++++++++++++++------- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/src/devices/nm-device-ppp.c b/src/devices/nm-device-ppp.c index 52784143a1..cbc871419a 100644 --- a/src/devices/nm-device-ppp.c +++ b/src/devices/nm-device-ppp.c @@ -71,9 +71,13 @@ ppp_ifindex_set (NMPPPManager *ppp_manager, gpointer user_data) { NMDevice *device = NM_DEVICE (user_data); + NMDevicePpp *self = NM_DEVICE_PPP (device); gs_free char *old_name = NULL; + gs_free_error GError *error = NULL; - if (!nm_device_take_over_link (device, ifindex, &old_name)) { + if (!nm_device_take_over_link (device, ifindex, &old_name, &error)) { + _LOGW (LOGD_DEVICE | LOGD_PPP, "could not take control of link %d: %s", + ifindex, error->message); nm_device_state_changed (device, NM_DEVICE_STATE_FAILED, NM_DEVICE_STATE_REASON_IP_CONFIG_UNAVAILABLE); return; diff --git a/src/devices/nm-device-private.h b/src/devices/nm-device-private.h index 4e260da2f3..cd72f3d712 100644 --- a/src/devices/nm-device-private.h +++ b/src/devices/nm-device-private.h @@ -62,7 +62,7 @@ gboolean nm_device_bring_up (NMDevice *self, gboolean wait, gboolean *no_firmwar void nm_device_take_down (NMDevice *self, gboolean block); -gboolean nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name); +gboolean nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name, GError **error); gboolean nm_device_hw_addr_set (NMDevice *device, const char *addr, diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index c46952de57..0c1ae60a55 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -1746,25 +1746,51 @@ nm_device_get_iface (NMDevice *self) return NM_DEVICE_GET_PRIVATE (self)->iface; } +/** + * nm_device_take_over_link: + * @self: the #NMDevice + * @ifindex: a ifindex + * @old_name: (transfer full): on return, the name of the old link, if + * the link was renamed + * @error: location to store error, or %NULL + * + * Given an existing link, move it under the control of a device. In + * particular, the link will be renamed to match the device name. If the + * link was renamed, the old name is returned in @old_name. + * + * Returns: %TRUE if the device took control of the link, %FALSE otherwise + */ gboolean -nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name) +nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name, GError **error) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); const NMPlatformLink *plink; NMPlatform *platform; - gboolean up, success = TRUE; - gs_free char *name = NULL; - - g_return_val_if_fail (priv->ifindex <= 0, FALSE); + nm_assert (ifindex > 0); NM_SET_OUT (old_name, NULL); + if ( priv->ifindex > 0 + && priv->ifindex != ifindex) { + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + "the device already has ifindex %d", + priv->ifindex); + return FALSE; + } + platform = nm_device_get_platform (self); plink = nm_platform_link_get (platform, ifindex); - if (!plink) + if (!plink) { + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + "link %d not found", ifindex); return FALSE; + } if (!nm_streq (plink->name, nm_device_get_iface (self))) { + gboolean up; + gboolean success; + gs_free char *name = NULL; + up = NM_FLAGS_HAS (plink->n_ifi_flags, IFF_UP); name = g_strdup (plink->name); @@ -1775,16 +1801,21 @@ nm_device_take_over_link (NMDevice *self, int ifindex, char **old_name) if (up) nm_platform_link_set_up (platform, ifindex, NULL); - if (success) - NM_SET_OUT (old_name, g_steal_pointer (&name)); + if (!success) { + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + "failure renaming link %d", ifindex); + return FALSE; + } + + NM_SET_OUT (old_name, g_steal_pointer (&name)); } - if (success) { + if (priv->ifindex != ifindex) { priv->ifindex = ifindex; _notify (self, PROP_IFINDEX); } - return success; + return TRUE; } int From d67ad4c86b03a247162e2f8bb18106b15d6d33da Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Fri, 3 Jul 2020 17:13:35 +0200 Subject: [PATCH 2/7] platform: always display route type when calling nmp_object_to_string() https://bugzilla.redhat.com/show_bug.cgi?id=1821787 --- src/platform/nm-platform.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 880e7b5d27..2181ca6f6f 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -6082,11 +6082,11 @@ nm_platform_ip4_route_to_string (const NMPlatformIP4Route *route, char *buf, gsi char s_network[INET_ADDRSTRLEN], s_gateway[INET_ADDRSTRLEN]; char s_pref_src[INET_ADDRSTRLEN]; char str_dev[TO_STRING_DEV_BUF_SIZE]; - char str_type[30]; char str_table[30]; char str_scope[30], s_source[50]; char str_tos[32], str_window[32], str_cwnd[32], str_initcwnd[32], str_initrwnd[32], str_mtu[32]; char str_rtm_flags[_RTM_FLAGS_TO_STRING_MAXLEN]; + char str_type[30]; if (!nm_utils_to_string_buffer_init_null (route, &buf, &len)) return buf; @@ -6097,7 +6097,7 @@ nm_platform_ip4_route_to_string (const NMPlatformIP4Route *route, char *buf, gsi _to_string_dev (NULL, route->ifindex, str_dev, sizeof (str_dev)); g_snprintf (buf, len, - "%s" /* type */ + "type %s " /* type */ "%s" /* table */ "%s/%d" " via %s" @@ -6115,7 +6115,7 @@ nm_platform_ip4_route_to_string (const NMPlatformIP4Route *route, char *buf, gsi "%s" /* initrwnd */ "%s" /* mtu */ "", - route->type_coerced ? nm_sprintf_buf (str_type, "type %s ", nm_utils_route_type2str (nm_platform_route_type_uncoerce (route->type_coerced), NULL, 0)) : "", + nm_utils_route_type2str (nm_platform_route_type_uncoerce (route->type_coerced), str_type, sizeof (str_type)), route->table_coerced ? nm_sprintf_buf (str_table, "table %u ", nm_platform_route_table_uncoerce (route->table_coerced, FALSE)) : "", s_network, route->plen, @@ -6185,7 +6185,7 @@ nm_platform_ip6_route_to_string (const NMPlatformIP6Route *route, char *buf, gsi _to_string_dev (NULL, route->ifindex, str_dev, sizeof (str_dev)); g_snprintf (buf, len, - "%s" /* type */ + "type %s " /* type */ "%s" /* table */ "%s/%d" " via %s" @@ -6203,7 +6203,7 @@ nm_platform_ip6_route_to_string (const NMPlatformIP6Route *route, char *buf, gsi "%s" /* mtu */ "%s" /* pref */ "", - route->type_coerced ? nm_sprintf_buf (str_type, "type %s ", nm_utils_route_type2str (nm_platform_route_type_uncoerce (route->type_coerced), NULL, 0)) : "", + nm_utils_route_type2str (nm_platform_route_type_uncoerce (route->type_coerced), str_type, sizeof (str_type)), route->table_coerced ? nm_sprintf_buf (str_table, "table %u ", nm_platform_route_table_uncoerce (route->table_coerced, FALSE)) : "", s_network, route->plen, From e3e7bdf96ef7a1c551dab375386e62d12689775c Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Fri, 3 Jul 2020 17:12:15 +0200 Subject: [PATCH 3/7] utils: add 'unspecified' to nm_utils_route_type2str() https://bugzilla.redhat.com/show_bug.cgi?id=1821787 --- libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c index 8d2ea09cdf..0e53016490 100644 --- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c +++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c @@ -207,4 +207,5 @@ NM_UTILS_ENUM2STR_DEFINE (nm_utils_route_type2str, guint8, NM_UTILS_ENUM2STR (RTN_THROW, "throw"), NM_UTILS_ENUM2STR (RTN_UNICAST, "unicast"), NM_UTILS_ENUM2STR (RTN_UNREACHABLE, "unreachable"), + NM_UTILS_ENUM2STR (RTN_UNSPEC, "unspecified"), ); From 04878193f754d1f7708b70f4d146caa48d276873 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Fri, 3 Jul 2020 17:16:35 +0200 Subject: [PATCH 4/7] platform: parse route type from netlink messages https://bugzilla.redhat.com/show_bug.cgi?id=1821787 --- src/platform/nm-linux-platform.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/platform/nm-linux-platform.c b/src/platform/nm-linux-platform.c index 40451df820..b05c50de48 100644 --- a/src/platform/nm-linux-platform.c +++ b/src/platform/nm-linux-platform.c @@ -3398,6 +3398,7 @@ rta_multipath_done: obj = nmp_object_new (is_v4 ? NMP_OBJECT_TYPE_IP4_ROUTE : NMP_OBJECT_TYPE_IP6_ROUTE, NULL); + obj->ip_route.type_coerced = nm_platform_route_type_coerce (rtm->rtm_type); obj->ip_route.table_coerced = nm_platform_route_table_coerce ( tb[RTA_TABLE] ? nla_get_u32 (tb[RTA_TABLE]) : (guint32) rtm->rtm_table); From cd89026c5f4fa29c7bf46edb5b610ec37c18be78 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Mon, 6 Jul 2020 12:44:54 +0200 Subject: [PATCH 5/7] core: add dependent multicast route configured by kernel for IPv6 Pre-generate the device multicast route in the local table that are configured by kernel when an ipv6-address is assigned to an interface. This helps NM taking into account routes that are not to be deleted when a connection is reapplied on an interface. https://bugzilla.redhat.com/show_bug.cgi?id=1821787 --- src/nm-ip6-config.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/nm-ip6-config.c b/src/nm-ip6-config.c index d772d6a255..404b201f87 100644 --- a/src/nm-ip6-config.c +++ b/src/nm-ip6-config.c @@ -456,6 +456,24 @@ nm_ip6_config_update_routes_metric (NMIP6Config *self, gint64 metric) g_object_thaw_notify (G_OBJECT (self)); } +static void +_add_multicast_route6 (NMIP6Config *self, int ifindex) +{ + nm_auto_nmpobj NMPObject *r = NULL; + NMPlatformIP6Route *route; + + r = nmp_object_new (NMP_OBJECT_TYPE_IP6_ROUTE, NULL); + route = NMP_OBJECT_CAST_IP6_ROUTE (r); + route->ifindex = ifindex; + route->network.s6_addr[0] = 0xffu; + route->plen = 8; + route->table_coerced = nm_platform_route_table_coerce (RT_TABLE_LOCAL); + route->type_coerced = nm_platform_route_type_coerce (RTN_UNICAST); + route->metric = 256; + + _add_route (self, r, NULL, NULL); +} + void nm_ip6_config_add_dependent_routes (NMIP6Config *self, guint32 route_table, @@ -476,6 +494,9 @@ nm_ip6_config_add_dependent_routes (NMIP6Config *self, * * For manually added IPv6 routes, add the device routes explicitly. */ + /* Pre-generate multicast route */ + _add_multicast_route6 (self, ifindex); + nm_ip_config_iter_ip6_address_for_each (&iter, self, &my_addr) { NMPlatformIP6Route *route; gboolean has_peer; From 9ecc27f6d33f306c0e814bedef293eef783f27f1 Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Wed, 8 Jul 2020 12:27:46 +0200 Subject: [PATCH 6/7] platform: do not prune kernel added routes IPv6 routes having metric 0 and routes having rt_source == kernel are entirely managed by kernel, NM should not try to remove them. https://bugzilla.redhat.com/show_bug.cgi?id=1821787 --- src/platform/nm-platform.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/platform/nm-platform.c b/src/platform/nm-platform.c index 2181ca6f6f..08822b1cc2 100644 --- a/src/platform/nm-platform.c +++ b/src/platform/nm-platform.c @@ -4230,9 +4230,18 @@ nm_platform_ip_route_get_prune_list (NMPlatform *self, } else if (route_table_sync == NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN) { if (!nm_platform_route_table_is_main (NMP_OBJECT_CAST_IP_ROUTE (obj)->table_coerced)) continue; - } else + } else { nm_assert (route_table_sync == NM_IP_ROUTE_TABLE_SYNC_MODE_ALL); + /* IPv6 routes having metric 0 and routes having rt_source == NM_IP_CONFIG_SOURCE_RTPROT_KERNEL + * are entirely managed by kernel, let's not touch them */ + if (addr_family == AF_INET6 && NMP_OBJECT_CAST_IP6_ROUTE (obj)->metric == 0) + continue; + if ( nm_platform_route_table_uncoerce (NMP_OBJECT_CAST_IP_ROUTE (obj)->table_coerced, TRUE) == RT_TABLE_LOCAL + && NMP_OBJECT_CAST_IP_ROUTE (obj)->rt_source == NM_IP_CONFIG_SOURCE_RTPROT_KERNEL) + continue; + } + g_ptr_array_add (routes_prune, (gpointer) nmp_object_ref (obj)); } From c5496f7372eb076fe55045cdac40415943c1b3fa Mon Sep 17 00:00:00 2001 From: Antonio Cardace Date: Mon, 6 Jul 2020 19:09:35 +0200 Subject: [PATCH 7/7] nm-device: change route table sync mode behaviour NM will now sync all tables when a connection has specified at least 1 local route in 'ipv[4|6].routes' to correctly reconcile local routes when reapplying connections on a device. If the connection has no local routes only the main table will be taken into account preserving the previous NM's behaviour. https://bugzilla.redhat.com/show_bug.cgi?id=1821787 --- src/devices/nm-device.c | 69 ++++++++++++++++++++++++++++------------- 1 file changed, 48 insertions(+), 21 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 0c1ae60a55..15162dabd1 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -411,8 +411,8 @@ typedef struct _NMDevicePrivate { bool v4_route_table_initialized:1; bool v6_route_table_initialized:1; - bool v4_route_table_full_sync_before:1; - bool v6_route_table_full_sync_before:1; + bool v4_route_table_all_sync_before:1; + bool v6_route_table_all_sync_before:1; NMDeviceAutoconnectBlockedFlags autoconnect_blocked_flags:5; @@ -2737,34 +2737,59 @@ _get_route_table_sync_mode_stateful (NMDevice *self, int addr_family) { NMDevicePrivate *priv = NM_DEVICE_GET_PRIVATE (self); - gboolean full_sync_now; - gboolean full_sync_eff; + NMDedupMultiIter ipconf_iter; + gboolean all_sync_now; + gboolean all_sync_eff; - full_sync_now = _get_route_table (self, addr_family) != 0u; + all_sync_now = _get_route_table (self, addr_family) != 0u; - if (full_sync_now) - full_sync_eff = TRUE; + if (!all_sync_now) { + /* If there's a local route switch to all-sync in order + * to properly manage the local table */ + if (addr_family == AF_INET) { + const NMPlatformIP4Route *route; + + nm_ip_config_iter_ip4_route_for_each (&ipconf_iter, priv->con_ip_config_4, &route) { + if (nm_platform_route_type_uncoerce (route->type_coerced) == RTN_LOCAL) { + all_sync_now = TRUE; + break; + } + } + } else { + const NMPlatformIP6Route *route; + + nm_ip_config_iter_ip6_route_for_each (&ipconf_iter, priv->con_ip_config_6, &route) { + if (nm_platform_route_type_uncoerce (route->type_coerced) == RTN_LOCAL) { + all_sync_now = TRUE; + break; + } + } + } + } + + if (all_sync_now) + all_sync_eff = TRUE; else { - /* When we change from full-sync to no full-sync, we do a last full-sync one - * more time. For that, we determine the effective full-state based on the - * cached/previous full-sync flag. + /* When we change from all-sync to no all-sync, we do a last all-sync one + * more time. For that, we determine the effective all-state based on the + * cached/previous all-sync flag. * * The purpose of this is to support reapply of route-table (and thus the - * full-sync mode). If reapply toggles from full-sync to no-full-sync, we must + * all-sync mode). If reapply toggles from all-sync to no-all-sync, we must * sync one last time. */ if (addr_family == AF_INET) - full_sync_eff = priv->v4_route_table_full_sync_before; + all_sync_eff = priv->v4_route_table_all_sync_before; else - full_sync_eff = priv->v6_route_table_full_sync_before; + all_sync_eff = priv->v6_route_table_all_sync_before; } if (addr_family == AF_INET) - priv->v4_route_table_full_sync_before = full_sync_now; + priv->v4_route_table_all_sync_before = all_sync_now; else - priv->v6_route_table_full_sync_before = full_sync_now; + priv->v6_route_table_all_sync_before = all_sync_now; - return full_sync_eff - ? NM_IP_ROUTE_TABLE_SYNC_MODE_FULL + return all_sync_eff + ? NM_IP_ROUTE_TABLE_SYNC_MODE_ALL : NM_IP_ROUTE_TABLE_SYNC_MODE_MAIN; } @@ -13244,7 +13269,8 @@ nm_device_set_ip_config (NMDevice *self, if (IS_IPv4) { success = nm_ip4_config_commit (NM_IP4_CONFIG (new_config), nm_device_get_platform (self), - _get_route_table_sync_mode_stateful (self, addr_family)); + _get_route_table_sync_mode_stateful (self, + AF_INET)); nm_platform_ip4_dev_route_blacklist_set (nm_device_get_platform (self), nm_ip_config_get_ifindex (new_config), ip4_dev_route_blacklist); @@ -13253,7 +13279,8 @@ nm_device_set_ip_config (NMDevice *self, success = nm_ip6_config_commit (NM_IP6_CONFIG (new_config), nm_device_get_platform (self), - _get_route_table_sync_mode_stateful (self, addr_family), + _get_route_table_sync_mode_stateful (self, + AF_INET6), &temporary_not_available); if (!_rt6_temporary_not_available_set (self, temporary_not_available)) @@ -15498,8 +15525,8 @@ _cleanup_generic_post (NMDevice *self, CleanupType cleanup_type) priv->v4_route_table_initialized = FALSE; priv->v6_route_table_initialized = FALSE; - priv->v4_route_table_full_sync_before = FALSE; - priv->v6_route_table_full_sync_before = FALSE; + priv->v4_route_table_all_sync_before = FALSE; + priv->v6_route_table_all_sync_before = FALSE; priv->default_route_metric_penalty_ip4_has = FALSE; priv->default_route_metric_penalty_ip6_has = FALSE;