Merge pull request #30953 from yuwata/network-nexthop-silently-removed-by-kernel

network/nexthop: forget nexthops silently removed by kernel

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);
 }
@@ -436,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;
 
@@ -451,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);
@@ -861,15 +932,22 @@ 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");
+
+        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.");
 
@@ -908,12 +986,12 @@ static int nexthop_update_group(NextHop *nexthop, const struct nexthop_grp *grou
 
         hashmap_free_free(nexthop->group);
         nexthop->group = TAKE_PTR(h);
+
+        nexthop_attach_to_group_members(nexthop);
         return 0;
 }
 
 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;
@@ -961,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);
@@ -1001,13 +1080,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);

src/network/networkd-nexthop.h

@@ -26,15 +26,23 @@ 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;
+
+        /* For managing nexthops that depend on this nexthop. */
+        Set *nexthops;
 } NextHop;
 
 NextHop* nexthop_ref(NextHop *nexthop);

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

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)