Re: [PATCH v3 5/7] time: Add struct __kernel_timex

[Arnd Bergmann]

On Sat, Jul 7, 2018 at 7:42 AM, Deepa Dinamani <deepa.kernel@xxxxxxxxx> 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:
> 1. 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.
>
> 2. 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.
>
> 3. 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