From c2aed128369218eb280faade3209be99c5de5eb1 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Dec 2023 01:27:09 +0900 Subject: [PATCH 01/10] network: do not trigger assertion when link_get_by_index() called with an invalid ifindex Should not change any behavior. Preparation for later commit. --- src/network/networkd-link.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index a063696c82..0162e2acb0 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -252,7 +252,9 @@ int link_get_by_index(Manager *m, int ifindex, Link **ret) { Link *link; assert(m); - assert(ifindex > 0); + + if (ifindex <= 0) + return -EINVAL; link = hashmap_get(m->links_by_index, INT_TO_PTR(ifindex)); if (!link) From 58830ef5ebe9c933dd230ffcdb97a77451abc967 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Dec 2023 01:42:12 +0900 Subject: [PATCH 02/10] network/nexthop: split-out nexthop_update_group() No functional change, just refactoring and preparation for later commits. --- src/network/networkd-nexthop.c | 88 ++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 36 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 442e16b026..e04b352747 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -797,6 +797,56 @@ void link_foreignize_nexthops(Link *link) { } } +static int nexthop_update_group(NextHop *nexthop, const struct nexthop_grp *group, size_t size) { + _cleanup_hashmap_free_free_ Hashmap *h = NULL; + size_t n_group; + int r; + + assert(nexthop); + assert(group || size == 0); + + if (size == 0 || size % sizeof(struct nexthop_grp) != 0) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), + "rtnl: received nexthop message with invalid nexthop group size, ignoring."); + + if ((uintptr_t) group % alignof(struct nexthop_grp) != 0) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), + "rtnl: received nexthop message with invalid alignment, ignoring."); + + n_group = size / sizeof(struct nexthop_grp); + for (size_t i = 0; i < n_group; i++) { + _cleanup_free_ struct nexthop_grp *nhg = NULL; + + if (group[i].id == 0) { + log_debug("rtnl: received nexthop message with invalid ID in group, ignoring."); + continue; + } + + if (group[i].weight > 254) { + log_debug("rtnl: received nexthop message with invalid weight in group, ignoring."); + continue; + } + + nhg = newdup(struct nexthop_grp, group + i, 1); + if (!nhg) + return log_oom(); + + r = hashmap_ensure_put(&h, NULL, UINT32_TO_PTR(nhg->id), nhg); + if (r == -ENOMEM) + return log_oom(); + if (r < 0) { + log_debug_errno(r, "Failed to store nexthop group, ignoring: %m"); + continue; + } + if (r > 0) + TAKE_PTR(nhg); + } + + hashmap_free_free(nexthop->group); + nexthop->group = TAKE_PTR(h); + return 0; +} + int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, Manager *m) { _cleanup_(nexthop_freep) NextHop *tmp = NULL; _cleanup_free_ void *raw_group = NULL; @@ -876,43 +926,9 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, log_link_warning_errno(link, r, "rtnl: could not get NHA_GROUP attribute, ignoring: %m"); return 0; } else if (r >= 0) { - struct nexthop_grp *group = raw_group; - size_t n_group; - - if (raw_group_size == 0 || raw_group_size % sizeof(struct nexthop_grp) != 0) { - log_link_warning(link, "rtnl: received nexthop message with invalid nexthop group size, ignoring."); + r = nexthop_update_group(tmp, raw_group, raw_group_size); + if (r < 0) return 0; - } - - assert((uintptr_t) group % alignof(struct nexthop_grp) == 0); - - n_group = raw_group_size / sizeof(struct nexthop_grp); - for (size_t i = 0; i < n_group; i++) { - _cleanup_free_ struct nexthop_grp *nhg = NULL; - - if (group[i].id == 0) { - log_link_warning(link, "rtnl: received nexthop message with invalid ID in group, ignoring."); - return 0; - } - if (group[i].weight > 254) { - log_link_warning(link, "rtnl: received nexthop message with invalid weight in group, ignoring."); - return 0; - } - - nhg = newdup(struct nexthop_grp, group + i, 1); - if (!nhg) - return log_oom(); - - r = hashmap_ensure_put(&tmp->group, NULL, UINT32_TO_PTR(nhg->id), nhg); - if (r == -ENOMEM) - return log_oom(); - if (r < 0) { - log_link_warning_errno(link, r, "Failed to store nexthop group, ignoring: %m"); - return 0; - } - if (r > 0) - TAKE_PTR(nhg); - } } if (tmp->family != AF_UNSPEC) { From c624143b5950dedc7b210065f0fee53873d396c5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Dec 2023 01:59:34 +0900 Subject: [PATCH 03/10] network/nexthop: do not assign invalid ID This also reduces the scope of the iterator. --- src/network/networkd-nexthop.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index e04b352747..b10ddc46b8 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -309,7 +309,6 @@ static int nexthop_add(Manager *manager, Link *link, NextHop *nexthop) { static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) { _cleanup_set_free_ Set *ids = NULL; Network *network; - uint32_t id; int r; assert(manager); @@ -337,16 +336,17 @@ static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) { } } - for (id = 1; id < UINT32_MAX; id++) { + for (uint32_t id = 1; id < UINT32_MAX; id++) { if (manager_get_nexthop_by_id(manager, id, NULL) >= 0) continue; if (set_contains(ids, UINT32_TO_PTR(id))) continue; - break; + + nexthop->id = id; + return 0; } - nexthop->id = id; - return 0; + return -EBUSY; } static void log_nexthop_debug(const NextHop *nexthop, const char *str, const Link *link) { From a533473a48ff05e84e7af3d4049dbf458b6c0f94 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Dec 2023 18:40:43 +0900 Subject: [PATCH 04/10] network/nexthop: NextHop.id is always positive when nexthop_configure() is called If a nexthop is requested without a valid ID, then nexthop_acquire_id() assigns an unused ID. So, at the time nexthop_configure() is called, the ID is always valid. --- src/network/networkd-nexthop.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index b10ddc46b8..367759f373 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -435,6 +435,7 @@ static int nexthop_configure(NextHop *nexthop, Link *link, Request *req) { int r; assert(nexthop); + assert(nexthop->id > 0); assert(IN_SET(nexthop->family, AF_UNSPEC, AF_INET, AF_INET6)); assert(link); assert(link->manager); @@ -448,11 +449,9 @@ static int nexthop_configure(NextHop *nexthop, Link *link, Request *req) { if (r < 0) return r; - if (nexthop->id > 0) { - r = sd_netlink_message_append_u32(m, NHA_ID, nexthop->id); - if (r < 0) - return r; - } + r = sd_netlink_message_append_u32(m, NHA_ID, nexthop->id); + if (r < 0) + return r; if (!hashmap_isempty(nexthop->group)) { _cleanup_free_ struct nexthop_grp *group = NULL; From 84faee414168edf434ec612440c750d1d534e062 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 13 Dec 2023 03:20:05 +0900 Subject: [PATCH 05/10] network/nexthop: rename nexthop_owned_by_link() -> nexthop_bound_to_link() No functional change. --- src/network/networkd-nexthop.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 367759f373..bcb114094e 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -203,6 +203,11 @@ static int nexthop_dup(const NextHop *src, NextHop **ret) { return 0; } +static bool nexthop_bound_to_link(const NextHop *nexthop) { + assert(nexthop); + return !nexthop->blackhole && hashmap_isempty(nexthop->group); +} + int manager_get_nexthop_by_id(Manager *manager, uint32_t id, NextHop **ret) { NextHop *nh; @@ -220,17 +225,13 @@ int manager_get_nexthop_by_id(Manager *manager, uint32_t id, NextHop **ret) { return 0; } -static bool nexthop_owned_by_link(const NextHop *nexthop) { - return !nexthop->blackhole && hashmap_isempty(nexthop->group); -} - static int nexthop_get(Manager *manager, Link *link, NextHop *in, NextHop **ret) { NextHop *nexthop; Set *nexthops; assert(in); - if (nexthop_owned_by_link(in)) { + if (nexthop_bound_to_link(in)) { if (!link) return -ENOENT; @@ -279,7 +280,7 @@ static int nexthop_add(Manager *manager, Link *link, NextHop *nexthop) { assert(nexthop); assert(nexthop->id > 0); - if (nexthop_owned_by_link(nexthop)) { + if (nexthop_bound_to_link(nexthop)) { assert(link); r = set_ensure_put(&link->nexthops, &nexthop_hash_ops, nexthop); @@ -523,7 +524,7 @@ static bool nexthop_is_ready_to_configure(Link *link, const NextHop *nexthop) { if (!link_is_ready_to_configure(link, false)) return false; - if (nexthop_owned_by_link(nexthop)) { + if (nexthop_bound_to_link(nexthop)) { /* TODO: fdb nexthop does not require IFF_UP. The conditions below needs to be updated * when fdb nexthop support is added. See rtm_to_nh_config() in net/ipv4/nexthop.c of * kernel. */ @@ -959,7 +960,7 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, /* All blackhole or group nexthops are managed by Manager. Note that the linux kernel does not * set NHA_OID attribute when NHA_BLACKHOLE or NHA_GROUP is set. Just for safety. */ - if (!nexthop_owned_by_link(tmp)) + if (!nexthop_bound_to_link(tmp)) link = NULL; (void) nexthop_get(m, link, tmp, &nexthop); From ac217903354895f904392ff5ddcd8129779b70a9 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 13 Dec 2023 03:23:50 +0900 Subject: [PATCH 06/10] network/nexthop: rename manager_get_nexthop_by_id() -> nexthop_get_by_id() --- src/network/networkd-nexthop.c | 6 +++--- src/network/networkd-nexthop.h | 2 +- src/network/networkd-route.c | 8 ++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index bcb114094e..dc0e93e587 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -208,7 +208,7 @@ static bool nexthop_bound_to_link(const NextHop *nexthop) { return !nexthop->blackhole && hashmap_isempty(nexthop->group); } -int manager_get_nexthop_by_id(Manager *manager, uint32_t id, NextHop **ret) { +int nexthop_get_by_id(Manager *manager, uint32_t id, NextHop **ret) { NextHop *nh; assert(manager); @@ -338,7 +338,7 @@ static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) { } for (uint32_t id = 1; id < UINT32_MAX; id++) { - if (manager_get_nexthop_by_id(manager, id, NULL) >= 0) + if (nexthop_get_by_id(manager, id, NULL) >= 0) continue; if (set_contains(ids, UINT32_TO_PTR(id))) continue; @@ -538,7 +538,7 @@ static bool nexthop_is_ready_to_configure(Link *link, const NextHop *nexthop) { HASHMAP_FOREACH(nhg, nexthop->group) { NextHop *g; - if (manager_get_nexthop_by_id(link->manager, nhg->id, &g) < 0) + if (nexthop_get_by_id(link->manager, nhg->id, &g) < 0) return false; if (!nexthop_exists(g)) diff --git a/src/network/networkd-nexthop.h b/src/network/networkd-nexthop.h index 6f2aa6f6dc..6e5643015b 100644 --- a/src/network/networkd-nexthop.h +++ b/src/network/networkd-nexthop.h @@ -46,7 +46,7 @@ void link_foreignize_nexthops(Link *link); int link_request_static_nexthops(Link *link, bool only_ipv4); -int manager_get_nexthop_by_id(Manager *manager, uint32_t id, NextHop **ret); +int nexthop_get_by_id(Manager *manager, uint32_t id, NextHop **ret); int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, Manager *m); DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(NextHop, nexthop); diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 7218d799fc..3cb1f80e5f 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -461,7 +461,7 @@ static int route_convert(Manager *manager, const Route *route, ConvertedRoutes * struct nexthop_grp *nhg; NextHop *nh; - r = manager_get_nexthop_by_id(manager, route->nexthop_id, &nh); + r = nexthop_get_by_id(manager, route->nexthop_id, &nh); if (r < 0) return r; @@ -489,7 +489,7 @@ static int route_convert(Manager *manager, const Route *route, ConvertedRoutes * HASHMAP_FOREACH(nhg, nh->group) { NextHop *h; - r = manager_get_nexthop_by_id(manager, nhg->id, &h); + r = nexthop_get_by_id(manager, nhg->id, &h); if (r < 0) return r; @@ -1284,7 +1284,7 @@ static int route_is_ready_to_configure(const Route *route, Link *link) { struct nexthop_grp *nhg; NextHop *nh; - if (manager_get_nexthop_by_id(link->manager, route->nexthop_id, &nh) < 0) + if (nexthop_get_by_id(link->manager, route->nexthop_id, &nh) < 0) return false; if (!nexthop_exists(nh)) @@ -1293,7 +1293,7 @@ static int route_is_ready_to_configure(const Route *route, Link *link) { HASHMAP_FOREACH(nhg, nh->group) { NextHop *g; - if (manager_get_nexthop_by_id(link->manager, nhg->id, &g) < 0) + if (nexthop_get_by_id(link->manager, nhg->id, &g) < 0) return false; if (!nexthop_exists(g)) From 352eba2e49453a1b784ffbdb9509ba3f8a945b59 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 13 Dec 2023 03:43:27 +0900 Subject: [PATCH 07/10] network/nexthop: manage all nexthops by manager The kernel manages nexthops by their IDs. Previously networkd manages nexthops in three ways: - by the corresponding link, if a nexthop has ifindex, - by the manager, if a nexthop does not have ifindex, - by the manager with their IDs. This unifies the three managements of nexthops into one, and use the same way as the kernel uses. This is the one for nexthop already done by aa9626ee3b96e0d2a9a816b5efd38fd7dc829def for neighbor. --- src/network/networkd-json.c | 12 +- src/network/networkd-link.c | 1 - src/network/networkd-link.h | 1 - src/network/networkd-manager.c | 1 - src/network/networkd-manager.h | 3 - src/network/networkd-nexthop.c | 335 ++++++++++++--------------------- src/network/networkd-nexthop.h | 12 +- src/network/networkd-route.c | 4 +- 8 files changed, 138 insertions(+), 231 deletions(-) diff --git a/src/network/networkd-json.c b/src/network/networkd-json.c index eed8d9f787..b4defb7091 100644 --- a/src/network/networkd-json.c +++ b/src/network/networkd-json.c @@ -185,16 +185,20 @@ static int nexthop_build_json(NextHop *n, JsonVariant **ret) { JSON_BUILD_PAIR_STRING("ConfigState", state))); } -static int nexthops_append_json(Set *nexthops, JsonVariant **v) { +static int nexthops_append_json(Manager *manager, int ifindex, JsonVariant **v) { _cleanup_(json_variant_unrefp) JsonVariant *array = NULL; NextHop *nexthop; int r; + assert(manager); assert(v); - SET_FOREACH(nexthop, nexthops) { + HASHMAP_FOREACH(nexthop, manager->nexthops_by_id) { _cleanup_(json_variant_unrefp) JsonVariant *e = NULL; + if (nexthop->ifindex != ifindex) + continue; + r = nexthop_build_json(nexthop, &e); if (r < 0) return r; @@ -1354,7 +1358,7 @@ int link_build_json(Link *link, JsonVariant **ret) { if (r < 0) return r; - r = nexthops_append_json(link->nexthops, &v); + r = nexthops_append_json(link->manager, link->ifindex, &v); if (r < 0) return r; @@ -1417,7 +1421,7 @@ int manager_build_json(Manager *manager, JsonVariant **ret) { if (r < 0) return r; - r = nexthops_append_json(manager->nexthops, &v); + r = nexthops_append_json(manager, /* ifindex = */ 0, &v); if (r < 0) return r; diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 0162e2acb0..dd3b448ab2 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -209,7 +209,6 @@ static Link *link_free(Link *link) { link_dns_settings_clear(link); link->routes = set_free(link->routes); - link->nexthops = set_free(link->nexthops); link->neighbors = set_free(link->neighbors); link->addresses = set_free(link->addresses); link->qdiscs = set_free(link->qdiscs); diff --git a/src/network/networkd-link.h b/src/network/networkd-link.h index 7458ea93bd..cad9fa8410 100644 --- a/src/network/networkd-link.h +++ b/src/network/networkd-link.h @@ -125,7 +125,6 @@ typedef struct Link { Set *addresses; Set *neighbors; Set *routes; - Set *nexthops; Set *qdiscs; Set *tclasses; diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index 6ee01b28e0..b162d21aa0 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -649,7 +649,6 @@ Manager* manager_free(Manager *m) { * set_free() must be called after the above sd_netlink_unref(). */ m->routes = set_free(m->routes); - m->nexthops = set_free(m->nexthops); m->nexthops_by_id = hashmap_free(m->nexthops_by_id); sd_event_source_unref(m->speed_meter_event_source); diff --git a/src/network/networkd-manager.h b/src/network/networkd-manager.h index a4eb7d78af..a9827d8a4b 100644 --- a/src/network/networkd-manager.h +++ b/src/network/networkd-manager.h @@ -74,9 +74,6 @@ struct Manager { /* Manage nexthops by id. */ Hashmap *nexthops_by_id; - /* Manager stores nexthops without RTA_OIF attribute. */ - Set *nexthops; - /* Manager stores routes without RTA_OIF attribute. */ unsigned route_remove_messages; Set *routes; diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index dc0e93e587..5f2a8282bf 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -29,18 +29,9 @@ NextHop *nexthop_free(NextHop *nexthop) { config_section_free(nexthop->section); - if (nexthop->link) { - set_remove(nexthop->link->nexthops, nexthop); - - if (nexthop->link->manager && nexthop->id > 0) - hashmap_remove(nexthop->link->manager->nexthops_by_id, UINT32_TO_PTR(nexthop->id)); - } - if (nexthop->manager) { - set_remove(nexthop->manager->nexthops, nexthop); - - if (nexthop->id > 0) - hashmap_remove(nexthop->manager->nexthops_by_id, UINT32_TO_PTR(nexthop->id)); + assert(nexthop->id > 0); + hashmap_remove(nexthop->manager->nexthops_by_id, UINT32_TO_PTR(nexthop->id)); } hashmap_free_free(nexthop->group); @@ -50,6 +41,14 @@ NextHop *nexthop_free(NextHop *nexthop) { DEFINE_SECTION_CLEANUP_FUNCTIONS(NextHop, nexthop_free); +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( + nexthop_hash_ops, + void, + trivial_hash_func, + trivial_compare_func, + NextHop, + nexthop_free); + static int nexthop_new(NextHop **ret) { _cleanup_(nexthop_freep) NextHop *nexthop = NULL; @@ -106,35 +105,54 @@ static int nexthop_new_static(Network *network, const char *filename, unsigned s static void nexthop_hash_func(const NextHop *nexthop, struct siphash *state) { assert(nexthop); + assert(state); - siphash24_compress(&nexthop->protocol, sizeof(nexthop->protocol), state); siphash24_compress(&nexthop->id, sizeof(nexthop->id), state); - siphash24_compress(&nexthop->blackhole, sizeof(nexthop->blackhole), state); - siphash24_compress(&nexthop->family, sizeof(nexthop->family), state); - - switch (nexthop->family) { - case AF_INET: - case AF_INET6: - siphash24_compress(&nexthop->gw, FAMILY_ADDRESS_SIZE(nexthop->family), state); - - break; - default: - /* treat any other address family as AF_UNSPEC */ - break; - } } static int nexthop_compare_func(const NextHop *a, const NextHop *b) { + assert(a); + assert(b); + + return CMP(a->id, b->id); +} + +static int nexthop_compare_full(const NextHop *a, const NextHop *b) { int r; + assert(a); + assert(b); + + /* This compares detailed configs, except for ID and ifindex. */ + r = CMP(a->protocol, b->protocol); if (r != 0) return r; - r = CMP(a->id, b->id); + r = CMP(a->flags, b->flags); if (r != 0) return r; + r = CMP(hashmap_size(a->group), hashmap_size(b->group)); + if (r != 0) + return r; + + if (!hashmap_isempty(a->group)) { + struct nexthop_grp *ga; + + HASHMAP_FOREACH(ga, a->group) { + struct nexthop_grp *gb; + + gb = hashmap_get(b->group, UINT32_TO_PTR(ga->id)); + if (!gb) + return CMP(ga, gb); + + r = CMP(ga->weight, gb->weight); + if (r != 0) + return r; + } + } + r = CMP(a->blackhole, b->blackhole); if (r != 0) return r; @@ -143,29 +161,15 @@ static int nexthop_compare_func(const NextHop *a, const NextHop *b) { if (r != 0) return r; - if (IN_SET(a->family, AF_INET, AF_INET6)) - return memcmp(&a->gw, &b->gw, FAMILY_ADDRESS_SIZE(a->family)); + if (IN_SET(a->family, AF_INET, AF_INET6)) { + r = memcmp(&a->gw, &b->gw, FAMILY_ADDRESS_SIZE(a->family)); + if (r != 0) + return r; + } return 0; } -DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( - nexthop_hash_ops, - NextHop, - nexthop_hash_func, - nexthop_compare_func, - nexthop_free); - -static bool nexthop_equal(const NextHop *a, const NextHop *b) { - if (a == b) - return true; - - if (!a || !b) - return false; - - return nexthop_compare_func(a, b) == 0; -} - static int nexthop_dup(const NextHop *src, NextHop **ret) { _cleanup_(nexthop_freep) NextHop *dest = NULL; struct nexthop_grp *nhg; @@ -180,7 +184,6 @@ static int nexthop_dup(const NextHop *src, NextHop **ret) { /* unset all pointers */ dest->manager = NULL; - dest->link = NULL; dest->network = NULL; dest->section = NULL; dest->group = NULL; @@ -225,45 +228,23 @@ int nexthop_get_by_id(Manager *manager, uint32_t id, NextHop **ret) { return 0; } -static int nexthop_get(Manager *manager, Link *link, NextHop *in, NextHop **ret) { +static int nexthop_get(Link *link, const NextHop *in, NextHop **ret) { NextHop *nexthop; - Set *nexthops; + int ifindex; + assert(link); + assert(link->manager); assert(in); - if (nexthop_bound_to_link(in)) { - if (!link) - return -ENOENT; - - nexthops = link->nexthops; - } else { - if (!manager) - return -ENOENT; - - nexthops = manager->nexthops; - } - - nexthop = set_get(nexthops, in); - if (nexthop) { - if (ret) - *ret = nexthop; - return 0; - } - if (in->id > 0) - return -ENOENT; + return nexthop_get_by_id(link->manager, in->id, ret); - /* Also find nexthop configured without ID. */ - SET_FOREACH(nexthop, nexthops) { - uint32_t id; - bool found; + ifindex = nexthop_bound_to_link(in) ? link->ifindex : 0; - id = nexthop->id; - nexthop->id = 0; - found = nexthop_equal(nexthop, in); - nexthop->id = id; - - if (!found) + HASHMAP_FOREACH(nexthop, link->manager->nexthops_by_id) { + if (nexthop->ifindex != ifindex) + continue; + if (nexthop_compare_full(nexthop, in) != 0) continue; if (ret) @@ -274,37 +255,21 @@ static int nexthop_get(Manager *manager, Link *link, NextHop *in, NextHop **ret) return -ENOENT; } -static int nexthop_add(Manager *manager, Link *link, NextHop *nexthop) { +static int nexthop_add(Manager *manager, NextHop *nexthop) { int r; + assert(manager); assert(nexthop); assert(nexthop->id > 0); - if (nexthop_bound_to_link(nexthop)) { - assert(link); + r = hashmap_ensure_put(&manager->nexthops_by_id, &nexthop_hash_ops, UINT32_TO_PTR(nexthop->id), nexthop); + if (r < 0) + return r; + if (r == 0) + return -EEXIST; - r = set_ensure_put(&link->nexthops, &nexthop_hash_ops, nexthop); - if (r < 0) - return r; - if (r == 0) - return -EEXIST; - - nexthop->link = link; - - manager = link->manager; - } else { - assert(manager); - - r = set_ensure_put(&manager->nexthops, &nexthop_hash_ops, nexthop); - if (r < 0) - return r; - if (r == 0) - return -EEXIST; - - nexthop->manager = manager; - } - - return hashmap_ensure_put(&manager->nexthops_by_id, NULL, UINT32_TO_PTR(nexthop->id), nexthop); + nexthop->manager = manager; + return 0; } static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) { @@ -350,18 +315,19 @@ static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) { return -EBUSY; } -static void log_nexthop_debug(const NextHop *nexthop, const char *str, const Link *link) { +static void log_nexthop_debug(const NextHop *nexthop, const char *str, Manager *manager) { _cleanup_free_ char *state = NULL, *group = NULL, *flags = NULL; struct nexthop_grp *nhg; + Link *link = NULL; assert(nexthop); assert(str); - - /* link may be NULL. */ + assert(manager); if (!DEBUG_LOGGING) return; + (void) link_get_by_index(manager, nexthop->ifindex, &link); (void) network_config_state_to_string_alloc(nexthop->state, &state); (void) route_flags_to_string_alloc(nexthop->flags, &flags); @@ -395,22 +361,20 @@ static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link static int nexthop_remove(NextHop *nexthop) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; Manager *manager; - Link *link; + Link *link = NULL; int r; - assert(nexthop); - assert(nexthop->manager || (nexthop->link && nexthop->link->manager)); + manager = ASSERT_PTR(ASSERT_PTR(nexthop)->manager); /* link may be NULL. */ - link = nexthop->link; - manager = nexthop->manager ?: nexthop->link->manager; + (void) link_get_by_index(manager, nexthop->ifindex, &link); if (nexthop->id == 0) { log_link_debug(link, "Cannot remove nexthop without valid ID, ignoring."); return 0; } - log_nexthop_debug(nexthop, "Removing", link); + log_nexthop_debug(nexthop, "Removing", manager); r = sd_rtnl_message_new_nexthop(manager->rtnl, &m, RTM_DELNEXTHOP, AF_UNSPEC, RTPROT_UNSPEC); if (r < 0) @@ -444,7 +408,7 @@ static int nexthop_configure(NextHop *nexthop, Link *link, Request *req) { assert(link->ifindex > 0); assert(req); - log_nexthop_debug(nexthop, "Configuring", link); + log_nexthop_debug(nexthop, "Configuring", link->manager); r = sd_rtnl_message_new_nexthop(link->manager->rtnl, &m, RTM_NEWNEXTHOP, nexthop->family, nexthop->protocol); if (r < 0) @@ -475,7 +439,9 @@ static int nexthop_configure(NextHop *nexthop, Link *link, Request *req) { if (r < 0) return r; } else { - r = sd_netlink_message_append_u32(m, NHA_OIF, link->ifindex); + assert(nexthop->ifindex == link->ifindex); + + r = sd_netlink_message_append_u32(m, NHA_OIF, nexthop->ifindex); if (r < 0) return r; @@ -525,6 +491,8 @@ static bool nexthop_is_ready_to_configure(Link *link, const NextHop *nexthop) { return false; if (nexthop_bound_to_link(nexthop)) { + assert(nexthop->ifindex == link->ifindex); + /* TODO: fdb nexthop does not require IFF_UP. The conditions below needs to be updated * when fdb nexthop support is added. See rtm_to_nh_config() in net/ipv4/nexthop.c of * kernel. */ @@ -585,7 +553,7 @@ static int link_request_nexthop(Link *link, NextHop *nexthop) { assert(nexthop); assert(nexthop->source != NETWORK_CONFIG_SOURCE_FOREIGN); - if (nexthop_get(link->manager, link, nexthop, &existing) < 0) { + if (nexthop_get(link, nexthop, &existing) < 0) { _cleanup_(nexthop_freep) NextHop *tmp = NULL; r = nexthop_dup(nexthop, &tmp); @@ -596,7 +564,10 @@ static int link_request_nexthop(Link *link, NextHop *nexthop) { if (r < 0) return r; - r = nexthop_add(link->manager, link, tmp); + if (nexthop_bound_to_link(tmp)) + tmp->ifindex = link->ifindex; + + r = nexthop_add(link->manager, tmp); if (r < 0) return r; @@ -604,7 +575,7 @@ static int link_request_nexthop(Link *link, NextHop *nexthop) { } else existing->source = nexthop->source; - log_nexthop_debug(existing, "Requesting", link); + log_nexthop_debug(existing, "Requesting", link->manager); r = link_queue_request_safe(link, REQUEST_TYPE_NEXTHOP, existing, NULL, nexthop_hash_func, @@ -649,14 +620,15 @@ int link_request_static_nexthops(Link *link, bool only_ipv4) { return 0; } -static void manager_mark_nexthops(Manager *manager, bool foreign, const Link *except) { +static void link_mark_nexthops(Link *link, bool foreign) { NextHop *nexthop; - Link *link; + Link *other; - assert(manager); + assert(link); + assert(link->manager); /* First, mark all nexthops. */ - SET_FOREACH(nexthop, manager->nexthops) { + HASHMAP_FOREACH(nexthop, link->manager->nexthops_by_id) { /* do not touch nexthop created by the kernel */ if (nexthop->protocol == RTPROT_KERNEL) continue; @@ -669,33 +641,40 @@ static void manager_mark_nexthops(Manager *manager, bool foreign, const Link *ex if (!nexthop_exists(nexthop)) continue; + /* Ignore nexthops bound to other links. */ + if (nexthop->ifindex > 0 && nexthop->ifindex != link->ifindex) + continue; + nexthop_mark(nexthop); } /* Then, unmark all nexthops requested by active links. */ - HASHMAP_FOREACH(link, manager->links_by_index) { - if (link == except) + HASHMAP_FOREACH(other, link->manager->links_by_index) { + if (!foreign && other == link) continue; - if (!IN_SET(link->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED)) + if (!IN_SET(other->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED)) continue; - HASHMAP_FOREACH(nexthop, link->network->nexthops_by_section) { + HASHMAP_FOREACH(nexthop, other->network->nexthops_by_section) { NextHop *existing; - if (nexthop_get(manager, NULL, nexthop, &existing) >= 0) + if (nexthop_get(other, nexthop, &existing) >= 0) nexthop_unmark(existing); } } } -static int manager_drop_marked_nexthops(Manager *manager) { +int link_drop_nexthops(Link *link, bool foreign) { NextHop *nexthop; int r = 0; - assert(manager); + assert(link); + assert(link->manager); - SET_FOREACH(nexthop, manager->nexthops) { + link_mark_nexthops(link, foreign); + + HASHMAP_FOREACH(nexthop, link->manager->nexthops_by_id) { if (!nexthop_is_marked(nexthop)) continue; @@ -705,91 +684,15 @@ static int manager_drop_marked_nexthops(Manager *manager) { return r; } -int link_drop_foreign_nexthops(Link *link) { - NextHop *nexthop; - int r = 0; - - assert(link); - assert(link->manager); - assert(link->network); - - /* First, mark all nexthops. */ - SET_FOREACH(nexthop, link->nexthops) { - /* do not touch nexthop created by the kernel */ - if (nexthop->protocol == RTPROT_KERNEL) - continue; - - /* Do not remove nexthops we configured. */ - if (nexthop->source != NETWORK_CONFIG_SOURCE_FOREIGN) - continue; - - /* Ignore nexthops not assigned yet or already removed. */ - if (!nexthop_exists(nexthop)) - continue; - - nexthop_mark(nexthop); - } - - /* Then, unmark all nexthops requested by active links. */ - HASHMAP_FOREACH(nexthop, link->network->nexthops_by_section) { - NextHop *existing; - - if (nexthop_get(NULL, link, nexthop, &existing) >= 0) - nexthop_unmark(existing); - } - - /* Finally, remove all marked rules. */ - SET_FOREACH(nexthop, link->nexthops) { - if (!nexthop_is_marked(nexthop)) - continue; - - RET_GATHER(r, nexthop_remove(nexthop)); - } - - manager_mark_nexthops(link->manager, /* foreign = */ true, NULL); - - return RET_GATHER(r, manager_drop_marked_nexthops(link->manager)); -} - -int link_drop_managed_nexthops(Link *link) { - NextHop *nexthop; - int r = 0; - - assert(link); - assert(link->manager); - - SET_FOREACH(nexthop, link->nexthops) { - /* do not touch nexthop created by the kernel */ - if (nexthop->protocol == RTPROT_KERNEL) - continue; - - /* Do not touch addresses managed by kernel or other tools. */ - if (nexthop->source == NETWORK_CONFIG_SOURCE_FOREIGN) - continue; - - /* Ignore nexthops not assigned yet or already removing. */ - if (!nexthop_exists(nexthop)) - continue; - - RET_GATHER(r, nexthop_remove(nexthop)); - } - - manager_mark_nexthops(link->manager, /* foreign = */ false, link); - - return RET_GATHER(r, manager_drop_marked_nexthops(link->manager)); -} - void link_foreignize_nexthops(Link *link) { NextHop *nexthop; assert(link); + assert(link->manager); - SET_FOREACH(nexthop, link->nexthops) - nexthop->source = NETWORK_CONFIG_SOURCE_FOREIGN; + link_mark_nexthops(link, /* foreign = */ false); - manager_mark_nexthops(link->manager, /* foreign = */ false, link); - - SET_FOREACH(nexthop, link->manager->nexthops) { + HASHMAP_FOREACH(nexthop, link->manager->nexthops_by_id) { if (!nexthop_is_marked(nexthop)) continue; @@ -963,19 +866,21 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, if (!nexthop_bound_to_link(tmp)) link = NULL; - (void) nexthop_get(m, link, tmp, &nexthop); + tmp->ifindex = link ? link->ifindex : 0; + + (void) nexthop_get_by_id(m, tmp->id, &nexthop); switch (type) { case RTM_NEWNEXTHOP: if (nexthop) { nexthop->flags = tmp->flags; nexthop_enter_configured(nexthop); - log_nexthop_debug(tmp, "Received remembered", link); + log_nexthop_debug(tmp, "Received remembered", m); } else { nexthop_enter_configured(tmp); - log_nexthop_debug(tmp, "Remembering", link); + log_nexthop_debug(tmp, "Remembering", m); - r = nexthop_add(m, link, tmp); + r = nexthop_add(m, tmp); if (r < 0) { log_link_warning_errno(link, r, "Could not remember foreign nexthop, ignoring: %m"); return 0; @@ -989,12 +894,12 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, if (nexthop) { nexthop_enter_removed(nexthop); if (nexthop->state == 0) { - log_nexthop_debug(nexthop, "Forgetting", link); + log_nexthop_debug(nexthop, "Forgetting", m); nexthop_free(nexthop); } else - log_nexthop_debug(nexthop, "Removed", link); + log_nexthop_debug(nexthop, "Removed", m); } else - log_nexthop_debug(tmp, "Kernel removed unknown", link); + log_nexthop_debug(tmp, "Kernel removed unknown", m); break; default: diff --git a/src/network/networkd-nexthop.h b/src/network/networkd-nexthop.h index 6e5643015b..c1d39e6e8e 100644 --- a/src/network/networkd-nexthop.h +++ b/src/network/networkd-nexthop.h @@ -20,13 +20,12 @@ typedef struct Network Network; typedef struct NextHop { Network *network; Manager *manager; - Link *link; ConfigSection *section; NetworkConfigSource source; NetworkConfigState state; uint8_t protocol; - + int ifindex; uint32_t id; bool blackhole; int family; @@ -40,8 +39,13 @@ NextHop *nexthop_free(NextHop *nexthop); void network_drop_invalid_nexthops(Network *network); -int link_drop_managed_nexthops(Link *link); -int link_drop_foreign_nexthops(Link *link); +int link_drop_nexthops(Link *link, bool foreign); +static inline int link_drop_foreign_nexthops(Link *link) { + return link_drop_nexthops(link, /* foreign = */ true); +} +static inline int link_drop_managed_nexthops(Link *link) { + return link_drop_nexthops(link, /* foreign = */ false); +} void link_foreignize_nexthops(Link *link); int link_request_static_nexthops(Link *link, bool only_ipv4); diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 3cb1f80e5f..c0189ae899 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -475,7 +475,7 @@ static int route_convert(Manager *manager, const Route *route, ConvertedRoutes * return r; route_apply_nexthop(c->routes[0], nh, UINT8_MAX); - c->links[0] = nh->link; + (void) link_get_by_index(manager, nh->ifindex, c->links); *ret = TAKE_PTR(c); return 1; @@ -498,7 +498,7 @@ static int route_convert(Manager *manager, const Route *route, ConvertedRoutes * return r; route_apply_nexthop(c->routes[i], h, nhg->weight); - c->links[i] = h->link; + (void) link_get_by_index(manager, h->ifindex, c->links + i); i++; } From cdf3bee6a2dd281e36ca1e1ac6e4a05e46caa64e Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 13 Dec 2023 03:55:45 +0900 Subject: [PATCH 08/10] network/nexthop: do not add NextHop object to Link on requesting Then, all nexthops managed by networkd really exist (unless the kernel silently removes a nexthop). This is the same for nexthop already done by 3c283289aefb3cfb8bfa5c759209368b63d1692c and 0a0c2672dbd22dc85d660e5baa7e1bef701beb88 (for address), and 5d098f5d3614d1c0be7c825925637e9ab3d904fb (for neighbor). --- src/network/networkd-nexthop.c | 318 +++++++++++++++++++-------------- src/network/networkd-queue.c | 32 ++-- 2 files changed, 206 insertions(+), 144 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 5f2a8282bf..0683146492 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -255,12 +255,70 @@ static int nexthop_get(Link *link, const NextHop *in, NextHop **ret) { return -ENOENT; } -static int nexthop_add(Manager *manager, NextHop *nexthop) { +static int nexthop_get_request_by_id(Manager *manager, uint32_t id, Request **ret) { + Request *req; + + assert(manager); + + req = ordered_set_get( + manager->request_queue, + &(Request) { + .type = REQUEST_TYPE_NEXTHOP, + .userdata = (void*) &(const NextHop) { .id = id }, + .hash_func = (hash_func_t) nexthop_hash_func, + .compare_func = (compare_func_t) nexthop_compare_func, + }); + if (!req) + return -ENOENT; + + if (ret) + *ret = req; + return 0; +} + +static int nexthop_get_request(Link *link, const NextHop *in, Request **ret) { + Request *req; + int ifindex; + + assert(link); + assert(link->manager); + assert(in); + + if (in->id > 0) + return nexthop_get_request_by_id(link->manager, in->id, ret); + + ifindex = nexthop_bound_to_link(in) ? link->ifindex : 0; + + ORDERED_SET_FOREACH(req, link->manager->request_queue) { + if (req->type != REQUEST_TYPE_NEXTHOP) + continue; + + NextHop *nexthop = ASSERT_PTR(req->userdata); + if (nexthop->ifindex != ifindex) + continue; + if (nexthop_compare_full(nexthop, in) != 0) + continue; + + if (ret) + *ret = req; + return 0; + } + + return -ENOENT; +} + +static int nexthop_add_new(Manager *manager, uint32_t id, NextHop **ret) { + _cleanup_(nexthop_freep) NextHop *nexthop = NULL; int r; assert(manager); - assert(nexthop); - assert(nexthop->id > 0); + assert(id > 0); + + r = nexthop_new(&nexthop); + if (r < 0) + return r; + + nexthop->id = id; r = hashmap_ensure_put(&manager->nexthops_by_id, &nexthop_hash_ops, UINT32_TO_PTR(nexthop->id), nexthop); if (r < 0) @@ -269,6 +327,11 @@ static int nexthop_add(Manager *manager, NextHop *nexthop) { return -EEXIST; nexthop->manager = manager; + + if (ret) + *ret = nexthop; + + TAKE_PTR(nexthop); return 0; } @@ -305,6 +368,8 @@ static int nexthop_acquire_id(Manager *manager, NextHop *nexthop) { for (uint32_t id = 1; id < UINT32_MAX; id++) { if (nexthop_get_by_id(manager, id, NULL) >= 0) continue; + if (nexthop_get_request_by_id(manager, id, NULL) >= 0) + continue; if (set_contains(ids, UINT32_TO_PTR(id))) continue; @@ -362,6 +427,7 @@ static int nexthop_remove(NextHop *nexthop) { _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *m = NULL; Manager *manager; Link *link = NULL; + Request *req; int r; manager = ASSERT_PTR(ASSERT_PTR(nexthop)->manager); @@ -392,6 +458,9 @@ static int nexthop_remove(NextHop *nexthop) { link_ref(link); /* link may be NULL, link_ref() is OK with that */ nexthop_enter_removing(nexthop); + if (nexthop_get_request_by_id(manager, nexthop->id, &req) >= 0) + nexthop_enter_removing(req->userdata); + return 0; } @@ -528,10 +597,12 @@ static bool nexthop_is_ready_to_configure(Link *link, const NextHop *nexthop) { } static int nexthop_process_request(Request *req, Link *link, NextHop *nexthop) { + NextHop *existing; int r; assert(req); assert(link); + assert(link->manager); assert(nexthop); if (!nexthop_is_ready_to_configure(link, nexthop)) @@ -542,42 +613,49 @@ static int nexthop_process_request(Request *req, Link *link, NextHop *nexthop) { return log_link_warning_errno(link, r, "Failed to configure nexthop"); nexthop_enter_configuring(nexthop); + if (nexthop_get_by_id(link->manager, nexthop->id, &existing) >= 0) + nexthop_enter_configuring(existing); + return 1; } -static int link_request_nexthop(Link *link, NextHop *nexthop) { - NextHop *existing; +static int link_request_nexthop(Link *link, const NextHop *nexthop) { + _cleanup_(nexthop_freep) NextHop *tmp = NULL; + NextHop *existing = NULL; int r; assert(link); + assert(link->manager); assert(nexthop); assert(nexthop->source != NETWORK_CONFIG_SOURCE_FOREIGN); + if (nexthop_get_request(link, nexthop, NULL) >= 0) + return 0; /* already requested, skipping. */ + + r = nexthop_dup(nexthop, &tmp); + if (r < 0) + return r; + if (nexthop_get(link, nexthop, &existing) < 0) { - _cleanup_(nexthop_freep) NextHop *tmp = NULL; - - r = nexthop_dup(nexthop, &tmp); - if (r < 0) - return r; - r = nexthop_acquire_id(link->manager, tmp); if (r < 0) return r; + } else { + /* Copy ID */ + assert(tmp->id == 0 || tmp->id == existing->id); + tmp->id = existing->id; - if (nexthop_bound_to_link(tmp)) - tmp->ifindex = link->ifindex; + /* Copy state for logging below. */ + tmp->state = existing->state; + } - r = nexthop_add(link->manager, tmp); - if (r < 0) - return r; + if (nexthop_bound_to_link(tmp)) + tmp->ifindex = link->ifindex; - existing = TAKE_PTR(tmp); - } else - existing->source = nexthop->source; - - log_nexthop_debug(existing, "Requesting", link->manager); + log_nexthop_debug(tmp, "Requesting", link->manager); r = link_queue_request_safe(link, REQUEST_TYPE_NEXTHOP, - existing, NULL, + tmp, + nexthop_free, nexthop_hash_func, nexthop_compare_func, nexthop_process_request, @@ -587,7 +665,11 @@ static int link_request_nexthop(Link *link, NextHop *nexthop) { if (r <= 0) return r; - nexthop_enter_requesting(existing); + nexthop_enter_requesting(tmp); + if (existing) + nexthop_enter_requesting(existing); + + TAKE_PTR(tmp); return 1; } @@ -751,13 +833,13 @@ static int nexthop_update_group(NextHop *nexthop, const struct nexthop_grp *grou } int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, Manager *m) { - _cleanup_(nexthop_freep) NextHop *tmp = NULL; _cleanup_free_ void *raw_group = NULL; - NextHop *nexthop = NULL; size_t raw_group_size; - uint32_t ifindex; uint16_t type; - Link *link = NULL; + uint32_t id, ifindex; + NextHop *nexthop = NULL; + Request *req = NULL; + bool is_new = false; int r; assert(rtnl); @@ -781,131 +863,107 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, return 0; } - r = sd_netlink_message_read_u32(message, NHA_OIF, &ifindex); - if (r < 0 && r != -ENODATA) { - log_warning_errno(r, "rtnl: could not get NHA_OIF attribute, ignoring: %m"); + r = sd_netlink_message_read_u32(message, NHA_ID, &id); + if (r == -ENODATA) { + log_warning_errno(r, "rtnl: received nexthop message without NHA_ID attribute, ignoring: %m"); return 0; - } else if (r >= 0) { - if (ifindex <= 0) { - log_warning("rtnl: received nexthop message with invalid ifindex %"PRIu32", ignoring.", ifindex); - return 0; - } + } else if (r < 0) { + log_warning_errno(r, "rtnl: could not get NHA_ID attribute, ignoring: %m"); + return 0; + } else if (id == 0) { + log_warning("rtnl: received nexthop message with invalid nexthop ID, ignoring: %m"); + return 0; + } - r = link_get_by_index(m, ifindex, &link); + (void) nexthop_get_by_id(m, id, &nexthop); + (void) nexthop_get_request_by_id(m, id, &req); + + if (type == RTM_DELNEXTHOP) { + if (nexthop) { + nexthop_enter_removed(nexthop); + log_nexthop_debug(nexthop, "Forgetting removed", m); + nexthop_free(nexthop); + } else + log_nexthop_debug(&(const NextHop) { .id = id }, "Kernel removed unknown", m); + + if (req) + nexthop_enter_removed(req->userdata); + + return 0; + } + + /* If we did not know the nexthop, then save it. */ + if (!nexthop) { + r = nexthop_add_new(m, id, &nexthop); if (r < 0) { - if (!m->enumerating) - log_warning("rtnl: received nexthop message for link (%"PRIu32") we do not know about, ignoring", ifindex); + log_warning_errno(r, "Failed to add received nexthop, ignoring: %m"); return 0; } + + is_new = true; } - r = nexthop_new(&tmp); + /* Also update information that cannot be obtained through netlink notification. */ + if (req && req->waiting_reply) { + NextHop *n = ASSERT_PTR(req->userdata); + + nexthop->source = n->source; + } + + r = sd_rtnl_message_get_family(message, &nexthop->family); if (r < 0) - return log_oom(); + log_debug_errno(r, "rtnl: could not get nexthop family, ignoring: %m"); - r = sd_rtnl_message_get_family(message, &tmp->family); - if (r < 0) { - log_link_warning_errno(link, r, "rtnl: could not get nexthop family, ignoring: %m"); - return 0; - } else if (!IN_SET(tmp->family, AF_UNSPEC, AF_INET, AF_INET6)) { - log_link_debug(link, "rtnl: received nexthop message with invalid family %d, ignoring.", tmp->family); - return 0; - } + r = sd_rtnl_message_nexthop_get_protocol(message, &nexthop->protocol); + if (r < 0) + log_debug_errno(r, "rtnl: could not get nexthop protocol, ignoring: %m"); - r = sd_rtnl_message_nexthop_get_protocol(message, &tmp->protocol); - if (r < 0) { - log_link_warning_errno(link, r, "rtnl: could not get nexthop protocol, ignoring: %m"); - return 0; - } - - r = sd_rtnl_message_nexthop_get_flags(message, &tmp->flags); - if (r < 0) { - log_link_warning_errno(link, r, "rtnl: could not get nexthop flags, ignoring: %m"); - return 0; - } + r = sd_rtnl_message_nexthop_get_flags(message, &nexthop->flags); + if (r < 0) + log_debug_errno(r, "rtnl: could not get nexthop flags, ignoring: %m"); r = sd_netlink_message_read_data(message, NHA_GROUP, &raw_group_size, &raw_group); - if (r < 0 && r != -ENODATA) { - log_link_warning_errno(link, r, "rtnl: could not get NHA_GROUP attribute, ignoring: %m"); - return 0; - } else if (r >= 0) { - r = nexthop_update_group(tmp, raw_group, raw_group_size); - if (r < 0) - return 0; - } + if (r == -ENODATA) + nexthop->group = hashmap_free_free(nexthop->group); + else if (r < 0) + log_debug_errno(r, "rtnl: could not get NHA_GROUP attribute, ignoring: %m"); + else + (void) nexthop_update_group(nexthop, raw_group, raw_group_size); - if (tmp->family != AF_UNSPEC) { - r = netlink_message_read_in_addr_union(message, NHA_GATEWAY, tmp->family, &tmp->gw); - if (r < 0 && r != -ENODATA) { - log_link_warning_errno(link, r, "rtnl: could not get NHA_GATEWAY attribute, ignoring: %m"); - return 0; - } + if (nexthop->family != AF_UNSPEC) { + r = netlink_message_read_in_addr_union(message, NHA_GATEWAY, nexthop->family, &nexthop->gw); + if (r == -ENODATA) + nexthop->gw = IN_ADDR_NULL; + else if (r < 0) + log_debug_errno(r, "rtnl: could not get NHA_GATEWAY attribute, ignoring: %m"); } r = sd_netlink_message_has_flag(message, NHA_BLACKHOLE); - if (r < 0) { - log_link_warning_errno(link, r, "rtnl: could not get NHA_BLACKHOLE attribute, ignoring: %m"); - return 0; - } - tmp->blackhole = r; + if (r < 0) + log_debug_errno(r, "rtnl: could not get NHA_BLACKHOLE attribute, ignoring: %m"); + else + nexthop->blackhole = r; - r = sd_netlink_message_read_u32(message, NHA_ID, &tmp->id); - if (r == -ENODATA) { - log_link_warning_errno(link, r, "rtnl: received nexthop message without NHA_ID attribute, ignoring: %m"); - return 0; - } else if (r < 0) { - log_link_warning_errno(link, r, "rtnl: could not get NHA_ID attribute, ignoring: %m"); - return 0; - } else if (tmp->id == 0) { - log_link_warning(link, "rtnl: received nexthop message with invalid nexthop ID, ignoring: %m"); - return 0; - } + r = sd_netlink_message_read_u32(message, NHA_OIF, &ifindex); + if (r == -ENODATA) + nexthop->ifindex = 0; + else if (r < 0) + log_debug_errno(r, "rtnl: could not get NHA_OIF attribute, ignoring: %m"); + else if (ifindex > INT32_MAX) + log_debug_errno(r, "rtnl: received invalid NHA_OIF attribute, ignoring: %m"); + else + nexthop->ifindex = (int) ifindex; /* All blackhole or group nexthops are managed by Manager. Note that the linux kernel does not * set NHA_OID attribute when NHA_BLACKHOLE or NHA_GROUP is set. Just for safety. */ - if (!nexthop_bound_to_link(tmp)) - link = NULL; + if (!nexthop_bound_to_link(nexthop)) + nexthop->ifindex = 0; - tmp->ifindex = link ? link->ifindex : 0; - - (void) nexthop_get_by_id(m, tmp->id, &nexthop); - - switch (type) { - case RTM_NEWNEXTHOP: - if (nexthop) { - nexthop->flags = tmp->flags; - nexthop_enter_configured(nexthop); - log_nexthop_debug(tmp, "Received remembered", m); - } else { - nexthop_enter_configured(tmp); - log_nexthop_debug(tmp, "Remembering", m); - - r = nexthop_add(m, tmp); - if (r < 0) { - log_link_warning_errno(link, r, "Could not remember foreign nexthop, ignoring: %m"); - return 0; - } - - TAKE_PTR(tmp); - } - - break; - case RTM_DELNEXTHOP: - if (nexthop) { - nexthop_enter_removed(nexthop); - if (nexthop->state == 0) { - log_nexthop_debug(nexthop, "Forgetting", m); - nexthop_free(nexthop); - } else - log_nexthop_debug(nexthop, "Removed", m); - } else - log_nexthop_debug(tmp, "Kernel removed unknown", m); - break; - - default: - assert_not_reached(); - } + nexthop_enter_configured(nexthop); + if (req) + nexthop_enter_configured(req->userdata); + log_nexthop_debug(nexthop, is_new ? "Remembering" : "Received remembered", m); return 1; } diff --git a/src/network/networkd-queue.c b/src/network/networkd-queue.c index 0d782dc9a7..a88418e28e 100644 --- a/src/network/networkd-queue.c +++ b/src/network/networkd-queue.c @@ -58,12 +58,14 @@ static void request_hash_func(const Request *req, struct siphash *state) { assert(req); assert(state); - siphash24_compress_boolean(req->link, state); - if (req->link) - siphash24_compress(&req->link->ifindex, sizeof(req->link->ifindex), state); - siphash24_compress(&req->type, sizeof(req->type), state); + if (req->type != REQUEST_TYPE_NEXTHOP) { + siphash24_compress_boolean(req->link, state); + if (req->link) + siphash24_compress(&req->link->ifindex, sizeof(req->link->ifindex), state); + } + siphash24_compress(&req->hash_func, sizeof(req->hash_func), state); siphash24_compress(&req->compare_func, sizeof(req->compare_func), state); @@ -77,20 +79,22 @@ static int request_compare_func(const struct Request *a, const struct Request *b assert(a); assert(b); - r = CMP(!!a->link, !!b->link); - if (r != 0) - return r; - - if (a->link) { - r = CMP(a->link->ifindex, b->link->ifindex); - if (r != 0) - return r; - } - r = CMP(a->type, b->type); if (r != 0) return r; + if (a->type != REQUEST_TYPE_NEXTHOP) { + r = CMP(!!a->link, !!b->link); + if (r != 0) + return r; + + if (a->link) { + r = CMP(a->link->ifindex, b->link->ifindex); + if (r != 0) + return r; + } + } + r = CMP(PTR_TO_UINT64(a->hash_func), PTR_TO_UINT64(b->hash_func)); if (r != 0) return r; From 68149118cba3623c92a61ce57a1f75cc1a6ea6ba Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 13 Dec 2023 15:33:13 +0900 Subject: [PATCH 09/10] network/nexthop: check existing nexthop can be replaced with requested one --- src/network/networkd-nexthop.c | 35 ++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 0683146492..aeb33ae8d6 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -702,6 +702,31 @@ int link_request_static_nexthops(Link *link, bool only_ipv4) { return 0; } +static bool nexthop_can_update(const NextHop *assigned_nexthop, const NextHop *requested_nexthop) { + assert(assigned_nexthop); + assert(assigned_nexthop->manager); + assert(requested_nexthop); + assert(requested_nexthop->network); + + /* A group nexthop cannot be replaced with a non-group nexthop, and vice versa. + * See replace_nexthop_grp() and replace_nexthop_single() in net/ipv4/nexthop.c of the kernel. */ + if (hashmap_isempty(assigned_nexthop->group) != hashmap_isempty(requested_nexthop->group)) + return false; + + /* There are several more conditions if we can replace a group nexthop, e.g. hash threshold and + * resilience. But, currently we do not support to modify that. Let's add checks for them in the + * future when we support to configure them.*/ + + /* When a nexthop is replaced with a blackhole nexthop, and a group nexthop has multiple nexthops + * including this nexthop, then the kernel refuses to replace the existing nexthop. + * So, here, for simplicity, let's unconditionally refuse to replace a non-blackhole nexthop with + * a blackhole nexthop. See replace_nexthop() in net/ipv4/nexthop.c of the kernel. */ + if (!assigned_nexthop->blackhole && requested_nexthop->blackhole) + return false; + + return true; +} + static void link_mark_nexthops(Link *link, bool foreign) { NextHop *nexthop; Link *other; @@ -741,8 +766,14 @@ static void link_mark_nexthops(Link *link, bool foreign) { HASHMAP_FOREACH(nexthop, other->network->nexthops_by_section) { NextHop *existing; - if (nexthop_get(other, nexthop, &existing) >= 0) - nexthop_unmark(existing); + if (nexthop_get(other, nexthop, &existing) < 0) + continue; + + if (!nexthop_can_update(existing, nexthop)) + continue; + + /* Found matching static configuration. Keep the existing nexthop. */ + nexthop_unmark(existing); } } } From 5f6ab79571e974dd49c0be9db596dcfa72103f04 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 13 Dec 2023 15:37:06 +0900 Subject: [PATCH 10/10] sd-netlink: the kernel ignores NLM_F_APPEND in RTM_NEWNEXTHOP message but uses NLM_F_REPLACE See insert_nexthop() in net/ipv4/nexthop.c of the kernel. --- src/libsystemd/sd-netlink/netlink-message-rtnl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsystemd/sd-netlink/netlink-message-rtnl.c b/src/libsystemd/sd-netlink/netlink-message-rtnl.c index 008e8022b1..09116b1438 100644 --- a/src/libsystemd/sd-netlink/netlink-message-rtnl.c +++ b/src/libsystemd/sd-netlink/netlink-message-rtnl.c @@ -336,7 +336,7 @@ int sd_rtnl_message_new_nexthop(sd_netlink *rtnl, sd_netlink_message **ret, return r; if (nlmsg_type == RTM_NEWNEXTHOP) - (*ret)->hdr->nlmsg_flags |= NLM_F_CREATE | NLM_F_APPEND; + (*ret)->hdr->nlmsg_flags |= NLM_F_CREATE | NLM_F_REPLACE; nhm = NLMSG_DATA((*ret)->hdr);