Re: [PATCH] dhcp,dhcp6,icmp6,time: Convert timestamps to CLOCK_BOOTTIME

Re: [PATCH] dhcp,dhcp6,icmp6,time: Convert timestamps to CLOCK_BOOTTIME

[Denis Kenzior]

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

Hi Andrew,

>> By the way, given that you're using struct timespec below, do you want to use
>> SO_TIMESTAMPNS?
> 
> It doesn't seem that this would simplify the arithmetics in this case.
> I wonder if we should bother to use the _NEW variant, which makes sure
> the timestamp is year 2038-proof on 32-bit systems.
> 

Probably might as well.

<snip>

>>> +     return ((int64_t) realtime.tv_sec - boottime.tv_sec) * 1000000 +
>>> +             ((int64_t) realtime.tv_nsec - boottime.tv_nsec) / 1000;
>>
>> Lets use the L_USEC_* defines when possible.
> 
> Ok.  (IIRC this was done for consistency with l_time_now)
> 

Ah, I need to fix that.  The problem is that this function was added before the 
#defines existed.

I have now added commit:
bd5a2dc26441 ("time: Add converters from struct {timespec|timeval}")

Please review.

Regards,
-Denis

Re: [PATCH] dhcp,dhcp6,icmp6,time: Convert timestamps to CLOCK_BOOTTIME

[Denis Kenzior]

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

Hi Andrew,

On 5/17/22 19:35, Andrew Zaborowski wrote:
> The frame reception timestamps obtained for DHCPv4, DHCPv6 and ICMPv6
> frames from the kernel are based on CLOCK_REALTIME, there's no option to
> switch them to CLOCK_BOOTTIME, which we use across our retransmission and
> renew logic.  Add a time-private.h utility to convert the timestamps
> right where we read them from recvmsg() to produce CLOCK_BOOTTIME based
> times.  This isn't 100% bulletproof because the offset between the two
> clocks can vary between the time the frame was received by the kernel and
> the time we read the offset.  The probability is very low though and we
> have no better solution at this time other than fixing it in the kernel.
> Using CLOCK_REALTIME for frame reception timestamps seems to mainly
> create potential for problems in the common usages.
> 
> Fixes: c78ad1bb6d7e ("dhcp: Set lease expiry based on frame reception times")
> ---
>   ell/dhcp-transport.c  |  3 ++-
>   ell/dhcp6-transport.c |  5 +++--
>   ell/icmp6.c           |  2 +-
>   ell/time-private.h    |  1 +
>   ell/time.c            | 34 ++++++++++++++++++++++++++++++++++
>   6 files changed, 44 insertions(+), 6 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH] dhcp,dhcp6,icmp6,time: Convert timestamps to CLOCK_BOOTTIME

[Andrew Zaborowski]

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

The frame reception timestamps obtained for DHCPv4, DHCPv6 and ICMPv6
frames from the kernel are based on CLOCK_REALTIME, there's no option to
switch them to CLOCK_BOOTTIME, which we use across our retransmission and
renew logic.  Add a time-private.h utility to convert the timestamps
right where we read them from recvmsg() to produce CLOCK_BOOTTIME based
times.  This isn't 100% bulletproof because the offset between the two
clocks can vary between the time the frame was received by the kernel and
the time we read the offset.  The probability is very low though and we
have no better solution at this time other than fixing it in the kernel.
Using CLOCK_REALTIME for frame reception timestamps seems to mainly
create potential for problems in the common usages.

Fixes: c78ad1bb6d7e ("dhcp: Set lease expiry based on frame reception times")
---
 ell/dhcp-transport.c  |  3 ++-
 ell/dhcp6-transport.c |  5 +++--
 ell/icmp6.c           |  2 +-
 ell/time-private.h    |  1 +
 ell/time.c            | 34 ++++++++++++++++++++++++++++++++++
 6 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/ell/dhcp-transport.c b/ell/dhcp-transport.c
index 52da2db..42872bc 100644
--- a/ell/dhcp-transport.c
+++ b/ell/dhcp-transport.c
@@ -45,6 +45,7 @@
 #include "util.h"
 #include "private.h"
 #include "time.h"
+#include "time-private.h"
 #include "dhcp-private.h"
 
 struct dhcp_default_transport {
@@ -170,7 +171,7 @@ static bool _dhcp_default_transport_read_handler(struct l_io *io,
 				CMSG_LEN(sizeof(struct timeval))) {
 			const struct timeval *tv = (void *) CMSG_DATA(cmsg);
 
-			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
+			timestamp = _time_realtime_to_boottime(tv);
 		}
 	}
 
diff --git a/ell/dhcp6-transport.c b/ell/dhcp6-transport.c
index 13545b8..24cc1ce 100644
--- a/ell/dhcp6-transport.c
+++ b/ell/dhcp6-transport.c
@@ -40,6 +40,7 @@
 #include "missing.h"
 #include "io.h"
 #include "time.h"
+#include "time-private.h"
 #include "dhcp6-private.h"
 
 struct dhcp6_default_transport {
@@ -81,9 +82,9 @@ static bool _dhcp6_default_transport_read_handler(struct l_io *io,
 				cmsg->cmsg_type == SCM_TIMESTAMP &&
 				cmsg->cmsg_len ==
 				CMSG_LEN(sizeof(struct timeval))) {
-		const struct timeval *tv = (void *) CMSG_DATA(cmsg);
+			const struct timeval *tv = (void *) CMSG_DATA(cmsg);
 
-			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
+			timestamp = _time_realtime_to_boottime(tv);
 		}
 	}
 
diff --git a/ell/icmp6.c b/ell/icmp6.c
index 96ba1ec..b790f19 100644
--- a/ell/icmp6.c
+++ b/ell/icmp6.c
@@ -249,7 +249,7 @@ static int icmp6_receive(int s, void *buf, size_t buf_len, struct in6_addr *src,
 				CMSG_LEN(sizeof(struct timeval))) {
 			const struct timeval *tv = (void *) CMSG_DATA(cmsg);
 
-			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
+			timestamp = _time_realtime_to_boottime(tv);
 		}
 	}
 
diff --git a/ell/time-private.h b/ell/time-private.h
index dc4d6c2..5295d94 100644
--- a/ell/time-private.h
+++ b/ell/time-private.h
@@ -23,3 +23,4 @@
 uint64_t _time_pick_interval_secs(uint32_t min_secs, uint32_t max_secs);
 uint64_t _time_fuzz_msecs(uint64_t ms);
 uint64_t _time_fuzz_secs(uint32_t secs, uint32_t max_offset);
+uint64_t _time_realtime_to_boottime(const struct timeval *ts);
diff --git a/ell/time.c b/ell/time.c
index d02dabe..5d5276a 100644
--- a/ell/time.c
+++ b/ell/time.c
@@ -105,3 +105,37 @@ uint64_t _time_fuzz_secs(uint32_t secs, uint32_t max_offset)
 
 	return ms;
 }
+
+/*
+ * Convert a *recent* CLOCK_REALTIME-based timestamp to a
+ * CLOCK_BOOTTIME-based usec count consistent with l_time functions.
+ * The longer the time since the input timestamp the higher the
+ * probability of the two clocks having diverged and the higher the
+ * expected error magnitude.
+ */
+uint64_t _time_realtime_to_boottime(const struct timeval *ts)
+{
+	uint64_t now_realtime;
+	uint64_t now_boottime = l_time_now();
+	struct timespec timespec;
+	uint64_t ts_realtime;
+	uint64_t offset;
+
+	clock_gettime(CLOCK_REALTIME, &timespec);
+	now_realtime = (uint64_t) timespec.tv_sec * L_USEC_PER_SEC +
+		timespec.tv_nsec / L_NSEC_PER_USEC;
+
+	ts_realtime = ts->tv_sec * L_USEC_PER_SEC + ts->tv_usec;
+
+	offset = l_time_diff(ts_realtime, now_realtime);
+
+	/* Most likely case, timestamp in the past */
+	if (l_time_before(ts_realtime, now_realtime)) {
+		if (offset > now_boottime)
+			return 0;
+
+		return now_boottime - offset;
+	}
+
+	return l_time_offset(now_boottime, offset);
+}
-- 
2.32.0

Re: [PATCH] dhcp, dhcp6, icmp6, time: Convert timestamps to CLOCK_BOOTTIME

[Andrew Zaborowski]

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

On Wed, 18 May 2022 at 01:29, Andrew Zaborowski
<andrew.zaborowski(a)intel.com> wrote:
> On Tue, 17 May 2022 at 16:37, Denis Kenzior <denkenz(a)gmail.com> wrote:
> > So I would avoid signed integers for these calculations.  We already have
> > l_time_before/l_time_after.  And l_time_offset takes care of overflows.  Lets
> > use those to implement this.  You also need to consider potential underflow,
> > though it is probably astronomically unlikely.
>
> Either int64_t or uin64_t should have worked for overflows and
> underflows as long as we operate on 64-bit numbers throughout.

Sorry, I didn't get which kind of overflow/underflow you meant.  You
could indeed have CLOCK_REALTIME-based timestamps that can't be
represented in CLOCK_BOOTTIME without an overflow or an underflow.

The resulting wrapped value would still work for calculating intervals
between frames, less so for expiry times if they still end up being
negative.

Best regards

Re: [PATCH] dhcp, dhcp6, icmp6, time: Convert timestamps to CLOCK_BOOTTIME

[Andrew Zaborowski]

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

Hi Denis,

On Tue, 17 May 2022 at 16:37, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 5/16/22 16:31, Andrew Zaborowski wrote:
> >
> > -                     timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
> > +                     /*
> > +                      * Not ideal but this is our best approximation of the
> > +                      * CLOCK_BOOTTIME-based Rx time.
> > +                      */
> > +                     timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec -
> > +                             _time_get_realtime_boottime_offset();
>
> Can we actually just send the timestamp as a 'struct timeval' to the private
> function?  Instead of duplicating the conversion calculation everywhere?

Ok.  (The idea was that having the offset you can convert in either direction)

>
> >               }
> >       }
> >
>
> <snip>
>
> > diff --git a/ell/time.c b/ell/time.c
> > index d02dabe..cda09be 100644
> > --- a/ell/time.c
> > +++ b/ell/time.c
> > @@ -105,3 +105,20 @@ uint64_t _time_fuzz_secs(uint32_t secs, uint32_t max_offset)
> >
> >       return ms;
> >   }
> > +
> > +/*
> > + * Get approximate difference between CLOCK_REALTIME and CLOCK_BOOTTIME,
> > + * for converting recent timestamps from one to the other.  The value can
> > + * change at any time.
> > + */
> > +int64_t _time_get_realtime_boottime_offset(void)
>
> So I would avoid signed integers for these calculations.  We already have
> l_time_before/l_time_after.  And l_time_offset takes care of overflows.  Lets
> use those to implement this.  You also need to consider potential underflow,
> though it is probably astronomically unlikely.

Either int64_t or uin64_t should have worked for overflows and
underflows as long as we operate on 64-bit numbers throughout.

>
> I'd name this something like:
>
> uint64_t _time_realtime_to_boottime(const struct timeval *ts)
>
> By the way, given that you're using struct timespec below, do you want to use
> SO_TIMESTAMPNS?

It doesn't seem that this would simplify the arithmetics in this case.
I wonder if we should bother to use the _NEW variant, which makes sure
the timestamp is year 2038-proof on 32-bit systems.

>
> > +{
> > +     struct timespec boottime;
> > +     struct timespec realtime;
> > +
> > +     clock_gettime(CLOCK_BOOTTIME, &boottime);
> > +     clock_gettime(CLOCK_REALTIME, &realtime);
> > +
>
> Then here do something like:
>
> offset = l_time_diff(ts, now_realtime);
>
> /* most likely case, timestamp in the past */
> if (l_time_before(ts, now_realtime)) {
>         if (offset > now_boot)
>                 return 0;
>
>         return now_boot - offset;
> }
>
> return l_time_offset(now_boot, offset);

This is a complicated way to perform two subtractions but ok ;-)

>
> > +     return ((int64_t) realtime.tv_sec - boottime.tv_sec) * 1000000 +
> > +             ((int64_t) realtime.tv_nsec - boottime.tv_nsec) / 1000;
>
> Lets use the L_USEC_* defines when possible.

Ok.  (IIRC this was done for consistency with l_time_now)

Best regards

Re: [PATCH] dhcp,dhcp6,icmp6,time: Convert timestamps to CLOCK_BOOTTIME

[Denis Kenzior]

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

Hi Andrew,

On 5/16/22 16:31, Andrew Zaborowski wrote:
> The frame reception timestamps obtained for DHCPv4, DHCPv6 and ICMPv6
> frames from the kernel are based on CLOCK_REALTIME, there's no option to
> switch them to CLOCK_BOOTTIME, which we use across our retransmission and
> renew logic.  Add a time-private.h utility to get the offset between the
> two clocks and offset the timestamps right where read them from recvmsg()
> to produce CLOCK_BOOTTIME based times.  This isn't 100% bulletproof
> because the offset between the two clocks can vary between the moment the
> frame was received by the kernel and the time we read the offset.  The
> probability is very low though and we have no better solution at this
> time other than fixing it in the kernel.  Using CLOCK_REALTIME for frame
> reception timestamps seems to only create potential for problems in the
> common usages.
> 
> Fixes: c78ad1bb6d7e ("dhcp: Set lease expiry based on frame reception times")
> ---
>   ell/dhcp-transport.c  |  8 +++++++-
>   ell/dhcp6-transport.c | 12 +++++++++---
>   ell/icmp6.c           |  7 ++++++-
>   ell/time-private.h    |  1 +
>   ell/time.c            | 17 +++++++++++++++++
>   5 files changed, 40 insertions(+), 5 deletions(-)
> 

Makes sense to me, just minor comments:

> diff --git a/ell/dhcp-transport.c b/ell/dhcp-transport.c
> index 52da2db..ba1423f 100644
> --- a/ell/dhcp-transport.c
> +++ b/ell/dhcp-transport.c
> @@ -45,6 +45,7 @@
>   #include "util.h"
>   #include "private.h"
>   #include "time.h"
> +#include "time-private.h"
>   #include "dhcp-private.h"
>   
>   struct dhcp_default_transport {
> @@ -170,7 +171,12 @@ static bool _dhcp_default_transport_read_handler(struct l_io *io,
>   				CMSG_LEN(sizeof(struct timeval))) {
>   			const struct timeval *tv = (void *) CMSG_DATA(cmsg);
>   
> -			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
> +			/*
> +			 * Not ideal but this is our best approximation of the
> +			 * CLOCK_BOOTTIME-based Rx time.
> +			 */
> +			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec -
> +				_time_get_realtime_boottime_offset();

Can we actually just send the timestamp as a 'struct timeval' to the private 
function?  Instead of duplicating the conversion calculation everywhere?

>   		}
>   	}
>   

<snip>

> diff --git a/ell/time.c b/ell/time.c
> index d02dabe..cda09be 100644
> --- a/ell/time.c
> +++ b/ell/time.c
> @@ -105,3 +105,20 @@ uint64_t _time_fuzz_secs(uint32_t secs, uint32_t max_offset)
>   
>   	return ms;
>   }
> +
> +/*
> + * Get approximate difference between CLOCK_REALTIME and CLOCK_BOOTTIME,
> + * for converting recent timestamps from one to the other.  The value can
> + * change at any time.
> + */
> +int64_t _time_get_realtime_boottime_offset(void)

So I would avoid signed integers for these calculations.  We already have 
l_time_before/l_time_after.  And l_time_offset takes care of overflows.  Lets 
use those to implement this.  You also need to consider potential underflow, 
though it is probably astronomically unlikely.

I'd name this something like:

uint64_t _time_realtime_to_boottime(const struct timeval *ts)

By the way, given that you're using struct timespec below, do you want to use 
SO_TIMESTAMPNS?

> +{
> +	struct timespec boottime;
> +	struct timespec realtime;
> +
> +	clock_gettime(CLOCK_BOOTTIME, &boottime);
> +	clock_gettime(CLOCK_REALTIME, &realtime);
> +

Then here do something like:

offset = l_time_diff(ts, now_realtime);

/* most likely case, timestamp in the past */
if (l_time_before(ts, now_realtime)) {
	if (offset > now_boot)
		return 0;

	return now_boot - offset;
}

return l_time_offset(now_boot, offset);

> +	return ((int64_t) realtime.tv_sec - boottime.tv_sec) * 1000000 +
> +		((int64_t) realtime.tv_nsec - boottime.tv_nsec) / 1000;

Lets use the L_USEC_* defines when possible.

> +}
> 

Regards,
-Denis

[PATCH] dhcp,dhcp6,icmp6,time: Convert timestamps to CLOCK_BOOTTIME

[Andrew Zaborowski]

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

The frame reception timestamps obtained for DHCPv4, DHCPv6 and ICMPv6
frames from the kernel are based on CLOCK_REALTIME, there's no option to
switch them to CLOCK_BOOTTIME, which we use across our retransmission and
renew logic.  Add a time-private.h utility to get the offset between the
two clocks and offset the timestamps right where read them from recvmsg()
to produce CLOCK_BOOTTIME based times.  This isn't 100% bulletproof
because the offset between the two clocks can vary between the moment the
frame was received by the kernel and the time we read the offset.  The
probability is very low though and we have no better solution at this
time other than fixing it in the kernel.  Using CLOCK_REALTIME for frame
reception timestamps seems to only create potential for problems in the
common usages.

Fixes: c78ad1bb6d7e ("dhcp: Set lease expiry based on frame reception times")
---
 ell/dhcp-transport.c  |  8 +++++++-
 ell/dhcp6-transport.c | 12 +++++++++---
 ell/icmp6.c           |  7 ++++++-
 ell/time-private.h    |  1 +
 ell/time.c            | 17 +++++++++++++++++
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/ell/dhcp-transport.c b/ell/dhcp-transport.c
index 52da2db..ba1423f 100644
--- a/ell/dhcp-transport.c
+++ b/ell/dhcp-transport.c
@@ -45,6 +45,7 @@
 #include "util.h"
 #include "private.h"
 #include "time.h"
+#include "time-private.h"
 #include "dhcp-private.h"
 
 struct dhcp_default_transport {
@@ -170,7 +171,12 @@ static bool _dhcp_default_transport_read_handler(struct l_io *io,
 				CMSG_LEN(sizeof(struct timeval))) {
 			const struct timeval *tv = (void *) CMSG_DATA(cmsg);
 
-			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
+			/*
+			 * Not ideal but this is our best approximation of the
+			 * CLOCK_BOOTTIME-based Rx time.
+			 */
+			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec -
+				_time_get_realtime_boottime_offset();
 		}
 	}
 
diff --git a/ell/dhcp6-transport.c b/ell/dhcp6-transport.c
index 13545b8..2bbdf8e 100644
--- a/ell/dhcp6-transport.c
+++ b/ell/dhcp6-transport.c
@@ -40,6 +40,7 @@
 #include "missing.h"
 #include "io.h"
 #include "time.h"
+#include "time-private.h"
 #include "dhcp6-private.h"
 
 struct dhcp6_default_transport {
@@ -81,9 +82,14 @@ static bool _dhcp6_default_transport_read_handler(struct l_io *io,
 				cmsg->cmsg_type == SCM_TIMESTAMP &&
 				cmsg->cmsg_len ==
 				CMSG_LEN(sizeof(struct timeval))) {
-		const struct timeval *tv = (void *) CMSG_DATA(cmsg);
-
-			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
+			const struct timeval *tv = (void *) CMSG_DATA(cmsg);
+
+			/*
+			 * Not ideal but this is our best approximation of the
+			 * CLOCK_BOOTTIME-based Rx time.
+			 */
+			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec -
+				_time_get_realtime_boottime_offset();
 		}
 	}
 
diff --git a/ell/icmp6.c b/ell/icmp6.c
index 96ba1ec..56019fc 100644
--- a/ell/icmp6.c
+++ b/ell/icmp6.c
@@ -249,7 +249,12 @@ static int icmp6_receive(int s, void *buf, size_t buf_len, struct in6_addr *src,
 				CMSG_LEN(sizeof(struct timeval))) {
 			const struct timeval *tv = (void *) CMSG_DATA(cmsg);
 
-			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec;
+			/*
+			 * Not ideal but this is our best approximation of the
+			 * CLOCK_BOOTTIME-based Rx time.
+			 */
+			timestamp = tv->tv_sec * L_USEC_PER_SEC + tv->tv_usec -
+				_time_get_realtime_boottime_offset();
 		}
 	}
 
diff --git a/ell/time-private.h b/ell/time-private.h
index dc4d6c2..8ff581b 100644
--- a/ell/time-private.h
+++ b/ell/time-private.h
@@ -23,3 +23,4 @@
 uint64_t _time_pick_interval_secs(uint32_t min_secs, uint32_t max_secs);
 uint64_t _time_fuzz_msecs(uint64_t ms);
 uint64_t _time_fuzz_secs(uint32_t secs, uint32_t max_offset);
+int64_t _time_get_realtime_boottime_offset(void);
diff --git a/ell/time.c b/ell/time.c
index d02dabe..cda09be 100644
--- a/ell/time.c
+++ b/ell/time.c
@@ -105,3 +105,20 @@ uint64_t _time_fuzz_secs(uint32_t secs, uint32_t max_offset)
 
 	return ms;
 }
+
+/*
+ * Get approximate difference between CLOCK_REALTIME and CLOCK_BOOTTIME,
+ * for converting recent timestamps from one to the other.  The value can
+ * change at any time.
+ */
+int64_t _time_get_realtime_boottime_offset(void)
+{
+	struct timespec boottime;
+	struct timespec realtime;
+
+	clock_gettime(CLOCK_BOOTTIME, &boottime);
+	clock_gettime(CLOCK_REALTIME, &realtime);
+
+	return ((int64_t) realtime.tv_sec - boottime.tv_sec) * 1000000 +
+		((int64_t) realtime.tv_nsec - boottime.tv_nsec) / 1000;
+}
-- 
2.32.0