From 410a7f15f0c6a96cd3b6a73f4a6465a5abfb9d0a Mon Sep 17 00:00:00 2001 From: Richard Maw Date: Tue, 6 Jun 2017 15:43:24 +0100 Subject: [PATCH 1/3] networkd: Clean up pool addresses on link down When the link comes up it assigns addresses by checking whether the address is 0.0.0.0, and if so pulling a new address range out of the pool. If the addresses aren't removed from the pool when the link goes down then the set of addresses allocated will grow until all the local address ranges are exhausted, while it gets a different IP address every time. This patch frees the addresses when link config is dropped to fix the address leak, and on systems which can expect all interfaces to be brought up or down in a deterministic order this conveniently makes use the same address each time. --- src/network/networkd-link.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 69f3e1212a4..fd81c1409a7 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -2443,7 +2443,7 @@ static int link_drop_foreign_config(Link *link) { } static int link_drop_config(Link *link) { - Address *address; + Address *address, *pool_address; Route *route; Iterator i; int r; @@ -2456,6 +2456,15 @@ static int link_drop_config(Link *link) { r = address_remove(address, link, link_address_remove_handler); if (r < 0) return r; + + /* If this address came from an address pool, clean up the pool */ + LIST_FOREACH(addresses, pool_address, link->pool_addresses) { + if (address_equal(address, pool_address)) { + LIST_REMOVE(addresses, link->pool_addresses, pool_address); + address_free(pool_address); + break; + } + } } SET_FOREACH(route, link->routes, i) { From c1835a427fddac41dcc780cf733b53e01721e8f2 Mon Sep 17 00:00:00 2001 From: Richard Maw Date: Wed, 7 Jun 2017 16:30:46 +0100 Subject: [PATCH 2/3] networkd: Wait for link to get carrier before setting addresses For containers the link is effectively always up, but for virtual and physical machines networkd may have started before the link has gained carrier. Networkd will configure addresses when carrier is gained, but should also configure addresses if the link is already up. Without this patch the addresses are set unconditionally. Normally this isn't a problem since addresses are either fixed, set over DHCP, or is never without carrier. But for machines that gain carrier and are configured to select an address from the unallocated local address pool this causes them to pick an address from the pool twice. This change to skip address configuration when a link is added before it has a carrier fixes having multiple addresses assigned if the machine starts networkd before it has gained carrier and is configured with an address from the pool. --- src/network/networkd-link.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index fd81c1409a7..4af61df7ff3 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -2135,6 +2135,12 @@ static int link_joined(Link *link) { log_link_error_errno(link, r, "Could not set bridge vlan: %m"); } + /* Skip setting up addresses until it gets carrier, + or it would try to set addresses twice, + which is bad for non-idempotent steps. */ + if (!link_has_carrier(link)) + return 0; + return link_enter_set_addresses(link); } From 45a9eac9a0c85c0c76b46388a530edd335ae44af Mon Sep 17 00:00:00 2001 From: Richard Maw Date: Fri, 9 Jun 2017 16:18:25 +0100 Subject: [PATCH 3/3] networkd: Allow DHCP servers to be re-configured on carrier gain In normal operation this would trigger an assertion when a DHCP server is configured every time the link goes up. This change makes sd_dhcp_server_configure_pool idempotent and stops the DHCP server when the link loses carrier. In addition to this stopping the assertion being triggered, this has the useful side-effect of allowing the link to be taken down and then brought back up as a way to have it use DNS from an "upstream" interface that got its DNS configuration via DHCP after the downstream link was configured. --- src/libsystemd-network/sd-dhcp-server.c | 46 +++++++++++++---------- src/libsystemd-network/test-dhcp-server.c | 2 +- src/network/networkd-link.c | 2 + 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/src/libsystemd-network/sd-dhcp-server.c b/src/libsystemd-network/sd-dhcp-server.c index 315cbf1ac5d..5a59c377f8c 100644 --- a/src/libsystemd-network/sd-dhcp-server.c +++ b/src/libsystemd-network/sd-dhcp-server.c @@ -34,6 +34,14 @@ #define DHCP_DEFAULT_LEASE_TIME_USEC USEC_PER_HOUR #define DHCP_MAX_LEASE_TIME_USEC (USEC_PER_HOUR*12) +static void dhcp_lease_free(DHCPLease *lease) { + if (!lease) + return; + + free(lease->client_id.data); + free(lease); +} + /* configures the server's address and subnet, and optionally the pool's size and offset into the subnet * the whole pool must fit into the subnet, and may not contain the first (any) nor last (broadcast) address * moreover, the server's own address may be in the pool, and is in that case reserved in order not to @@ -47,7 +55,6 @@ int sd_dhcp_server_configure_pool(sd_dhcp_server *server, struct in_addr *addres assert_return(address, -EINVAL); assert_return(address->s_addr != INADDR_ANY, -EINVAL); assert_return(prefixlen <= 32, -ERANGE); - assert_return(server->address == INADDR_ANY, -EBUSY); assert_se(in_addr_prefixlen_to_netmask(&netmask_addr, prefixlen)); netmask = netmask_addr.s_addr; @@ -78,19 +85,28 @@ int sd_dhcp_server_configure_pool(sd_dhcp_server *server, struct in_addr *addres else size = size_max; - server->bound_leases = new0(DHCPLease*, size); - if (!server->bound_leases) - return -ENOMEM; + if (server->address != address->s_addr || server->netmask != netmask || server->pool_size != size || server->pool_offset != offset) { + DHCPLease *lease; - server->pool_offset = offset; - server->pool_size = size; + free(server->bound_leases); + server->bound_leases = new0(DHCPLease*, size); + if (!server->bound_leases) + return -ENOMEM; - server->address = address->s_addr; - server->netmask = netmask; - server->subnet = address->s_addr & netmask; + server->pool_offset = offset; + server->pool_size = size; - if (server_off >= offset && server_off - offset < size) - server->bound_leases[server_off - offset] = &server->invalid_lease; + server->address = address->s_addr; + server->netmask = netmask; + server->subnet = address->s_addr & netmask; + + if (server_off >= offset && server_off - offset < size) + server->bound_leases[server_off - offset] = &server->invalid_lease; + + /* Drop any leases associated with the old address range */ + while ((lease = hashmap_steal_first(server->leases_by_client_id))) + dhcp_lease_free(lease); + } return 0; } @@ -143,14 +159,6 @@ static const struct hash_ops client_id_hash_ops = { .compare = client_id_compare_func }; -static void dhcp_lease_free(DHCPLease *lease) { - if (!lease) - return; - - free(lease->client_id.data); - free(lease); -} - sd_dhcp_server *sd_dhcp_server_unref(sd_dhcp_server *server) { DHCPLease *lease; diff --git a/src/libsystemd-network/test-dhcp-server.c b/src/libsystemd-network/test-dhcp-server.c index e81c508c7ff..26f217835fb 100644 --- a/src/libsystemd-network/test-dhcp-server.c +++ b/src/libsystemd-network/test-dhcp-server.c @@ -63,7 +63,7 @@ static int test_basic(sd_event *event) { assert_se(sd_dhcp_server_configure_pool(server, &address_any, 28, 0, 0) == -EINVAL); assert_se(sd_dhcp_server_configure_pool(server, &address_lo, 38, 0, 0) == -ERANGE); assert_se(sd_dhcp_server_configure_pool(server, &address_lo, 8, 0, 0) >= 0); - assert_se(sd_dhcp_server_configure_pool(server, &address_lo, 8, 0, 0) == -EBUSY); + assert_se(sd_dhcp_server_configure_pool(server, &address_lo, 8, 0, 0) >= 0); test_pool(&address_any, 1, -EINVAL); test_pool(&address_lo, 1, 0); diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 4af61df7ff3..4c57fa17938 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -3060,6 +3060,8 @@ static int link_carrier_lost(Link *link) { return r; } + (void) sd_dhcp_server_stop(link->dhcp_server); + r = link_drop_config(link); if (r < 0) return r;