[v7,0/3] y2038: Linux: Introduce __clock_settime64 function

  On Fri, Sep 6, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com> wrote:
>
> On Fri, 6 Sep 2019, Alistair Francis wrote:
>
> > Which I can fix with this diff:
>
> This diff is not the right way to fix this build failure.
>
> One of the design principles in the Y2038 support is that is __TIMESIZE ==
> 64, the time functions *aren't* trivial wrappers of time64 functions;
> rather, the time64 function definitions are remapped (via macros) so they
> define the function name with no "64".  For each case where there is a
> pair of functions (for different time_t types) in the __TIMESIZE == 32
> case, there should just be the one function when __TIMESIZE == 64.
>
> This ensures that the Y2038 changes don't add any overhead at all in the
> glibc binaries on existing platforms with __TIMESIZE == 64.
>
> You should look at exactly what the types in question are, that are being
> reported as conflicting in your build (probably by looking at preprocessed
> source).  __timespec64 and timespec are supposed to be the same type (via
> #define) when __TIMESIZE == 64, to avoid such incompatibilities.

Looking at the place where the__timespec64 is defined they aren't the
same for __TIMESIZE == 64 on a 32-bit system.

The code is below:

#if __WORDSIZE == 64 \
  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
# define __timespec64 timespec
#else
/* The glibc Y2038-proof struct __timespec64 structure for a time value.
   To keep things Posix-ish, we keep the nanoseconds field a 32-bit
   signed long, but since the Linux field is a 64-bit signed int, we
   pad our tv_nsec with a 32-bit int.

   As a general rule the Linux kernel is ignoring upper 32 bits of
   tv_nsec field.  */
struct __timespec64
{
  __time64_t tv_sec;         /* Seconds */
# if BYTE_ORDER == BIG_ENDIAN
  __int32_t tv_pad;          /* Padding */
  __int32_t tv_nsec;         /* Nanoseconds */
# else
  __int32_t tv_nsec;         /* Nanoseconds */
  __int32_t tv_pad;          /* Padding */
# endif
};
#endif

So looking at that with __TIMESIZE == 64 but __WORDSIZE == 32 (as on
RV32) we will specify a __timespec64 struct that is different to the
timespec struct.

Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned above?

This diff fixes the build for me:

 /* The glibc Y2038-proof struct __timespec64 structure for a time value.

Alistair

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
  

On Mon, 16 Sep 2019, Alistair Francis wrote:

> Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned above?

Those other conditions imply __TIMESIZE == 64.  So replacing the existing 
conditions by __TIMESIZE == 64 might be correct, but not adding it while 
leaving the existing (then redundant) conditions there as well.

I think you should replace the conditional by __TIMESIZE == 64 (if that 
works), which I think should be safe for the reasons explained below.  
There is a possibility that we end up needing two different internal names 
instead of the present one, but I don't think that should block changing 
the conditional at this point to make further progress unless that causes 
problems with the existing patches on existing configurations.

The design for defining 64-bit-time functions with *64 names and then 
remapping those names to the public ones via #define if __TIMESIZE == 64 
implies that the type used in declaring / defining *64 functions needs to 
be mapped to struct timespec if __TIMESIZE == 64.

But if you need to set padding explicitly to zero when passing 64-bit 
timespecs into the kernel (the case of kernels 5.1.0 through 5.1.4, on 
32-bit architectures for which those kernel versions had a corresponding 
64-bit architecture with compat syscall support), you then need a type 
with named padding (whereas the public struct timespec needs to have an 
unnamed bit-field for the padding to stay compatible with existing sources 
with { tv_sec, tv_nsec } initializers).

So this links into what things should look like at the API level inside 
glibc for dealing with zeroing padding, if we do that at all.  However, 
unless we fix x32 to use "long int" for tv_nsec we don't need to deal with 
zeroing padding for any existing system with __TIMESIZE == 64 (and I don't 
think it's likely to be relevant for future 32-bit ports that use 
__TIMESIZE == 64 either, because the kernel issue is only for compat 
syscalls for 32-bit binaries under 64-bit kernels, which I don't think are 
relevant to any 32-bit architectures supported in 5.1 kernels but not yet 
in glibc).
  

Hi Alistair, Joseph,

> On Fri, Sep 6, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com>
> wrote:
> >
> > On Fri, 6 Sep 2019, Alistair Francis wrote:
> >  
> > > Which I can fix with this diff:  
> >
> > This diff is not the right way to fix this build failure.
> >
> > One of the design principles in the Y2038 support is that is
> > __TIMESIZE == 64, the time functions *aren't* trivial wrappers of
> > time64 functions; rather, the time64 function definitions are
> > remapped (via macros) so they define the function name with no
> > "64".  For each case where there is a pair of functions (for
> > different time_t types) in the __TIMESIZE == 32 case, there should
> > just be the one function when __TIMESIZE == 64.
> >
> > This ensures that the Y2038 changes don't add any overhead at all
> > in the glibc binaries on existing platforms with __TIMESIZE == 64.
> >
> > You should look at exactly what the types in question are, that are
> > being reported as conflicting in your build (probably by looking at
> > preprocessed source).  __timespec64 and timespec are supposed to be
> > the same type (via #define) when __TIMESIZE == 64, to avoid such
> > incompatibilities.  
> 
> Looking at the place where the__timespec64 is defined they aren't the
> same for __TIMESIZE == 64 on a 32-bit system.
> 
> The code is below:
> 
> #if __WORDSIZE == 64 \
>   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
   ^^^^^^^^^^^^^ - [1]

> # define __timespec64 timespec
> #else
> /* The glibc Y2038-proof struct __timespec64 structure for a time
> value. To keep things Posix-ish, we keep the nanoseconds field a
> 32-bit signed long, but since the Linux field is a 64-bit signed int,
> we pad our tv_nsec with a 32-bit int.
> 
>    As a general rule the Linux kernel is ignoring upper 32 bits of
>    tv_nsec field.  */
> struct __timespec64
         ^^^^^^^^^^ - [3]

> {
>   __time64_t tv_sec;         /* Seconds */
> # if BYTE_ORDER == BIG_ENDIAN
>   __int32_t tv_pad;          /* Padding */
>   __int32_t tv_nsec;         /* Nanoseconds */
> # else
>   __int32_t tv_nsec;         /* Nanoseconds */
>   __int32_t tv_pad;          /* Padding */
> # endif
> };
> #endif
> 
> So looking at that with __TIMESIZE == 64 but __WORDSIZE == 32 (as on
> RV32) we will specify a __timespec64 struct that is different to the
> timespec struct.
> 
> Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned
> above?

I've replaced the condition [1] with plain #if __TIMESIZE == 64 [2], but
then there was a concern that: 

Replace __TIMESIZE with __WORDSIZE (as architectures with
__TIMESIZE==64 will need to use this struct with 32 bit tv_nsec field).


Alistair - I guess that the size of RISC-V's struct timespec tv_nsec
is 4? (as it shall be long tv_nsec; [4]) ?

Then if you replace the condition [1] with #if __TIMESIZE == 64 you
would have:

struct timespec
{
  __time_t tv_sec;		/* Seconds.  */
  __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
}

And then,

You would have tv_sec (8 bytes) and tv_nsec (4 bytes) [*]. Kernel's
clock_settime64 expects to receive two 8 bytes values.

In the best case you would leak glibc data to the kenel. In the worst
case kernel will modify this data, which may be some other struct's
field.

The condition [1] prevents from this for machines with __WORDSIZE == 32
(excluding x32).

Joseph is the above concern valid ?

> 
> This diff fixes the build for me:
> 
> diff --git a/include/time.h b/include/time.h
> index 5f7529f10e9..ff5f18ec56c 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -51,7 +51,8 @@ extern void __tz_compute (__time64_t timer, struct
> tm *tm, int use_localtime)
>    __THROW attribute_hidden;
> 
>  #if __WORDSIZE == 64 \
> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) || \
> +  __TIMESIZE == 64
>  # define __timespec64 timespec
>  #else
>  /* The glibc Y2038-proof struct __timespec64 structure for a time
> value.
> 
> Alistair
> 
> >
> > --
> > Joseph S. Myers
> > joseph@codesourcery.com  

Note:

[2] - https://patchwork.ozlabs.org/patch/1092580/
[4] - https://linux.die.net/man/3/clock_gettime
[*] - what is the output of sizeof(__syscall_slong_t) on RISCV 32 bit?

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On Tue, 17 Sep 2019, Lukasz Majewski wrote:

> Then if you replace the condition [1] with #if __TIMESIZE == 64 you
> would have:
> 
> struct timespec
> {
>   __time_t tv_sec;		/* Seconds.  */
>   __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
> }

The *public* struct timespec (defined in 
time/bits/types/struct_timespec.h) should be changed for ports that define 
__TIMESIZE == 64 while __SYSCALL_WORDSIZE == 32.

That is, if __TIMESIZE == 64, and if __SYSCALL_WORDSIZE (if defined) is 32 
or __WORDSIZE (if __SYSCALL_WORDSIZE is not defined), then struct timespec 
needs endian-dependent padding (defined as an *unnamed* 32-bit bit-field, 
so that it gets ignored for initializers).  (This is the same padding as 
would be needed in the case where __TIMESIZE == 32 but _TIME_BITS=64 is 
defined, but _TIME_BITS=64 support for headers comes later.)

RV32 has got away without that change to struct timespec because it's 
little-endian, and as long as __time_t is 8-byte-aligned implicit padding 
works as well as explicit in the little-endian case.  If BE, or if 8-byte 
__time_t is only 4-byte-aligned in structs (and so the struct ends up as 
12-byte without explicit padding), there would be problems.  I think it's 
cleanest to make the padding explicit even in the cases where in fact 
implicit padding would give the same layout.

RV32 does not need any support for clearing the padding before passing 
struct timespec to the kernel, because that's only relevant for compat 
syscalls in Linux 5.1.0 to 5.1.4 and the RISC-V kernel doesn't yet have 
compat syscall support for running RV32 binaries under RV64 kernels.
  

Hi Joseph, Alistair,

> On Tue, 17 Sep 2019, Lukasz Majewski wrote:
> 
> > Then if you replace the condition [1] with #if __TIMESIZE == 64 you
> > would have:
> > 
> > struct timespec
> > {
> >   __time_t tv_sec;		/* Seconds.  */
> >   __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
> > }  
> 
> The *public* struct timespec (defined in 
> time/bits/types/struct_timespec.h) should be changed for ports that
> define __TIMESIZE == 64 while __SYSCALL_WORDSIZE == 32.
> 
> That is, if __TIMESIZE == 64, and if __SYSCALL_WORDSIZE (if defined)
> is 32 or __WORDSIZE (if __SYSCALL_WORDSIZE is not defined), then
> struct timespec needs endian-dependent padding (defined as an
> *unnamed* 32-bit bit-field, so that it gets ignored for
> initializers). 

Ok, I will prepare proper patch with adjusting the *public* struct
timespec.

> (This is the same padding as would be needed in the
> case where __TIMESIZE == 32 but _TIME_BITS=64 is defined, but
> _TIME_BITS=64 support for headers comes later.)

As for example here [1].

Just for (my) confirmation:

- New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE ==
  64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64
  during the compilation. They will just get 64 bit time API support
  from the outset.

- Already supported 32 bits architectures (like armv7-a with __WORDSIZE
  == 32) will keep __TIMESIZE == 32 and require -D_TIME_BITS=64 for
  compilation. 
  After glibc sets the minimal supported kernel version to 5.1 and all
  conversions for syscalls to support 64 bit time API are done the
  __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will not be required
  anymore for compilation.

  Until the above switch happens we will redirect time calls - like in
  [2].

> 
> RV32 has got away without that change to struct timespec because it's 
> little-endian, and as long as __time_t is 8-byte-aligned implicit
> padding works as well as explicit in the little-endian case. 

Ok.

> If BE,
> or if 8-byte __time_t is only 4-byte-aligned in structs (and so the
> struct ends up as 12-byte without explicit padding), there would be
> problems. 

Yes.

> I think it's cleanest to make the padding explicit even in
> the cases where in fact implicit padding would give the same layout.

Ok. So then we shall keep the condition:

#if __WORDSIZE == 64 \
  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
# define __timespec64 timespec
#else

// struct __timespec64 with endian dependent, explicit padding and
// __time64_t tv_sec;

#fi

And RV32 shall use the explicitly defined struct __timespec64 (from
#else) as presented in [3] ?

> 
> RV32 does not need any support for clearing the padding before
> passing struct timespec to the kernel, because that's only relevant
> for compat syscalls in Linux 5.1.0 to 5.1.4 and the RISC-V kernel
> doesn't yet have compat syscall support for running RV32 binaries
> under RV64 kernels.

Ok. Thanks for clarification.

> 


Note:

[1] -
https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994#diff-4ddbc47d3262d4f00f3825e4f3627dbbR10

[2] -
https://github.com/lmajewski/y2038_glibc/commit/c6740c05ea9b224a552847d10402f98da8376994#diff-07934c1fe09f0e6357413e138856c786R225

[3] -
https://github.com/lmajewski/y2038_glibc/commit/926634e657fa5a927152bf7eb06a62e8468f75ae#diff-5b9f1c6457e0e10079f657f283c19861R53

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
  

On Tue, 17 Sep 2019, Lukasz Majewski wrote:

> - New 32 bits glibc ports (like RISC-V 32) will get __TIMESIZE ==
>   64 (__WORDSIZE == 32) and no need to define the -D_TIME_BITS=64
>   during the compilation. They will just get 64 bit time API support
>   from the outset.

Yes, at least if such ports wish to use 64-bit time; I don't think we've 
really discussed if we want to *require* 64-bit time for future ports 
(e.g. the next revised resubmissions of the ARC and NDS32 ports).  
Certainly the work required right now for ARC or NDS32 to use 64-bit time 
would be significantly more than the work for RV32 (because they also 
support older kernel versions without the 64-bit-time syscalls, so all the 
Y2038 work for fallback at runtime to older syscalls becomes relevant), 
unless they decide on 5.1 or later as minimum kernel version.

> - Already supported 32 bits architectures (like armv7-a with __WORDSIZE
>   == 32) will keep __TIMESIZE == 32 and require -D_TIME_BITS=64 for
>   compilation. 

Yes.

>   After glibc sets the minimal supported kernel version to 5.1 and all
>   conversions for syscalls to support 64 bit time API are done the
>   __TIMESIZE will be set to 64 and -D_TIME_BITS=64 will not be required
>   anymore for compilation.

No.  __TIMESIZE means the size of time_t in the unsuffixed ABIs in glibc, 
not the _TIME_BITS-dependent size of time_t in the current compilation.  
We hope in future to make _TIME_BITS=64 the default and only API supported 
for new compilations (which is independent of what the minimum kernel 
version is), but __TIMESIZE would still be 32, because the unsuffixed ABIs 
would remain compatible with existing binaries using 32-bit time.

> Ok. So then we shall keep the condition:
> 
> #if __WORDSIZE == 64 \
>   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> # define __timespec64 timespec
> #else

No.  __timespec64 should be defined to timespec whenever __TIMESIZE == 64.  
The timespec to which it is defined, in the public header, would gain 
padding.

The condition

#if __WORDSIZE == 64 \
  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)

is correct as a condition for struct timespec (in the public header) *not* 
to have padding.