Merge pull request #22607 from yuwata/network-address-hash-func

network: compare addresses more strictly

src/network/networkd-address.c

@@ -174,8 +174,9 @@ void link_mark_addresses(Link *link, NetworkConfigSource source, const struct in
         }
 }
 
-static bool address_may_have_broadcast(const Address *a) {
+static bool address_needs_to_set_broadcast(const Address *a, Link *link) {
         assert(a);
+        assert(link);
 
         if (a->family != AF_INET)
                 return false;
@@ -188,38 +189,26 @@ static bool address_may_have_broadcast(const Address *a) {
         if (a->prefixlen > 30)
                 return false;
 
+        /* If explicitly configured, do not update the address. */
+        if (in4_addr_is_set(&a->broadcast))
+                return false;
+
         if (a->set_broadcast >= 0)
                 return a->set_broadcast;
 
-        return true; /* Defaults to true. */
+        /* Defaults to true, except for wireguard, as typical configuration for wireguard does not set
+         * broadcast. */
+        return !streq_ptr(link->kind, "wireguard");
 }
 
-void address_set_broadcast(Address *a) {
-        assert(a);
-
-        if (!address_may_have_broadcast(a))
-                return;
-
-        /* If explicitly configured, do not update the address. */
-        if (in4_addr_is_set(&a->broadcast))
-                return;
-
-        /* If Address= is 0.0.0.0, then the broadcast address will be set later in address_acquire(). */
-        if (in4_addr_is_null(&a->in_addr.in))
-                return;
-
-        a->broadcast.s_addr = a->in_addr.in.s_addr | htobe32(UINT32_C(0xffffffff) >> a->prefixlen);
-}
-
-static bool address_may_set_broadcast(const Address *a, const Link *link) {
+void address_set_broadcast(Address *a, Link *link) {
         assert(a);
         assert(link);
 
-        if (!address_may_have_broadcast(a))
-                return false;
+        if (!address_needs_to_set_broadcast(a, link))
+                return;
 
-        /* Typical configuration for wireguard does not set broadcast. */
-        return !streq_ptr(link->kind, "wireguard");
+        a->broadcast.s_addr = a->in_addr.in.s_addr | htobe32(UINT32_C(0xffffffff) >> a->prefixlen);
 }
 
 static struct ifa_cacheinfo *address_set_cinfo(const Address *a, struct ifa_cacheinfo *cinfo) {
@@ -271,7 +260,7 @@ static uint32_t address_prefix(const Address *a) {
                 return be32toh(a->in_addr.in.s_addr) >> (32 - a->prefixlen);
 }
 
-void address_hash_func(const Address *a, struct siphash *state) {
+static void address_kernel_hash_func(const Address *a, struct siphash *state) {
         assert(a);
 
         siphash24_compress(&a->family, sizeof(a->family), state);
@@ -293,7 +282,7 @@ void address_hash_func(const Address *a, struct siphash *state) {
         }
 }
 
-int address_compare_func(const Address *a1, const Address *a2) {
+static int address_kernel_compare_func(const Address *a1, const Address *a2) {
         int r;
 
         r = CMP(a1->family, a2->family);
@@ -322,10 +311,68 @@ int address_compare_func(const Address *a1, const Address *a2) {
 }
 
 DEFINE_PRIVATE_HASH_OPS(
-        address_hash_ops,
+        address_kernel_hash_ops,
         Address,
-        address_hash_func,
-        address_compare_func);
+        address_kernel_hash_func,
+        address_kernel_compare_func);
+
+void address_hash_func(const Address *a, struct siphash *state) {
+        assert(a);
+
+        siphash24_compress(&a->family, sizeof(a->family), state);
+
+        /* treat any other address family as AF_UNSPEC */
+        if (!IN_SET(a->family, AF_INET, AF_INET6))
+                return;
+
+        siphash24_compress(&a->prefixlen, sizeof(a->prefixlen), state);
+        siphash24_compress(&a->in_addr, FAMILY_ADDRESS_SIZE(a->family), state);
+        siphash24_compress(&a->in_addr_peer, FAMILY_ADDRESS_SIZE(a->family), state);
+
+        if (a->family == AF_INET) {
+                /* On update, the kernel ignores the address label and broadcast address, hence we need
+                 * to distinguish addresses with different labels or broadcast addresses. Otherwise,
+                 * the label or broadcast address change will not be applied when we reconfigure the
+                 * interface. */
+                siphash24_compress_string(a->label, state);
+                siphash24_compress(&a->broadcast, sizeof(a->broadcast), state);
+        }
+}
+
+int address_compare_func(const Address *a1, const Address *a2) {
+        int r;
+
+        r = CMP(a1->family, a2->family);
+        if (r != 0)
+                return r;
+
+        if (!IN_SET(a1->family, AF_INET, AF_INET6))
+                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 (a1->family == AF_INET) {
+                r = strcmp_ptr(a1->label, a2->label);
+                if (r != 0)
+                        return r;
+
+                r = CMP(a1->broadcast.s_addr, a2->broadcast.s_addr);
+                if (r != 0)
+                        return r;
+        }
+
+        return 0;
+}
 
 DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR(
         address_hash_ops_free,
@@ -496,53 +543,54 @@ int address_get(Link *link, const Address *in, Address **ret) {
         return 0;
 }
 
-int link_get_ipv6_address(Link *link, const struct in6_addr *address, Address **ret) {
-        _cleanup_(address_freep) Address *a = NULL;
+int link_get_address(Link *link, int family, const union in_addr_union *address, unsigned char prefixlen, Address **ret) {
+        Address *a;
         int r;
 
         assert(link);
+        assert(IN_SET(family, AF_INET, AF_INET6));
         assert(address);
 
-        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, ret);
-}
-
-int link_get_ipv4_address(Link *link, const struct in_addr *address, unsigned char prefixlen, Address **ret) {
-        int r;
-
-        assert(link);
-        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. */
 
         if (prefixlen != 0) {
-                _cleanup_(address_freep) Address *a = NULL;
+                _cleanup_(address_freep) Address *tmp = NULL;
 
                 /* If prefixlen is set, then we can use address_get(). */
 
-                r = address_new(&a);
+                r = address_new(&tmp);
                 if (r < 0)
                         return r;
 
-                a->family = AF_INET;
-                a->in_addr.in = *address;
-                a->prefixlen = prefixlen;
+                tmp->family = family;
+                tmp->in_addr = *address;
+                tmp->prefixlen = prefixlen;
+                address_set_broadcast(tmp, link);
 
-                return address_get(link, a, ret);
-        } else {
-                Address *a;
+                if (address_get(link, tmp, &a) >= 0) {
+                        if (ret)
+                                *ret = a;
+
+                        return 0;
+                }
+
+                if (family == AF_INET6)
+                        return -ENOENT;
+
+                /* IPv4 addresses may have label and/or non-default broadcast address.
+                 * Hence, we need to always fallback below. */
+        }
 
         SET_FOREACH(a, link->addresses) {
-                        if (a->family != AF_INET)
+                if (a->family != family)
                         continue;
 
-                        if (!in4_addr_equal(&a->in_addr.in, address))
+                if (!in_addr_equal(family, &a->in_addr, address))
+                        continue;
+
+                if (in_addr_is_set(family, &a->in_addr_peer))
                         continue;
 
                 if (ret)
@@ -552,36 +600,19 @@ int link_get_ipv4_address(Link *link, const struct in_addr *address, unsigned ch
         }
 
         return -ENOENT;
-        }
 }
 
 int manager_has_address(Manager *manager, int family, const union in_addr_union *address, bool check_ready) {
         Address *a;
         Link *link;
-        int r;
 
         assert(manager);
         assert(IN_SET(family, AF_INET, AF_INET6));
         assert(address);
 
-        if (family == AF_INET) {
         HASHMAP_FOREACH(link, manager->links_by_index)
-                        if (link_get_ipv4_address(link, &address->in, 0, &a) >= 0)
+                if (link_get_address(link, family, address, 0, &a) >= 0)
                         return check_ready ? address_is_ready(a) : address_exists(a);
-        } else {
-                _cleanup_(address_freep) Address *tmp = NULL;
-
-                r = address_new(&tmp);
-                if (r < 0)
-                        return r;
-
-                tmp->family = family;
-                tmp->in_addr = *address;
-
-                HASHMAP_FOREACH(link, manager->links_by_index)
-                        if (address_get(link, tmp, &a) >= 0)
-                                return check_ready ? address_is_ready(a) : address_exists(a);
-        }
 
         return false;
 }
@@ -963,7 +994,6 @@ static int address_acquire(Link *link, const Address *original, Address **ret) {
                 return r;
 
         na->in_addr = in_addr;
-        address_set_broadcast(na);
 
         *ret = TAKE_PTR(na);
         return 1;
@@ -1025,7 +1055,7 @@ static int address_configure(
                 r = netlink_message_append_in_addr_union(req, IFA_ADDRESS, address->family, &address->in_addr_peer);
                 if (r < 0)
                         return log_link_error_errno(link, r, "Could not append IFA_ADDRESS attribute: %m");
-        } else if (address_may_set_broadcast(address, link)) {
+        } else if (in4_addr_is_set(&address->broadcast)) {
                 r = sd_netlink_message_append_in_addr(req, IFA_BROADCAST, &address->broadcast);
                 if (r < 0)
                         return log_link_error_errno(link, r, "Could not append IFA_BROADCAST attribute: %m");
@@ -1120,6 +1150,21 @@ int link_request_address(
                 consume_object = true;
         }
 
+        if (address_needs_to_set_broadcast(address, link)) {
+                if (!consume_object) {
+                        Address *a;
+
+                        r = address_dup(address, &a);
+                        if (r < 0)
+                                return r;
+
+                        address = a;
+                        consume_object = true;
+                }
+
+                address_set_broadcast(address, link);
+        }
+
         if (address_get(link, address, &existing) < 0) {
                 _cleanup_(address_freep) Address *tmp = NULL;
 
@@ -1910,10 +1955,11 @@ static int address_section_verify(Address *address) {
                                          address->section->filename, address->section->line);
         }
 
-        if (address_may_have_broadcast(address))
-                address_set_broadcast(address);
-        else if (address->broadcast.s_addr != 0) {
-                log_warning("%s: broadcast address is set for IPv6 address or IPv4 address with prefixlength larger than 30. "
+        if (in4_addr_is_set(&address->broadcast) &&
+            (address->family == AF_INET6 || address->prefixlen > 30 ||
+             in_addr_is_set(address->family, &address->in_addr_peer))) {
+                log_warning("%s: broadcast address is set for an IPv6 address, "
+                            "an IPv4 address with peer address, or with prefix length larger than 30. "
                             "Ignoring Broadcast= setting in the [Address] section from line %u.",
                             address->section->filename, address->section->line);
 
@@ -1980,8 +2026,10 @@ int network_drop_invalid_addresses(Network *network) {
                         address_free(dup);
                 }
 
-                /* Do not use address_hash_ops_free here. Otherwise, all address settings will be freed. */
-                r = set_ensure_put(&addresses, &address_hash_ops, address);
+                /* Use address_kernel_hash_ops here. The function address_kernel_compare_func() matches
+                 * how kernel compares addresses, and is more lenient than address_compare_func().
+                 * Hence, the logic of dedup here is stricter than when address_hash_ops is used. */
+                r = set_ensure_put(&addresses, &address_kernel_hash_ops, address);
                 if (r < 0)
                         return log_oom();
                 assert(r > 0);

src/network/networkd-address.h

@@ -70,7 +70,7 @@ int address_configure_handler_internal(sd_netlink *rtnl, sd_netlink_message *m,
 int address_remove(Address *address);
 int address_dup(const Address *src, Address **ret);
 bool address_is_ready(const Address *a);
-void address_set_broadcast(Address *a);
+void address_set_broadcast(Address *a, Link *link);
 
 DEFINE_SECTION_CLEANUP_FUNCTIONS(Address, address_free);
 
@@ -79,8 +79,15 @@ 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_ipv6_address(Link *link, const struct in6_addr *address, Address **ret);
-int link_get_ipv4_address(Link *link, const struct in_addr *address, unsigned char prefixlen, Address **ret);
+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) {
+        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_has_address(Manager *manager, int family, const union in_addr_union *address, bool check_ready);
 
 void address_cancel_request(Address *address);

src/network/networkd-dhcp-server.c

@@ -106,7 +106,7 @@ int link_request_dhcp_server_address(Link *link) {
         address->family = AF_INET;
         address->in_addr.in = link->network->dhcp_server_address;
         address->prefixlen = link->network->dhcp_server_address_prefixlen;
-        address_set_broadcast(address);
+        address_set_broadcast(address, link);
 
         if (address_get(link, address, &existing) >= 0 &&
             address_exists(existing) &&

src/network/networkd-dhcp4.c

@@ -925,8 +925,7 @@ static int dhcp4_request_address(Link *link, bool announce) {
         addr->lifetime_preferred_usec = lifetime_usec;
         addr->lifetime_valid_usec = lifetime_usec;
         addr->prefixlen = prefixlen;
-        if (prefixlen <= 30)
-                addr->broadcast.s_addr = address.s_addr | ~netmask.s_addr;
+        address_set_broadcast(addr, link);
         SET_FLAG(addr->flags, IFA_F_NOPREFIXROUTE, !link_prefixroute(link));
         addr->route_metric = link->network->dhcp_route_metric;
         addr->duplicate_address_detection = link->network->dhcp_send_decline ? ADDRESS_FAMILY_IPV4 : ADDRESS_FAMILY_NO;

src/network/networkd-dhcp6.c

@@ -155,7 +155,7 @@ 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) {
+static int verify_dhcp6_address(Link *link, const Address *address) {
         _cleanup_free_ char *buffer = NULL;
         bool by_ndisc = false;
         Address *existing;
@@ -167,7 +167,8 @@ static void log_dhcp6_address(Link *link, const Address *address) {
 
         (void) in6_addr_to_string(&address->in_addr.in6, &buffer);
 
-        if (address_get(link, address, &existing) < 0) {
+        if (address_get(link, address, &existing) < 0 &&
+            link_get_address(link, AF_INET6, &address->in_addr, 0, &existing) < 0) {
                 /* New address. */
                 log_level = LOG_INFO;
                 goto simple_log;
@@ -181,20 +182,23 @@ static void log_dhcp6_address(Link *link, const Address *address) {
         if (existing->source == NETWORK_CONFIG_SOURCE_NDISC)
                 by_ndisc = true;
 
-        log_link_warning(link, "DHCPv6 address %s/%u (valid %s, preferred %s) conflicts the address %s/%u%s.",
+        log_link_warning(link, "Ignoring DHCPv6 address %s/%u (valid %s, preferred %s) which conflicts with %s/%u%s.",
                          strna(buffer), address->prefixlen,
                          FORMAT_LIFETIME(address->lifetime_valid_usec),
                          FORMAT_LIFETIME(address->lifetime_preferred_usec),
                          strna(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=" : "");
-        return;
+                         by_ndisc ? " assigned by NDisc" : "");
+        if (by_ndisc)
+                log_link_warning(link, "Hint: use IPv6Token= setting to change the address generated by NDisc or set UseAutonomousPrefix=no.");
+
+        return -EEXIST;
 
 simple_log:
         log_link_full(link, log_level, "DHCPv6 address %s/%u (valid %s, preferred %s)",
                       strna(buffer), address->prefixlen,
                       FORMAT_LIFETIME(address->lifetime_valid_usec),
                       FORMAT_LIFETIME(address->lifetime_preferred_usec));
+        return 0;
 }
 
 static int dhcp6_request_address(
@@ -221,7 +225,8 @@ static int dhcp6_request_address(
         addr->lifetime_preferred_usec = lifetime_preferred_usec;
         addr->lifetime_valid_usec = lifetime_valid_usec;
 
-        log_dhcp6_address(link, addr);
+        if (verify_dhcp6_address(link, addr) < 0)
+                return 0;
 
         if (address_get(link, addr, &existing) < 0)
                 link->dhcp6_configured = false;

src/network/networkd-ipv4ll.c

@@ -34,7 +34,7 @@ static int address_new_from_ipv4ll(Link *link, Address **ret) {
         address->prefixlen = 16;
         address->scope = RT_SCOPE_LINK;
         address->route_metric = IPV4LL_ROUTE_METRIC;
-        address_set_broadcast(address);
+        address_set_broadcast(address, link);
 
         *ret = TAKE_PTR(address);
         return 0;

src/network/networkd-ndisc.c

@@ -332,7 +332,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 (link_get_ipv6_address(link, &gateway, NULL) >= 0) {
+        if (link_get_ipv6_address(link, &gateway, 0, NULL) >= 0) {
                 if (DEBUG_LOGGING) {
                         _cleanup_free_ char *buffer = NULL;
 
@@ -644,7 +644,7 @@ static int ndisc_router_process_route(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 (link_get_ipv6_address(link, &gateway, NULL) >= 0) {
+        if (link_get_ipv6_address(link, &gateway, 0, NULL) >= 0) {
                 if (DEBUG_LOGGING) {
                         _cleanup_free_ char *buf = NULL;
 

src/network/test-network.c

@@ -208,8 +208,10 @@ 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));
+        assert_se(in_addr_from_string(AF_INET, "192.168.3.10", &a2->in_addr_peer) >= 0);
         assert_se(address_equal(a1, a2));
         a1->prefixlen = 10;
         assert_se(!address_equal(a1, a2));
@@ -225,8 +227,9 @@ static void test_address_equality(void) {
         assert_se(address_equal(a1, a2));
 
         a2->prefixlen = 8;
-        assert_se(address_equal(a1, a2));
+        assert_se(!address_equal(a1, a2));
 
+        a2->prefixlen = 10;
         assert_se(in_addr_from_string(AF_INET6, "2001:4ca0:4f01::1", &a2->in_addr) >= 0);
         assert_se(!address_equal(a1, a2));
 }

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

@@ -2149,6 +2149,18 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities):
         call('ip netns del ns99', stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
 
     def test_address_static(self):
+        # test for #22515. The address will be removed and replaced with /64 prefix.
+        rc = call('ip link add dummy98 type dummy')
+        self.assertEqual(rc, 0)
+        rc = call('ip link set dev dummy98 up')
+        self.assertEqual(rc, 0)
+        rc = call('ip -6 address add 2001:db8:0:f101::15/128 dev dummy98')
+        self.assertEqual(rc, 0)
+        self.wait_address('dummy98', '2001:db8:0:f101::15/128', ipv='-6')
+        rc = call('ip -4 address add 10.3.2.3/16 brd 10.3.255.250 scope global label dummy98:hoge dev dummy98')
+        self.assertEqual(rc, 0)
+        self.wait_address('dummy98', '10.3.2.3/16 brd 10.3.255.250', ipv='-4')
+
         copy_unit_to_networkd_unit_path('25-address-static.network', '12-dummy.netdev')
         start_networkd()