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

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

[Andrew Zaborowski]

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

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)
 		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;
+	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;
 
 	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,
-- 
2.30.2

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

[Denis Kenzior]

[-- 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

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

[Andrew Zaborowski]

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

Hi Denis,

On Fri, 4 Jun 2021 at 18:49, Denis Kenzior <denkenz@gmail.com> wrote:
> 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.

l_time_after is basically what we currently have here (a simple "<'),
and the idea was to make the comparison a little safer just in case.

>  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_*.

Right, we could just convert it to the time.h format, uint64_t
microseconds, but it would break the API.

>
> >               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?

"unsigned int" is the type of the l_timeout_modify parameter so I
changed it to the type of the l_timeout_modify_ms parameter since that
will limit the max value even if we pass a larger type.

>
> > +     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?

My logic was to make sure the multiplication is treated as uint32_t *
long rather than uint32_t * int to avoid an overflow (if the result
fits in a long...).  I'm not sure if this is needed but checking with
gdb seems to indicate it is:

(gdb) print sizeof(int)
$1 = 4
(gdb) print sizeof(long)
$2 = 8
(gdb) print (unsigned int) 0x10000000 * 1000
$3 = 3892314112
(gdb) print (unsigned int) 0x10000000 * 1000L
$4 = 16777216000

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

As for l_time_after, I was trying to improve on the overflow safety a
bit like I mentioned in the commit.

As for l_time_diff, we can use it, I just thought it might be worth
saving that one extra comparison in it.

As for l_time_to_msecs we're converting from secs to msecs, not from
usecs to msecs, but I can use L_MSEC_PER_SEC instead of 1000L.

Best regards

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

[Denis Kenzior]

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

Hi Andrew,

On 6/4/21 12:35 PM, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Fri, 4 Jun 2021 at 18:49, Denis Kenzior <denkenz@gmail.com> wrote:
>> 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.
> 
> l_time_after is basically what we currently have here (a simple "<'),
> and the idea was to make the comparison a little safer just in case.

We avoid the whole Y2038 problem by using uint64_t and l_time* utilities.  So 
why make things 'a little' safer, when you can make them fully safe? :)

> 
>>   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_*.
> 
> Right, we could just convert it to the time.h format, uint64_t
> microseconds, but it would break the API.
> 

What API?

>>
>>>                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?
> 
> "unsigned int" is the type of the l_timeout_modify parameter so I
> changed it to the type of the l_timeout_modify_ms parameter since that
> will limit the max value even if we pass a larger type.

So just FYI, sizeof(unsigned long) == sizeof(unsigned int) on 32 bit.  So you're 
actually introducing differing behavior depending on the platform.  This is the 
point I'm making above...

> 
>>
>>> +     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?
> 
> My logic was to make sure the multiplication is treated as uint32_t *
> long rather than uint32_t * int to avoid an overflow (if the result

See above.  Also, shouldn't this be 1000UL/ULL?  Probably doesn't matter, but if 
you're going through the trouble, might as well get it perfect...

> fits in a long...).  I'm not sure if this is needed but checking with
> gdb seems to indicate it is:
> 
> (gdb) print sizeof(int)
> $1 = 4
> (gdb) print sizeof(long)
> $2 = 8

On 64 bit, sure...

> (gdb) print (unsigned int) 0x10000000 * 1000
> $3 = 3892314112
> (gdb) print (unsigned int) 0x10000000 * 1000L
> $4 = 16777216000
> 
>>
>> Also, why not use existing l_time_after, l_time_diff and l_time_to_msecs utilities?
> 
> As for l_time_after, I was trying to improve on the overflow safety a
> bit like I mentioned in the commit.

I suspect you need a sanity check regardless if the l_time_to_msec conversion 
results in something > UINT_MAX since the result is just truncated and that is 
likely not what you want anyway.

> 
> As for l_time_diff, we can use it, I just thought it might be worth
> saving that one extra comparison in it.
> 
> As for l_time_to_msecs we're converting from secs to msecs, not from
> usecs to msecs, but I can use L_MSEC_PER_SEC instead of 1000L.
> 

Yes, that would be better.

Regards,
-Denis

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

[Andrew Zaborowski]

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

On Fri, 4 Jun 2021 at 20:00, Denis Kenzior <denkenz@gmail.com> wrote:
> On 6/4/21 12:35 PM, Andrew Zaborowski wrote:
> > On Fri, 4 Jun 2021 at 18:49, Denis Kenzior <denkenz@gmail.com> wrote:
> >> Why not use l_time_before / l_time_after.
> >
> > l_time_after is basically what we currently have here (a simple "<'),
> > and the idea was to make the comparison a little safer just in case.
>
> We avoid the whole Y2038 problem by using uint64_t and l_time* utilities.  So
> why make things 'a little' safer, when you can make them fully safe? :)

It's the "commit early, commit often" kind of question, we have a
completely broken lease expiry here so the main goal was to make it
even work and then while I was changing that line... it felt wrong to
not make such a simple improvement, and then edited this line for
consistency with that line but without delving into the reasons this
uses uint32_t.

>
> >
> >>   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_*.
> >
> > Right, we could just convert it to the time.h format, uint64_t
> > microseconds, but it would break the API.
> >
>
> What API?

l_dhcp_lease_get_lifetime, though I guess you could just add a second
getter for the full resolution value.

>
> >>
> >>>                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?
> >
> > "unsigned int" is the type of the l_timeout_modify parameter so I
> > changed it to the type of the l_timeout_modify_ms parameter since that
> > will limit the max value even if we pass a larger type.
>
> So just FYI, sizeof(unsigned long) == sizeof(unsigned int) on 32 bit.

Sure...

>  So you're
> actually introducing differing behavior depending on the platform.  This is the
> point I'm making above...

It's l_timeout_modify_ms that introduced this behaviour really, we
can't get around it by using a different type for the variable.  In
fact it might add to the confusion if we do.

>
> >
> >>
> >>> +     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?
> >
> > My logic was to make sure the multiplication is treated as uint32_t *
> > long rather than uint32_t * int to avoid an overflow (if the result
>
> See above.  Also, shouldn't this be 1000UL/ULL?  Probably doesn't matter, but if
> you're going through the trouble, might as well get it perfect...

It gets cast to an unsigned value in the end but why not.

>
> > fits in a long...).  I'm not sure if this is needed but checking with
> > gdb seems to indicate it is:
> >
> > (gdb) print sizeof(int)
> > $1 = 4
> > (gdb) print sizeof(long)
> > $2 = 8
>
> On 64 bit, sure...

Right, that's why I said *if* the result fits in a long.  If it
doesn't there's nothing you can do.

>
> > (gdb) print (unsigned int) 0x10000000 * 1000
> > $3 = 3892314112
> > (gdb) print (unsigned int) 0x10000000 * 1000L
> > $4 = 16777216000
> >
> >>
> >> Also, why not use existing l_time_after, l_time_diff and l_time_to_msecs utilities?
> >
> > As for l_time_after, I was trying to improve on the overflow safety a
> > bit like I mentioned in the commit.
>
> I suspect you need a sanity check regardless if the l_time_to_msec conversion
> results in something > UINT_MAX since the result is just truncated and that is
> likely not what you want anyway.

I guess the behaviour (if we want to worry about it) should be that if
the expiry time is too far out into the future then we set the longest
timeout we can with the current API, and then in lease_expired_cb() we
just reschedule the timeout if the lease has not expired, possibly
this is what you mean too.

Best regards

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

[Denis Kenzior]

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

Hi Andrew,

> It's the "commit early, commit often" kind of question, we have a
> completely broken lease expiry here so the main goal was to make it
> even work and then while I was changing that line... it felt wrong to
> not make such a simple improvement, and then edited this line for
> consistency with that line but without delving into the reasons this
> uses uint32_t.

I agree, but it feels like in this case you're trading something completely 
broken for something not-quite-completely-broken and it isn't much of an 
improvement.  You touch it, you own it ;)

> 
>>
>>>
>>>>    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_*.
>>>
>>> Right, we could just convert it to the time.h format, uint64_t
>>> microseconds, but it would break the API.
>>>
>>
>> What API?
> 
> l_dhcp_lease_get_lifetime, though I guess you could just add a second
> getter for the full resolution value.

The lease lifetime seems to be getting abused anyway.  In DHCP client case it is 
just the plain lifetime value sent by the gateway, in the server case it seems 
to be an absolute time.  So I suspect we need to keep storing it in seconds, but 
either change the timeout logic or keep additional info about the absolute 
timeout, preferably in l_time* compatible units.

>> I suspect you need a sanity check regardless if the l_time_to_msec conversion
>> results in something > UINT_MAX since the result is just truncated and that is
>> likely not what you want anyway.
> 
> I guess the behaviour (if we want to worry about it) should be that if
> the expiry time is too far out into the future then we set the longest
> timeout we can with the current API, and then in lease_expired_cb() we

Yep, that would be ideal.  Really, this should never-ever happen since our 
lifetimes are quite short.

> just reschedule the timeout if the lease has not expired, possibly
> this is what you mean too.
> 

Yep, that's what I had in mind.

Regards,
-Denis