From 9a45125ae7810ff1d3f2927538c510fe70148f63 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 7 Jul 2023 11:30:27 +0900 Subject: [PATCH 1/7] network: split-out address_section_adjust_broadcast() No functional change, just refactoring. --- src/network/networkd-address.c | 37 +++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 57a9ceb229..d393d3374b 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -2072,6 +2072,32 @@ int config_parse_address_netlabel( return 0; } +static void address_section_adjust_broadcast(Address *address) { + assert(address); + assert(address->section); + + if (!in4_addr_is_set(&address->broadcast)) + return; + + if (address->family == AF_INET6) + log_warning("%s: broadcast address is set for an IPv6 address. " + "Ignoring Broadcast= setting in the [Address] section from line %u.", + address->section->filename, address->section->line); + else if (address->prefixlen > 30) + log_warning("%s: broadcast address is set for an IPv4 address with prefix length larger than 30. " + "Ignoring Broadcast= setting in the [Address] section from line %u.", + address->section->filename, address->section->line); + else if (in4_addr_is_set(&address->in_addr_peer.in)) + log_warning("%s: broadcast address is set for an IPv4 address with peer address. " + "Ignoring Broadcast= setting in the [Address] section from line %u.", + address->section->filename, address->section->line); + else + /* Otherwise, keep the specified broadcast address. */ + return; + + address->broadcast.s_addr = 0; +} + static int address_section_verify(Address *address) { if (section_is_invalid(address->section)) return -EINVAL; @@ -2087,16 +2113,7 @@ static int address_section_verify(Address *address) { assert(IN_SET(address->family, AF_INET, AF_INET6)); - 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); - - address->broadcast.s_addr = 0; - } + address_section_adjust_broadcast(address); if (address->family == AF_INET6 && address->label) { log_warning("%s: address label is set for IPv6 address in the [Address] section from line %u. " From 0acc6470107bddc2afbd6d1011915142c1da6b74 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 7 Jul 2023 11:31:25 +0900 Subject: [PATCH 2/7] network: ignore Broadcast= setting when the address is null When an address is requested with null address, then broadcast should be determined after an address aquired that will be assigned to the interface. --- src/network/networkd-address.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index d393d3374b..e0a7b1c169 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -2091,6 +2091,10 @@ static void address_section_adjust_broadcast(Address *address) { log_warning("%s: broadcast address is set for an IPv4 address with peer address. " "Ignoring Broadcast= setting in the [Address] section from line %u.", address->section->filename, address->section->line); + else if (!in4_addr_is_set(&address->in_addr.in)) + log_warning("%s: broadcast address is set for an IPv4 address with null address. " + "Ignoring Broadcast= setting in the [Address] section from line %u.", + address->section->filename, address->section->line); else /* Otherwise, keep the specified broadcast address. */ return; From 14044a41144e9b793317c90e6b8a53bc05d759f7 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 7 Jul 2023 15:35:53 +0900 Subject: [PATCH 3/7] network: log broadcast address and address label --- src/network/networkd-address.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index e0a7b1c169..b5a4041c07 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -708,16 +708,22 @@ static void log_address_debug(const Address *address, const char *str, const Lin const char *peer = in_addr_is_set(address->family, &address->in_addr_peer) ? IN_ADDR_TO_STRING(address->family, &address->in_addr_peer) : NULL; + const char *broadcast = (address->family == AF_INET && in4_addr_is_set(&address->broadcast)) ? + IN4_ADDR_TO_STRING(&address->broadcast) : NULL; + (void) address_flags_to_string_alloc(address->flags, address->family, &flags_str); (void) route_scope_to_string_alloc(address->scope, &scope_str); - log_link_debug(link, "%s %s address (%s): %s%s%s/%u (valid %s, preferred %s), flags: %s, scope: %s", + log_link_debug(link, "%s %s address (%s): %s%s%s/%u%s%s (valid %s, preferred %s), flags: %s, scope: %s%s%s", str, strna(network_config_source_to_string(address->source)), strna(state), IN_ADDR_TO_STRING(address->family, &address->in_addr), peer ? " peer " : "", strempty(peer), address->prefixlen, + broadcast ? " broadcast " : "", strempty(broadcast), FORMAT_LIFETIME(address->lifetime_valid_usec), FORMAT_LIFETIME(address->lifetime_preferred_usec), - strna(flags_str), strna(scope_str)); + strna(flags_str), strna(scope_str), + address->family == AF_INET ? ", label: " : "", + address->family == AF_INET ? strna(address->label) : ""); } static int address_set_netlink_message(const Address *address, sd_netlink_message *m, Link *link) { From 0b8b30f6bbd9d4dda37a84f06f270a2158164189 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 7 Jul 2023 13:18:15 +0900 Subject: [PATCH 4/7] network: also use address_kernel_{hash,compare}_func() for managing address requests Request may refer Address object owned by Link, but its broadcast address or label may be updated by address_equalify(), hence these fields cannot be used in the hash and compare functions. --- src/network/networkd-address.c | 36 ++++++++-------------------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index b5a4041c07..14f0cf3140 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -339,30 +339,6 @@ DEFINE_PRIVATE_HASH_OPS_WITH_KEY_DESTRUCTOR( address_kernel_compare_func, address_free); -/* The functions below are mainly used by managing Request. */ -static 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; @@ -386,6 +362,10 @@ int address_compare_func(const Address *a1, const Address *a2) { return r; if (a1->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. */ r = strcmp_ptr(a1->label, a2->label); if (r != 0) return r; @@ -1295,8 +1275,8 @@ int link_request_address( log_address_debug(existing, "Requesting", link); r = link_queue_request_safe(link, REQUEST_TYPE_ADDRESS, existing, NULL, - address_hash_func, - address_compare_func, + address_kernel_hash_func, + address_kernel_compare_func, address_process_request, message_counter, netlink_handler, ret); if (r < 0) @@ -1383,8 +1363,8 @@ void address_cancel_request(Address *address) { .link = address->link, .type = REQUEST_TYPE_ADDRESS, .userdata = address, - .hash_func = (hash_func_t) address_hash_func, - .compare_func = (compare_func_t) address_compare_func, + .hash_func = (hash_func_t) address_kernel_hash_func, + .compare_func = (compare_func_t) address_kernel_compare_func, }; request_detach(address->link->manager, &req); From 9684a8ded083dd427f843b0c40aa0292e6c7ae06 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 7 Jul 2023 11:20:49 +0900 Subject: [PATCH 5/7] network: always copy input address for link_request_address() Then, the code becomes much simpler. This also adds several missing log messages. No functional change, just refactoring. --- src/network/networkd-address.c | 79 +++++++++++----------------------- 1 file changed, 26 insertions(+), 53 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 14f0cf3140..3d83eda985 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -1036,10 +1036,8 @@ static int address_acquire(Link *link, const Address *original, Address **ret) { assert(ret); /* Something useful was configured? just use it */ - if (in_addr_is_set(original->family, &original->in_addr)) { - *ret = NULL; - return 0; - } + if (in_addr_is_set(original->family, &original->in_addr)) + return address_dup(original, ret); /* The address is configured to be 0.0.0.0 or [::] by the user? * Then let's acquire something more useful from the pool. */ @@ -1062,7 +1060,7 @@ static int address_acquire(Link *link, const Address *original, Address **ret) { na->in_addr = in_addr; *ret = TAKE_PTR(na); - return 1; + return 0; } int address_configure_handler_internal(sd_netlink *rtnl, sd_netlink_message *m, Link *link, const char *error_msg) { @@ -1193,79 +1191,54 @@ int link_request_address( address_netlink_handler_t netlink_handler, Request **ret) { - Address *acquired, *existing = NULL; + _unused_ _cleanup_(address_freep) Address *address_will_be_freed = NULL; + Address *existing; int r; assert(link); assert(address); assert(address->source != NETWORK_CONFIG_SOURCE_FOREIGN); - r = address_acquire(link, address, &acquired); - if (r < 0) - return log_link_warning_errno(link, r, "Failed to acquire an address from pool: %m"); - if (r > 0) { - if (consume_object) - address_free(address); + if (consume_object) + address_will_be_freed = address; - address = acquired; - 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); - } - - (void) address_get(link, address, &existing); - - if (address->lifetime_valid_usec == 0) { - if (consume_object) - address_free(address); - - /* The requested address is outdated. Let's remove it. */ - return address_remove_and_drop(existing); - } - - if (!existing) { + if (address_get(link, address, &existing) < 0) { _cleanup_(address_freep) Address *tmp = NULL; - if (consume_object) - tmp = address; - else { - r = address_dup(address, &tmp); - if (r < 0) - return r; - } + if (address->lifetime_valid_usec == 0) + /* The requested address is outdated. Let's ignore the request. */ + return 0; + + r = address_acquire(link, address, &tmp); + if (r < 0) + return log_link_warning_errno(link, r, "Failed to acquire an address from pool: %m"); + + address_set_broadcast(tmp, link); /* Consider address tentative until we get the real flags from the kernel */ tmp->flags |= IFA_F_TENTATIVE; r = address_add(link, tmp); if (r < 0) - return r; + return log_link_warning_errno(link, r, "Failed to save requested address: %m"); existing = TAKE_PTR(tmp); } else { + if (address->lifetime_valid_usec == 0) + /* The requested address is outdated. Let's remove it. */ + return address_remove_and_drop(existing); + r = address_equalify(existing, address); if (r < 0) - return r; + return log_link_warning_errno(link, r, "Failed to equalify saved address with the requested one: %m"); + + address_set_broadcast(existing, link); + existing->source = address->source; existing->provider = address->provider; existing->duplicate_address_detection = address->duplicate_address_detection; existing->lifetime_valid_usec = address->lifetime_valid_usec; existing->lifetime_preferred_usec = address->lifetime_preferred_usec; - if (consume_object) - address_free(address); } r = ipv4acd_configure(existing); From d0009d929037e680cb26c59bc20f6d3b9d93b219 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 7 Jul 2023 11:24:03 +0900 Subject: [PATCH 6/7] network: make address_get() work for null address When a static null address is requested, e.g. Address=0.0.0.0/24, then the corresponding Address object owned by Network object has null address, and previously it did not match any addresses already assigne to the interface. Let's search matching Address object when originally it is requested as the null address. Then, the address configured with requested as a null address will be kept on reconfigure. Fixes #26113. --- src/network/networkd-address.c | 89 +++++++++++++++++++++++++++++----- src/network/networkd-address.h | 1 + 2 files changed, 77 insertions(+), 13 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 3d83eda985..221f5c9efe 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -262,6 +262,19 @@ static void address_set_lifetime(Manager *m, Address *a, const struct ifa_cachei a->lifetime_preferred_usec = sec_to_usec(cinfo->ifa_prefered, now_usec); } +static bool address_is_static_null(const Address *address) { + assert(address); + + if (!address->network) + return false; + + if (!address->requested_as_null) + return false; + + assert(!in_addr_is_set(address->family, &address->in_addr)); + return true; +} + static uint32_t address_prefix(const Address *a) { assert(a); @@ -389,16 +402,31 @@ int address_equal(const Address *a1, const Address *a2) { } static int address_equalify(Address *address, const Address *src) { + bool src_is_null; int r; assert(address); assert(src); - if (address_kernel_compare_func(address, src) != 0) + src_is_null = address_is_static_null(src); + + if (src_is_null) { + /* When the source address has an null address, then we cannot compare the two objects with + * address_kernel_compare_func(). Let's do minimal check here. */ + if (address->family != src->family) + return -EINVAL; + if (address->prefixlen != src->prefixlen) + return -EINVAL; + + } else if (address_kernel_compare_func(address, src) != 0) return -EINVAL; if (address->family == AF_INET) { - address->broadcast = src->broadcast; + /* When the source is null, then the broadcast address will be determined based on the + * acquired address. Hence, do not copy it here. */ + if (!src_is_null) + address->broadcast = src->broadcast; + r = free_and_strdup(&address->label, src->label); if (r < 0) return r; @@ -552,18 +580,40 @@ static int address_drop(Address *address) { } int address_get(Link *link, const Address *in, Address **ret) { - Address *existing; + Address *a; assert(link); assert(in); - existing = set_get(link->addresses, in); - if (!existing) - return -ENOENT; + a = set_get(link->addresses, in); + if (a) { + if (ret) + *ret = a; + return 0; + } - if (ret) - *ret = existing; - return 0; + /* Find matching address that originally requested as null address. */ + if (address_is_static_null(in)) + SET_FOREACH(a, link->addresses) { + if (!a->requested_as_null) + continue; + + /* Currently, null address is supported only by static addresses. Note that static + * address may be set as foreign during reconfiguring the interface. */ + if (!IN_SET(a->source, NETWORK_CONFIG_SOURCE_FOREIGN, NETWORK_CONFIG_SOURCE_STATIC)) + continue; + + if (a->family != in->family) + continue; + if (a->prefixlen != in->prefixlen) + continue; + + if (ret) + *ret = a; + return 0; + } + + return -ENOENT; } int link_get_address(Link *link, int family, const union in_addr_union *address, unsigned char prefixlen, Address **ret) { @@ -971,12 +1021,24 @@ int link_drop_foreign_addresses(Link *link) { ORDERED_HASHMAP_FOREACH(address, link->network->addresses_by_section) { Address *existing; + if (address_get(link, address, &existing) < 0) + continue; + /* On update, the kernel ignores the address label and broadcast address. Hence we need to * distinguish addresses with different labels or broadcast addresses. Thus, we need to check * the existing address with address_equal(). Otherwise, the label or broadcast address * change will not be applied when we reconfigure the interface. */ - if (address_get(link, address, &existing) >= 0 && address_equal(address, existing)) - address_unmark(existing); + if (address_is_static_null(address)) { + /* If the static configuration has null address, then the broadcast address will be + * determined automatically. Hence, here we need to compare only address label. */ + if (address->family == AF_INET && !streq_ptr(address->label, existing->label)) + continue; + + } else if (!address_equal(address, existing)) + continue; + + /* Found matching static configuration. Keep the existing address. */ + address_unmark(existing); } /* Finally, remove all marked addresses. */ @@ -1700,9 +1762,10 @@ int config_parse_address( n->family = f; n->prefixlen = prefixlen; - if (streq(lvalue, "Address")) + if (streq(lvalue, "Address")) { n->in_addr = buffer; - else + n->requested_as_null = !in_addr_is_set(n->family, &n->in_addr); + } else n->in_addr_peer = buffer; TAKE_PTR(n); diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index 18eeb6e48e..766da8ee75 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -53,6 +53,7 @@ struct Address { bool scope_set:1; bool ip_masquerade_done:1; + bool requested_as_null:1; /* duplicate_address_detection is only used by static or IPv4 dynamic addresses. * To control DAD for IPv6 dynamic addresses, set IFA_F_NODAD to flags. */ From fc25920c947ee7d9b605aba5953d6679bf6da0db Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 7 Jul 2023 10:23:48 +0900 Subject: [PATCH 7/7] test-network: add tests for null addresses For issue #26113. --- .../test-network/conf/25-address-null.network | 14 ++++++++++ test/test-network/systemd-networkd-tests.py | 28 +++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 test/test-network/conf/25-address-null.network diff --git a/test/test-network/conf/25-address-null.network b/test/test-network/conf/25-address-null.network new file mode 100644 index 0000000000..d192900468 --- /dev/null +++ b/test/test-network/conf/25-address-null.network @@ -0,0 +1,14 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=dummy98 + +[Network] +IPv6AcceptRA=no +Address=0.0.0.0/16 +Address=0.0.0.0/30 +Address=::/64 + +[Address] +Address=0.0.0.0/24 +# The broadcast address will be ignored. +Broadcast=192.168.0.255 diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index ee9bb81fef..60654c71f9 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -2342,6 +2342,34 @@ class NetworkdNetworkTests(unittest.TestCase, Utilities): # TODO: check json string check_output(*networkctl_cmd, '--json=short', 'status', env=env) + def test_address_null(self): + copy_network_unit('25-address-null.network', '12-dummy.netdev') + start_networkd() + + self.wait_online(['dummy98:routable']) + + output = check_output('ip address show dev dummy98 scope global') + print(output) + + ipv4_address_16 = re.findall(r'inet 172.[0-9]*.0.1/16 brd 172.[0-9]*.255.255', output) + self.assertEqual(len(ipv4_address_16), 1) + ipv4_address_24 = re.findall(r'inet 192.168.[0-9]*.1/24 brd 192.168.[0-9]*.255', output) + self.assertEqual(len(ipv4_address_24), 1) + ipv4_address_30 = re.findall(r'inet 192.168.[0-9]*.[0-9]*/30 brd 192.168.[0-9]*.[0-9]*', output) + self.assertEqual(len(ipv4_address_30), 1) + ipv6_address = re.findall(r'inet6 fd[0-9a-f:]*/64', output) + self.assertEqual(len(ipv6_address), 1) + + networkctl_reconfigure('dummy98') + self.wait_online(['dummy98:routable']) + + output = check_output('ip address show dev dummy98 scope global') + print(output) + self.assertIn(ipv4_address_16[0], output) + self.assertIn(ipv4_address_24[0], output) + self.assertIn(ipv4_address_30[0], output) + self.assertIn(ipv6_address[0], output) + def test_address_ipv4acd(self): check_output('ip netns add ns99') check_output('ip link add veth99 type veth peer veth-peer')