[PATCH 1/4] dhcp-server: Fix lease expiry time calculation

[PATCH 1/4] dhcp-server: Fix lease expiry time calculation

[Andrew Zaborowski]

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

We were comparing the l_time_now() (usecs) with lease->lifetime (secs)
and then converting the result from usecs to secs, so the "diff" and the
"to_sec" operations should be switched around.  While there also replace
the "diff" operation so that if lease->lifetime is already in the past
(e.g. because processing took to long), we schedule the timeout in 0
seconds instead of the absolute value of the difference.

Since we're using uint32_t for the timestamps, use the overflow-safe(r)
comparison.
---
 ell/dhcp-server.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index d390856..9e39fe6 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -192,6 +192,7 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
 {
 	struct l_dhcp_lease *next;
 	unsigned int next_timeout;
+	uint32_t now;
 
 	/*
 	 * If this is an expiring lease put it into the expired queue, removing
@@ -213,8 +214,9 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
 		return;
 	}
 
-	next_timeout = l_time_to_secs(l_time_diff(l_time_now(),
-							next->lifetime));
+	now = l_time_to_secs(l_time_now());
+	next_timeout = (int32_t) (next->lifetime - now) > 0 ?
+		next->lifetime - now : 0;
 
 	if (server->next_expire)
 		l_timeout_modify(server->next_expire, next_timeout);
-- 
2.30.2

[PATCH 2/4] dhcp-server: Expire the lease from the right list

[Andrew Zaborowski]

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

We were trying to expire the oldest lease from server->expired_list but
we should be expiring the oldest lease in server->lease_list.  We were
never adding any new entries to server->expired_list and it would be
always empty.
---
 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 9e39fe6..9226ba4 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -229,7 +229,7 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
 static void lease_expired_cb(struct l_timeout *timeout, void *user_data)
 {
 	struct l_dhcp_server *server = user_data;
-	struct l_dhcp_lease *lease = l_queue_peek_tail(server->expired_list);
+	struct l_dhcp_lease *lease = l_queue_peek_tail(server->lease_list);
 
 	if (server->event_handler)
 		server->event_handler(server, L_DHCP_SERVER_EVENT_LEASE_EXPIRED,
-- 
2.30.2

[PATCH 3/4] dhcp-server: Don't check if lease expired in lease_release

[Andrew Zaborowski]

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

When lease_release() is called by listener_event(), the lease must have
been found in server->lease_list meaning that its expiry timeout hasn't
happened yet.  Even if lease->lifetime happens to have just expired, we
probably want to emit the LEASE_EXPIRED notification immediately and
move the lease from least_list and expired_list, rather than wait for
the timeout.
---
 ell/dhcp-server.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 9226ba4..217844b 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -287,14 +287,6 @@ 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)
 {
-	/*
-	 * 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;
-
 	if (server->event_handler)
 		server->event_handler(server, L_DHCP_SERVER_EVENT_LEASE_EXPIRED,
 					server->user_data, lease);
-- 
2.30.2

[PATCH 4/4] dhcp-server: Validate the offering state when handling message

[Andrew Zaborowski]

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

Make extra sure we don't call remove_lease() on a lease that is not in
the "offering" state because that would remove the lease without
respecting the lease_list/expire_lease semantics or emitting the
LEASE_EXPIRED event.  Similarly make sure we don't call lease_expire()
on a lease that is in the "offering" state because that would move it
directly to expire_lease, emit a false LEASE_EXPIRED event and update
the expiry timeout wrongly.
---
I wonder if instead we should insert the leases being offered into
server->lease_list same as the active leases (i.e. according to their
leas->lifetime), and call set_next_expire_timer() so that their lifetimes
are tracked and so that they're also expired.  We'd only need to check
the lease->offered flag before emitting the LEASE_EXPIRED events.

As it is now, I think a lease that is offered never times out if the
client never replies to the offer, the 5-minute lifetime is ignored.

 ell/dhcp-server.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 217844b..f4d08d6 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -646,7 +646,8 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	case DHCP_MESSAGE_TYPE_DECLINE:
 		SERVER_DEBUG("Received DECLINE");
 
-		if (!server_id_opt || !requested_ip_opt || !lease)
+		if (!server_id_opt || !requested_ip_opt || !lease ||
+				!lease->offering)
 			break;
 
 		if (requested_ip_opt == lease->address)
@@ -656,7 +657,7 @@ static void listener_event(const void *data, size_t len, void *user_data)
 	case DHCP_MESSAGE_TYPE_RELEASE:
 		SERVER_DEBUG("Received RELEASE");
 
-		if (!server_id_opt || !lease)
+		if (!server_id_opt || !lease || lease->offering)
 			break;
 
 		if (message->ciaddr == lease->address)
-- 
2.30.2

Re: [PATCH 1/4] dhcp-server: Fix lease expiry time calculation

[Denis Kenzior]

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

Hi Andrew,

On 5/31/21 9:03 PM, Andrew Zaborowski wrote:
> We were comparing the l_time_now() (usecs) with lease->lifetime (secs)
> and then converting the result from usecs to secs, so the "diff" and the
> "to_sec" operations should be switched around.  While there also replace
> the "diff" operation so that if lease->lifetime is already in the past
> (e.g. because processing took to long), we schedule the timeout in 0
> seconds instead of the absolute value of the difference.
> 
> Since we're using uint32_t for the timestamps, use the overflow-safe(r)
> comparison.
> ---
>   ell/dhcp-server.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
> index d390856..9e39fe6 100644
> --- a/ell/dhcp-server.c
> +++ b/ell/dhcp-server.c
> @@ -192,6 +192,7 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
>   {
>   	struct l_dhcp_lease *next;
>   	unsigned int next_timeout;
> +	uint32_t now;
>   
>   	/*
>   	 * If this is an expiring lease put it into the expired queue, removing
> @@ -213,8 +214,9 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
>   		return;
>   	}
>   
> -	next_timeout = l_time_to_secs(l_time_diff(l_time_now(),
> -							next->lifetime));
> +	now = l_time_to_secs(l_time_now());
> +	next_timeout = (int32_t) (next->lifetime - now) > 0 ?
> +		next->lifetime - now : 0;

0 is not a valid timeout value, it is used to rearm the timer.  So this case 
probably needs a different approach.

>   
>   	if (server->next_expire)
>   		l_timeout_modify(server->next_expire, next_timeout);
> 

Regards,
-Denis

Re: [PATCH 2/4] dhcp-server: Expire the lease from the right list

[Denis Kenzior]

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

Hi Andrew,

On 5/31/21 9:03 PM, Andrew Zaborowski wrote:
> We were trying to expire the oldest lease from server->expired_list but
> we should be expiring the oldest lease in server->lease_list.  We were
> never adding any new entries to server->expired_list and it would be
> always empty.
> ---
>   ell/dhcp-server.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 

Patch 2-4 applied, thanks.

Regards,
-Denis