From 26f8847102323d5d66c17f21685f3e6473354458 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 8 Sep 2023 05:34:32 +0900 Subject: [PATCH 1/5] network: call network_adjust_dhcp_server() from network_drop_invalid_addresses() We need to find a suitable static address for the DHCP server. So, all static addresses must be verified before network_adjust_dhcp_server() is called. For safety, let's call it from network_drop_invalid_addresses(). No functional change, just refactoring and preparation for later commits. --- src/network/networkd-address.c | 2 ++ src/network/networkd-dhcp-server.c | 3 +-- src/network/networkd-network.c | 3 --- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 048f288df8..990d24e9b1 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -2444,6 +2444,8 @@ int network_drop_invalid_addresses(Network *network) { assert(r > 0); } + network_adjust_dhcp_server(network); + return 0; } diff --git a/src/network/networkd-dhcp-server.c b/src/network/networkd-dhcp-server.c index c1d9fbc17c..3663a506f6 100644 --- a/src/network/networkd-dhcp-server.c +++ b/src/network/networkd-dhcp-server.c @@ -57,8 +57,7 @@ void network_adjust_dhcp_server(Network *network) { bool have = false; ORDERED_HASHMAP_FOREACH(address, network->addresses_by_section) { - if (section_is_invalid(address->section)) - continue; + assert(!section_is_invalid(address->section)); if (address->family != AF_INET) continue; diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 7feb178c80..e0a8be6b54 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -20,7 +20,6 @@ #include "networkd-bridge-mdb.h" #include "networkd-dhcp-common.h" #include "networkd-dhcp-server-static-lease.h" -#include "networkd-dhcp-server.h" #include "networkd-ipv6-proxy-ndp.h" #include "networkd-manager.h" #include "networkd-ndisc.h" @@ -326,8 +325,6 @@ int network_verify(Network *network) { return r; /* sr_iov_drop_invalid_sections() logs internally. */ network_drop_invalid_static_leases(network); - network_adjust_dhcp_server(network); - return 0; } From 5459e11d39f8885ec945829df0d8e1ca62e56f75 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 8 Sep 2023 03:27:07 +0900 Subject: [PATCH 2/5] network: find DHCP server address only on loading .network file Previously, we periodically search suitable address for DHCP server, 1. when .network file is loaded, 2. when checking if we can configure sd_dhcp_server object, 3. when configuring sd_dhcp_server. Especially, the step 2 may be triggered several times. This makes, when .network file is loaded, find a corresponding Address object, add a new Address object if not found, then save the found or added Address object. So, it is not necessary to find address again on configuring sd_dhcp_server object. --- src/network/networkd-address.c | 14 ++- src/network/networkd-address.h | 5 ++ src/network/networkd-dhcp-server.c | 132 ++++++++++++++--------------- src/network/networkd-dhcp-server.h | 4 +- src/network/networkd-network.h | 4 +- 5 files changed, 80 insertions(+), 79 deletions(-) diff --git a/src/network/networkd-address.c b/src/network/networkd-address.c index 990d24e9b1..c43655f57a 100644 --- a/src/network/networkd-address.c +++ b/src/network/networkd-address.c @@ -110,7 +110,7 @@ int address_new(Address **ret) { return 0; } -static int address_new_static(Network *network, const char *filename, unsigned section_line, Address **ret) { +int address_new_static(Network *network, const char *filename, unsigned section_line, Address **ret) { _cleanup_(config_section_freep) ConfigSection *n = NULL; _cleanup_(address_freep) Address *address = NULL; int r; @@ -401,7 +401,7 @@ static int address_compare_func(const Address *a1, const Address *a2) { } } -DEFINE_PRIVATE_HASH_OPS( +DEFINE_HASH_OPS( address_hash_ops, Address, address_hash_func, @@ -1561,10 +1561,6 @@ int link_request_static_addresses(Link *link) { if (r < 0) return r; - r = link_request_dhcp_server_address(link); - if (r < 0) - return r; - if (link->static_address_messages == 0) { link->static_addresses_configured = true; link_check_ready(link); @@ -2334,7 +2330,7 @@ static void address_section_adjust_broadcast(Address *address) { address->broadcast.s_addr = 0; } -static int address_section_verify(Address *address) { +int address_section_verify(Address *address) { if (section_is_invalid(address->section)) return -EINVAL; @@ -2444,7 +2440,9 @@ int network_drop_invalid_addresses(Network *network) { assert(r > 0); } - network_adjust_dhcp_server(network); + r = network_adjust_dhcp_server(network, &addresses); + if (r < 0) + return r; return 0; } diff --git a/src/network/networkd-address.h b/src/network/networkd-address.h index 87c7d36486..35acb81aa3 100644 --- a/src/network/networkd-address.h +++ b/src/network/networkd-address.h @@ -7,6 +7,7 @@ #include "conf-parser.h" #include "firewall-util.h" +#include "hash-funcs.h" #include "in-addr-util.h" #include "networkd-link.h" #include "networkd-util.h" @@ -73,7 +74,10 @@ const char* format_lifetime(char *buf, size_t l, usec_t lifetime_usec) _warn_unu int address_flags_to_string_alloc(uint32_t flags, int family, char **ret); +extern const struct hash_ops address_hash_ops; + int address_new(Address **ret); +int address_new_static(Network *network, const char *filename, unsigned section_line, Address **ret); Address* address_free(Address *address); int address_get(Link *link, const Address *in, Address **ret); int address_get_harder(Link *link, const Address *in, Address **ret); @@ -115,6 +119,7 @@ int link_request_static_addresses(Link *link); int manager_rtnl_process_address(sd_netlink *nl, sd_netlink_message *message, Manager *m); +int address_section_verify(Address *address); int network_drop_invalid_addresses(Network *network); DEFINE_NETWORK_CONFIG_STATE_FUNCTIONS(Address, address); diff --git a/src/network/networkd-dhcp-server.c b/src/network/networkd-dhcp-server.c index 3663a506f6..63c520fa89 100644 --- a/src/network/networkd-dhcp-server.c +++ b/src/network/networkd-dhcp-server.c @@ -39,22 +39,28 @@ static bool link_dhcp4_server_enabled(Link *link) { return link->network->dhcp_server; } -void network_adjust_dhcp_server(Network *network) { +int network_adjust_dhcp_server(Network *network, Set **addresses) { + int r; + assert(network); + assert(addresses); if (!network->dhcp_server) - return; + return 0; if (network->bond) { log_warning("%s: DHCPServer= is enabled for bond slave. Disabling DHCP server.", network->filename); network->dhcp_server = false; - return; + return 0; } - if (!in4_addr_is_set(&network->dhcp_server_address)) { + assert(network->dhcp_server_address_prefixlen <= 32); + + if (network->dhcp_server_address_prefixlen == 0) { Address *address; - bool have = false; + + /* If the server address is not specified, then find suitable static address. */ ORDERED_HASHMAP_FOREACH(address, network->addresses_by_section) { assert(!section_is_invalid(address->section)); @@ -71,83 +77,69 @@ void network_adjust_dhcp_server(Network *network) { if (in4_addr_is_set(&address->in_addr_peer.in)) continue; - have = true; + /* TODO: check if the prefix length is small enough for the pool. */ + + network->dhcp_server_address = address; break; } - if (!have) { - log_warning("%s: DHCPServer= is enabled, but no static address configured. " + if (!network->dhcp_server_address) { + log_warning("%s: DHCPServer= is enabled, but no suitable static address configured. " "Disabling DHCP server.", network->filename); network->dhcp_server = false; - return; + return 0; } - } -} -int link_request_dhcp_server_address(Link *link) { - _cleanup_(address_freep) Address *address = NULL; - Address *existing; - int r; + } else { + _cleanup_(address_freep) Address *a = NULL; + Address *existing; + unsigned line; - assert(link); - assert(link->network); + /* TODO: check if the prefix length is small enough for the pool. */ - if (!link_dhcp4_server_enabled(link)) - return 0; + /* If an address is explicitly specified, then check if the corresponding [Address] section + * is configured, and add one if not. */ - if (!in4_addr_is_set(&link->network->dhcp_server_address)) - return 0; + existing = set_get(*addresses, + &(Address) { + .family = AF_INET, + .in_addr.in = network->dhcp_server_address_in_addr, + .prefixlen = network->dhcp_server_address_prefixlen, + }); + if (existing) { + /* Corresponding [Address] section already exists. */ + network->dhcp_server_address = existing; + return 0; + } - r = address_new(&address); - if (r < 0) - return r; + r = ordered_hashmap_by_section_find_unused_line(network->addresses_by_section, network->filename, &line); + if (r < 0) + return log_warning_errno(r, "%s: Failed to find unused line number for DHCP server address: %m", + network->filename); - address->source = NETWORK_CONFIG_SOURCE_STATIC; - address->family = AF_INET; - address->in_addr.in = link->network->dhcp_server_address; - address->prefixlen = link->network->dhcp_server_address_prefixlen; + r = address_new_static(network, network->filename, line, &a); + if (r < 0) + return log_warning_errno(r, "%s: Failed to add new static address object for DHCP server: %m", + network->filename); - if (address_get_harder(link, address, &existing) >= 0 && - (address_exists(existing) || address_is_requesting(existing)) && - existing->source == NETWORK_CONFIG_SOURCE_STATIC) - /* The same address seems explicitly configured in [Address] or [Network] section. - * Configure the DHCP server address only when it is not. */ - return 0; + a->family = AF_INET; + a->prefixlen = network->dhcp_server_address_prefixlen; + a->in_addr.in = network->dhcp_server_address_in_addr; + a->requested_as_null = !in4_addr_is_set(&network->dhcp_server_address_in_addr); - return link_request_static_address(link, address); -} + r = address_section_verify(a); + if (r < 0) + return r; -static int link_find_dhcp_server_address(Link *link, Address **ret) { - Address *address; + r = set_ensure_put(addresses, &address_hash_ops, a); + if (r < 0) + return log_oom(); + assert(r > 0); - assert(link); - assert(link->network); - - /* If ServerAddress= is specified, then use the address. */ - if (in4_addr_is_set(&link->network->dhcp_server_address)) - return link_get_ipv4_address(link, &link->network->dhcp_server_address, - link->network->dhcp_server_address_prefixlen, ret); - - /* If not, then select one from static addresses. */ - SET_FOREACH(address, link->addresses) { - if (address->source != NETWORK_CONFIG_SOURCE_STATIC) - continue; - if (!address_exists(address)) - continue; - if (address->family != AF_INET) - continue; - if (in4_addr_is_localhost(&address->in_addr.in)) - continue; - if (in4_addr_is_link_local(&address->in_addr.in)) - continue; - if (in4_addr_is_set(&address->in_addr_peer.in)) - continue; - - *ret = address; - return 0; + network->dhcp_server_address = TAKE_PTR(a); } - return -ENOENT; + return 0; } static int dhcp_server_find_uplink(Link *link, Link **ret) { @@ -369,6 +361,8 @@ static int dhcp4_server_configure(Link *link) { int r; assert(link); + assert(link->network); + assert(link->network->dhcp_server_address); log_link_debug(link, "Configuring DHCP Server."); @@ -387,7 +381,7 @@ static int dhcp4_server_configure(Link *link) { if (r < 0) return log_link_warning_errno(link, r, "Failed to set callback for DHCPv4 server instance: %m"); - r = link_find_dhcp_server_address(link, &address); + r = address_get(link, link->network->dhcp_server_address, &address); if (r < 0) return log_link_error_errno(link, r, "Failed to find suitable address for DHCPv4 server instance: %m"); @@ -535,6 +529,8 @@ static bool dhcp_server_is_ready_to_configure(Link *link) { Address *a; assert(link); + assert(link->network); + assert(link->network->dhcp_server_address); if (!link_is_ready_to_configure(link, /* allow_unmanaged = */ false)) return false; @@ -545,7 +541,7 @@ static bool dhcp_server_is_ready_to_configure(Link *link) { if (!link->static_addresses_configured) return false; - if (link_find_dhcp_server_address(link, &a) < 0) + if (address_get(link, link->network->dhcp_server_address, &a) < 0) return false; if (!address_is_ready(a)) @@ -711,7 +707,7 @@ int config_parse_dhcp_server_address( assert(rvalue); if (isempty(rvalue)) { - network->dhcp_server_address = (struct in_addr) {}; + network->dhcp_server_address_in_addr = (struct in_addr) {}; network->dhcp_server_address_prefixlen = 0; return 0; } @@ -729,7 +725,7 @@ int config_parse_dhcp_server_address( return 0; } - network->dhcp_server_address = a.in; + network->dhcp_server_address_in_addr = a.in; network->dhcp_server_address_prefixlen = prefixlen; return 0; } diff --git a/src/network/networkd-dhcp-server.h b/src/network/networkd-dhcp-server.h index cb2a8b6a34..4fd4429deb 100644 --- a/src/network/networkd-dhcp-server.h +++ b/src/network/networkd-dhcp-server.h @@ -2,13 +2,13 @@ #pragma once #include "conf-parser.h" +#include "set.h" typedef struct Link Link; typedef struct Network Network; -void network_adjust_dhcp_server(Network *network); +int network_adjust_dhcp_server(Network *network, Set **addresses); -int link_request_dhcp_server_address(Link *link); int link_request_dhcp_server(Link *link); CONFIG_PARSER_PROTOTYPE(config_parse_dhcp_server_relay_agent_suboption); diff --git a/src/network/networkd-network.h b/src/network/networkd-network.h index 98cc1f5abc..34bc179d75 100644 --- a/src/network/networkd-network.h +++ b/src/network/networkd-network.h @@ -15,6 +15,7 @@ #include "ipoib.h" #include "net-condition.h" #include "netdev.h" +#include "networkd-address.h" #include "networkd-bridge-vlan.h" #include "networkd-dhcp-common.h" #include "networkd-dhcp4.h" @@ -199,7 +200,8 @@ struct Network { bool dhcp_server; bool dhcp_server_bind_to_interface; unsigned char dhcp_server_address_prefixlen; - struct in_addr dhcp_server_address; + struct in_addr dhcp_server_address_in_addr; + const Address *dhcp_server_address; int dhcp_server_uplink_index; char *dhcp_server_uplink_name; struct in_addr dhcp_server_relay_target; From a0dfce0a3f3cec2db0d10eb13987a363ef4f620f Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 8 Sep 2023 06:35:26 +0900 Subject: [PATCH 3/5] network: allow to set null address for [DHCPServer] ServerAddress= And refuse a link-local address. With the previous commit, now ServerAddress= can take a null address, but the config parser refused that. Let's allow that now. --- src/network/networkd-dhcp-server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-dhcp-server.c b/src/network/networkd-dhcp-server.c index 63c520fa89..cf7b8e1b62 100644 --- a/src/network/networkd-dhcp-server.c +++ b/src/network/networkd-dhcp-server.c @@ -718,9 +718,9 @@ int config_parse_dhcp_server_address( "Failed to parse %s=, ignoring assignment: %s", lvalue, rvalue); return 0; } - if (in4_addr_is_null(&a.in) || in4_addr_is_localhost(&a.in)) { + if (in4_addr_is_localhost(&a.in) || in4_addr_is_link_local(&a.in)) { log_syntax(unit, LOG_WARNING, filename, line, 0, - "DHCP server address cannot be the ANY address or a localhost address, " + "DHCP server address cannot be a localhost or link-local address, " "ignoring assignment: %s", rvalue); return 0; } From e443a88a9af340e8f3897ee899840a104e527e19 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 8 Sep 2023 04:08:32 +0900 Subject: [PATCH 4/5] man: update [DHCPServer] ServerAddress= --- man/systemd.network.xml | 41 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/man/systemd.network.xml b/man/systemd.network.xml index c52b80f285..9ac7c13811 100644 --- a/man/systemd.network.xml +++ b/man/systemd.network.xml @@ -3276,12 +3276,43 @@ Token=prefixstable:2002:da8:1:: ServerAddress= - Specifies server address for the DHCP server. Takes an IPv4 address with prefix - length, for example 192.168.0.1/24. This setting may be useful when the link on - which the DHCP server is running has multiple static addresses. When unset, one of static addresses - in the link will be automatically selected. Defaults to unset. + + Specifies the server address for the DHCP server. Takes an IPv4 address with prefix length + separated with a slash, e.g. 192.168.0.1/24. Defaults to unset, and one of + static IPv4 addresses configured in [Network] or [Address] section will be automatically selected. + This setting may be useful when the interface on which the DHCP server is running has multiple + static IPv4 addresses. + This implies Address= in [Network] or [Address] section with the same + address and prefix length. That is, + [Network] +DHCPServer=yes +Address=192.168.0.1/24 +Address=192.168.0.2/24 +[DHCPServer] +ServerAddress=192.168.0.1/24 + or + [Network] +DHCPServer=yes +[Address] +Address=192.168.0.1/24 +[Address] +Address=192.168.0.2/24 +[DHCPServer] +ServerAddress=192.168.0.1/24 + are equivalent to the following. + [Network] +DHCPServer=yes +Address=192.168.0.2/24 +[DHCPServer] +ServerAddress=192.168.0.1/24 + + Since version 255, like the Address= setting in [Network] or [Address] + section, this also supports a null address, e.g. 0.0.0.0/24, and an unused + address will be automatically selected. For more details about the automatic address selection, + see Address= setting in [Network] section in the above. - + + From 47f1ce1677ba6bfd86b32c87df70f729a72af9a6 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Fri, 8 Sep 2023 06:33:19 +0900 Subject: [PATCH 5/5] test-network: add testcase for [DHCPServer] ServerAddress= with null address --- ...25-dhcp-server-null-server-address.network | 14 +++++++++++ test/test-network/systemd-networkd-tests.py | 23 +++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 test/test-network/conf/25-dhcp-server-null-server-address.network diff --git a/test/test-network/conf/25-dhcp-server-null-server-address.network b/test/test-network/conf/25-dhcp-server-null-server-address.network new file mode 100644 index 0000000000..fff83f4c27 --- /dev/null +++ b/test/test-network/conf/25-dhcp-server-null-server-address.network @@ -0,0 +1,14 @@ +# SPDX-License-Identifier: LGPL-2.1-or-later +[Match] +Name=veth-peer + +[Network] +IPv6AcceptRA=false +DHCPServer=yes + +[DHCPServer] +ServerAddress=0.0.0.0/24 +PoolOffset=10 +PoolSize=50 +DNS=_server_address +NTP=_server_address diff --git a/test/test-network/systemd-networkd-tests.py b/test/test-network/systemd-networkd-tests.py index 90d68b54db..9d1b273f95 100755 --- a/test/test-network/systemd-networkd-tests.py +++ b/test/test-network/systemd-networkd-tests.py @@ -4935,6 +4935,29 @@ class NetworkdDHCPServerTests(unittest.TestCase, Utilities): output = check_output(*networkctl_cmd, '-n', '0', 'status', 'veth-peer', env=env) self.assertRegex(output, "Offered DHCP leases: 192.168.5.[0-9]*") + def test_dhcp_server_null_server_address(self): + copy_network_unit('25-veth.netdev', '25-dhcp-client.network', '25-dhcp-server-null-server-address.network') + start_networkd() + self.wait_online(['veth99:routable', 'veth-peer:routable']) + + output = check_output('ip --json address show dev veth-peer') + server_address = json.loads(output)[0]['addr_info'][0]['local'] + print(server_address) + + output = check_output('ip --json address show dev veth99') + client_address = json.loads(output)[0]['addr_info'][0]['local'] + print(client_address) + + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'veth99', env=env) + print(output) + self.assertRegex(output, rf'Address: {client_address} \(DHCP4 via {server_address}\)') + self.assertIn(f'Gateway: {server_address}', output) + self.assertIn(f'DNS: {server_address}', output) + self.assertIn(f'NTP: {server_address}', output) + + output = check_output(*networkctl_cmd, '-n', '0', 'status', 'veth-peer', env=env) + self.assertIn(f'Offered DHCP leases: {client_address}', output) + def test_dhcp_server_with_uplink(self): copy_network_unit('25-veth.netdev', '25-dhcp-client.network', '25-dhcp-server-downstream.network', '12-dummy.netdev', '25-dhcp-server-uplink.network')