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

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Andrew,

On 6/1/21 6:53 PM, Andrew Zaborowski wrote:
> We were comparing the l_time_now() (usecs) to lease->lifetime (secs)
> and then converting the result from usecs to secs, so the "diff" and the
> "to_secs" operations need to be switched around.  While there, use the
> opportunity to 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 1 millisec instead of the absolute value of the difference.
> 
> Since we're using uint32_t for the timestamps, use the overflow-safe(r)
> comparisons in set_next_expire_timer and in is_expired_lease.
> ---
>   ell/dhcp-server.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
> index ebd4438..38193a2 100644
> --- a/ell/dhcp-server.c
> +++ b/ell/dhcp-server.c
> @@ -104,7 +104,7 @@ struct l_dhcp_server {
>   
>   static bool is_expired_lease(struct l_dhcp_lease *lease)
>   {
> -	if (lease->lifetime < l_time_to_secs(l_time_now()))
> +	if ((int32_t) (lease->lifetime - l_time_to_secs(l_time_now())) < 0)

Why not use l_time_before / l_time_after.  And with these changes I'm not sure 
there's any point to store lease->lifetime in seconds.  I suspect this was done 
originally to use 'second' versions of l_timeout_*.

>   		return true;
>   
>   	return false;
> @@ -191,7 +191,8 @@ static void set_next_expire_timer(struct l_dhcp_server *server,
>   					struct l_dhcp_lease *expired)
>   {
>   	struct l_dhcp_lease *next;
> -	unsigned int next_timeout;
> +	unsigned long next_timeout;

Why unsigned long?  It is different between 32 bit and 64 bit, so if you need 
overflow protection, just use uint64_t instead?

> +	uint32_t now;
>   
>   	/*
>   	 * If this is an expiring lease put it into the expired queue, removing
> @@ -214,11 +215,12 @@ 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) * 1000L : 1;

Why 1000L?  next_timeout is unsigned?

Also, why not use existing l_time_after, l_time_diff and l_time_to_msecs utilities?

>   
>   	if (server->next_expire)
> -		l_timeout_modify(server->next_expire, next_timeout);
> +		l_timeout_modify_ms(server->next_expire, next_timeout);
>   	else
>   		server->next_expire = l_timeout_create(server->lease_seconds,
>   							lease_expired_cb,
> 

Regards,
-Denis