From 2437ebee20ed9ce1e789c866728a73f244f80318 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Nov 2024 02:03:44 +0900 Subject: [PATCH 1/5] network/ndisc: introduce route_is_bound_to_link() helper function and use it where applicable No functional change, and preparation for later commits. --- src/network/networkd-ndisc.c | 8 ++++---- src/network/networkd-route.c | 12 ++++++++++++ src/network/networkd-route.h | 1 + 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index c1d2373f65..3b130ee4cc 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -2062,7 +2062,7 @@ static int ndisc_drop_outdated(Link *link, const struct in6_addr *router, usec_t if (route->source != NETWORK_CONFIG_SOURCE_NDISC) continue; - if (route->nexthop.ifindex != link->ifindex) + if (!route_is_bound_to_link(route, link)) continue; if (route->protocol == RTPROT_REDIRECT) @@ -2198,7 +2198,7 @@ static int ndisc_setup_expire(Link *link) { if (route->source != NETWORK_CONFIG_SOURCE_NDISC) continue; - if (route->nexthop.ifindex != link->ifindex) + if (!route_is_bound_to_link(route, link)) continue; if (!route_exists(route)) @@ -2436,7 +2436,7 @@ static int ndisc_neighbor_handle_router_message(Link *link, sd_ndisc_neighbor *n if (route->source != NETWORK_CONFIG_SOURCE_NDISC) continue; - if (route->nexthop.ifindex != link->ifindex) + if (!route_is_bound_to_link(route, link)) continue; if (!in6_addr_equal(&route->provider.in6, &original_address)) @@ -2729,7 +2729,7 @@ int link_drop_ndisc_config(Link *link, Network *network) { if (route->source != NETWORK_CONFIG_SOURCE_NDISC) continue; - if (route->nexthop.ifindex != link->ifindex) + if (!route_is_bound_to_link(route, link)) continue; if (route->protocol == RTPROT_REDIRECT) diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index 5104223396..b85d2a037a 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -352,6 +352,18 @@ static int route_get_link(Manager *manager, const Route *route, Link **ret) { return route_nexthop_get_link(manager, &route->nexthop, ret); } +bool route_is_bound_to_link(const Route *route, Link *link) { + assert(route); + assert(link); + assert(link->manager); + + Link *route_link; + if (route_get_link(link->manager, route, &route_link) < 0) + return false; + + return route_link->ifindex == link->ifindex; +} + int route_get_request(Manager *manager, const Route *route, Request **ret) { Request *req; diff --git a/src/network/networkd-route.h b/src/network/networkd-route.h index f391d95714..b9d2dbf9a6 100644 --- a/src/network/networkd-route.h +++ b/src/network/networkd-route.h @@ -100,6 +100,7 @@ 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); +bool route_is_bound_to_link(const Route *route, Link *link); int route_get_request(Manager *manager, const Route *route, Request **ret); bool route_can_update(const Route *existing, const Route *requesting); From 42d9660f10e928bfb575712b1a01773bbf8cf86f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Nov 2024 02:06:21 +0900 Subject: [PATCH 2/5] network/ndisc: several cleanups for ndisc_remove_route() - drop unnecessary call of ndisc_set_route_priority() at the beginning, as it is called later in the loop below, - use RET_GATHER() and remove all possible routes even if failed. --- src/network/networkd-ndisc.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index 3b130ee4cc..698fdbdb03 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -292,14 +292,12 @@ static int ndisc_request_router_route(Route *route, Link *link, sd_ndisc_router } static int ndisc_remove_route(Route *route, Link *link) { - int r; + int r, ret = 0; assert(route); assert(link); assert(link->manager); - ndisc_set_route_priority(link, route); - if (!route->table_set) route->table = link_get_ndisc_route_table(link); @@ -331,9 +329,7 @@ static int ndisc_remove_route(Route *route, Link *link) { if (existing->source == NETWORK_CONFIG_SOURCE_STATIC) continue; - r = route_remove_and_cancel(existing, link->manager); - if (r < 0) - return r; + RET_GATHER(ret, route_remove_and_cancel(existing, link->manager)); } /* Then, check if the route exists. */ @@ -341,13 +337,11 @@ static int ndisc_remove_route(Route *route, Link *link) { if (existing->source == NETWORK_CONFIG_SOURCE_STATIC) continue; - r = route_remove_and_cancel(existing, link->manager); - if (r < 0) - return r; + RET_GATHER(ret, route_remove_and_cancel(existing, link->manager)); } } - return 0; + return ret; } static int ndisc_address_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, Address *address) { From 74e0b590dd4defad123a7876784009d86c1b1e32 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Nov 2024 04:31:55 +0900 Subject: [PATCH 3/5] network/ndisc: introduce ndisc_route_prepare() and ndisc_router_route_prepare() These applies common parameters to the route to be requested or removed. No functional change, just refactoring and preparation for later commits. --- src/network/networkd-ndisc.c | 62 ++++++++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 16 deletions(-) diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index 698fdbdb03..0833cd3773 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -196,11 +196,6 @@ static int ndisc_request_route(Route *route, Link *link) { assert(link->manager); assert(link->network); - route->source = NETWORK_CONFIG_SOURCE_NDISC; - - if (!route->table_set) - route->table = link_get_ndisc_route_table(link); - r = route_metric_set(&route->metric, RTAX_QUICKACK, link->network->ndisc_quickack); if (r < 0) return r; @@ -274,6 +269,29 @@ static int ndisc_request_route(Route *route, Link *link) { return 0; } +static void ndisc_route_prepare(Route *route, Link *link) { + assert(route); + assert(link); + + route->source = NETWORK_CONFIG_SOURCE_NDISC; + + if (!route->table_set) + route->table = link_get_ndisc_route_table(link); +} + +static int ndisc_router_route_prepare(Route *route, Link *link, sd_ndisc_router *rt) { + assert(route); + assert(link); + assert(rt); + + ndisc_route_prepare(route, link); + + if (!route->protocol_set) + route->protocol = RTPROT_RA; + + return sd_ndisc_router_get_sender_address(rt, &route->provider.in6); +} + static int ndisc_request_router_route(Route *route, Link *link, sd_ndisc_router *rt) { int r; @@ -281,13 +299,10 @@ static int ndisc_request_router_route(Route *route, Link *link, sd_ndisc_router assert(link); assert(rt); - r = sd_ndisc_router_get_sender_address(rt, &route->provider.in6); + r = ndisc_router_route_prepare(route, link, rt); if (r < 0) return r; - if (!route->protocol_set) - route->protocol = RTPROT_RA; - return ndisc_request_route(route, link); } @@ -298,9 +313,6 @@ static int ndisc_remove_route(Route *route, Link *link) { assert(link); assert(link->manager); - if (!route->table_set) - route->table = link_get_ndisc_route_table(link); - r = route_adjust_nexthops(route, link); if (r < 0) return r; @@ -344,6 +356,20 @@ static int ndisc_remove_route(Route *route, Link *link) { return ret; } +static int ndisc_remove_router_route(Route *route, Link *link, sd_ndisc_router *rt) { + int r; + + assert(route); + assert(link); + assert(rt); + + r = ndisc_router_route_prepare(route, link, rt); + if (r < 0) + return r; + + return ndisc_remove_route(route, link); +} + static int ndisc_address_handler(sd_netlink *rtnl, sd_netlink_message *m, Request *req, Link *link, Address *address) { int r; @@ -474,6 +500,8 @@ static int ndisc_remove_redirect_route(Link *link, sd_ndisc_redirect *rd) { if (r < 0) return r; + ndisc_route_prepare(route, link); + return ndisc_remove_route(route, link); } @@ -674,6 +702,8 @@ static int ndisc_redirect_handler(Link *link, sd_ndisc_redirect *rd) { if (r < 0) return r; + ndisc_route_prepare(route, link); + return ndisc_request_route(route, link); } @@ -749,7 +779,7 @@ static int ndisc_router_drop_default(Link *link, sd_ndisc_router *rt) { route->nexthop.family = AF_INET6; route->nexthop.gw.in6 = gateway; - r = ndisc_remove_route(route, link); + r = ndisc_remove_router_route(route, link, rt); if (r < 0) return log_link_warning_errno(link, r, "Failed to remove the default gateway configured by RA: %m"); @@ -769,7 +799,7 @@ static int ndisc_router_drop_default(Link *link, sd_ndisc_router *rt) { tmp->nexthop.gw.in6 = gateway; - r = ndisc_remove_route(tmp, link); + r = ndisc_remove_router_route(tmp, link, rt); if (r < 0) return log_link_warning_errno(link, r, "Could not remove semi-static gateway: %m"); } @@ -1274,7 +1304,7 @@ static int ndisc_router_process_onlink_prefix(Link *link, sd_ndisc_router *rt) { * received advertisement, reset its invalidation timer to the Valid Lifetime value in the Prefix * Information option. If the new Lifetime value is zero, timeout the prefix immediately. */ if (lifetime_usec == 0) { - r = ndisc_remove_route(route, link); + r = ndisc_remove_router_route(route, link, rt); if (r < 0) return log_link_warning_errno(link, r, "Failed to remove prefix route: %m"); } else { @@ -1412,7 +1442,7 @@ static int ndisc_router_process_route(Link *link, sd_ndisc_router *rt) { if (r < 0) return log_link_warning_errno(link, r, "Could not request additional route: %m"); } else { - r = ndisc_remove_route(route, link); + r = ndisc_remove_router_route(route, link, rt); if (r < 0) return log_link_warning_errno(link, r, "Could not remove additional route with zero lifetime: %m"); } From e2060bc124238581d48be159b4da44017f96a26d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Nov 2024 02:11:12 +0900 Subject: [PATCH 4/5] network/ndisc: restore the original preference and priority before checking if existing route can be updated Follow-up for 972f1d17ab461a51142a142609dd3ec50bae8440. This fixes the logic of removing unnecessary routes configured by the previously received RAs. Previously, we wrongly handled existing routes could be updated, and unexpected routes would be kept. --- src/network/networkd-ndisc.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index 0833cd3773..3871618e5f 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -224,6 +224,9 @@ static int ndisc_request_route(Route *route, Link *link) { * and also an existing pending request, then the source may be updated by the request. So, * we first need to check the source of the requested route. */ if (route_get_request(link->manager, route, &req) >= 0) { + route->pref = pref_original; + ndisc_set_route_priority(link, route); + existing = ASSERT_PTR(req->userdata); if (!route_can_update(existing, route)) { if (existing->source == NETWORK_CONFIG_SOURCE_STATIC) { @@ -238,8 +241,14 @@ static int ndisc_request_route(Route *route, Link *link) { } } + route->pref = pref; + ndisc_set_route_priority(link, route); + /* Then, check if a conflicting route exists. */ if (route_get(link->manager, route, &existing) >= 0) { + route->pref = pref_original; + ndisc_set_route_priority(link, route); + if (!route_can_update(existing, route)) { if (existing->source == NETWORK_CONFIG_SOURCE_STATIC) { log_link_debug(link, "Found an existing route that conflicts with new route based on a received RA, ignoring request."); From d7b323c2dd1fbb3c3d40d8d053da12f3dee69365 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 12 Nov 2024 16:04:16 +0900 Subject: [PATCH 5/5] test-network: several cleanups - fix verifiers in test_router_preference() to make them actually check if unnecessary routes are removed, - stop radv in test_ndisc_vs_static_route() before checking if the static route is preserved even when the router sends a RA with zero lifetime, - make verifiers in NetworkdIPv6PrefixTests stricter. --- test/test-network/systemd-networkd-tests.py | 89 ++++++++++----------- 1 file changed, 43 insertions(+), 46 deletions(-) diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 3944996868..5ae57dd951 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -6312,14 +6312,11 @@ class NetworkdRATests(unittest.TestCase, Utilities): self.wait_route('client', 'default via fe80::1034:56ff:fe78:9a99 proto ra metric 512', ipv='-6', timeout_sec=10) self.wait_route('client', 'default via fe80::1034:56ff:fe78:9a98 proto ra metric 2048', ipv='-6', timeout_sec=10) - output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a99') + print('### ip -6 route show dev client default') + output = check_output('ip -6 route show dev client default') 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) + self.assertRegex(output, r'default via fe80::1034:56ff:fe78:9a99 proto ra metric 512 expires [0-9]*sec pref high') + self.assertRegex(output, r'default via fe80::1034:56ff:fe78:9a98 proto ra metric 2048 expires [0-9]*sec pref low') with open(os.path.join(network_unit_dir, '25-veth-client.network'), mode='a', encoding='utf-8') as f: f.write('\n[Link]\nMACAddress=12:34:56:78:9a:01\n[IPv6AcceptRA]\nRouteMetric=100:200:300\n') @@ -6332,16 +6329,13 @@ class NetworkdRATests(unittest.TestCase, Utilities): self.wait_route('client', 'default via fe80::1034:56ff:fe78:9a99 proto ra metric 100', ipv='-6', timeout_sec=10) self.wait_route('client', 'default via fe80::1034:56ff:fe78:9a98 proto ra metric 300', ipv='-6', timeout_sec=10) - output = check_output('ip -6 route show dev client default via fe80::1034:56ff:fe78:9a99') + print('### ip -6 route show dev client default') + output = check_output('ip -6 route show dev client default') print(output) - self.assertIn('metric 100', output) + self.assertRegex(output, r'default via fe80::1034:56ff:fe78:9a99 proto ra metric 100 expires [0-9]*sec pref high') + self.assertRegex(output, r'default via fe80::1034:56ff:fe78:9a98 proto ra metric 300 expires [0-9]*sec pref low') 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') @@ -6350,42 +6344,39 @@ class NetworkdRATests(unittest.TestCase, Utilities): 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('### ip -6 route show dev client default') + output = check_output('ip -6 route show dev client default') 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) + self.assertRegex(output, r'default via fe80::1034:56ff:fe78:9a99 proto ra metric 300 expires [0-9]*sec pref low') + self.assertRegex(output, r'default via fe80::1034:56ff:fe78:9a98 proto ra metric 100 expires [0-9]*sec pref high') + self.assertNotRegex(output, 'default via fe80::1034:56ff:fe78:9a99 proto ra metric 100') + self.assertNotRegex(output, 'default via fe80::1034:56ff:fe78:9a98 proto ra metric 300') + self.assertNotIn('metric 512', output) + self.assertNotIn('metric 2048', output) def test_ndisc_vs_static_route(self): copy_network_unit('25-veth.netdev', '25-ipv6-prefix.network', '25-ipv6-prefix-veth-static-route.network') start_networkd() self.wait_online('veth99:routable', 'veth-peer:degraded') - output = check_output('ip -6 route show dev veth99 table all') - print(output) - # If a conflicting static route is already configured, do not override the static route. - output = check_output('ip -6 route show dev veth99 default via fe80::1034:56ff:fe78:9abd') + print('### ip -6 route show dev veth99 default') + output = check_output('ip -6 route show dev veth99 default') print(output) - self.assertIn('default proto static metric 256 pref medium', output) + self.assertIn('via fe80::1034:56ff:fe78:9abd proto static metric 256 pref medium', output) self.assertNotIn('proto ra', output) - if not os.path.exists(test_ndisc_send): - self.skipTest(f"{test_ndisc_send} does not exist.") - # Also check if the static route is protected from RA with zero lifetime - check_output(f'{test_ndisc_send} --interface veth-peer --type router-advertisement --lifetime 0') - time.sleep(2) - output = check_output('ip -6 route show dev veth99 default via fe80::1034:56ff:fe78:9abd') + with open(os.path.join(network_unit_dir, '25-ipv6-prefix.network'), mode='a', encoding='utf-8') as f: + f.write('\n[Network]\nIPv6SendRA=no\n') + networkctl_reload() # This makes veth-peer being reconfigured, and send RA with zero lifetime + self.wait_route_dropped('veth99', 'default via fe80::1034:56ff:fe78:9abd proto ra metric 256', ipv='-6', timeout_sec=10) + + print('### ip -6 route show dev veth99 default') + output = check_output('ip -6 route show dev veth99 default') print(output) - self.assertIn('default proto static metric 256 pref medium', output) + self.assertIn('via fe80::1034:56ff:fe78:9abd proto static metric 256 pref medium', output) + self.assertNotIn('proto ra', output) # radvd supports captive portal since v2.20. # https://github.com/radvd-project/radvd/commit/791179a7f730decbddb2290ef0e34aa85d71b1bc @@ -8275,22 +8266,25 @@ class NetworkdIPv6PrefixTests(unittest.TestCase, Utilities): start_networkd() self.wait_online('veth99:routable', 'veth-peer:routable', 'dummy98:routable') - output = check_output('ip address show dev veth-peer') + print('### ip -6 address show dev veth-peer') + output = check_output('ip -6 address show dev veth-peer') print(output) self.assertIn('inet6 2001:db8:0:1:', output) self.assertNotIn('inet6 2001:db8:0:2:', output) self.assertNotIn('inet6 2001:db8:0:3:', output) + print('### ip -6 route show dev veth-peer') output = check_output('ip -6 route show dev veth-peer') print(output) self.assertIn('2001:db8:0:1::/64 proto ra', output) self.assertNotIn('2001:db8:0:2::/64 proto ra', output) self.assertNotIn('2001:db8:0:3::/64 proto ra', output) - self.assertIn('2001:db0:fff::/64 via ', output) - self.assertNotIn('2001:db1:fff::/64 via ', output) - self.assertNotIn('2001:db2:fff::/64 via ', output) + self.assertRegex(output, '2001:db0:fff::/64 via fe80::1034:56ff:fe78:9abc') + self.assertNotIn('2001:db1:fff::/64', output) + self.assertNotIn('2001:db2:fff::/64', output) - output = check_output('ip address show dev veth99') + print('### ip -6 address show dev veth99') + output = check_output('ip -6 address show dev veth99') print(output) self.assertNotIn('inet6 2001:db8:0:1:', output) self.assertIn('inet6 2001:db8:0:2:1a:2b:3c:4d', output) @@ -8326,19 +8320,22 @@ class NetworkdIPv6PrefixTests(unittest.TestCase, Utilities): start_networkd() self.wait_online('veth99:routable', 'veth-peer:routable', 'dummy98:routable') - output = check_output('ip address show dev veth-peer') + print('### ip -6 address show dev veth-peer') + output = check_output('ip -6 address show dev veth-peer') print(output) self.assertIn('inet6 2001:db8:0:1:', output) self.assertNotIn('inet6 2001:db8:0:2:', output) + print('### ip -6 route show dev veth-peer') output = check_output('ip -6 route show dev veth-peer') print(output) self.assertIn('2001:db8:0:1::/64 proto ra', output) self.assertNotIn('2001:db8:0:2::/64 proto ra', output) - self.assertIn('2001:db0:fff::/64 via ', output) - self.assertNotIn('2001:db1:fff::/64 via ', output) + self.assertRegex(output, '2001:db0:fff::/64 via fe80::1034:56ff:fe78:9abc') + self.assertNotIn('2001:db1:fff::/64', output) - output = check_output('ip address show dev veth99') + print('### ip -6 address show dev veth99') + output = check_output('ip -6 address show dev veth99') print(output) self.assertNotIn('inet6 2001:db8:0:1:', output) self.assertIn('inet6 2001:db8:0:2:', output)