From bf3ecd9031f085cb73789250c44cfdf038a32fc8 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Fri, 10 Jan 2025 14:03:05 +0100 Subject: [PATCH] l3cfg: fix DNS routes The current approach is flawed. During a commit of the L3 configuration we do a RTM_GETROUTE to find the next-hop to the DNS server on the current interface, in order to create the DNS route to inject into the l3cd. However, we haven't added routes to kernel yet and so the result of the RTM_GETROUTE is going to be wrong. In some cases, for example when IPv4 DAD is enabled, the bug can't be easily noticed because we perform multiple commits for the interface, and the regular routes are already set in kernel from the 2nd commit on. To fix the problem, do the following: during a commit we first add addresses and routes to platform. Then, we create a list of DNS routes to configure, we collect the old DNS routes, and do a comparison. If they changed, we need to add the DNS routes to platform in a 2nd step. Note that in the previous approach we tracked the routes in the committed-l3cd object of the l3cfg, and so they were applied to kernel automatically. Because of the 2-step requirement, that no longer works and we must apply the DNS routes manually. Fixes: 5449b18a9486 ('core: support automatically adding DNS routes') --- src/core/nm-l3cfg.c | 146 ++++++++++++++++++++++++++++++++------------ src/core/nm-l3cfg.h | 1 - 2 files changed, 106 insertions(+), 41 deletions(-) diff --git a/src/core/nm-l3cfg.c b/src/core/nm-l3cfg.c index 14f9caffd5..e7c2ec5b8b 100644 --- a/src/core/nm-l3cfg.c +++ b/src/core/nm-l3cfg.c @@ -3901,39 +3901,88 @@ out_ip4_address: #define DNS_ROUTES_FWMARK_TABLE_PRIO 20053 -static void -_l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd) +static gboolean +_l3cfg_routed_dns_equal(GPtrArray *routes_old, GPtrArray *routes_new) { + guint i; + + if (nm_g_ptr_array_len(routes_old) != nm_g_ptr_array_len(routes_new)) + return FALSE; + + if (routes_old) { + nm_platform_route_objs_sort(routes_old, NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY); + } + + if (routes_new) { + nm_platform_route_objs_sort(routes_new, NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY); + } + + for (i = 0; i < nm_g_ptr_array_len(routes_old); i++) { + if (nmp_object_cmp(routes_old->pdata[i], routes_new->pdata[i]) != 0) + return FALSE; + } + + return TRUE; +} + +static GPtrArray * +_l3cfg_routed_dns_get_existing_routes(NML3Cfg *self, int addr_family) +{ + GPtrArray *routes = NULL; + NMPLookup lookup; + const NMDedupMultiHeadEntry *head_entry; + CList *iter; + + nmp_lookup_init_object_by_ifindex(&lookup, + NMP_OBJECT_TYPE_IP_ROUTE(NM_IS_IPv4(addr_family)), + self->priv.ifindex); + + head_entry = nm_platform_lookup(self->priv.platform, &lookup); + if (!head_entry) + return NULL; + + c_list_for_each (iter, &head_entry->lst_entries_head) { + const NMPObject *obj = c_list_entry(iter, NMDedupMultiEntry, lst_entries)->obj; + + if (nm_platform_route_table_uncoerce(obj->ipx_route.rx.table_coerced, FALSE) + != DNS_ROUTES_FWMARK_TABLE_PRIO) + continue; + + if (!routes) + routes = g_ptr_array_new_with_free_func((GDestroyNotify) nmp_object_unref); + g_ptr_array_add(routes, (gpointer) nmp_object_ref(obj)); + } + + return routes; +} + +static void +_l3cfg_routed_dns_apply(NML3Cfg *self, const NML3ConfigData *l3cd) +{ + if (!l3cd) + return; + for (int IS_IPv4 = 1; IS_IPv4 >= 0; IS_IPv4--) { - const char *const *nameservers; - guint i; - guint len; - int addr_family; - gboolean route_added = FALSE; + const char *const *nameservers; + guint i; + guint len; + int addr_family; + nm_auto_unref_ptrarray GPtrArray *old_routes = NULL; + nm_auto_unref_ptrarray GPtrArray *new_routes = NULL; addr_family = IS_IPv4 ? AF_INET : AF_INET6; - if (!nm_l3_config_data_get_routed_dns(l3cd, addr_family)) { - if (self->priv.dns_route_added_x[IS_IPv4]) { - /* Even if the DNS-routes feature is disabled, it was enabled - * before. Therefore, we need to set one last time the routing - * table sync mode to FULL, to clear the DNS routes added - * previously. */ - self->priv.dns_route_added_x[IS_IPv4] = FALSE; - nm_l3_config_data_set_route_table_sync( - l3cd, - addr_family, - NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL); - } - continue; - } + + if (!nm_l3_config_data_get_routed_dns(l3cd, addr_family)) + goto update_routes; + + _LOGT("configuring IPv%c DNS routes", nm_utils_addr_family_to_char(addr_family)); + + new_routes = g_ptr_array_new_with_free_func((GDestroyNotify) nmp_object_unref); nameservers = nm_l3_config_data_get_nameservers(l3cd, addr_family, &len); - nm_l3_config_data_set_route_table_sync(l3cd, - addr_family, - NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL); - for (i = 0; i < len; i++) { nm_auto_nmpobj NMPObject *obj = NULL; + NMPObject *obj_new; const NMPlatformIPXRoute *route; NMPlatformIPXRoute route_new; char addr_buf[INET6_ADDRSTRLEN]; @@ -3955,7 +4004,7 @@ _l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd) self->priv.ifindex, &obj); if (r < 0) { - _LOGT("could not get route to DNS %s", + _LOGD("could not get route to DNS server %s", nm_inet_ntop(addr_family, dns.addr.addr_ptr, addr_buf)); continue; } @@ -3964,6 +4013,7 @@ _l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd) if (IS_IPv4) { route_new.r4 = (NMPlatformIP4Route) { + .ifindex = self->priv.ifindex, .network = dns.addr.addr4, .plen = 32, .table_any = FALSE, @@ -3975,18 +4025,15 @@ _l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd) nm_platform_ip_route_normalize(addr_family, &route_new.rx); - _LOGT("route to %s: %s", + _LOGT("route to DNS server %s: %s", nm_inet4_ntop(dns.addr.addr4, addr_buf), nm_platform_ip4_route_to_string(&route_new.r4, route_buf, sizeof(route_buf))); - nm_l3_config_data_add_route_4(l3cd, &route_new.r4); - nm_l3_config_data_set_route_table_sync( - l3cd, - AF_INET, - NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_EXCEPT_LOCAL); - route_added = TRUE; + obj_new = nmp_object_new(NMP_OBJECT_TYPE_IP4_ROUTE, &route_new); + g_ptr_array_add(new_routes, obj_new); } else { route_new.r6 = (NMPlatformIP6Route) { + .ifindex = self->priv.ifindex, .network = dns.addr.addr6, .plen = 128, .table_any = FALSE, @@ -3998,16 +4045,16 @@ _l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd) nm_platform_ip_route_normalize(addr_family, &route_new.rx); - _LOGT("route to %s: %s", + _LOGT("route to DNS server %s: %s", nm_inet6_ntop(&dns.addr.addr6, addr_buf), nm_platform_ip6_route_to_string(&route_new.r6, route_buf, sizeof(route_buf))); - nm_l3_config_data_add_route_6(l3cd, &route_new.r6); - route_added = TRUE; + obj_new = nmp_object_new(NMP_OBJECT_TYPE_IP6_ROUTE, &route_new); + g_ptr_array_add(new_routes, obj_new); } } - if (route_added) { + if (new_routes->len > 0) { NMPlatformRoutingRule rule; NMPObject rule_obj; @@ -4039,7 +4086,26 @@ _l3cfg_routed_dns(NML3Cfg *self, NML3ConfigData *l3cd) } } - self->priv.dns_route_added_x[IS_IPv4] = route_added; +update_routes: + old_routes = _l3cfg_routed_dns_get_existing_routes(self, addr_family); + + if (!_l3cfg_routed_dns_equal(old_routes, new_routes)) { + if (old_routes) { + _LOGT("deleting old DNS routes"); + for (i = 0; i < old_routes->len; i++) { + nm_platform_object_delete(self->priv.platform, old_routes->pdata[i]); + } + } + if (new_routes) { + _LOGT("adding new DNS routes"); + for (i = 0; i < new_routes->len; i++) { + nm_platform_ip_route_add(self->priv.platform, + NMP_NLM_FLAG_REPLACE, + new_routes->pdata[i], + NULL); + } + } + } } } @@ -4210,8 +4276,6 @@ _l3cfg_update_combined_config(NML3Cfg *self, nm_assert(l3cd); nm_assert(nm_l3_config_data_get_ifindex(l3cd) == self->priv.ifindex); - _l3cfg_routed_dns(self, l3cd); - nm_l3_config_data_seal(l3cd); } @@ -5335,6 +5399,8 @@ _l3_commit(NML3Cfg *self, NML3CfgCommitType commit_type, gboolean is_idle) _l3_commit_one(self, AF_INET, commit_type, l3cd_old); _l3_commit_one(self, AF_INET6, commit_type, l3cd_old); + _l3cfg_routed_dns_apply(self, self->priv.p->combined_l3cd_commited); + _failedobj_reschedule(self, 0); _l3_commit_mptcp(self, commit_type); diff --git a/src/core/nm-l3cfg.h b/src/core/nm-l3cfg.h index a352860d4c..8581f6c45e 100644 --- a/src/core/nm-l3cfg.h +++ b/src/core/nm-l3cfg.h @@ -198,7 +198,6 @@ struct _NML3Cfg { const NMPObject *plobj; const NMPObject *plobj_next; int ifindex; - gboolean dns_route_added_x[2]; /* index with IS_IPv4 */ } priv; /* NML3Cfg strongly cooperates with NMNetns. The latter is