From 3caed9ea0830b811ba0e5222e57c5d7deda204e3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 9 Jan 2024 15:49:20 +0900 Subject: [PATCH 1/7] network/route: introduce route_remove_and_cancel() Then, replace route_remove_and_drop() with it. If a route is requested, and the request is already called, we may not received its reply and notification from the kernel, and the corresponding Route object may not be remembered. Even in such case, we need to remove the route, otherwise the route will come later after the function called. This is the version for route of f22b586a215962416bdbd692aabb89b1ac2999d0. --- src/network/networkd-dhcp-prefix-delegation.c | 7 +-- src/network/networkd-dhcp4.c | 3 +- src/network/networkd-dhcp6.c | 3 +- src/network/networkd-ndisc.c | 2 +- src/network/networkd-route.c | 48 +++++++++---------- src/network/networkd-route.h | 5 +- 6 files changed, 29 insertions(+), 39 deletions(-) diff --git a/src/network/networkd-dhcp-prefix-delegation.c b/src/network/networkd-dhcp-prefix-delegation.c index b8b03f6d3b..d8f9a2c053 100644 --- a/src/network/networkd-dhcp-prefix-delegation.c +++ b/src/network/networkd-dhcp-prefix-delegation.c @@ -176,8 +176,7 @@ int dhcp_pd_remove(Link *link, bool only_marked) { link_remove_dhcp_pd_subnet_prefix(link, &route->dst.in6); - RET_GATHER(ret, route_remove(route)); - route_cancel_request(route, link); + RET_GATHER(ret, route_remove_and_cancel(route, link->manager)); } } else { Address *address; @@ -612,9 +611,7 @@ void dhcp_pd_prefix_lost(Link *uplink) { .address = route->dst })) continue; - (void) route_remove(route); - - route_cancel_request(route, uplink); + (void) route_remove_and_cancel(route, uplink->manager); } set_clear(uplink->dhcp_pd_prefixes); diff --git a/src/network/networkd-dhcp4.c b/src/network/networkd-dhcp4.c index be7abcf6f3..1a1836cf66 100644 --- a/src/network/networkd-dhcp4.c +++ b/src/network/networkd-dhcp4.c @@ -256,8 +256,7 @@ static int dhcp4_remove_address_and_routes(Link *link, bool only_marked) { if (only_marked && !route_is_marked(route)) continue; - RET_GATHER(ret, route_remove(route)); - route_cancel_request(route, link); + RET_GATHER(ret, route_remove_and_cancel(route, link->manager)); } SET_FOREACH(address, link->addresses) { diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index 26ed034bbe..eb1420be5c 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -62,8 +62,7 @@ static int dhcp6_remove(Link *link, bool only_marked) { if (only_marked && !route_is_marked(route)) continue; - RET_GATHER(ret, route_remove(route)); - route_cancel_request(route, link); + RET_GATHER(ret, route_remove_and_cancel(route, link->manager)); } SET_FOREACH(address, link->addresses) { diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index 7186187fd8..488c8c0294 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -1173,7 +1173,7 @@ static int ndisc_drop_outdated(Link *link, usec_t timestamp_usec) { if (route->lifetime_usec >= timestamp_usec) continue; /* the route is still valid */ - r = route_remove_and_drop(route); + r = route_remove_and_cancel(route, link->manager); if (r < 0) RET_GATHER(ret, log_link_warning_errno(link, r, "Failed to remove outdated SLAAC route, ignoring: %m")); } diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 3a33769f8b..efb121cb82 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -510,13 +510,13 @@ static int route_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *l return 1; } -int route_remove(Route *route) { +int route_remove(Route *route, Manager *manager) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; Link *link = NULL; int r; assert(route); - Manager *manager = ASSERT_PTR(route->manager); + assert(manager); log_route_debug(route, "Removing", manager); @@ -541,17 +541,27 @@ int route_remove(Route *route) { return 0; } -int route_remove_and_drop(Route *route) { - if (!route) - return 0; +int route_remove_and_cancel(Route *route, Manager *manager) { + bool waiting = false; + Request *req; - route_cancel_request(route, NULL); + assert(route); + assert(manager); - if (route_exists(route)) - return route_remove(route); + /* If the route is remembered by the manager, then use the remembered object. */ + (void) route_get(manager, route, &route); - if (route->state == 0) - route_free(route); + /* Cancel the request for the route. If the request is already called but we have not received the + * notification about the request, then explicitly remove the route. */ + if (route_get_request(manager, route, &req) >= 0) { + waiting = req->waiting_reply; + request_detach(req); + route_cancel_requesting(route); + } + + /* If we know that the route will come or already exists, remove it. */ + if (waiting || (route->manager && route_exists(route))) + return route_remove(route, manager); return 0; } @@ -562,7 +572,7 @@ static int route_expire_handler(sd_event_source *s, uint64_t usec, void *userdat assert(route->manager); - r = route_remove(route); + r = route_remove(route, route->manager); if (r < 0) { Link *link = NULL; (void) route_get_link(route->manager, route, &link); @@ -944,20 +954,6 @@ int link_request_static_routes(Link *link, bool only_ipv4) { return 0; } -void route_cancel_request(Route *route, Link *link) { - assert(route); - Manager *manager = ASSERT_PTR(route->manager ?: ASSERT_PTR(link)->manager); - - if (!route_is_requesting(route)) - return; - - Request *req; - if (route_get_request(manager, route, &req) >= 0) - request_detach(req); - - route_cancel_requesting(route); -} - static int process_route_one( Manager *manager, uint16_t type, @@ -1376,7 +1372,7 @@ int link_drop_routes(Link *link, bool foreign) { if (!route_is_marked(route)) continue; - RET_GATHER(r, route_remove(route)); + RET_GATHER(r, route_remove(route, link->manager)); } return r; diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index 090bf34a61..51a06dd0f1 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -88,8 +88,8 @@ int route_new_static(Network *network, const char *filename, unsigned section_li int route_dup(const Route *src, const RouteNextHop *nh, Route **ret); int route_configure_handler_internal(sd_netlink *rtnl, sd_netlink_message *m, Link *link, Route *route, const char *error_msg); -int route_remove(Route *route); -int route_remove_and_drop(Route *route); +int route_remove(Route *route, Manager *manager); +int route_remove_and_cancel(Route *route, Manager *manager); int route_get(Manager *manager, const Route *route, Route **ret); @@ -102,7 +102,6 @@ static inline int link_drop_foreign_routes(Link *link) { } int link_foreignize_routes(Link *link); -void route_cancel_request(Route *route, Link *link); int link_request_route( Link *link, const Route *route, From 74c301b9ee9479ffb5033f5ee1ec4c18ef79efa0 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 7 Jan 2024 15:16:03 +0900 Subject: [PATCH 2/7] network/route: introduce ref/unref functions for Route object Then, Route object can live if it is detached from the owner (Manager, Network, or Wireguard object). This is the one for routes of ebd96906477aac2bbc6b9de0d6e9bd0f39db5581. --- src/network/netdev/wireguard.c | 2 +- src/network/networkd-dhcp-prefix-delegation.c | 6 +- src/network/networkd-dhcp4.c | 12 +- src/network/networkd-ndisc.c | 8 +- src/network/networkd-network.c | 4 +- src/network/networkd-route-metric.c | 4 +- src/network/networkd-route-nexthop.c | 8 +- src/network/networkd-route.c | 107 ++++++++++++------ src/network/networkd-route.h | 7 +- 9 files changed, 97 insertions(+), 61 deletions(-) diff --git a/src/network/netdev/wireguard.c b/src/network/netdev/wireguard.c index 0195dce1bf..95c9c8c330 100644 --- a/src/network/netdev/wireguard.c +++ b/src/network/netdev/wireguard.c @@ -1190,7 +1190,7 @@ static int wireguard_verify(NetDev *netdev, const char *filename) { continue; LIST_FOREACH(ipmasks, ipmask, peer->ipmasks) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; r = route_new(&route); if (r < 0) diff --git a/src/network/networkd-dhcp-prefix-delegation.c b/src/network/networkd-dhcp-prefix-delegation.c index d8f9a2c053..61295d9ce6 100644 --- a/src/network/networkd-dhcp-prefix-delegation.c +++ b/src/network/networkd-dhcp-prefix-delegation.c @@ -282,7 +282,7 @@ static int dhcp_pd_route_handler(sd_netlink *rtnl, sd_netlink_message *m, Reques } static int dhcp_pd_request_route(Link *link, const struct in6_addr *prefix, usec_t lifetime_usec) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; Route *existing; int r; @@ -670,7 +670,7 @@ static int dhcp_request_unreachable_route( route_netlink_handler_t callback, bool *configured) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; Route *existing; int r; @@ -781,7 +781,7 @@ static int dhcp_pd_prefix_add(Link *link, const struct in6_addr *prefix, uint8_t } static int dhcp4_pd_request_default_gateway_on_6rd_tunnel(Link *link, const struct in_addr *br_address, usec_t lifetime_usec) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; Route *existing; int r; diff --git a/src/network/networkd-dhcp4.c b/src/network/networkd-dhcp4.c index 1a1836cf66..f9baf0d535 100644 --- a/src/network/networkd-dhcp4.c +++ b/src/network/networkd-dhcp4.c @@ -410,7 +410,7 @@ static bool link_prefixroute(Link *link) { } static int dhcp4_request_prefix_route(Link *link) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; int r; assert(link); @@ -438,7 +438,7 @@ static int dhcp4_request_prefix_route(Link *link) { } static int dhcp4_request_route_to_gateway(Link *link, const struct in_addr *gw) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; struct in_addr address; int r; @@ -556,7 +556,7 @@ static int dhcp4_request_classless_static_or_static_routes(Link *link) { return r; FOREACH_ARRAY(e, routes, n_routes) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; struct in_addr gw; r = route_new(&route); @@ -584,7 +584,7 @@ static int dhcp4_request_classless_static_or_static_routes(Link *link) { } static int dhcp4_request_default_gateway(Link *link) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; struct in_addr address, router; int r; @@ -639,7 +639,7 @@ static int dhcp4_request_semi_static_routes(Link *link) { assert(link->network); HASHMAP_FOREACH(rt, link->network->routes_by_section) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; struct in_addr gw; if (!rt->gateway_from_dhcp_or_ra) @@ -690,7 +690,7 @@ static int dhcp4_request_routes_to_servers( assert(servers || n_servers == 0); FOREACH_ARRAY(dst, servers, n_servers) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; struct in_addr gw; if (in4_addr_is_null(dst)) diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index 488c8c0294..7a67c55299 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -316,7 +316,7 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { return log_link_warning_errno(link, r, "Failed to get default router preference from RA: %m"); if (link->network->ipv6_accept_ra_use_gateway) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; r = route_new(&route); if (r < 0) @@ -335,7 +335,7 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { Route *route_gw; HASHMAP_FOREACH(route_gw, link->network->routes_by_section) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; if (!route_gw->gateway_from_dhcp_or_ra) continue; @@ -498,7 +498,7 @@ static int ndisc_router_process_autonomous_prefix(Link *link, sd_ndisc_router *r } static int ndisc_router_process_onlink_prefix(Link *link, sd_ndisc_router *rt) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; unsigned prefixlen, preference; usec_t lifetime_usec; struct in6_addr prefix; @@ -600,7 +600,7 @@ static int ndisc_router_process_prefix(Link *link, sd_ndisc_router *rt) { } static int ndisc_router_process_route(Link *link, sd_ndisc_router *rt) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; unsigned preference, prefixlen; struct in6_addr gateway, dst; usec_t lifetime_usec; diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 16c679b343..300fba5dd9 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -188,7 +188,7 @@ int network_verify(Network *network) { network->filename); network->addresses_by_section = ordered_hashmap_free(network->addresses_by_section); - network->routes_by_section = hashmap_free_with_destructor(network->routes_by_section, route_free); + network->routes_by_section = hashmap_free(network->routes_by_section); } if (network->link_local < 0) { @@ -782,7 +782,7 @@ static Network *network_free(Network *network) { /* static configs */ set_free_free(network->ipv6_proxy_ndp_addresses); ordered_hashmap_free(network->addresses_by_section); - hashmap_free_with_destructor(network->routes_by_section, route_free); + hashmap_free(network->routes_by_section); ordered_hashmap_free(network->nexthops_by_section); hashmap_free_with_destructor(network->bridge_fdb_entries_by_section, bridge_fdb_free); hashmap_free_with_destructor(network->bridge_mdb_entries_by_section, bridge_mdb_free); diff --git a/src/network/networkd-route-metric.c b/src/network/networkd-route-metric.c index b27b3c1294..9c417a4fc4 100644 --- a/src/network/networkd-route-metric.c +++ b/src/network/networkd-route-metric.c @@ -374,7 +374,7 @@ static int config_parse_route_metric_boolean_impl( void *userdata) { \ \ Network *network = ASSERT_PTR(userdata); \ - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; \ + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; \ uint16_t attr_type = ltype; \ int r; \ \ @@ -436,7 +436,7 @@ int config_parse_route_metric_tcp_congestion( void *userdata) { Network *network = ASSERT_PTR(userdata); - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; assert(filename); diff --git a/src/network/networkd-route-nexthop.c b/src/network/networkd-route-nexthop.c index 5f1da28604..8ccd4e66e8 100644 --- a/src/network/networkd-route-nexthop.c +++ b/src/network/networkd-route-nexthop.c @@ -899,7 +899,7 @@ int config_parse_gateway( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; assert(filename); @@ -986,7 +986,7 @@ int config_parse_route_gateway_onlink( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; assert(filename); @@ -1026,7 +1026,7 @@ int config_parse_route_nexthop( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; uint32_t id; int r; @@ -1079,7 +1079,7 @@ int config_parse_multipath_route( void *userdata) { _cleanup_(route_nexthop_freep) RouteNextHop *nh = NULL; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; _cleanup_free_ char *word = NULL; Network *network = userdata; const char *p; diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index efb121cb82..89a96cbda1 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -21,20 +21,41 @@ #include "vrf.h" #include "wireguard.h" -Route* route_free(Route *route) { - if (!route) - return NULL; +static Route* route_detach_impl(Route *route) { + assert(route); + assert(!!route->network + !!route->manager + !!route->wireguard <= 1); if (route->network) { assert(route->section); hashmap_remove(route->network->routes_by_section, route->section); + route->network = NULL; + return route; } - if (route->manager) + if (route->manager) { set_remove(route->manager->routes, route); + route->manager = NULL; + return route; + } - if (route->wireguard) + if (route->wireguard) { set_remove(route->wireguard->routes, route); + route->wireguard = NULL; + return route; + } + + return NULL; +} + +static void route_detach(Route *route) { + route_unref(route_detach_impl(route)); +} + +static Route* route_free(Route *route) { + if (!route) + return NULL; + + route_detach_impl(route); config_section_free(route->section); route_nexthops_done(route); @@ -44,6 +65,8 @@ Route* route_free(Route *route) { return mfree(route); } +DEFINE_TRIVIAL_REF_UNREF_FUNC(Route, route, route_free); + static void route_hash_func(const Route *route, struct siphash *state) { assert(route); @@ -195,16 +218,25 @@ DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR( Route, route_hash_func, route_compare_func, - route_free); + route_detach); + +DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + route_section_hash_ops, + ConfigSection, + config_section_hash_func, + config_section_compare_func, + Route, + route_detach); int route_new(Route **ret) { - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; route = new(Route, 1); if (!route) return -ENOMEM; *route = (Route) { + .n_ref = 1, .family = AF_UNSPEC, .scope = RT_SCOPE_UNIVERSE, .protocol = RTPROT_UNSPEC, @@ -221,7 +253,7 @@ int route_new(Route **ret) { int route_new_static(Network *network, const char *filename, unsigned section_line, Route **ret) { _cleanup_(config_section_freep) ConfigSection *n = NULL; - _cleanup_(route_freep) Route *route = NULL; + _cleanup_(route_unrefp) Route *route = NULL; int r; assert(network); @@ -251,7 +283,7 @@ int route_new_static(Network *network, const char *filename, unsigned section_li route->section = TAKE_PTR(n); route->source = NETWORK_CONFIG_SOURCE_STATIC; - r = hashmap_ensure_put(&network->routes_by_section, &config_section_hash_ops, route->section, route); + r = hashmap_ensure_put(&network->routes_by_section, &route_section_hash_ops, route->section, route); if (r < 0) return r; @@ -259,7 +291,7 @@ int route_new_static(Network *network, const char *filename, unsigned section_li return 0; } -static int route_add(Manager *manager, Route *route) { +static int route_attach(Manager *manager, Route *route) { int r; assert(manager); @@ -334,7 +366,7 @@ static int route_get_request(Manager *manager, const Route *route, Request **ret } int route_dup(const Route *src, const RouteNextHop *nh, Route **ret) { - _cleanup_(route_freep) Route *dest = NULL; + _cleanup_(route_unrefp) Route *dest = NULL; int r; assert(src); @@ -344,7 +376,8 @@ int route_dup(const Route *src, const RouteNextHop *nh, Route **ret) { if (!dest) return -ENOMEM; - /* Unset all pointers */ + /* Unset number of reference and all pointers */ + dest->n_ref = 1; dest->manager = NULL; dest->network = NULL; dest->wireguard = NULL; @@ -570,7 +603,8 @@ static int route_expire_handler(sd_event_source *s, uint64_t usec, void *userdat Route *route = ASSERT_PTR(userdata); int r; - assert(route->manager); + if (!route->manager) + return 0; /* already detached. */ r = route_remove(route, route->manager); if (r < 0) { @@ -683,7 +717,7 @@ static int route_configure(const Route *route, uint32_t lifetime_sec, Link *link static int route_requeue_request(Request *req, Link *link, const Route *route) { _unused_ _cleanup_(request_unrefp) Request *req_unref = NULL; - _cleanup_(route_freep) Route *tmp = NULL; + _cleanup_(route_unrefp) Route *tmp = NULL; int r; assert(req); @@ -806,7 +840,7 @@ static int link_request_route_one( unsigned *message_counter, route_netlink_handler_t netlink_handler) { - _cleanup_(route_freep) Route *tmp = NULL; + _cleanup_(route_unrefp) Route *tmp = NULL; Route *existing = NULL; int r; @@ -829,7 +863,7 @@ static int link_request_route_one( log_route_debug(tmp, "Requesting", link->manager); r = link_queue_request_safe(link, REQUEST_TYPE_ROUTE, tmp, - route_free, + route_unref, route_hash_func, route_compare_func, route_process_request, @@ -957,10 +991,9 @@ int link_request_static_routes(Link *link, bool only_ipv4) { static int process_route_one( Manager *manager, uint16_t type, - Route *in, + Route *tmp, const struct rta_cacheinfo *cacheinfo) { - _cleanup_(route_freep) Route *tmp = in; Request *req = NULL; Route *route = NULL; Link *link = NULL; @@ -987,13 +1020,13 @@ static int process_route_one( } /* If we do not know the route, then save it. */ - r = route_add(manager, tmp); + r = route_attach(manager, tmp); if (r < 0) { log_link_warning_errno(link, r, "Failed to remember foreign route, ignoring: %m"); return 0; } - route = TAKE_PTR(tmp); + route = route_ref(tmp); is_new = true; } else @@ -1020,7 +1053,7 @@ static int process_route_one( if (route) { route_enter_removed(route); log_route_debug(route, "Forgetting removed", manager); - route_free(route); + route_detach(route); } else log_route_debug(tmp, manager->manage_foreign_routes ? "Kernel removed unknown" : "Ignoring received", @@ -1047,7 +1080,7 @@ static int process_route_one( } int manager_rtnl_process_route(sd_netlink *rtnl, sd_netlink_message *message, Manager *m) { - _cleanup_(route_freep) Route *tmp = NULL; + _cleanup_(route_unrefp) Route *tmp = NULL; int r; assert(rtnl); @@ -1190,17 +1223,17 @@ int manager_rtnl_process_route(sd_netlink *rtnl, sd_netlink_message *message, Ma has_cacheinfo = r >= 0; if (tmp->family == AF_INET || ordered_set_isempty(tmp->nexthops)) - return process_route_one(m, type, TAKE_PTR(tmp), has_cacheinfo ? &cacheinfo : NULL); + return process_route_one(m, type, tmp, has_cacheinfo ? &cacheinfo : NULL); RouteNextHop *nh; ORDERED_SET_FOREACH(nh, tmp->nexthops) { - _cleanup_(route_freep) Route *dup = NULL; + _cleanup_(route_unrefp) Route *dup = NULL; r = route_dup(tmp, nh, &dup); if (r < 0) return log_oom(); - r = process_route_one(m, type, TAKE_PTR(dup), has_cacheinfo ? &cacheinfo : NULL); + r = process_route_one(m, type, dup, has_cacheinfo ? &cacheinfo : NULL); if (r < 0) return r; } @@ -1248,7 +1281,7 @@ static bool route_by_kernel(const Route *route) { } static int link_unmark_route(Link *link, const Route *route, const RouteNextHop *nh) { - _cleanup_(route_freep) Route *tmp = NULL; + _cleanup_(route_unrefp) Route *tmp = NULL; Route *existing; int r; @@ -1400,7 +1433,7 @@ int link_foreignize_routes(Link *link) { } int network_add_ipv4ll_route(Network *network) { - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; unsigned section_line; int r; @@ -1435,7 +1468,7 @@ int network_add_ipv4ll_route(Network *network) { } int network_add_default_route_on_device(Network *network) { - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; unsigned section_line; int r; @@ -1475,7 +1508,7 @@ int config_parse_preferred_src( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; assert(filename); @@ -1520,7 +1553,7 @@ int config_parse_destination( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; union in_addr_union *buffer; unsigned char *prefixlen; int r; @@ -1578,7 +1611,7 @@ int config_parse_route_priority( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; assert(filename); @@ -1621,7 +1654,7 @@ int config_parse_route_scope( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; assert(filename); @@ -1663,7 +1696,7 @@ int config_parse_route_table( void *data, void *userdata) { - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; Network *network = userdata; int r; @@ -1707,7 +1740,7 @@ int config_parse_ipv6_route_preference( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; r = route_new_static(network, filename, section_line, &route); @@ -1748,7 +1781,7 @@ int config_parse_route_protocol( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int r; r = route_new_static(network, filename, section_line, &route); @@ -1786,7 +1819,7 @@ int config_parse_route_type( void *userdata) { Network *network = userdata; - _cleanup_(route_free_or_set_invalidp) Route *route = NULL; + _cleanup_(route_unref_or_set_invalidp) Route *route = NULL; int t, r; r = route_new_static(network, filename, section_line, &route); @@ -1871,5 +1904,5 @@ void network_drop_invalid_routes(Network *network) { HASHMAP_FOREACH(route, network->routes_by_section) if (route_section_verify(route) < 0) - route_free(route); + route_detach(route); } diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index 51a06dd0f1..004295e599 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -35,6 +35,8 @@ struct Route { NetworkConfigState state; union in_addr_union provider; /* DHCP server or router address */ + unsigned n_ref; + /* rtmsg header */ int family; unsigned char dst_prefixlen; @@ -80,8 +82,9 @@ struct Route { extern const struct hash_ops route_hash_ops; -Route* route_free(Route *route); -DEFINE_SECTION_CLEANUP_FUNCTIONS(Route, route_free); +Route* route_ref(Route *route); +Route* route_unref(Route *route); +DEFINE_SECTION_CLEANUP_FUNCTIONS(Route, route_unref); int route_new(Route **ret); int route_new_static(Network *network, const char *filename, unsigned section_line, Route **ret); From d529b12a01ff251b0fe631e5ad6dc90f4faad8ce Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 7 Jan 2024 14:41:56 +0900 Subject: [PATCH 3/7] network/route: drop Route object even if we fail to remove the route If we could not remove a route, then previously the corresponding Route object was never removed, as it was freed only when we receive remove notification from the kernel. So, we might confused that the route still exists and being removed, and might block reconfiguring the route. With this change, even if we fail to remove a route, the corresponding Route object will be freed. This is the one for routes of 56a995fe8e50b2432ff930ed0431cc70adbe492d. --- src/network/networkd-route.c | 37 ++++++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 89a96cbda1..147d9cb14f 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -526,19 +526,35 @@ static int route_set_netlink_message(const Route *route, sd_netlink_message *m) return 0; } -static int route_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { +static int route_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) { int r; assert(m); + assert(rreq); - /* link may be NULL. */ - - if (link && IN_SET(link->state, LINK_STATE_FAILED, LINK_STATE_LINGER)) - return 0; + Manager *manager = ASSERT_PTR(rreq->manager); + Route *route = ASSERT_PTR(rreq->userdata); r = sd_netlink_message_get_errno(m); - if (r < 0 && r != -ESRCH) - log_link_message_warning_errno(link, m, r, "Could not drop route, ignoring"); + if (r < 0) { + log_message_full_errno(m, + (r == -ESRCH || /* the route is already removed? */ + (r == -EINVAL && route->nexthop_id != 0) || /* The nexthop is already removed? */ + !route->manager) ? /* already detached? */ + LOG_DEBUG : LOG_WARNING, + r, "Could not drop route, ignoring"); + + if (route->manager) { + /* If the route cannot be removed, then assume the route is already removed. */ + log_route_debug(route, "Forgetting", manager); + + Request *req; + if (route_get_request(manager, route, &req) >= 0) + route_enter_removed(req->userdata); + + route_detach(route); + } + } return 1; } @@ -563,12 +579,9 @@ int route_remove(Route *route, Manager *manager) { if (r < 0) return log_link_warning_errno(link, r, "Could not fill netlink message: %m"); - r = netlink_call_async(manager->rtnl, NULL, m, route_remove_handler, - link ? link_netlink_destroy_callback : NULL, link); + r = manager_remove_request_add(manager, route, route, manager->rtnl, m, route_remove_handler); if (r < 0) - return log_link_warning_errno(link, r, "Could not send netlink message: %m"); - - link_ref(link); + return log_link_warning_errno(link, r, "Could not queue rtnetlink message: %m"); route_enter_removing(route); return 0; From 97979ece0e046b4868130a9c80c15722e9c79dd8 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 7 Jan 2024 06:11:09 +0900 Subject: [PATCH 4/7] network/route: also remove route on cancelling request Otherwise, the route may arrive after we call link_drop_foreign_address() or so on reconfiguring interface. This is the one for routes of 4303e9806befc0c5b8067e45225e5d952f427b3a. --- src/network/networkd-link.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index a79ab0d43c..194d31eafe 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -997,6 +997,13 @@ static int link_drop_requests(Link *link) { RET_GATHER(ret, nexthop_remove(nexthop, link->manager)); break; } + case REQUEST_TYPE_ROUTE: { + Route *route = ASSERT_PTR(req->userdata); + + if (route_get(link->manager, route, NULL) < 0) + RET_GATHER(ret, route_remove(route, link->manager)); + break; + } default: ; } From 6f09031e4d04727cc72164fefcbc763e37556493 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Jan 2024 13:02:16 +0900 Subject: [PATCH 5/7] network/route: introduce reverse map for route with nexthop ID It is not used in this commit, but will be used later. Preparation for later commits. This is the one for routes of 531c7246829a41dd7e51847bd4d77aa012ff478f. --- src/network/networkd-nexthop.c | 1 + src/network/networkd-nexthop.h | 3 +- src/network/networkd-route-nexthop.c | 43 ++++++++++++++++++++++++++++ src/network/networkd-route-nexthop.h | 3 ++ src/network/networkd-route.c | 8 ++++++ src/network/networkd-route.h | 1 + 6 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index cf62e0e82d..67eba509d3 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -97,6 +97,7 @@ static NextHop* nexthop_free(NextHop *nexthop) { config_section_free(nexthop->section); hashmap_free_free(nexthop->group); set_free(nexthop->nexthops); + set_free(nexthop->routes); return mfree(nexthop); } diff --git a/src/network/networkd-nexthop.h b/src/network/networkd-nexthop.h index 74b23bd772..9ce36c6cd9 100644 --- a/src/network/networkd-nexthop.h +++ b/src/network/networkd-nexthop.h @@ -41,8 +41,9 @@ typedef struct NextHop { /* Only used in conf parser and nexthop_section_verify(). */ int onlink; - /* For managing nexthops that depend on this nexthop. */ + /* For managing routes and nexthops that depend on this nexthop. */ Set *nexthops; + Set *routes; } NextHop; NextHop* nexthop_ref(NextHop *nexthop); diff --git a/src/network/networkd-route-nexthop.c b/src/network/networkd-route-nexthop.c index 8ccd4e66e8..f7a2201b6b 100644 --- a/src/network/networkd-route-nexthop.c +++ b/src/network/networkd-route-nexthop.c @@ -5,6 +5,7 @@ #include "alloc-util.h" #include "extract-word.h" #include "netlink-util.h" +#include "networkd-manager.h" #include "networkd-network.h" #include "networkd-nexthop.h" #include "networkd-route.h" @@ -13,6 +14,48 @@ #include "parse-util.h" #include "string-util.h" +void route_detach_from_nexthop(Route *route) { + NextHop *nh; + + assert(route); + assert(route->manager); + + if (route->nexthop_id == 0) + return; + + if (nexthop_get_by_id(route->manager, route->nexthop_id, &nh) < 0) + return; + + route_unref(set_remove(nh->routes, route)); +} + +void route_attach_to_nexthop(Route *route) { + NextHop *nh; + int r; + + assert(route); + assert(route->manager); + + if (route->nexthop_id == 0) + return; + + r = nexthop_get_by_id(route->manager, route->nexthop_id, &nh); + if (r < 0) { + if (route->manager->manage_foreign_nexthops) + log_debug_errno(r, "Route has unknown nexthop ID (%"PRIu32"), ignoring.", + route->nexthop_id); + return; + } + + r = set_ensure_put(&nh->routes, &route_hash_ops_unref, route); + if (r < 0) + return (void) log_debug_errno(r, "Failed to save route to nexthop, ignoring: %m"); + if (r == 0) + return (void) log_debug("Duplicated route assigned to nexthop, ignoring."); + + route_ref(route); +} + static void route_nexthop_done(RouteNextHop *nh) { assert(nh); diff --git a/src/network/networkd-route-nexthop.h b/src/network/networkd-route-nexthop.h index 5e4602d3cd..f9a147839d 100644 --- a/src/network/networkd-route-nexthop.h +++ b/src/network/networkd-route-nexthop.h @@ -26,6 +26,9 @@ typedef struct RouteNextHop { #define ROUTE_NEXTHOP_NULL ((const RouteNextHop) {}) +void route_detach_from_nexthop(Route *route); +void route_attach_to_nexthop(Route *route); + RouteNextHop* route_nexthop_free(RouteNextHop *nh); DEFINE_TRIVIAL_CLEANUP_FUNC(RouteNextHop*, route_nexthop_free); diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 147d9cb14f..80e20a286b 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -33,6 +33,7 @@ static Route* route_detach_impl(Route *route) { } if (route->manager) { + route_detach_from_nexthop(route); set_remove(route->manager->routes, route); route->manager = NULL; return route; @@ -220,6 +221,13 @@ DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR( route_compare_func, route_detach); +DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR( + route_hash_ops_unref, + Route, + route_hash_func, + route_compare_func, + route_unref); + DEFINE_HASH_OPS_WITH_VALUE_DESTRUCTOR( route_section_hash_ops, ConfigSection, diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index 004295e599..c5c2693ac4 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -81,6 +81,7 @@ struct Route { }; extern const struct hash_ops route_hash_ops; +extern const struct hash_ops route_hash_ops_unref; Route* route_ref(Route *route); Route* route_unref(Route *route); From b91743e0197bb286c6c638b0f1a423e8402b53eb Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 16 Jan 2024 12:01:50 +0900 Subject: [PATCH 6/7] network/nexthop: drop dependent routes on removal If a nexthop is removed, dependent routes are silently removed by the kernel. Hence, networkd may be confused that routes that depends on the nexthop still exist, and may fail to configure other routes or so. This is the one for routes of 3cbbe8635a16f096a3b0eff993f7681401535605. --- src/network/networkd-nexthop.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 67eba509d3..07a971a0e9 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -12,6 +12,7 @@ #include "networkd-network.h" #include "networkd-nexthop.h" #include "networkd-queue.h" +#include "networkd-route.h" #include "networkd-route-util.h" #include "parse-util.h" #include "set.h" @@ -489,7 +490,7 @@ static int nexthop_remove_dependents(NextHop *nexthop, Manager *manager) { assert(nexthop); assert(manager); - /* If a nexthop is removed, the kernel silently removes nexthops that depend on the + /* If a nexthop is removed, the kernel silently removes nexthops and routes that depend on the * removed nexthop. Let's remove them for safety (though, they are already removed in the kernel, * hence that should fail), and forget them. */ @@ -503,6 +504,10 @@ static int nexthop_remove_dependents(NextHop *nexthop, Manager *manager) { RET_GATHER(r, nexthop_remove(nh, manager)); } + Route *route; + SET_FOREACH(route, nexthop->routes) + RET_GATHER(r, route_remove(route, manager)); + return r; } From b5edf3a9968ad4f6bf2f8bcfc61e03381b23465a Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 16 Jan 2024 12:47:40 +0900 Subject: [PATCH 7/7] test-network: check if networkd forgets routes silently removed by the kernel --- test/test-network/systemd-networkd-tests.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 064ca53193..20c5f91a5d 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -4082,7 +4082,9 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.assertIn('nexthop via 192.168.5.3 dev veth99 weight 3', output) self.assertIn('nexthop via 192.168.20.1 dev dummy98 weight 1', output) - check_json(networkctl_json()) + output = networkctl_json() + check_json(output) + self.assertNotIn('"Destination":[10.10.10.14]', output) def _test_nexthop(self, manage_foreign_nexthops): if not manage_foreign_nexthops: @@ -4141,6 +4143,11 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): output = networkctl_status('test1') self.assertIn('State: routable (configuring)', output) + # Check if the route which needs nexthop 20 and 21 are forgotten. + output = networkctl_json() + check_json(output) + self.assertNotIn('"Destination":[10.10.10.14]', output) + # Reconfigure the interface that has nexthop with ID 20 and 21, # then the route requested by test1 can be configured. networkctl_reconfigure('dummy98')