From 1d30fc5cb64ecba2f03fe42aa0d8c65c3decad82 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 4 Dec 2020 16:41:08 +0900 Subject: [PATCH 1/4] network: revert previous changes to address_compare_func() This partially reverts fe841414ef157f7f01d339c5d5730126e7b5fe0a and 2a236f9fc0ff8fb2152032551436fde74da7217a. For IPv4, kernel compares the local address, prefix, and prefixlen. For IPv6, kernel compares only the local address. Let's follow the kernel's comparison way. Fixes #17831. --- src/network/networkd-address.c | 79 ++++++++++++++++++---------------- src/network/test-network.c | 7 +-- 2 files changed, 44 insertions(+), 42 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 9de4d82662..bc7e0a5b59 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -153,26 +153,40 @@ static bool address_may_have_broadcast(const Address *a) { return a->family == AF_INET && in4_addr_is_null(&a->in_addr_peer.in) && a->prefixlen <= 30; } +static uint32_t address_prefix(const Address *a) { + assert(a); + + /* make sure we don't try to shift by 32. + * See ISO/IEC 9899:TC3 ยง 6.5.7.3. */ + if (a->prefixlen == 0) + return 0; + + if (a->in_addr_peer.in.s_addr != 0) + return be32toh(a->in_addr_peer.in.s_addr) >> (32 - a->prefixlen); + else + return be32toh(a->in_addr.in.s_addr) >> (32 - a->prefixlen); +} + void address_hash_func(const Address *a, struct siphash *state) { assert(a); siphash24_compress(&a->family, sizeof(a->family), state); - if (!IN_SET(a->family, AF_INET, AF_INET6)) - /* treat non-IPv4 or IPv6 address family as AF_UNSPEC */ - return; + switch (a->family) { + case AF_INET: + siphash24_compress(&a->prefixlen, sizeof(a->prefixlen), state); - if (a->family == AF_INET) - siphash24_compress_string(a->label, state); + uint32_t prefix = address_prefix(a); + siphash24_compress(&prefix, sizeof(prefix), state); - siphash24_compress(&a->prefixlen, sizeof(a->prefixlen), state); - /* local address */ - siphash24_compress(&a->in_addr, FAMILY_ADDRESS_SIZE(a->family), state); - /* peer address */ - siphash24_compress(&a->in_addr_peer, FAMILY_ADDRESS_SIZE(a->family), state); - - if (address_may_have_broadcast(a)) - siphash24_compress(&a->broadcast, sizeof(a->broadcast), state); + _fallthrough_; + case AF_INET6: + siphash24_compress(&a->in_addr, FAMILY_ADDRESS_SIZE(a->family), state); + break; + default: + /* treat any other address family as AF_UNSPEC */ + break; + } } int address_compare_func(const Address *a1, const Address *a2) { @@ -182,32 +196,25 @@ int address_compare_func(const Address *a1, const Address *a2) { if (r != 0) return r; - if (!IN_SET(a1->family, AF_INET, AF_INET6)) - /* treat non-IPv4 or IPv6 address family as AF_UNSPEC */ - return 0; - - if (a1->family == AF_INET) { - r = strcmp_ptr(a1->label, a2->label); + switch (a1->family) { + case AF_INET: + /* See kernel's find_matching_ifa() in net/ipv4/devinet.c */ + r = CMP(a1->prefixlen, a2->prefixlen); if (r != 0) return r; + + r = CMP(address_prefix(a1), address_prefix(a2)); + if (r != 0) + return r; + + _fallthrough_; + case AF_INET6: + /* See kernel's ipv6_get_ifaddr() in net/ipv6/addrconf.c */ + return memcmp(&a1->in_addr, &a2->in_addr, FAMILY_ADDRESS_SIZE(a1->family)); + default: + /* treat any other address family as AF_UNSPEC */ + return 0; } - - r = CMP(a1->prefixlen, a2->prefixlen); - if (r != 0) - return r; - - r = memcmp(&a1->in_addr, &a2->in_addr, FAMILY_ADDRESS_SIZE(a1->family)); - if (r != 0) - return r; - - r = memcmp(&a1->in_addr_peer, &a2->in_addr_peer, FAMILY_ADDRESS_SIZE(a1->family)); - if (r != 0) - return r; - - if (address_may_have_broadcast(a1)) - return CMP(a1->broadcast.s_addr, a2->broadcast.s_addr); - - return 0; } DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(address_hash_ops, Address, address_hash_func, address_compare_func, address_free); diff --git a/src/network/test-network.c b/src/network/test-network.c index bb67c74e9b..03c94409fa 100644 --- a/src/network/test-network.c +++ b/src/network/test-network.c @@ -159,10 +159,8 @@ static void test_address_equality(void) { assert_se(in_addr_from_string(AF_INET, "192.168.3.9", &a2->in_addr) >= 0); assert_se(address_equal(a1, a2)); assert_se(in_addr_from_string(AF_INET, "192.168.3.10", &a1->in_addr_peer) >= 0); - assert_se(!address_equal(a1, a2)); + assert_se(address_equal(a1, a2)); assert_se(in_addr_from_string(AF_INET, "192.168.3.11", &a2->in_addr_peer) >= 0); - assert_se(!address_equal(a1, a2)); - a2->in_addr_peer = a1->in_addr_peer; assert_se(address_equal(a1, a2)); a1->prefixlen = 10; assert_se(!address_equal(a1, a2)); @@ -173,13 +171,10 @@ static void test_address_equality(void) { assert_se(!address_equal(a1, a2)); a2->family = AF_INET6; - a1->in_addr_peer = a2->in_addr_peer = IN_ADDR_NULL; assert_se(in_addr_from_string(AF_INET6, "2001:4ca0:4f01::2", &a1->in_addr) >= 0); assert_se(in_addr_from_string(AF_INET6, "2001:4ca0:4f01::2", &a2->in_addr) >= 0); assert_se(address_equal(a1, a2)); - a1->prefixlen = 8; - assert_se(!address_equal(a1, a2)); a2->prefixlen = 8; assert_se(address_equal(a1, a2)); From c5a0aeb33ab2fbf52568dc0f69f49f3ac368d085 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 4 Dec 2020 17:29:16 +0900 Subject: [PATCH 2/4] network: use address_get() in address_exists() And rename address_exists() to link_has_ipv6_address(). --- src/network/networkd-address.c | 34 ++++++++++++++-------------------- src/network/networkd-address.h | 2 +- src/network/networkd-ndisc.c | 2 +- 3 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index bc7e0a5b59..18eecf6c8a 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -447,29 +447,23 @@ int address_get(Link *link, const Address *in, Address **ret) { return -ENOENT; } -static bool address_exists_internal(Set *addresses, int family, const union in_addr_union *in_addr) { - Address *address; +int link_has_ipv6_address(Link *link, const struct in6_addr *address) { + _cleanup_(address_freep) Address *a = NULL; + int r; - SET_FOREACH(address, addresses) { - if (address->family != family) - continue; - if (in_addr_equal(address->family, &address->in_addr, in_addr)) - return true; - } - - return false; -} - -bool address_exists(Link *link, int family, const union in_addr_union *in_addr) { assert(link); - assert(IN_SET(family, AF_INET, AF_INET6)); - assert(in_addr); + assert(address); - if (address_exists_internal(link->addresses, family, in_addr)) - return true; - if (address_exists_internal(link->addresses_foreign, family, in_addr)) - return true; - return false; + r = address_new(&a); + if (r < 0) + return r; + + /* address_compare_func() only compares the local address for IPv6 case. So, it is enough to + * set only family and the address. */ + a->family = AF_INET6; + a->in_addr.in6 = *address; + + return address_get(link, a, NULL) >= 0; } static int address_remove_handler(sd_netlink *rtnl, sd_netlink_message *m, Link *link) { diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index 036ac7a564..4764766996 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -49,7 +49,6 @@ typedef struct Address { int address_new(Address **ret); Address *address_free(Address *address); int address_get(Link *link, const Address *in, Address **ret); -bool address_exists(Link *link, int family, const union in_addr_union *in_addr); int address_configure(const Address *address, Link *link, link_netlink_message_handler_t callback, bool update, Address **ret); int address_remove(const Address *address, Link *link, link_netlink_message_handler_t callback); bool address_equal(const Address *a1, const Address *a2); @@ -63,6 +62,7 @@ int link_set_addresses(Link *link); int link_drop_addresses(Link *link); int link_drop_foreign_addresses(Link *link); bool link_address_is_dynamic(const Link *link, const Address *address); +int link_has_ipv6_address(Link *link, const struct in6_addr *address); void ipv4_dad_unref(Link *link); int ipv4_dad_stop(Link *link); diff --git a/src/network/networkd-ndisc.c b/src/network/networkd-ndisc.c index d2aa3db175..903c995e74 100644 --- a/src/network/networkd-ndisc.c +++ b/src/network/networkd-ndisc.c @@ -483,7 +483,7 @@ static int ndisc_router_process_default(Link *link, sd_ndisc_router *rt) { if (r < 0) return log_link_error_errno(link, r, "Failed to get gateway address from RA: %m"); - if (address_exists(link, AF_INET6, &gateway)) { + if (link_has_ipv6_address(link, &gateway.in6) > 0) { if (DEBUG_LOGGING) { _cleanup_free_ char *buffer = NULL; From 450fa34bd0315f2ca4728050b37c3a3d9e0f1b78 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 4 Dec 2020 20:50:34 +0900 Subject: [PATCH 3/4] network: fix SIGABRT related to unreachable route with DHCP6 After #17834, unreachable routes generated through DHCP6 are managed by Manager. But they are referrenced by the DHCP6 uplink. So, the routes managed by Manager must be freed after all Link objects are freed. Follow-up for 575f14eef010101c60e9d4d970e542c815be1994. Fixes SIGABRT reproted in #17831. --- src/network/networkd-manager.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index a10860b63b..8af17b1194 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -888,13 +888,17 @@ void manager_free(Manager *m) { m->rules_foreign = set_free(m->rules_foreign); set_free(m->rules_saved); - m->routes = set_free(m->routes); - m->routes_foreign = set_free(m->routes_foreign); - sd_netlink_unref(m->rtnl); sd_netlink_unref(m->genl); sd_resolve_unref(m->resolve); + /* reject (e.g. unreachable) type routes are managed by Manager, but may be referenced by a + * link. E.g., DHCP6 with prefix delegation creates unreachable routes, and they are referenced + * by the upstream link. And the links may be referenced by netlink slots. Hence, two + * set_free() must be called after the above sd_netlink_unref(). */ + m->routes = set_free(m->routes); + m->routes_foreign = set_free(m->routes_foreign); + sd_event_source_unref(m->speed_meter_event_source); sd_event_unref(m->event); From b432080dc8ad35e9b3b4fd487001e311c6d2776d Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Tue, 8 Dec 2020 13:35:48 +0900 Subject: [PATCH 4/4] network: warn when NDISC and DHCPv6 provide the same address With some router, the address in NDISC generated with EUI-64 conflicts with an address provided by DHCPv6. Prompted by #17831. --- src/network/networkd-dhcp6.c | 73 +++++++++++++++++++++++++++++++++--- 1 file changed, 67 insertions(+), 6 deletions(-) diff --git a/src/network/networkd-dhcp6.c b/src/network/networkd-dhcp6.c index 6592cdbfe0..51a34693c2 100644 --- a/src/network/networkd-dhcp6.c +++ b/src/network/networkd-dhcp6.c @@ -969,6 +969,70 @@ static int dhcp6_address_handler(sd_netlink *rtnl, sd_netlink_message *m, Link * return 1; } +static void log_dhcp6_address(Link *link, const Address *address, char **ret) { + char valid_buf[FORMAT_TIMESPAN_MAX], preferred_buf[FORMAT_TIMESPAN_MAX]; + const char *valid_str = NULL, *preferred_str = NULL; + _cleanup_free_ char *buffer = NULL; + bool by_ndisc = false; + Address *existing; + NDiscAddress *na; + int log_level, r; + + assert(link); + assert(address); + + (void) in_addr_to_string(address->family, &address->in_addr, &buffer); + if (address->cinfo.ifa_valid != CACHE_INFO_INFINITY_LIFE_TIME) + valid_str = format_timespan(valid_buf, FORMAT_TIMESPAN_MAX, + address->cinfo.ifa_valid * USEC_PER_SEC, + USEC_PER_SEC); + if (address->cinfo.ifa_prefered != CACHE_INFO_INFINITY_LIFE_TIME) + preferred_str = format_timespan(preferred_buf, FORMAT_TIMESPAN_MAX, + address->cinfo.ifa_prefered * USEC_PER_SEC, + USEC_PER_SEC); + + r = address_get(link, address, &existing); + if (r < 0) { + /* New address. */ + log_level = LOG_INFO; + goto simple_log; + } else + log_level = LOG_DEBUG; + + if (set_contains(link->dhcp6_addresses, address)) + /* Already warned. */ + goto simple_log; + + if (address->prefixlen == existing->prefixlen) + /* Currently, only conflict in prefix length is reported. */ + goto simple_log; + + SET_FOREACH(na, link->ndisc_addresses) + if (address_compare_func(na->address, existing)) { + by_ndisc = true; + break; + } + + log_link_warning(link, "DHCPv6 address %s/%u (valid %s%s, preferred %s%s) conflicts the existing address %s/%u%s.", + strnull(buffer), address->prefixlen, + valid_str ? "for " : "forever", strempty(valid_str), + preferred_str ? "for " : "forever", strempty(preferred_str), + strnull(buffer), existing->prefixlen, + by_ndisc ? "assigned by NDISC. Please try to use or update IPv6Token= setting " + "to change the address generated by NDISC, or disable UseAutonomousPrefix=" : ""); + goto finalize; + +simple_log: + log_link_full(link, log_level, "DHCPv6 address %s/%u (valid %s%s, preferred %s%s)", + strnull(buffer), address->prefixlen, + valid_str ? "for " : "forever", strempty(valid_str), + preferred_str ? "for " : "forever", strempty(preferred_str)); + +finalize: + if (ret) + *ret = TAKE_PTR(buffer); +} + static int dhcp6_update_address( Link *link, const struct in6_addr *ip6_addr, @@ -991,22 +1055,19 @@ static int dhcp6_update_address( addr->cinfo.ifa_prefered = lifetime_preferred; addr->cinfo.ifa_valid = lifetime_valid; - (void) in_addr_to_string(addr->family, &addr->in_addr, &buffer); - log_link_full(link, set_contains(link->dhcp6_addresses, addr) ? LOG_DEBUG : LOG_INFO, - "DHCPv6 address %s/%u timeout preferred %d valid %d", - strna(buffer), addr->prefixlen, lifetime_preferred, lifetime_valid); + log_dhcp6_address(link, addr, &buffer); r = address_configure(addr, link, dhcp6_address_handler, true, &ret); if (r < 0) return log_link_error_errno(link, r, "Failed to set DHCPv6 address %s/%u: %m", - strna(buffer), addr->prefixlen); + strnull(buffer), addr->prefixlen); link->dhcp6_address_messages++; r = set_ensure_put(&link->dhcp6_addresses, &address_hash_ops, ret); if (r < 0) return log_link_error_errno(link, r, "Failed to store DHCPv6 address %s/%u: %m", - strna(buffer), addr->prefixlen); + strnull(buffer), addr->prefixlen); (void) set_remove(link->dhcp6_addresses_old, ret);