Merge pull request #31173 from yuwata/network-route-check-conflict

network/route: check if existing route can be updated

src/network/networkd-address.c

@@ -1383,15 +1383,16 @@ int link_drop_foreign_addresses(Link *link) {
         return r;
 }
 
-int link_drop_managed_addresses(Link *link) {
+int link_drop_static_addresses(Link *link) {
         Address *address;
         int r = 0;
 
         assert(link);
 
         SET_FOREACH(address, link->addresses) {
-                /* Do not touch addresses managed by kernel or other tools. */
-                if (address->source == NETWORK_CONFIG_SOURCE_FOREIGN)
+                /* Remove only static addresses here. Dynamic addresses will be removed e.g. on lease
+                 * expiration or stopping the DHCP client. */
+                if (address->source != NETWORK_CONFIG_SOURCE_STATIC)
                         continue;
 
                 /* Ignore addresses not assigned yet or already removing. */

src/network/networkd-address.h

@@ -103,7 +103,7 @@ bool link_check_addresses_ready(Link *link, NetworkConfigSource source);
 
 DEFINE_SECTION_CLEANUP_FUNCTIONS(Address, address_unref);
 
-int link_drop_managed_addresses(Link *link);
+int link_drop_static_addresses(Link *link);
 int link_drop_foreign_addresses(Link *link);
 int link_drop_ipv6ll_addresses(Link *link);
 void link_foreignize_addresses(Link *link);

src/network/networkd-link.c

@@ -1115,12 +1115,11 @@ static int link_drop_managed_config(Link *link) {
         assert(link);
         assert(link->manager);
 
-        r = link_drop_managed_routes(link);
-
-        RET_GATHER(r, link_drop_managed_nexthops(link));
-        RET_GATHER(r, link_drop_managed_addresses(link));
-        RET_GATHER(r, link_drop_managed_neighbors(link));
-        RET_GATHER(r, link_drop_managed_routing_policy_rules(link));
+        r = link_drop_static_routes(link);
+        RET_GATHER(r, link_drop_static_nexthops(link));
+        RET_GATHER(r, link_drop_static_addresses(link));
+        RET_GATHER(r, link_drop_static_neighbors(link));
+        RET_GATHER(r, link_drop_static_routing_policy_rules(link));
 
         return r;
 }

src/network/networkd-ndisc.c

@@ -205,7 +205,6 @@ static int ndisc_request_route(Route *route, Link *link, sd_ndisc_router *rt) {
         route->provider.in6 = router;
         if (!route->table_set)
                 route->table = link_get_ipv6_accept_ra_route_table(link);
-        ndisc_set_route_priority(link, route);
         if (!route->protocol_set)
                 route->protocol = RTPROT_RA;
         r = route_metric_set(&route->metric, RTAX_MTU, mtu);
@@ -222,6 +221,47 @@ static int ndisc_request_route(Route *route, Link *link, sd_ndisc_router *rt) {
         if (r < 0)
                 return r;
 
+        uint8_t pref, pref_original = route->pref;
+        FOREACH_ARGUMENT(pref, SD_NDISC_PREFERENCE_LOW, SD_NDISC_PREFERENCE_MEDIUM, SD_NDISC_PREFERENCE_HIGH) {
+                Route *existing;
+                Request *req;
+
+                /* If the preference is specified by the user config (that is, for semi-static routes),
+                 * rather than RA, then only search conflicting routes that have the same preference. */
+                if (route->pref_set && pref != pref_original)
+                        continue;
+
+                route->pref = pref;
+                ndisc_set_route_priority(link, route);
+
+                /* Note, here do not call route_remove_and_cancel() with 'route' directly, otherwise
+                 * existing route(s) may be removed needlessly. */
+
+                if (route_get(link->manager, route, &existing) >= 0) {
+                        /* Found an existing route that may conflict with this route. */
+                        if (!route_can_update(existing, route)) {
+                                log_link_debug(link, "Found an existing route that conflicts with new route based on a received RA, removing.");
+                                r = route_remove_and_cancel(existing, link->manager);
+                                if (r < 0)
+                                        return r;
+                        }
+                }
+
+                if (route_get_request(link->manager, route, &req) >= 0) {
+                        existing = ASSERT_PTR(req->userdata);
+                        if (!route_can_update(existing, route)) {
+                                log_link_debug(link, "Found a pending route request that conflicts with new request based on a received RA, cancelling.");
+                                r = route_remove_and_cancel(existing, link->manager);
+                                if (r < 0)
+                                        return r;
+                        }
+                }
+        }
+
+        /* The preference (and priority) may be changed in the above loop. Restore it. */
+        route->pref = pref_original;
+        ndisc_set_route_priority(link, route);
+
         is_new = route_get(link->manager, route, NULL) < 0;
 
         r = link_request_route(link, route, &link->ndisc_messages, ndisc_route_handler);
@@ -444,6 +484,41 @@ static int ndisc_router_process_retransmission_time(Link *link, sd_ndisc_router
         return 0;
 }
 
+static int ndisc_router_process_hop_limit(Link *link, sd_ndisc_router *rt) {
+        uint8_t hop_limit;
+        int r;
+
+        assert(link);
+        assert(link->network);
+        assert(rt);
+
+        if (!link->network->ipv6_accept_ra_use_hop_limit)
+                return 0;
+
+        r = sd_ndisc_router_get_hop_limit(rt, &hop_limit);
+        if (r < 0)
+                return log_link_warning_errno(link, r, "Failed to get hop limit from RA: %m");
+
+        /* 0 is the unspecified value and must not be set (see RFC4861, 6.3.4):
+         *
+         * A Router Advertisement field (e.g., Cur Hop Limit, Reachable Time, and Retrans Timer) may contain
+         * a value denoting that it is unspecified. In such cases, the parameter should be ignored and the
+         * host should continue using whatever value it is already using. In particular, a host MUST NOT
+         * interpret the unspecified value as meaning change back to the default value that was in use before
+         * the first Router Advertisement was received.
+         *
+         * If the received Cur Hop Limit value is non-zero, the host SHOULD set
+         * its CurHopLimit variable to the received value.*/
+        if (hop_limit <= 0)
+                return 0;
+
+        r = sysctl_write_ip_property_uint32(AF_INET6, link->ifname, "hop_limit", (uint32_t) hop_limit);
+        if (r < 0)
+                log_link_warning_errno(link, r, "Failed to apply hop_limit (%u), ignoring: %m", hop_limit);
+
+        return 0;
+}
+
 static int ndisc_router_process_autonomous_prefix(Link *link, sd_ndisc_router *rt) {
         usec_t lifetime_valid_usec, lifetime_preferred_usec;
         _cleanup_set_free_ Set *addresses = NULL;
@@ -1480,6 +1555,10 @@ static int ndisc_router_handler(Link *link, sd_ndisc_router *rt) {
         if (r < 0)
                 return r;
 
+        r = ndisc_router_process_hop_limit(link, rt);
+        if (r < 0)
+                return r;
+
         r = ndisc_router_process_options(link, rt);
         if (r < 0)
                 return r;
@@ -1640,7 +1719,8 @@ int ndisc_stop(Link *link) {
 void ndisc_flush(Link *link) {
         assert(link);
 
-        /* Remove all RDNSS, DNSSL, and Captive Portal entries, without exception. */
+        /* Remove all addresses, routes, RDNSS, DNSSL, and Captive Portal entries, without exception. */
+        (void) ndisc_drop_outdated(link, /* timestamp_usec = */ USEC_INFINITY);
 
         link->ndisc_rdnss = set_free(link->ndisc_rdnss);
         link->ndisc_dnssl = set_free(link->ndisc_dnssl);

src/network/networkd-neighbor.c

@@ -502,7 +502,7 @@ int link_drop_foreign_neighbors(Link *link) {
         return r;
 }
 
-int link_drop_managed_neighbors(Link *link) {
+int link_drop_static_neighbors(Link *link) {
         Neighbor *neighbor;
         int r = 0;
 
@@ -510,7 +510,7 @@ int link_drop_managed_neighbors(Link *link) {
 
         SET_FOREACH(neighbor, link->neighbors) {
                 /* Do not touch nexthops managed by kernel or other tools. */
-                if (neighbor->source == NETWORK_CONFIG_SOURCE_FOREIGN)
+                if (neighbor->source != NETWORK_CONFIG_SOURCE_STATIC)
                         continue;
 
                 /* Ignore neighbors not assigned yet or already removing. */

src/network/networkd-neighbor.h

@@ -36,7 +36,7 @@ int neighbor_remove(Neighbor *neighbor, Link *link);
 
 int network_drop_invalid_neighbors(Network *network);
 
-int link_drop_managed_neighbors(Link *link);
+int link_drop_static_neighbors(Link *link);
 int link_drop_foreign_neighbors(Link *link);
 void link_foreignize_neighbors(Link *link);
 

src/network/networkd-nexthop.c

@@ -869,7 +869,7 @@ static void link_mark_nexthops(Link *link, bool foreign) {
                         continue;
 
                 /* When 'foreign' is true, mark only foreign nexthops, and vice versa. */
-                if (foreign != (nexthop->source == NETWORK_CONFIG_SOURCE_FOREIGN))
+                if (nexthop->source != (foreign ? NETWORK_CONFIG_SOURCE_FOREIGN : NETWORK_CONFIG_SOURCE_STATIC))
                         continue;
 
                 /* Ignore nexthops not assigned yet or already removed. */

src/network/networkd-nexthop.h

@@ -57,7 +57,7 @@ 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) {
+static inline int link_drop_static_nexthops(Link *link) {
         return link_drop_nexthops(link, /* foreign = */ false);
 }
 void link_foreignize_nexthops(Link *link);

src/network/networkd-route-metric.c

@@ -63,6 +63,28 @@ int route_metric_compare_func(const RouteMetric *a, const RouteMetric *b) {
         return strcmp_ptr(a->tcp_congestion_control_algo, b->tcp_congestion_control_algo);
 }
 
+bool route_metric_can_update(const RouteMetric *a, const RouteMetric *b, bool expiration_by_kernel) {
+        assert(a);
+        assert(b);
+
+        /* If the kernel has expiration timer for the route, then only MTU can be updated. */
+
+        if (!expiration_by_kernel)
+                return route_metric_compare_func(a, b) == 0;
+
+        if (a->n_metrics != b->n_metrics)
+                return false;
+
+        for (size_t i = 1; i < a->n_metrics; i++) {
+                if (i != RTAX_MTU)
+                        continue;
+                if (a->metrics[i] != b->metrics[i])
+                        return false;
+        }
+
+        return streq_ptr(a->tcp_congestion_control_algo, b->tcp_congestion_control_algo);
+}
+
 int route_metric_set_full(RouteMetric *metric, uint16_t attr, uint32_t value, bool force) {
         assert(metric);
 

src/network/networkd-route-metric.h

@@ -26,6 +26,7 @@ int route_metric_copy(const RouteMetric *src, RouteMetric *dest);
 
 void route_metric_hash_func(const RouteMetric *metric, struct siphash *state);
 int route_metric_compare_func(const RouteMetric *a, const RouteMetric *b);
+bool route_metric_can_update(const RouteMetric *a, const RouteMetric *b, bool expiration_by_kernel);
 
 int route_metric_set_full(RouteMetric *metric, uint16_t attr, uint32_t value, bool force);
 static inline int route_metric_set(RouteMetric *metric, uint16_t attr, uint32_t value) {

src/network/networkd-route.c

@@ -352,7 +352,7 @@ static int route_get_link(Manager *manager, const Route *route, Link **ret) {
         return route_nexthop_get_link(manager, &route->nexthop, ret);
 }
 
-static int route_get_request(Manager *manager, const Route *route, Request **ret) {
+int route_get_request(Manager *manager, const Route *route, Request **ret) {
         Request *req;
 
         assert(manager);
@@ -1305,6 +1305,43 @@ static bool route_by_kernel(const Route *route) {
         return false;
 }
 
+bool route_can_update(const Route *existing, const Route *requesting) {
+        assert(existing);
+        assert(requesting);
+
+        if (route_compare_func(existing, requesting) != 0)
+                return false;
+
+        switch (existing->family) {
+        case AF_INET:
+                if (existing->nexthop.weight != requesting->nexthop.weight)
+                        return false;
+                return true;
+
+        case AF_INET6:
+                if (existing->protocol != requesting->protocol)
+                        return false;
+                if (existing->type != requesting->type)
+                        return false;
+                if (existing->flags != requesting->flags)
+                        return false;
+                if (!in6_addr_equal(&existing->prefsrc.in6, &requesting->prefsrc.in6))
+                        return false;
+                if (existing->pref != requesting->pref)
+                        return false;
+                if (existing->expiration_managed_by_kernel && requesting->lifetime_usec != USEC_INFINITY)
+                        return false; /* We cannot disable expiration timer in the kernel. */
+                if (!route_metric_can_update(&existing->metric, &requesting->metric, existing->expiration_managed_by_kernel))
+                        return false;
+                if (existing->nexthop.weight != requesting->nexthop.weight)
+                        return false;
+                return true;
+
+        default:
+                assert_not_reached();
+        }
+}
+
 static int link_unmark_route(Link *link, const Route *route, const RouteNextHop *nh) {
         _cleanup_(route_unrefp) Route *tmp = NULL;
         Route *existing;
@@ -1324,6 +1361,9 @@ static int link_unmark_route(Link *link, const Route *route, const RouteNextHop
         if (route_get(link->manager, tmp, &existing) < 0)
                 return 0;
 
+        if (!route_can_update(existing, tmp))
+                return 0;
+
         route_unmark(existing);
         return 1;
 }

src/network/networkd-route.h

@@ -96,9 +96,12 @@ 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);
+int route_get_request(Manager *manager, const Route *route, Request **ret);
+
+bool route_can_update(const Route *existing, const Route *requesting);
 
 int link_drop_routes(Link *link, bool foreign);
-static inline int link_drop_managed_routes(Link *link) {
+static inline int link_drop_static_routes(Link *link) {
         return link_drop_routes(link, false);
 }
 static inline int link_drop_foreign_routes(Link *link) {

src/network/networkd-routing-policy-rule.c

@@ -654,7 +654,7 @@ static void manager_mark_routing_policy_rules(Manager *m, bool foreign, const Li
                         continue;
 
                 /* When 'foreign' is true, mark only foreign rules, and vice versa. */
-                if (foreign != (rule->source == NETWORK_CONFIG_SOURCE_FOREIGN))
+                if (rule->source != (foreign ? NETWORK_CONFIG_SOURCE_FOREIGN : NETWORK_CONFIG_SOURCE_STATIC))
                         continue;
 
                 /* Ignore rules not assigned yet or already removing. */

src/network/networkd-routing-policy-rule.h

@@ -66,7 +66,7 @@ int manager_drop_routing_policy_rules_internal(Manager *m, bool foreign, const L
 static inline int manager_drop_foreign_routing_policy_rules(Manager *m) {
         return manager_drop_routing_policy_rules_internal(m, true, NULL);
 }
-static inline int link_drop_managed_routing_policy_rules(Link *link) {
+static inline int link_drop_static_routing_policy_rules(Link *link) {
         assert(link);
         return manager_drop_routing_policy_rules_internal(link->manager, false, link);
 }

test/test-network/conf/25-veth-bridge.network

@@ -1,10 +1,10 @@
 # SPDX-License-Identifier: LGPL-2.1-or-later
 [Match]
 Name=client-p
+Name=router-p
 Name=router-high-p
 Name=router-low-p
 
 [Network]
 Bridge=bridge99
 IPv6AcceptRA=no
-IPv6SendRA=yes

test/test-network/conf/25-veth-router-high2.network

@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Match]
+Name=router-low
+
+[Network]
+IPv6AcceptRA=no
+IPv6SendRA=yes
+
+[IPv6SendRA]
+# changed from low to high
+RouterPreference=high
+EmitDNS=no
+EmitDomains=no
+
+[IPv6Prefix]
+Prefix=2002:da8:1:98::/64
+PreferredLifetimeSec=1000s
+ValidLifetimeSec=2100s

test/test-network/conf/25-veth-router-hop-limit.network

@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Match]
+Name=router
+
+[Network]
+IPv6AcceptRA=no
+IPv6SendRA=yes
+
+[IPv6SendRA]
+RouterPreference=high
+EmitDNS=no
+EmitDomains=no
+HopLimit=42
+
+[IPv6Prefix]
+Prefix=2002:da8:1:99::/64
+PreferredLifetimeSec=1000s
+ValidLifetimeSec=2100s

test/test-network/conf/25-veth-router-low2.network

@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[Match]
+Name=router-high
+
+[Network]
+IPv6AcceptRA=no
+IPv6SendRA=yes
+
+[IPv6SendRA]
+# changed from high to low
+RouterPreference=low
+EmitDNS=no
+EmitDomains=no
+
+[IPv6Prefix]
+Prefix=2002:da8:1:99::/64
+PreferredLifetimeSec=1000s
+ValidLifetimeSec=2100s

test/test-network/conf/25-veth-router.netdev

@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: LGPL-2.1-or-later
+[NetDev]
+Name=router
+Kind=veth
+MACAddress=12:34:56:78:9a:99
+
+[Peer]
+Name=router-p
+MACAddress=12:34:56:78:9b:99

test/test-network/systemd-networkd-tests.py

@@ -5288,6 +5288,37 @@ class NetworkdRATests(unittest.TestCase, Utilities):
         self.assertIn('2002:da8:1:0:b47e:7975:fc7a:7d6e', output)
         self.assertIn('2002:da8:2:0:f689:561a:8eda:7443', output)
 
+    def check_router_hop_limit(self, hop_limit):
+        self.wait_route('client', rf'default via fe80::1034:56ff:fe78:9a99 proto ra .* hoplimit {hop_limit}', ipv='-6', timeout_sec=10)
+
+        output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a99')
+        print(output)
+        self.assertIn(f'hoplimit {hop_limit}', output)
+
+        self.check_ipv6_sysctl_attr('client', 'hop_limit', f'{hop_limit}')
+
+    def test_router_hop_limit(self):
+        copy_network_unit('25-veth-client.netdev',
+                          '25-veth-router.netdev',
+                          '26-bridge.netdev',
+                          '25-veth-bridge.network',
+                          '25-veth-client.network',
+                          '25-veth-router-hop-limit.network',
+                          '25-bridge99.network')
+        start_networkd()
+        self.wait_online('client-p:enslaved',
+                         'router:degraded', 'router-p:enslaved',
+                         'bridge99:routable')
+
+        self.check_router_hop_limit(42)
+
+        with open(os.path.join(network_unit_dir, '25-veth-router-hop-limit.network'), mode='a', encoding='utf-8') as f:
+            f.write('\n[IPv6SendRA]\nHopLimit=43\n')
+
+        networkctl_reload()
+
+        self.check_router_hop_limit(43)
+
     def test_router_preference(self):
         copy_network_unit('25-veth-client.netdev',
                           '25-veth-router-high.netdev',
@@ -5314,9 +5345,11 @@ class NetworkdRATests(unittest.TestCase, Utilities):
 
         output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a99')
         print(output)
+        self.assertIn('metric 512', output)
         self.assertIn('pref high', output)
         output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a98')
         print(output)
+        self.assertIn('metric 2048', output)
         self.assertIn('pref low', output)
 
         with open(os.path.join(network_unit_dir, '25-veth-client.network'), mode='a', encoding='utf-8') as f:
@@ -5332,11 +5365,35 @@ class NetworkdRATests(unittest.TestCase, Utilities):
 
         output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a99')
         print(output)
+        self.assertIn('metric 100', output)
+        self.assertNotIn('metric 512', output)
         self.assertIn('pref high', output)
         output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a98')
         print(output)
+        self.assertIn('metric 300', output)
+        self.assertNotIn('metric 2048', output)
         self.assertIn('pref low', output)
 
+        # swap the preference (for issue #28439)
+        remove_network_unit('25-veth-router-high.network', '25-veth-router-low.network')
+        copy_network_unit('25-veth-router-high2.network', '25-veth-router-low2.network')
+        networkctl_reload()
+        self.wait_route('client', 'default via fe80::1034:56ff:fe78:9a99 proto ra metric 300', ipv='-6', timeout_sec=10)
+        self.wait_route('client', 'default via fe80::1034:56ff:fe78:9a98 proto ra metric 100', ipv='-6', timeout_sec=10)
+
+        output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a99')
+        print(output)
+        self.assertIn('metric 300', output)
+        self.assertNotIn('metric 100', output)
+        self.assertIn('pref low', output)
+        self.assertNotIn('pref high', output)
+        output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a98')
+        print(output)
+        self.assertIn('metric 100', output)
+        self.assertNotIn('metric 300', output)
+        self.assertIn('pref high', output)
+        self.assertNotIn('pref low', output)
+
     @unittest.skipUnless(radvd_check_config('captive-portal.conf'), "Installed radvd doesn't support captive portals")
     def test_captive_portal(self):
         copy_network_unit('25-veth-client.netdev',