On Sat, Jul 7, 2018 at 7:42 AM, Deepa Dinamani deepa.kernel@gmail.com wrote:
struct timex uses struct timeval internally. struct timeval is not y2038 safe. Introduce a new UAPI type struct __kernel_timex that is y2038 safe.
struct __kernel_timex uses a timeval type that is similar to struct __kernel_timespec which preserves the same structure size across 32 bit and 64 bit ABIs. struct __kernel_timex also restructures other members of the structure to make the structure the same on 64 bit and 32 bit architectures. Note that struct __kernel_timex is the same as struct timex on a 64 bit architecture.
The above solution is similar to other new y2038 syscalls that are being introduced: both 32 bit and 64 bit ABIs have a common entry, and the compat entry supports the old 32 bit syscall interface.
Alternatives considered were:
Add new time type to struct timex that makes use of padded bits. This time type could be based on the struct __kernel_timespec. modes will use a flag to notify which time structure should be used internally. This needs some application level changes on both 64 bit and 32 bit architectures. Although 64 bit machines could continue to use the older timeval structure without any changes.
Add a new u8 type to struct timex that makes use of padded bits. This can be used to save higher order tv_sec bits. modes will use a flag to notify presence of such a type. This will need some application level changes on 32 bit architectures.
Add a new compat_timex structure that differs in only the size of the time type; keep rest of struct timex the same. This requires extra syscalls to manage all 3 cases on 64 bit architectures. This will not need any application level changes but will add more complexity from kernel side.
Hi Deepa,
sorry for taking so long with my reply
I've done a similar patch for the itimerval and the rusuage structures that have the same basic problem of embedding a timeval, and after a lot of back-and-forth on my end, I came to the same conclusion with the structure layout: using a single binary layout that is shared between 32-bit and 64-bit ABIs is better than any of the other approaches that one of us tried.
However, I'm still undecided about how to exactly put that in the uapi headers.
#define ADJ_ADJTIME 0x8000 /* switch between adjtime/adjtimex modes */ diff --git a/include/uapi/linux/timex.h b/include/uapi/linux/timex.h index 92685d826444..a1c6b73016a5 100644 --- a/include/uapi/linux/timex.h +++ b/include/uapi/linux/timex.h @@ -92,6 +92,47 @@ struct timex { int :32; int :32; int :32; };
+struct __kernel_timex_timeval {
__kernel_time64_t tv_sec;long long tv_usec;+};
+#ifndef __kernel_timex +struct __kernel_timex {
unsigned int modes; /* mode selector */int :32; /* pad */long long offset; /* time offset (usec) */long long freq; /* frequency offset (scaled ppm) */
The main disadvantage here is that a a typical ntp daemon that includes this header will now call the new system call, but still see the old structure definition that no longer matches, unless one modifies it to use __kernel_timex.
I checked the most important libc implementations to see how they pass this structure to user space:
glibc, musl, uclibc: sys/timex.h contains a private version of this structure, the kernel header is not included
bionic: sys/timex.h includes linux/timex.h, which is shipped with the libc and generated from kernel headers
klibc: there is no sys/timex.h, but also no adjtimex()/clock_adjtime() syscall
In short, this probably only really matters for bionic, and of course only if they choose to even support 64-bit time_t (which in turn requires other changes to the libc)
I can see two possible ways to define 'struct timex' in the kernel in a way that always matches the respective adjtimex.
a) rename the old timex to __kernel_old_timex, and add a snippet that will #define one of the two to be called 'struct timex' again:
#ifdef __USE_TIME_BITS64 #define __kernel_old_timex timex #define __kernel_old_timex_timeval timeval #else #define __kernel_timex timex #define __kernel_timex_timeval timeval #endif
b) use only one definition in the header file, but use configuration dependent types, don't rename timex to __kernel_timex, and only use compat_timex for the old 32-bit version inside of the kernel:
/* for fields that are the same size as time_t */ #ifdef __KERNEL__ typedef __s64 timex_long_t; #else typedef time_t timex_long_t; #endif
struct timex { unsigned int modes; /* mode selector */ char __pad1[sizeof(timex_long_t) - sizeof(int); __timex_long_t offset; /* time offset (usec) */ __timex_long_t freq; /* frequency offset (scaled ppm) */ __timex_long_t maxerror; /* maximum error (usec) */ __timex_long_t esterror; /* estimated error (usec) */ int status; /* clock command/status */ char __pad1[sizeof(timex_long_t) - sizeof(int); __timex_long_t constant; /* pll time constant */ ... };
Both a) and b) should work fine with bionic and any application that includes linux/timex.h instead of sys/timex.h
Arnd