From d9ef1cc40757ff6d6875812510cb4c03e7e488b5 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 31 Jul 2024 00:25:43 +0900 Subject: [PATCH 1/3] network/ipv4acd: manage ACD engines with Address object IPv4 addresses are managed with local and peer addresses and prefix length. So, potentially, the same address with different prefix length can be assigned on a link, e.g. 192.168.0.1/24 and 192.168.0.1/26. If one of the address is configured with ACD but the other is not, then previously ACD might be unexpectedly disabled or enabled on them, as we managed ACD engines with only local addresses. This makes ACD engines managed with the corresponding Address objects. --- src/network/networkd-address.c | 6 ++-- src/network/networkd-address.h | 6 ++-- src/network/networkd-ipv4acd.c | 63 +++++++++++++++++++++------------- 3 files changed, 44 insertions(+), 31 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index b4ac0bc41b..6e55c4f022 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -135,8 +135,6 @@ void link_get_address_states( *ret_all = address_state_from_scope(MIN(ipv4_scope, ipv6_scope)); } -static void address_hash_func(const Address *a, struct siphash *state); -static int address_compare_func(const Address *a1, const Address *a2); static void address_detach(Address *address); DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( @@ -446,7 +444,7 @@ static int address_ipv4_prefix(const Address *a, struct in_addr *ret) { return 0; } -static void address_hash_func(const Address *a, struct siphash *state) { +void address_hash_func(const Address *a, struct siphash *state) { assert(a); siphash24_compress_typesafe(a->family, state); @@ -476,7 +474,7 @@ static void address_hash_func(const Address *a, struct siphash *state) { } } -static int address_compare_func(const Address *a1, const Address *a2) { +int address_compare_func(const Address *a1, const Address *a2) { int r; r = CMP(a1->family, a2->family); diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index e09551ecda..8e8d04f817 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -88,6 +88,8 @@ void link_get_address_states( LinkAddressState *ret_ipv6, LinkAddressState *ret_all); +void address_hash_func(const Address *a, struct siphash *state); +int address_compare_func(const Address *a1, const Address *a2); extern const struct hash_ops address_hash_ops; bool address_can_update(const Address *existing, const Address *requesting); @@ -118,10 +120,6 @@ static inline int link_get_ipv6_address(Link *link, const struct in6_addr *addre assert(address); return link_get_address(link, AF_INET6, &(union in_addr_union) { .in6 = *address }, prefixlen, ret); } -static inline int link_get_ipv4_address(Link *link, const struct in_addr *address, unsigned char prefixlen, Address **ret) { - assert(address); - return link_get_address(link, AF_INET, &(union in_addr_union) { .in = *address }, prefixlen, ret); -} int manager_get_address(Manager *manager, int family, const union in_addr_union *address, unsigned char prefixlen, Address **ret); bool manager_has_address(Manager *manager, int family, const union in_addr_union *address); diff --git a/src/network/networkd-ipv4acd.c b/src/network/networkd-ipv4acd.c index de03293fff..6b1abc42e8 100644 --- a/src/network/networkd-ipv4acd.c +++ b/src/network/networkd-ipv4acd.c @@ -13,10 +13,14 @@ #include "networkd-link.h" #include "networkd-manager.h" -DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR( +DEFINE_PRIVATE_HASH_OPS_FULL( ipv4acd_hash_ops, - void, trivial_hash_func, trivial_compare_func, - sd_ipv4acd, sd_ipv4acd_unref); + Address, + address_hash_func, + address_compare_func, + address_unref, + sd_ipv4acd, + sd_ipv4acd_unref); bool link_ipv4acd_supported(Link *link) { assert(link); @@ -71,7 +75,7 @@ bool ipv4acd_bound(Link *link, const Address *address) { if (address->family != AF_INET) return true; - acd = hashmap_get(link->ipv4acd_by_address, IN4_ADDR_TO_PTR(&address->in_addr.in)); + acd = hashmap_get(link->ipv4acd_by_address, address); if (!acd) return true; @@ -127,24 +131,22 @@ static int dhcp4_address_on_conflict(Link *link) { static void on_acd(sd_ipv4acd *acd, int event, void *userdata) { Link *link = ASSERT_PTR(userdata); Address *address = NULL; - struct in_addr a; int r; assert(acd); - r = sd_ipv4acd_get_address(acd, &a); - if (r < 0) { - log_link_warning_errno(link, r, "Failed to get address from IPv4ACD: %m"); - link_enter_failed(link); - } + void *val, *key; + HASHMAP_FOREACH_KEY(val, key, link->ipv4acd_by_address) + if (val == acd) { + (void) address_get(link, key, &address); + break; + } - (void) link_get_ipv4_address(link, &a, 0, &address); + if (!address) + return; switch (event) { case SD_IPV4ACD_EVENT_STOP: - if (!address) - break; - if (address->source == NETWORK_CONFIG_SOURCE_STATIC) { r = static_ipv4acd_address_remove(link, address, /* on_conflict = */ false); if (r < 0) @@ -156,14 +158,11 @@ static void on_acd(sd_ipv4acd *acd, int event, void *userdata) { break; case SD_IPV4ACD_EVENT_BIND: - log_link_debug(link, "Successfully claimed address %s", IN4_ADDR_TO_STRING(&a)); + log_link_debug(link, "Successfully claimed address %s", IN4_ADDR_TO_STRING(&address->in_addr.in)); break; case SD_IPV4ACD_EVENT_CONFLICT: - if (!address) - break; - - log_link_warning(link, "Dropping address %s, as an address conflict was detected.", IN4_ADDR_TO_STRING(&a)); + log_link_warning(link, "Dropping address %s, as an address conflict was detected.", IN4_ADDR_TO_STRING(&address->in_addr.in)); if (address->source == NETWORK_CONFIG_SOURCE_STATIC) r = static_ipv4acd_address_remove(link, address, /* on_conflict = */ true); @@ -207,6 +206,7 @@ static int ipv4acd_start_one(Link *link, sd_ipv4acd *acd) { int ipv4acd_configure(Link *link, const Address *address) { _cleanup_(sd_ipv4acd_unrefp) sd_ipv4acd *acd = NULL; + _cleanup_(address_unrefp) Address *a = NULL; sd_ipv4acd *existing; int r; @@ -217,7 +217,7 @@ int ipv4acd_configure(Link *link, const Address *address) { if (address->family != AF_INET) return 0; - existing = hashmap_get(link->ipv4acd_by_address, IN4_ADDR_TO_PTR(&address->in_addr.in)); + existing = hashmap_get(link->ipv4acd_by_address, address); if (!address_ipv4acd_enabled(link, address)) return sd_ipv4acd_stop(existing); @@ -227,6 +227,15 @@ int ipv4acd_configure(Link *link, const Address *address) { log_link_debug(link, "Configuring IPv4ACD for address %s.", IN4_ADDR_TO_STRING(&address->in_addr.in)); + r = address_new(&a); + if (r < 0) + return r; + + a->family = AF_INET; + a->in_addr = address->in_addr; + a->in_addr_peer = address->in_addr_peer; + a->prefixlen = address->prefixlen; + r = sd_ipv4acd_new(&acd); if (r < 0) return r; @@ -255,11 +264,17 @@ int ipv4acd_configure(Link *link, const Address *address) { if (r < 0) return r; - r = hashmap_ensure_put(&link->ipv4acd_by_address, &ipv4acd_hash_ops, IN4_ADDR_TO_PTR(&address->in_addr.in), acd); + r = ipv4acd_start_one(link, acd); if (r < 0) return r; - return ipv4acd_start_one(link, TAKE_PTR(acd)); + r = hashmap_ensure_put(&link->ipv4acd_by_address, &ipv4acd_hash_ops, a, acd); + if (r < 0) + return r; + + TAKE_PTR(a); + TAKE_PTR(acd); + return 0; } void ipv4acd_detach(Link *link, const Address *address) { @@ -269,7 +284,9 @@ void ipv4acd_detach(Link *link, const Address *address) { if (address->family != AF_INET) return; - sd_ipv4acd_unref(hashmap_remove(link->ipv4acd_by_address, IN4_ADDR_TO_PTR(&address->in_addr.in))); + Address *a; + sd_ipv4acd_unref(hashmap_remove2(link->ipv4acd_by_address, address, (void**) &a)); + address_unref(a); } int ipv4acd_update_mac(Link *link) { From 56f91e2d0b218d09ee41981b24d2b53aabf599b7 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 31 Jul 2024 00:59:28 +0900 Subject: [PATCH 2/3] network: make link_get_address() provide matching address with peer As all callers do not care if the address has peer address. This also drops prefixlen argument as it is always zero. Fixes a bug introduced by 42f8b6a80878e688b821adfb315c0a1f0a7076ce. Fixes #31950. --- src/network/netdev/l2tp-tunnel.c | 4 +-- src/network/networkd-address.c | 50 +++++++++++++++++-------------- src/network/networkd-address.h | 28 +++++++++++++---- src/network/networkd-ndisc.c | 2 +- src/network/networkd-route-util.c | 4 +-- src/network/networkd-route.c | 12 ++++---- 6 files changed, 63 insertions(+), 37 deletions(-) diff --git a/src/network/netdev/l2tp-tunnel.c b/src/network/netdev/l2tp-tunnel.c index 8b9406bb08..4f9e7189d1 100644 --- a/src/network/netdev/l2tp-tunnel.c +++ b/src/network/netdev/l2tp-tunnel.c @@ -306,9 +306,9 @@ static int l2tp_get_local_address(NetDev *netdev, union in_addr_union *ret) { if (!a) { if (link) - r = link_get_address(link, t->family, &t->local, 0, &a); + r = link_get_address(link, t->family, &t->local, &a); else - r = manager_get_address(netdev->manager, t->family, &t->local, 0, &a); + r = manager_get_address(netdev->manager, t->family, &t->local, &a); if (r < 0) return r; diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 6e55c4f022..7377c22407 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -986,7 +986,14 @@ int address_get_harder(Link *link, const Address *in, Address **ret) { return 0; } -int link_get_address(Link *link, int family, const union in_addr_union *address, unsigned char prefixlen, Address **ret) { +int link_get_address_full( + Link *link, + int family, + const union in_addr_union *address, + const union in_addr_union *peer, /* optional, can be NULL */ + unsigned char prefixlen, /* optional, can be 0 */ + Address **ret) { + Address *a; int r; @@ -994,11 +1001,11 @@ int link_get_address(Link *link, int family, const union in_addr_union *address, assert(IN_SET(family, AF_INET, AF_INET6)); assert(address); - /* This find an Address object on the link which matches the given address and prefix length - * and does not have peer address. When the prefixlen is zero, then an Address object with an - * arbitrary prefixlen will be returned. */ + /* This finds an Address object on the link which matches the given address, peer, and prefix length. + * If the prefixlen is zero, then an Address object with an arbitrary prefixlen will be returned. + * If the peer is NULL, then an Address object with an arbitrary peer will be returned. */ - if (family == AF_INET6 || prefixlen != 0) { + if (family == AF_INET6 || (prefixlen != 0 && peer)) { _cleanup_(address_unrefp) Address *tmp = NULL; /* In this case, we can use address_get(). */ @@ -1009,6 +1016,8 @@ int link_get_address(Link *link, int family, const union in_addr_union *address, tmp->family = family; tmp->in_addr = *address; + if (peer) + tmp->in_addr_peer = *peer; tmp->prefixlen = prefixlen; r = address_get(link, tmp, &a); @@ -1018,7 +1027,7 @@ int link_get_address(Link *link, int family, const union in_addr_union *address, if (family == AF_INET6) { /* IPv6 addresses are managed without peer address and prefix length. Hence, we need * to check them explicitly. */ - if (in_addr_is_set(family, &a->in_addr_peer)) + if (peer && !in_addr_equal(family, &a->in_addr_peer, peer)) return -ENOENT; if (prefixlen != 0 && a->prefixlen != prefixlen) return -ENOENT; @@ -1037,7 +1046,10 @@ int link_get_address(Link *link, int family, const union in_addr_union *address, if (!in_addr_equal(family, &a->in_addr, address)) continue; - if (in_addr_is_set(family, &a->in_addr_peer)) + if (peer && !in_addr_equal(family, &a->in_addr_peer, peer)) + continue; + + if (prefixlen != 0 && a->prefixlen != prefixlen) continue; if (ret) @@ -1049,7 +1061,14 @@ int link_get_address(Link *link, int family, const union in_addr_union *address, return -ENOENT; } -int manager_get_address(Manager *manager, int family, const union in_addr_union *address, unsigned char prefixlen, Address **ret) { +int manager_get_address_full( + Manager *manager, + int family, + const union in_addr_union *address, + const union in_addr_union *peer, + unsigned char prefixlen, + Address **ret) { + Link *link; assert(manager); @@ -1060,26 +1079,13 @@ int manager_get_address(Manager *manager, int family, const union in_addr_union if (!IN_SET(link->state, LINK_STATE_CONFIGURING, LINK_STATE_CONFIGURED)) continue; - if (link_get_address(link, family, address, prefixlen, ret) >= 0) + if (link_get_address_full(link, family, address, peer, prefixlen, ret) >= 0) return 0; } return -ENOENT; } -bool manager_has_address(Manager *manager, int family, const union in_addr_union *address) { - Address *a; - - assert(manager); - assert(IN_SET(family, AF_INET, AF_INET6)); - assert(address); - - if (manager_get_address(manager, family, address, 0, &a) < 0) - return false; - - return address_is_ready(a); -} - const char* format_lifetime(char *buf, size_t l, usec_t lifetime_usec) { assert(buf); assert(l > 4); diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index 8e8d04f817..fcad445e17 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -115,13 +115,31 @@ int link_drop_foreign_addresses(Link *link); int link_drop_ipv6ll_addresses(Link *link); void link_foreignize_addresses(Link *link); bool link_address_is_dynamic(const Link *link, const Address *address); -int link_get_address(Link *link, int family, const union in_addr_union *address, unsigned char prefixlen, Address **ret); -static inline int link_get_ipv6_address(Link *link, const struct in6_addr *address, unsigned char prefixlen, Address **ret) { + +int link_get_address_full( + Link *link, + int family, + const union in_addr_union *address, + const union in_addr_union *peer, /* optional, can be NULL */ + unsigned char prefixlen, /* optional, can be 0 */ + Address **ret); +static inline int link_get_address(Link *link, int family, const union in_addr_union *address, Address **ret) { + return link_get_address_full(link, family, address, NULL, 0, ret); +} +static inline int link_get_ipv6_address(Link *link, const struct in6_addr *address, Address **ret) { assert(address); - return link_get_address(link, AF_INET6, &(union in_addr_union) { .in6 = *address }, prefixlen, ret); + return link_get_address(link, AF_INET6, &(union in_addr_union) { .in6 = *address }, ret); +} +int manager_get_address_full( + Manager *manager, + int family, + const union in_addr_union *address, + const union in_addr_union *peer, + unsigned char prefixlen, + Address **ret); +static inline int manager_get_address(Manager *manager, int family, const union in_addr_union *address, Address **ret) { + return manager_get_address_full(manager, family, address, NULL, 0, ret); } -int manager_get_address(Manager *manager, int family, const union in_addr_union *address, unsigned char prefixlen, Address **ret); -bool manager_has_address(Manager *manager, int family, const union in_addr_union *address); int link_request_address( Link *link, diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index 521a64253c..481014bba9 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -1382,7 +1382,7 @@ static int ndisc_router_process_route(Link *link, sd_ndisc_router *rt) { if (r < 0) return log_link_warning_errno(link, r, "Failed to get gateway address from RA: %m"); - if (link_get_ipv6_address(link, &gateway, 0, NULL) >= 0) { + if (link_get_ipv6_address(link, &gateway, NULL) >= 0) { if (DEBUG_LOGGING) log_link_debug(link, "Advertised route gateway %s is local to the link, ignoring route", IN6_ADDR_TO_STRING(&gateway)); diff --git a/src/network/networkd-route-util.c b/src/network/networkd-route-util.c index b7e07f4662..8d63b9b058 100644 --- a/src/network/networkd-route-util.c +++ b/src/network/networkd-route-util.c @@ -259,7 +259,7 @@ int link_address_is_reachable( return 0; } - r = link_get_address(link, route->family, &route->prefsrc, 0, &a); + r = link_get_address(link, route->family, &route->prefsrc, &a); if (r < 0) return r; @@ -312,7 +312,7 @@ int manager_address_is_reachable( if (r < 0) return r; - r = link_get_address(link, found->family, &found->prefsrc, 0, &a); + r = link_get_address(link, found->family, &found->prefsrc, &a); if (r < 0) return r; diff --git a/src/network/networkd-route.c b/src/network/networkd-route.c index d596fd81e6..9b6be2f2bf 100644 --- a/src/network/networkd-route.c +++ b/src/network/networkd-route.c @@ -790,8 +790,6 @@ static int route_requeue_request(Request *req, Link *link, const Route *route) { } static int route_is_ready_to_configure(const Route *route, Link *link) { - int r; - assert(route); assert(link); @@ -799,9 +797,13 @@ static int route_is_ready_to_configure(const Route *route, Link *link) { return false; if (in_addr_is_set(route->family, &route->prefsrc) > 0) { - r = manager_has_address(link->manager, route->family, &route->prefsrc); - if (r <= 0) - return r; + Address *a; + + if (manager_get_address(link->manager, route->family, &route->prefsrc, &a) < 0) + return false; + + if (!address_is_ready(a)) + return false; } return route_nexthops_is_ready_to_configure(route, link->manager); From f320c0777af30c1844ddb4d3d0a4535ab7d694bc Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Wed, 31 Jul 2024 01:24:00 +0900 Subject: [PATCH 3/3] test-network: add test case for preferred source with peer For issue #31950. --- test/test-network/conf/25-route-preferred-source.network | 9 +++++++++ test/test-network/systemd-networkd-tests.py | 4 ++++ 2 files changed, 13 insertions(+) diff --git a/test/test-network/conf/25-route-preferred-source.network b/test/test-network/conf/25-route-preferred-source.network index 5a55460349..2a337c4db6 100644 --- a/test/test-network/conf/25-route-preferred-source.network +++ b/test/test-network/conf/25-route-preferred-source.network @@ -6,7 +6,16 @@ Name=dummy98 Address=2001:1234:56:8f63::1/64 IPv6AcceptRA=no +[Address] +Address=10.10.10.1/32 +Peer=192.168.30.1/32 + [Route] Destination=abcd::/16 Gateway=2001:1234:56:8f63::1:1 PreferredSource=2001:1234:56:8f63::1 + +[Route] +Destination=10.123.0.0/16 +Gateway=192.168.30.1 +PreferredSource=10.10.10.1 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 1b521d73ce..b4d5f80e11 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -3707,6 +3707,10 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): print(output) self.assertIn('abcd::/16 via 2001:1234:56:8f63::1:1 proto static src 2001:1234:56:8f63::1', output) + output = check_output('ip -4 route list dev dummy98') + print(output) + self.assertIn('10.123.0.0/16 via 192.168.30.1 proto static src 10.10.10.1', output) + def test_ip_link_mac_address(self): copy_network_unit('25-address-link-section.network', '12-dummy.netdev') start_networkd()