From 04a25cc425ff9651d375fbc14eeca3ebe48b5c20 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Jan 2024 11:59:21 +0900 Subject: [PATCH 1/5] network/nexthop: reorder elements in NextHop --- src/network/networkd-nexthop.h | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/src/network/networkd-nexthop.h b/src/network/networkd-nexthop.h index bc2bacfa40d..18b30e28be8 100644 --- a/src/network/networkd-nexthop.h +++ b/src/network/networkd-nexthop.h @@ -26,15 +26,20 @@ typedef struct NextHop { unsigned n_ref; - uint8_t protocol; - int ifindex; - uint32_t id; - bool blackhole; + /* struct nhmsg */ int family; - union in_addr_union gw; + uint8_t protocol; uint8_t flags; - int onlink; /* Only used in conf parser and nexthop_section_verify(). */ - Hashmap *group; + + /* attributes */ + uint32_t id; /* NHA_ID */ + Hashmap *group; /* NHA_GROUP */ + bool blackhole; /* NHA_BLACKHOLE */ + int ifindex; /* NHA_OIF */ + union in_addr_union gw; /* NHA_GATEWAY */ + + /* Only used in conf parser and nexthop_section_verify(). */ + int onlink; } NextHop; NextHop* nexthop_ref(NextHop *nexthop); From 4e9795ebfa3a7bb27f34153c858dba39b3018c4b Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Jan 2024 12:35:47 +0900 Subject: [PATCH 2/5] network/nexthop: read netlink message in nexthop_update_group() No functional change, preparation for later commits. --- src/network/networkd-nexthop.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 4de1e09999b..f440187c310 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -861,15 +861,20 @@ void link_foreignize_nexthops(Link *link) { } } -static int nexthop_update_group(NextHop *nexthop, const struct nexthop_grp *group, size_t size) { +static int nexthop_update_group(NextHop *nexthop, sd_netlink_message *message) { _cleanup_hashmap_free_free_ Hashmap *h = NULL; - size_t n_group; + _cleanup_free_ struct nexthop_grp *group = NULL; + size_t size = 0, n_group; int r; assert(nexthop); - assert(group || size == 0); + assert(message); - if (size == 0 || size % sizeof(struct nexthop_grp) != 0) + r = sd_netlink_message_read_data(message, NHA_GROUP, &size, (void**) &group); + if (r < 0 && r != -ENODATA) + return log_debug_errno(r, "rtnl: could not get NHA_GROUP attribute, ignoring: %m"); + + if (size % sizeof(struct nexthop_grp) != 0) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "rtnl: received nexthop message with invalid nexthop group size, ignoring."); @@ -912,8 +917,6 @@ 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_free_ void *raw_group = NULL; - size_t raw_group_size; uint16_t type; uint32_t id, ifindex; NextHop *nexthop = NULL; @@ -1001,13 +1004,7 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, 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 == -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); + (void) nexthop_update_group(nexthop, message); if (nexthop->family != AF_UNSPEC) { r = netlink_message_read_in_addr_union(message, NHA_GATEWAY, nexthop->family, &nexthop->gw); From 531c7246829a41dd7e51847bd4d77aa012ff478f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Jan 2024 12:39:19 +0900 Subject: [PATCH 3/5] network/nexthop: introduce a reverse map of nexthop group members It is not used in this commit, but will be used later. Preparation for later commits. --- src/network/networkd-nexthop.c | 50 ++++++++++++++++++++++++++++++++++ src/network/networkd-nexthop.h | 3 ++ 2 files changed, 53 insertions(+) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index f440187c310..49856c483a7 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -18,6 +18,48 @@ #include "stdio-util.h" #include "string-util.h" +static void nexthop_detach_from_group_members(NextHop *nexthop) { + assert(nexthop); + assert(nexthop->manager); + assert(nexthop->id > 0); + + struct nexthop_grp *nhg; + HASHMAP_FOREACH(nhg, nexthop->group) { + NextHop *nh; + + if (nexthop_get_by_id(nexthop->manager, nhg->id, &nh) < 0) + continue; + + set_remove(nh->nexthops, UINT32_TO_PTR(nexthop->id)); + } +} + +static void nexthop_attach_to_group_members(NextHop *nexthop) { + int r; + + assert(nexthop); + assert(nexthop->manager); + assert(nexthop->id > 0); + + struct nexthop_grp *nhg; + HASHMAP_FOREACH(nhg, nexthop->group) { + NextHop *nh; + + r = nexthop_get_by_id(nexthop->manager, nhg->id, &nh); + if (r < 0) { + if (nexthop->manager->manage_foreign_nexthops) + log_debug_errno(r, "Nexthop (id=%"PRIu32") has unknown group member (%"PRIu32"), ignoring.", + nexthop->id, nhg->id); + continue; + } + + r = set_ensure_put(&nh->nexthops, NULL, UINT32_TO_PTR(nexthop->id)); + if (r < 0) + log_debug_errno(r, "Failed to save nexthop ID (%"PRIu32") to group member (%"PRIu32"), ignoring: %m", + nexthop->id, nhg->id); + } +} + static NextHop* nexthop_detach_impl(NextHop *nexthop) { assert(nexthop); assert(!nexthop->manager || !nexthop->network); @@ -31,6 +73,9 @@ static NextHop* nexthop_detach_impl(NextHop *nexthop) { if (nexthop->manager) { assert(nexthop->id > 0); + + nexthop_detach_from_group_members(nexthop); + hashmap_remove(nexthop->manager->nexthops_by_id, UINT32_TO_PTR(nexthop->id)); nexthop->manager = NULL; return nexthop; @@ -51,6 +96,7 @@ static NextHop* nexthop_free(NextHop *nexthop) { config_section_free(nexthop->section); hashmap_free_free(nexthop->group); + set_free(nexthop->nexthops); return mfree(nexthop); } @@ -874,6 +920,8 @@ static int nexthop_update_group(NextHop *nexthop, sd_netlink_message *message) { if (r < 0 && r != -ENODATA) return log_debug_errno(r, "rtnl: could not get NHA_GROUP attribute, ignoring: %m"); + nexthop_detach_from_group_members(nexthop); + if (size % sizeof(struct nexthop_grp) != 0) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "rtnl: received nexthop message with invalid nexthop group size, ignoring."); @@ -913,6 +961,8 @@ static int nexthop_update_group(NextHop *nexthop, sd_netlink_message *message) { hashmap_free_free(nexthop->group); nexthop->group = TAKE_PTR(h); + + nexthop_attach_to_group_members(nexthop); return 0; } diff --git a/src/network/networkd-nexthop.h b/src/network/networkd-nexthop.h index 18b30e28be8..74b23bd7720 100644 --- a/src/network/networkd-nexthop.h +++ b/src/network/networkd-nexthop.h @@ -40,6 +40,9 @@ typedef struct NextHop { /* Only used in conf parser and nexthop_section_verify(). */ int onlink; + + /* For managing nexthops that depend on this nexthop. */ + Set *nexthops; } NextHop; NextHop* nexthop_ref(NextHop *nexthop); From 3cbbe8635a16f096a3b0eff993f7681401535605 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Jan 2024 13:14:46 +0900 Subject: [PATCH 4/5] network/nexthop: drop dependent nexthops on removal If a nexthop is removed, dependent nexthops are silently removed by the kernel. Hence, networkd may be confused that nexthops that depends on the nexthop still exist, and may fail to configure other routes or so. --- src/network/networkd-nexthop.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/network/networkd-nexthop.c b/src/network/networkd-nexthop.c index 49856c483a7..cf62e0e82dc 100644 --- a/src/network/networkd-nexthop.c +++ b/src/network/networkd-nexthop.c @@ -482,6 +482,29 @@ static void log_nexthop_debug(const NextHop *nexthop, const char *str, Manager * yes_no(nexthop->blackhole), strna(group), strna(flags)); } +static int nexthop_remove_dependents(NextHop *nexthop, Manager *manager) { + int r = 0; + + assert(nexthop); + assert(manager); + + /* If a nexthop is removed, the kernel silently removes nexthops 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. */ + + void *id; + SET_FOREACH(id, nexthop->nexthops) { + NextHop *nh; + + if (nexthop_get_by_id(manager, PTR_TO_UINT32(id), &nh) < 0) + continue; + + RET_GATHER(r, nexthop_remove(nh, manager)); + } + + return r; +} + static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, RemoveRequest *rreq) { int r; @@ -497,6 +520,8 @@ static int nexthop_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Remov (r == -ENOENT || !nexthop->manager) ? LOG_DEBUG : LOG_WARNING, r, "Could not drop nexthop, ignoring"); + (void) nexthop_remove_dependents(nexthop, manager); + if (nexthop->manager) { /* If the nexthop cannot be removed, then assume the nexthop is already removed. */ log_nexthop_debug(nexthop, "Forgetting", manager); @@ -1014,6 +1039,7 @@ int manager_rtnl_process_nexthop(sd_netlink *rtnl, sd_netlink_message *message, if (nexthop) { nexthop_enter_removed(nexthop); log_nexthop_debug(nexthop, "Forgetting removed", m); + (void) nexthop_remove_dependents(nexthop, m); nexthop_detach(nexthop); } else log_nexthop_debug(&(const NextHop) { .id = id }, "Kernel removed unknown", m); From f9b5c276458cae63d7587a1ad691a84078ef0fb3 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Mon, 15 Jan 2024 19:48:48 +0900 Subject: [PATCH 5/5] test-network: add test case for removal of nexthop that is a member of a group nexthop --- .../conf/25-nexthop-test1.network | 12 +++++++++ test/test-network/systemd-networkd-tests.py | 27 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 test/test-network/conf/25-nexthop-test1.network diff --git a/test/test-network/conf/25-nexthop-test1.network b/test/test-network/conf/25-nexthop-test1.network new file mode 100644 index 00000000000..5a4c596d3aa --- /dev/null +++ b/test/test-network/conf/25-nexthop-test1.network @@ -0,0 +1,12 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=test1 + +[Network] +Address=192.168.20.21/24 +IPv6AcceptRA=no + +[Route] +Destination=10.10.11.10 +# Nexthop 21 is configured as a group nexthop of 1 and 20 +NextHop=21 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index edf09b81204..6d074e67ff5 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -4033,6 +4033,33 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): self.check_nexthop(manage_foreign_nexthops, first=True) + # Remove nexthop with ID 20 + check_output('ip nexthop del id 20') + copy_network_unit('11-dummy.netdev', '25-nexthop-test1.network') + networkctl_reload() + + # 25-nexthop-test1.network requests a route with nexthop ID 21, + # which is silently removed by the kernel when nexthop with ID 20 is removed in the above, + # hence test1 should be stuck in the configuring state. + self.wait_operstate('test1', operstate='routable', setup_state='configuring') + + # Wait for a while, and check if the interface is still in the configuring state. + time.sleep(1) + output = networkctl_status('test1') + self.assertIn('State: routable (configuring)', output) + + # Reconfigure the interface that has nexthop with ID 20 and 21, + # then the route requested by test1 can be configured. + networkctl_reconfigure('dummy98') + self.wait_online(['test1:routable']) + + # Check if the requested route actually configured. + output = check_output('ip route show 10.10.11.10') + print(output) + self.assertIn('10.10.11.10 nhid 21 proto static', output) + self.assertIn('nexthop via 192.168.5.1 dev veth99 weight 3', output) + self.assertIn('nexthop via 192.168.20.1 dev dummy98 weight 1', output) + remove_link('veth99') time.sleep(2)