From 557644f5e03a77b3ebe09ceba672217959cf3bdc Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 19 Mar 2021 21:20:52 +0100 Subject: [PATCH] core: don't add dependent local route for addresses When adding an IPv4 address, kernel automatically adds a local route. This is done by fib_add_ifaddr(). Note that if the address is IFA_F_SECONDARY, then the "src" is the primary address. That means, with nmcli connection add con-name t type ethernet ifname t autoconnect no \ ipv4.method manual ipv6.method disabled \ ipv4.addresses '192.168.77.10/24, 192.168.77.11/24' we get two routes: "local 192.168.77.10 dev t table local proto kernel scope host src 192.168.77.10" "local 192.168.77.11 dev t table local proto kernel scope host src 192.168.77.10" Our code would only generate instead: "local 192.168.77.10 dev t table local proto kernel scope host src 192.168.77.10" "local 192.168.77.11 dev t table local proto kernel scope host src 192.168.77.11" Afterwards, this artificial route will be leaked: #!/bin/bash set -vx nmcli connection delete t || : ip link delete t || : ip link add name t type veth peer t-veth nmcli connection add con-name t type ethernet ifname t autoconnect no ipv4.method manual ipv4.addresses '192.168.77.10/24, 192.168.77.11/24' ipv6.method disabled nmcli connection up t ip route show table all dev t | grep --color '^\|192.168.77.11' sleep 1 nmcli device modify t -ipv4.addresses 192.168.77.11/24 ip route show table all dev t | grep --color '^\|192.168.77.11' ip route show table all dev t | grep -q 192.168.77.11 && echo "the local route 192.168.77.11 is still there, because NM adds a local route with wrong pref-src" It will also be leaked because in the example above ipv4.route-table is unset, so we are not in full route sync mode and the local table is not synced. This was introduced by commit 3e5fc04df320 ('core: add dependent local routes configured by kernel'), but it's unclear to me why we really need this. Drop it again and effectively revert commit 3e5fc04df320 ('core: add dependent local routes configured by kernel'). I think this "solution" is still bad. We need to improve our route sync approach with L3Cfg rework. For now, it's probably good enough. https://bugzilla.redhat.com/show_bug.cgi?id=1907661 --- src/core/nm-ip4-config.c | 15 ------ src/core/nm-ip6-config.c | 16 ------ src/libnm-platform/nm-platform.c | 91 ++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 31 deletions(-) diff --git a/src/core/nm-ip4-config.c b/src/core/nm-ip4-config.c index e2e4251fe3..9f8238841f 100644 --- a/src/core/nm-ip4-config.c +++ b/src/core/nm-ip4-config.c @@ -649,21 +649,6 @@ nm_ip4_config_add_dependent_routes(NMIP4Config *self, if (my_addr->external) continue; - /* Pre-generate local route added by kernel */ - r = nmp_object_new(NMP_OBJECT_TYPE_IP4_ROUTE, NULL); - route = NMP_OBJECT_CAST_IP4_ROUTE(r); - route->ifindex = ifindex; - route->rt_source = NM_IP_CONFIG_SOURCE_KERNEL; - route->network = my_addr->address; - route->plen = 32; - route->pref_src = my_addr->address; - route->type_coerced = nm_platform_route_type_coerce(RTN_LOCAL); - route->scope_inv = nm_platform_route_scope_inv(RT_SCOPE_HOST); - route->table_coerced = - nm_platform_route_table_coerce(is_vrf ? route_table : RT_TABLE_LOCAL); - _add_route(self, r, NULL, NULL); - nm_clear_pointer(&r, nmp_object_unref); - if (nm_utils_ip4_address_is_zeronet(network)) { /* Kernel doesn't add device-routes for destinations that * start with 0.x.y.z. Skip them. */ diff --git a/src/core/nm-ip6-config.c b/src/core/nm-ip6-config.c index 5c33f2ba3c..155594c8fc 100644 --- a/src/core/nm-ip6-config.c +++ b/src/core/nm-ip6-config.c @@ -404,22 +404,6 @@ nm_ip6_config_add_dependent_routes(NMIP6Config *self, if (my_addr->external) continue; - { - nm_auto_nmpobj NMPObject *r = NULL; - - /* Pre-generate local route added by kernel */ - r = nmp_object_new(NMP_OBJECT_TYPE_IP6_ROUTE, NULL); - route = NMP_OBJECT_CAST_IP6_ROUTE(r); - route->ifindex = ifindex; - route->network = my_addr->address; - route->plen = 128; - route->type_coerced = nm_platform_route_type_coerce(RTN_LOCAL); - route->metric = 0; - route->table_coerced = - nm_platform_route_table_coerce(is_vrf ? route_table : RT_TABLE_LOCAL); - _add_route(self, r, NULL, NULL); - } - if (NM_FLAGS_HAS(my_addr->n_ifa_flags, IFA_F_NOPREFIXROUTE)) continue; if (my_addr->plen == 0) diff --git a/src/libnm-platform/nm-platform.c b/src/libnm-platform/nm-platform.c index e3f8be9cc7..9eee50fdbe 100644 --- a/src/libnm-platform/nm-platform.c +++ b/src/libnm-platform/nm-platform.c @@ -4312,6 +4312,11 @@ nm_platform_ip_route_get_prune_list(NMPlatform * self, GPtrArray * routes_prune; const NMDedupMultiHeadEntry *head_entry; CList * iter; + NMPlatformIP4Route rt_local4; + NMPlatformIP6Route rt_local6; + const NMPlatformLink * pllink; + const NMPlatformLnkVrf * lnk_vrf; + guint32 local_table; nm_assert(NM_IS_PLATFORM(self)); nm_assert(NM_IN_SET(addr_family, AF_INET, AF_INET6)); @@ -4326,6 +4331,14 @@ nm_platform_ip_route_get_prune_list(NMPlatform * self, if (!head_entry) return NULL; + lnk_vrf = nm_platform_link_get_lnk_vrf(self, ifindex, &pllink); + if (!lnk_vrf && pllink && pllink->master > 0) + lnk_vrf = nm_platform_link_get_lnk_vrf(self, pllink->master, NULL); + local_table = lnk_vrf ? lnk_vrf->table : RT_TABLE_LOCAL; + + rt_local4.plen = 0; + rt_local6.plen = 0; + routes_prune = g_ptr_array_new_full(head_entry->len, (GDestroyNotify) nm_dedup_multi_obj_unref); c_list_for_each (iter, &head_entry->lst_entries_head) { @@ -4342,6 +4355,84 @@ nm_platform_ip_route_get_prune_list(NMPlatform * self, continue; break; case NM_IP_ROUTE_TABLE_SYNC_MODE_ALL: + + /* FIXME: we should better handle routes that are automatically added by kernel. + * + * For now, make a good guess which are those routes and exclude them from + * pruning them. */ + + if (NM_IS_IPv4(addr_family)) { + /* for each IPv4 address kernel adds a route like + * + * local $ADDR dev $IFACE table local proto kernel scope host src $PRIMARY_ADDR + * + * Check whether route could be of that kind. */ + if (nm_platform_ip_route_get_effective_table(&rt->rx) == local_table + && rt->rx.plen == 32 && rt->rx.rt_source == NM_IP_CONFIG_SOURCE_RTPROT_KERNEL + && rt->rx.metric == 0 + && rt->r4.scope_inv == nm_platform_route_scope_inv(RT_SCOPE_HOST) + && rt->r4.gateway == INADDR_ANY) { + if (rt_local4.plen == 0) { + rt_local4 = (NMPlatformIP4Route){ + .ifindex = ifindex, + .type_coerced = nm_platform_route_type_coerce(RTN_LOCAL), + .plen = 32, + .rt_source = NM_IP_CONFIG_SOURCE_RTPROT_KERNEL, + .metric = 0, + .table_coerced = nm_platform_route_table_coerce(local_table), + .scope_inv = nm_platform_route_scope_inv(RT_SCOPE_HOST), + .gateway = INADDR_ANY, + }; + } + + /* the possible "network" depends on the addresses we have. We don't check that + * carefully. If the other parameters match, we assume that this route is the one + * generated by kernel. */ + rt_local4.network = rt->r4.network; + rt_local4.pref_src = rt->r4.pref_src; + + /* to be more confident about comparing the value, use our nm_platform_ip4_route_cmp() + * implementation. That will also consider parameters that we leave unspecified here. */ + if (nm_platform_ip4_route_cmp(&rt->r4, + &rt_local4, + NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) + == 0) + continue; + } + } else { + /* for each IPv6 address (that is no longer tentative) kernel adds a route like + * + * local $ADDR dev $IFACE table local proto kernel metric 0 pref medium + * + * Same as for the IPv4 case. */ + if (nm_platform_ip_route_get_effective_table(&rt->rx) == local_table + && rt->rx.plen == 128 && rt->rx.rt_source == NM_IP_CONFIG_SOURCE_RTPROT_KERNEL + && rt->rx.metric == 0 && rt->r6.rt_pref == NM_ICMPV6_ROUTER_PREF_MEDIUM + && IN6_IS_ADDR_UNSPECIFIED(&rt->r6.gateway)) { + if (rt_local6.plen == 0) { + rt_local6 = (NMPlatformIP6Route){ + .ifindex = ifindex, + .type_coerced = nm_platform_route_type_coerce(RTN_LOCAL), + .plen = 128, + .rt_source = NM_IP_CONFIG_SOURCE_RTPROT_KERNEL, + .metric = 0, + .table_coerced = nm_platform_route_table_coerce(local_table), + .rt_pref = NM_ICMPV6_ROUTER_PREF_MEDIUM, + .gateway = IN6ADDR_ANY_INIT, + }; + } + + rt_local6.network = rt->r6.network; + + if (nm_platform_ip6_route_cmp(&rt->r6, + &rt_local6, + NM_PLATFORM_IP_ROUTE_CMP_TYPE_SEMANTICALLY) + == 0) + continue; + } + } + break; + case NM_IP_ROUTE_TABLE_SYNC_MODE_ALL_PRUNE: break;