[PATCH 1/6] dhcp: release lease when client stops

[PATCH 1/6] dhcp: release lease when client stops

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 3492 bytes --]

RFC 2131 does not explicitly require this but clients MAY
release their IP address when they are done using it, e.g.
when shutting down. This is also described as a "graceful shutdown"
so its probably best the DHCP client does this.
---
 ell/dhcp.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 57 insertions(+), 9 deletions(-)

diff --git a/ell/dhcp.c b/ell/dhcp.c
index e6fbda7..5f4d99a 100644
--- a/ell/dhcp.c
+++ b/ell/dhcp.c
@@ -367,6 +367,19 @@ static int dhcp_client_send_discover(struct l_dhcp_client *client)
 					discover, len);
 }
 
+static int dhcp_client_send_unicast(struct l_dhcp_client *client,
+					struct dhcp_message *request,
+					unsigned int len)
+{
+	struct sockaddr_in si;
+
+	memset(&si, 0, sizeof(si));
+	si.sin_family = AF_INET;
+	si.sin_port = L_CPU_TO_BE16(DHCP_PORT_SERVER);
+	si.sin_addr.s_addr = client->lease->server_address;
+	return client->transport->send(client->transport, &si, request, len);
+}
+
 static int dhcp_client_send_request(struct l_dhcp_client *client)
 {
 	struct dhcp_message_builder builder;
@@ -450,15 +463,8 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
 	 * 'server identifier' option for any unicast requests to the DHCP
 	 * server.
 	 */
-	if (client->state == DHCP_STATE_RENEWING) {
-		struct sockaddr_in si;
-		memset(&si, 0, sizeof(si));
-		si.sin_family = AF_INET;
-		si.sin_port = L_CPU_TO_BE16(DHCP_PORT_SERVER);
-		si.sin_addr.s_addr = client->lease->server_address;
-		return client->transport->send(client->transport,
-							&si, request, len);
-	}
+	if (client->state == DHCP_STATE_RENEWING)
+		return dhcp_client_send_unicast(client, request, len);
 
 	return client->transport->l2_send(client->transport,
 					INADDR_ANY, DHCP_PORT_CLIENT,
@@ -466,6 +472,45 @@ static int dhcp_client_send_request(struct l_dhcp_client *client)
 					NULL, request, len);
 }
 
+static void dhcp_client_send_release(struct l_dhcp_client *client)
+{
+	struct dhcp_message_builder builder;
+	size_t optlen = DHCP_MIN_OPTIONS_SIZE;
+	size_t len = sizeof(struct dhcp_message) + optlen;
+	L_AUTO_FREE_VAR(struct dhcp_message *, request);
+	int err;
+	struct sockaddr_in si;
+
+	CLIENT_DEBUG("");
+
+	memset(&si, 0, sizeof(si));
+	si.sin_family = AF_INET;
+	si.sin_port = L_CPU_TO_BE16(DHCP_PORT_SERVER);
+	si.sin_addr.s_addr = client->lease->server_address;
+
+	request = (struct dhcp_message *) l_new(uint8_t, len);
+
+	_dhcp_message_builder_init(&builder, request, len,
+					DHCP_MESSAGE_TYPE_RELEASE);
+
+	err = client_message_init(client, request, &builder);
+	if (err < 0)
+		return;
+
+	request->ciaddr = client->lease->address;
+
+	if (!_dhcp_message_builder_append(&builder,
+					L_DHCP_OPTION_SERVER_IDENTIFIER,
+					4, &client->lease->server_address)) {
+		CLIENT_DEBUG("Failed to append server ID");
+		return;
+	}
+
+	_dhcp_message_builder_finalize(&builder, &len);
+
+	dhcp_client_send_unicast(client, request, len);
+}
+
 static void dhcp_client_timeout_resend(struct l_timeout *timeout,
 								void *user_data)
 {
@@ -1063,6 +1108,9 @@ LIB_EXPORT bool l_dhcp_client_stop(struct l_dhcp_client *client)
 	if (unlikely(!client))
 		return false;
 
+	if (client->state == DHCP_STATE_BOUND)
+		dhcp_client_send_release(client);
+
 	if (client->rtnl_add_cmdid) {
 		l_netlink_cancel(client->rtnl, client->rtnl_add_cmdid);
 		client->rtnl_add_cmdid = 0;
-- 
2.26.2

[PATCH 2/6] dhcp-server: fix incorrect setting of lease OFFER_TIME

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

When in the offer stage the lease expiration was being set to
OFFER_TIME, rather than the current time plus OFFER_TIME.
---
 ell/dhcp-server.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 4a7e8e3..fc6bdbf 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -350,8 +350,8 @@ static void send_offer(struct l_dhcp_server *server,
 		return;
 	}
 
-	lease = add_lease(server, OFFER_TIME, client_msg->chaddr,
-				reply->yiaddr);
+	lease = add_lease(server, l_time_to_secs(l_time_now()) + OFFER_TIME,
+				client_msg->chaddr, reply->yiaddr);
 	if (!lease) {
 		SERVER_DEBUG("No free IP addresses, OFFER abandoned");
 		return;
-- 
2.26.2

[PATCH 3/6] dhcp-private: add flag in lease to distinguish offered/active

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

The DHCP server did not distinguish between a lease which has been
offered, and one that is active.
---
 ell/dhcp-private.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index 62abfca..17d0574 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -158,6 +158,9 @@ struct l_dhcp_lease {
 	char *domain_name;
 	/* for server */
 	uint8_t mac[6];
+
+	/* set for an offered lease, but not ACK'ed */
+	bool offering : 1;
 };
 
 struct l_dhcp_lease *_dhcp_lease_new(void);
-- 
2.26.2

[PATCH 4/6] dhcp-server: add timer for tracking expired leases

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 6374 bytes --]

Currently dhcp-server has a limitation where it only calls the
lease expired callback if a client actually releases the lease
itself.

This patchs adds a single timer to the server object which tracks
the soonest expiring lease. When this lease expires the event is
triggered and the timeout is reset to the (new) soonest expiring
lease.
---
 ell/dhcp-server.c | 135 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 129 insertions(+), 6 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index fc6bdbf..f418ed5 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -37,6 +37,7 @@
 #include "queue.h"
 #include "util.h"
 #include "strv.h"
+#include "timeout.h"
 
 /* 8 hours */
 #define DEFAULT_DHCP_LEASE_SEC (8*60*60)
@@ -62,6 +63,9 @@ struct l_dhcp_server {
 
 	struct l_queue *lease_list;
 
+	/* Next lease expiring */
+	struct l_timeout *next_expire;
+
 	l_dhcp_debug_cb_t debug_handler;
 	void *debug_data;
 	l_dhcp_destroy_cb_t debug_destroy;
@@ -164,6 +168,81 @@ static int compare_lifetime(const void *a, const void *b, void *user_data)
 	return lease2->lifetime - lease1->lifetime;
 }
 
+struct lease_expire_data {
+	struct l_dhcp_lease *lease;
+	struct l_dhcp_server *server;
+};
+
+static void next_expiring_lease(void *data, void *user_data)
+{
+	struct l_dhcp_lease *lease = data;
+	struct l_dhcp_lease **next = user_data;
+
+	/* Don't count leases which are in offer stage */
+	if (lease->offering)
+		return;
+
+	/* Sanity check that the lease lifetime hasn't passed */
+	if (l_time_after(l_time_now(), lease->lifetime))
+		return;
+
+	/* Does this lease expire before our current candidate? */
+	if (l_time_before(lease->lifetime, (*next)->lifetime))
+		*next = lease;
+}
+
+static void lease_expired_cb(struct l_timeout *timeout, void *user_data);
+
+static void set_next_expire_timer(struct l_dhcp_server *server,
+					struct l_dhcp_lease *expired)
+{
+	struct lease_expire_data *data;
+	struct l_dhcp_lease *next;
+	unsigned int next_timeout;
+
+	/*
+	 * Head of the queue isn't an active lease, or its the lease which
+	 * triggered this timeout.
+	 */
+	next = l_queue_peek_head(server->lease_list);
+	if (next == expired || !next || (next && is_expired_lease(next))) {
+		server->next_expire = NULL;
+		return;
+	}
+
+	/*
+	 * Find the next lease that is expiring. Start at a known good lease
+	 * (head) and find a lease which expires soonest.
+	 */
+	l_queue_foreach(server->lease_list, next_expiring_lease, &next);
+
+	next_timeout = l_time_to_secs(l_time_diff(l_time_now(), next->lifetime));
+
+	/* data gets freed by destroy, so we can reuse the pointer here */
+	data = l_new(struct lease_expire_data, 1);
+
+	data->server = server;
+	data->lease = next;
+
+	l_timeout_remove(server->next_expire);
+
+	server->next_expire = l_timeout_create(next_timeout, lease_expired_cb,
+						data, l_free);
+}
+
+static void lease_expired_cb(struct l_timeout *timeout, void *user_data)
+{
+	struct lease_expire_data *data = user_data;
+	struct l_dhcp_server *server = data->server;
+	struct l_dhcp_lease *lease = data->lease;
+
+	if (server->event_handler)
+		server->event_handler(server, L_DHCP_SERVER_EVENT_LEASE_EXPIRED,
+					server->user_data, lease);
+
+	set_next_expire_timer(server, lease);
+}
+
 static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
 					uint32_t expire, const uint8_t *chaddr,
 					uint32_t yiaddr)
@@ -180,11 +259,35 @@ static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
 	memcpy(lease->mac, chaddr, ETH_ALEN);
 	lease->address = yiaddr;
 
-	if (expire == 0)
+	/*
+	 * Somewhat convoluted but a zero expire time means this is an actual
+	 * lease that was offered and accepted. Anything else is a lease that
+	 * was offered but not yet accepted/ACK'ed.
+	 *
+	 * If we do not yet have a lease expire timer (first lease) we set one
+	 * now.
+	 */
+	if (expire == 0) {
 		lease->lifetime = l_time_to_secs(l_time_now()) +
 						server->lease_seconds;
-	else
+		lease->offering = false;
+
+		if (!server->next_expire) {
+			struct lease_expire_data *data = l_new(
+						struct lease_expire_data, 1);
+
+			data->lease = lease;
+			data->server = server;
+
+			server->next_expire = l_timeout_create(
+							server->lease_seconds,
+							lease_expired_cb,
+							data, l_free);
+		}
+	} else {
 		lease->lifetime = expire;
+		lease->offering = true;
+	}
 
 	l_queue_insert(server->lease_list, lease, compare_lifetime, NULL);
 
@@ -196,17 +299,38 @@ static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
 }
 
 static void lease_release(struct l_dhcp_server *server,
-			struct l_dhcp_lease *lease, uint32_t expire)
+			struct l_dhcp_lease *lease)
 {
+	/*
+	 * If the client released the lease after the server timeout expired
+	 * there is nothing to do. Otherwise the client is releasing the
+	 * lease early which may require re-setting the lease expire timer
+	 */
+	if (is_expired_lease(lease))
+		return;
+
 	l_queue_remove(server->lease_list, lease);
 
-	lease->lifetime = expire;
+	/*
+	 * We want to ensure that this lease does not get counted as active
+	 * when resetting the expire time. Since the expiration is seconds
+	 * based, setting the lifetime to 'now' could easily be counted as an
+	 * active lease due to rounding errors. This is why its being set to
+	 * 'now' minus one second.
+	 */
+	lease->lifetime = l_time_to_secs(l_time_now()) - 1;
 
 	l_queue_insert(server->lease_list, lease, compare_lifetime, NULL);
 
 	if (server->event_handler)
 		server->event_handler(server, L_DHCP_SERVER_EVENT_LEASE_EXPIRED,
 					server->user_data, lease);
+
+	/*
+	 * Since this is not a lease which timed out we don't pass the lease
+	 * object. This could result in the lease timeout not re-arming.
+	 */
+	set_next_expire_timer(server, NULL);
 }
 
 static bool match_lease_ip(const void *data, const void *user_data)
@@ -567,8 +691,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 			break;
 
 		if (message->ciaddr == lease->address)
-			lease_release(server, lease,
-						l_time_to_secs(l_time_now()));
+			lease_release(server, lease);
 
 		break;
 	case DHCP_MESSAGE_TYPE_INFORM:
-- 
2.26.2

[PATCH 5/6] unit: add client expire checks to test-dhcp

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 2864 bytes --]

Verifies the proper events fire when clients release their lease.
---
 unit/test-dhcp.c | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/unit/test-dhcp.c b/unit/test-dhcp.c
index 877d649..2f41609 100644
--- a/unit/test-dhcp.c
+++ b/unit/test-dhcp.c
@@ -764,6 +764,24 @@ static void do_debug(const char *str, void *user_data)
 	l_info("%s%s", prefix, str);
 }
 
+static char *new_client;
+static char *expired_client;
+
+static void server_event(struct l_dhcp_server *server,
+					enum l_dhcp_server_event event,
+					void *user_data,
+					const struct l_dhcp_lease *lease)
+{
+	switch (event) {
+	case L_DHCP_SERVER_EVENT_NEW_LEASE:
+		new_client = l_dhcp_lease_get_address(lease);
+		break;
+	case L_DHCP_SERVER_EVENT_LEASE_EXPIRED:
+		expired_client = l_dhcp_lease_get_address(lease);
+		break;
+	}
+}
+
 static void test_complete_run(const void *data)
 {
 	static const uint8_t addr1[6] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06 };
@@ -794,6 +812,8 @@ static void test_complete_run(const void *data)
 	assert(l_dhcp_server_set_netmask(server, "255.255.255.0"));
 	assert(l_dhcp_server_set_gateway(server, "192.168.1.1"));
 	assert(l_dhcp_server_set_dns(server, dns));
+	assert(l_dhcp_server_set_event_handler(server, server_event,
+						NULL, NULL));
 
 	if (verbose)
 		l_dhcp_server_set_debug(server, do_debug, "[DHCP SERV] ", NULL);
@@ -865,6 +885,11 @@ static void test_complete_run(const void *data)
 	cli_addr = l_dhcp_lease_get_address(cli_lease);
 	assert(cli_addr);
 	assert(!strcmp(cli_addr, "192.168.1.2"));
+	assert(new_client);
+	assert(!strcmp(new_client, cli_addr));
+	l_free(new_client);
+	new_client = NULL;
+
 	srv_addr = l_dhcp_lease_get_server_id(cli_lease);
 	assert(!strcmp(srv_addr, "192.168.1.1"));
 	l_free(srv_addr);
@@ -910,14 +935,30 @@ static void test_complete_run(const void *data)
 	cli_addr = l_dhcp_lease_get_address(cli_lease);
 	assert(cli_addr);
 	assert(!strcmp(cli_addr, "192.168.1.3"));
+	assert(new_client);
+	assert(!strcmp(new_client, cli_addr));
+	l_free(new_client);
+	new_client = NULL;
 	srv_addr = l_dhcp_lease_get_server_id(cli_lease);
 	assert(!strcmp(srv_addr, "192.168.1.1"));
 	l_free(srv_addr);
 	l_free(cli_addr);
 
-
 	l_dhcp_client_stop(client1);
+
+	srv_transport->rx_cb(client_packet, client_packet_len, server);
+	assert(expired_client);
+	assert(!strcmp(expired_client, "192.168.1.2"));
+	l_free(expired_client);
+	expired_client = NULL;
+
 	l_dhcp_client_stop(client2);
+	srv_transport->rx_cb(client_packet, client_packet_len, server);
+	assert(expired_client);
+	assert(!strcmp(expired_client, "192.168.1.3"));
+	l_free(expired_client);
+	expired_client = NULL;
+
 	l_dhcp_client_destroy(client1);
 	l_dhcp_client_destroy(client2);
 
-- 
2.26.2

[PATCH 6/6] dhcp-server: print actual lifetime for new lease

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

The leases expiration time was being printed, which is an
offset off the current time rather than the actual lease
time.
---
 ell/dhcp-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index f418ed5..c51ebf3 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -293,7 +293,7 @@ static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
 
 	SERVER_DEBUG("added lease IP %s for "MAC " lifetime=%u",
 			IP_STR(yiaddr), MAC_STR(chaddr),
-			lease->lifetime);
+			server->lease_seconds);
 
 	return lease;
 }
-- 
2.26.2

Re: [PATCH 2/6] dhcp-server: fix incorrect setting of lease OFFER_TIME

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

Hi James,

On 11/18/20 1:00 PM, James Prestwood wrote:
> When in the offer stage the lease expiration was being set to
> OFFER_TIME, rather than the current time plus OFFER_TIME.
> ---
>   ell/dhcp-server.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

Re: [PATCH 6/6] dhcp-server: print actual lifetime for new lease

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 320 bytes --]

Hi James,

On 11/18/20 1:00 PM, James Prestwood wrote:
> The leases expiration time was being printed, which is an
> offset off the current time rather than the actual lease
> time.
> ---
>   ell/dhcp-server.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Applied, thanks.

Regards,
-Denis