diff mbox

[v8,3/3] y2038: linux: Provide __clock_settime64 implementation

Message ID 20190918211603.8444-4-lukma@denx.de
State New
Headers show

Commit Message

Lukasz Majewski Sept. 18, 2019, 9:16 p.m. UTC
This patch provides new __clock_settime64 explicit 64 bit function for
setting the time. Moreover, a 32 bit version - __clock_settime - has been
refactored to internally use __clock_settime64.

The __clock_settime is now supposed to be used on systems still supporting
32 bit time (__TIMESIZE != 64) - hence the necessary conversion to 64 bit
struct timespec.

The new clock_settime64 syscall available from Linux 5.1+ has been used,
when applicable.

In this patch the internal padding (tv_pad) of struct __timespec64 is
left untouched (on systems with __WORDSIZE == 32) as Linux kernel ignores
upper 32 bits of tv_nsec.

Tests:
- The code has been tested with x86_64/x86 (native compilation):
make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"

- Run specific tests on ARM/x86 32bit systems (qemu):
https://github.com/lmajewski/meta-y2038
and run tests:
https://github.com/lmajewski/y2038-tests/commits/master
on kernels with and without 64 bit time support.

No regressions were observed.

* include/time.h (__clock_settime64):
  Add __clock_settime alias according to __TIMESIZE define
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime):
  Refactor this function to be used only on 32 bit machines as a wrapper
  on __clock_settime64.
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime64): Add
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime64):
  Use clock_settime64 kernel syscall (available from 5.1+ Linux)


---
Changes for v8:
- None

Changes for v7:
- Revert changes to __clock_settime - as a result it now behaves as in v5
  (It is a wrapper on __clock_settime64).
- Fix the code to apply on newest master (after moving clock_settime to
  libc from librt).

Changes for v6:
- In the __clock_settime function do not call __clock_settime64 - just use
  the clock_settime 32 bit ABI syscall. Such approach will facilitate
  updating systems with __WORDSIZE==32 to be Y2038 safe by disabling for
  example clock_settime 32 bit syscall in the Linux kernel.

Changes for v5:
- Use __ASSUME_TIME64_SYSCALLS to indicate Linux kernel support for 64 bit
  time.
- Move the in_time_t_range() check to __clock_settime64
- Alias __NR_clock_settime64 to __NR_clock_settime if the former is not
  defined in the headers.

Changes for v4:
- __ASSUME_TIME64_SYSCALLS for fall back path
- Use __SYSCALL_WORDSIZE to exclude 'x32' from execution path (so it will
  use x86_64 syscall
- Rewrite the commit message

Changes for v3:
- Rename __ASSUME_64BIT_TIME to __ASSUME_TIME64_SYSCALLS
- Refactor in-code comment (add information regarding Linux kernel ignorance
  of padding
- Do not use __TIMESIZE to select main execution path (for Y2038 systems
  __TIMESIZE would be changed from 32 to 64 bits at some point to indicate
  full Y2038 support

Changes for v2:
- Add support for __ASSUME_64BIT_TIME flag when Linux kernel provides syscalls
  supporting 64 bit time on 32 bit systems
- Provide fallback to 32 bit version of clock_settime when clock_settime64
  is not available
- Do not copy *tp to timespec - this seems like an overkill as in clock_settime()
  the 32 bit struct timespec is copied to internal 64 bit struct __timespec64

# Conflicts:
#	sysdeps/unix/sysv/linux/clock_settime.c
---
 include/time.h                          |  8 ++++++
 sysdeps/unix/sysv/linux/clock_settime.c | 38 ++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 4 deletions(-)

Comments

Joseph Myers Sept. 18, 2019, 9:43 p.m. UTC | #1
On Wed, 18 Sep 2019, Lukasz Majewski wrote:

> Tests:
> - The code has been tested with x86_64/x86 (native compilation):
> make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> 
> - Run specific tests on ARM/x86 32bit systems (qemu):
> https://github.com/lmajewski/meta-y2038
> and run tests:
> https://github.com/lmajewski/y2038-tests/commits/master
> on kernels with and without 64 bit time support.

Could you give more details of the configurations (including kernel 
headers version, --enable-kernel version, kernel version used at runtime) 
for which you have built glibc and run the full glibc testsuite?  I 
suspect this code is pretty close to being ready to go in - but such 
patches have plenty of scope for mistakes (such as were in earlier RV32 
patch versions) such as typos, transposed parameters, etc., that are hard 
for humans to spot reliably and easy for compilers and testsuites to spot, 
so it's important to know that sufficient automated tests have been run to 
catch any such mistakes.

I gave a list in 
<https://sourceware.org/ml/libc-alpha/2019-08/msg00234.html> of five 
configurations I think are relevant to cover the different cases in 
patches such as this ("new" = 5.1 or later; Florian's changes to provide 
syscall tables in glibc would allow "#ifdef __NR_clock_settime64" to be 
removed and eliminate case (b)):

(a) one that has always had 64-bit time, e.g. x86_64;

(b) one with 32-bit time and old kernel headers (any kernel version at 
runtime);

(c) one with 32-bit time and new kernel headers, old kernel at runtime;

(d) one with 32-bit time and new kernel headers, new kernel at runtime but 
no --enable-kernel;

(e) one with 32-bit time and new kernel at runtime and new 
--enable-kernel.

If some of these are problematic to test, you can ask for help testing 
them.  For *this particular* patch you might not need to test both (c) and 
(d), because they are identical as far as compilation is concerned and the 
testsuite doesn't really cover execution of clock_settime anyway.  But 
it's in the nature of the Y2038 changes - involving lots of conditional 
code - that it's necessary to test in several different configurations to 
cover the conditionals adequately.

(For avoidance of doubt, it is *not* necessary to run build-many-glibcs.py 
for this patch, and nor would running build-many-glibcs.py be sufficient 
since it doesn't do execution testing or cover the kernel headers version 
and --enable-kernel variants.)
Lukasz Majewski Sept. 18, 2019, 10:33 p.m. UTC | #2
Hi Joseph,

> On Wed, 18 Sep 2019, Lukasz Majewski wrote:
> 
> > Tests:
> > - The code has been tested with x86_64/x86 (native compilation):
> > make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> > 
> > - Run specific tests on ARM/x86 32bit systems (qemu):
> > https://github.com/lmajewski/meta-y2038
> > and run tests:
> > https://github.com/lmajewski/y2038-tests/commits/master
> > on kernels with and without 64 bit time support.  
> 
> Could you give more details of the configurations (including kernel 
> headers version, --enable-kernel version, kernel version used at
> runtime) for which you have built glibc and run the full glibc
> testsuite?  I suspect this code is pretty close to being ready to go
> in - but such patches have plenty of scope for mistakes (such as were
> in earlier RV32 patch versions) such as typos, transposed parameters,
> etc., that are hard for humans to spot reliably and easy for
> compilers and testsuites to spot, so it's important to know that
> sufficient automated tests have been run to catch any such mistakes.
> 

So I tested this on ARM32 QEMU BSP build with meta-y2038 [1].

I. Build testing:
- Machine's
MACHINE=qemux86-64-x32
MACHINE=qemux86-64
MACHINE=qemux86
MACHINE=qemuarm

- Using glib's
test-wrapper='/opt/Y2038/glibc/src/scripts/cross-test-ssh.sh

for qemu ARM.

- ../src/scripts/build-many-glibcs.py


II. Runtime testing - with BSP [1]:
runqemu -d y2038arm nographic

- PREFERRED_VERSION_linux-y2038 = "5.1%" &&
  Y2038_GLIBC_MIN_KERNEL_VERSION="5.1.0"
  --enable-kernel=${Y2038_GLIBC_MIN_KERNEL_VERSION}

  Linux y2038arm 5.1.21-y2038-4a9b1eb8bc3ba4ad8b3b1aa3317cf8d4a3aaad83

  (Support __ASSUME_TIME64_SYSCALLS defined)

- Only PREFERRED_VERSION_linux-y2038 = "5.1%" - the minimal kernel
  version is default one for current mainline glibc

  (The __ASSUME_TIME64_SYSCALLS not defined, but kernel supports
  __clock_settime64 syscalls).

- PREFERRED_VERSION_linux-lts = "4.19%"
  with default minimal kernel version for contemporary glibc
  (SHA1: 87accae3978c77c1a50d19ea8e3da3f0248d2612)

  This kernel doesn't support __clock_settime64 syscalls, so we
  fallback to clock_settime.

Above tests are performed with Y2038 redirection applied [2] as well as
without (so the __TIMESIZE != 64 execution path is checked as well).

III. Syscalls unit tests - test_y2038 program [3] installed on BSP.

IV. For x86_64 I do run the - as I can do it on my HOST PC

make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"

(before and after the patchset).

> I gave a list in 
> <https://sourceware.org/ml/libc-alpha/2019-08/msg00234.html> of five 
> configurations I think are relevant to cover the different cases in 
> patches such as this ("new" = 5.1 or later; Florian's changes to
> provide syscall tables in glibc would allow "#ifdef
> __NR_clock_settime64" to be removed and eliminate case (b)):
> 
> (a) one that has always had 64-bit time, e.g. x86_64;
> 

Point IV.

> (b) one with 32-bit time and old kernel headers (any kernel version
> at runtime);

Point II.

> 
> (c) one with 32-bit time and new kernel headers, old kernel at
> runtime;
> 
> (d) one with 32-bit time and new kernel headers, new kernel at
> runtime but no --enable-kernel;
> 
> (e) one with 32-bit time and new kernel at runtime and new 
> --enable-kernel.

Point II.

> 

I will double check if the above points are in sync with points a) to
e).

> If some of these are problematic to test, you can ask for help
> testing them.  For *this particular* patch you might not need to test
> both (c) and (d), because they are identical as far as compilation is
> concerned and the testsuite doesn't really cover execution of
> clock_settime anyway.  But it's in the nature of the Y2038 changes -
> involving lots of conditional code - that it's necessary to test in
> several different configurations to cover the conditionals adequately.

QEMU (and OE/Yocto) helps here ...

> 
> (For avoidance of doubt, it is *not* necessary to run
> build-many-glibcs.py for this patch, and nor would running
> build-many-glibcs.py be sufficient since it doesn't do execution
> testing or cover the kernel headers version and --enable-kernel
> variants.)
> 

The build-many-glibcs helps with checking if there aren't some odd
build breaks. It is one of many test approaches for testing the Y2038
problem.

Note:

[1] - https://github.com/lmajewski/meta-y2038
[2] -
https://github.com/lmajewski/y2038_glibc/commit/1229b54508d0bb130a017a5b5591209167255665
[3] - https://github.com/lmajewski/y2038-tests/commits/master

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
Lukasz Majewski Sept. 19, 2019, 10 p.m. UTC | #3
Hi Joseph,

> Hi Joseph,
> 
> > On Wed, 18 Sep 2019, Lukasz Majewski wrote:
> >   
> > > Tests:
> > > - The code has been tested with x86_64/x86 (native compilation):
> > > make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> > > 
> > > - Run specific tests on ARM/x86 32bit systems (qemu):
> > > https://github.com/lmajewski/meta-y2038
> > > and run tests:
> > > https://github.com/lmajewski/y2038-tests/commits/master
> > > on kernels with and without 64 bit time support.    
> > 
> > Could you give more details of the configurations (including kernel 
> > headers version, --enable-kernel version, kernel version used at
> > runtime) for which you have built glibc and run the full glibc
> > testsuite?  I suspect this code is pretty close to being ready to go
> > in - but such patches have plenty of scope for mistakes (such as
> > were in earlier RV32 patch versions) such as typos, transposed
> > parameters, etc., that are hard for humans to spot reliably and
> > easy for compilers and testsuites to spot, so it's important to
> > know that sufficient automated tests have been run to catch any
> > such mistakes. 
> 
> So I tested this on ARM32 QEMU BSP build with meta-y2038 [1].
> 
> I. Build testing:
> - Machine's
> MACHINE=qemux86-64-x32
> MACHINE=qemux86-64
> MACHINE=qemux86
> MACHINE=qemuarm
> 
> - Using glib's
> test-wrapper='/opt/Y2038/glibc/src/scripts/cross-test-ssh.sh
> 
> for qemu ARM.
> 
> - ../src/scripts/build-many-glibcs.py
> 
> 
> II. Runtime testing - with BSP [1]:
> runqemu -d y2038arm nographic
> 
> - PREFERRED_VERSION_linux-y2038 = "5.1%" &&
>   Y2038_GLIBC_MIN_KERNEL_VERSION="5.1.0"
>   --enable-kernel=${Y2038_GLIBC_MIN_KERNEL_VERSION}
> 
>   Linux y2038arm 5.1.21-y2038-4a9b1eb8bc3ba4ad8b3b1aa3317cf8d4a3aaad83
> 
>   (Support __ASSUME_TIME64_SYSCALLS defined)
> 
> - Only PREFERRED_VERSION_linux-y2038 = "5.1%" - the minimal kernel
>   version is default one for current mainline glibc
> 
>   (The __ASSUME_TIME64_SYSCALLS not defined, but kernel supports
>   __clock_settime64 syscalls).
> 
> - PREFERRED_VERSION_linux-lts = "4.19%"
>   with default minimal kernel version for contemporary glibc
>   (SHA1: 87accae3978c77c1a50d19ea8e3da3f0248d2612)
> 
>   This kernel doesn't support __clock_settime64 syscalls, so we
>   fallback to clock_settime.
> 
> Above tests are performed with Y2038 redirection applied [2] as well
> as without (so the __TIMESIZE != 64 execution path is checked as
> well).
> 
> III. Syscalls unit tests - test_y2038 program [3] installed on BSP.
> 
> IV. For x86_64 I do run the - as I can do it on my HOST PC
> 
> make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"
> 
> (before and after the patchset).
> 
> > I gave a list in 
> > <https://sourceware.org/ml/libc-alpha/2019-08/msg00234.html> of
> > five configurations I think are relevant to cover the different
> > cases in patches such as this ("new" = 5.1 or later; Florian's
> > changes to provide syscall tables in glibc would allow "#ifdef
> > __NR_clock_settime64" to be removed and eliminate case (b)):
> > 
> > (a) one that has always had 64-bit time, e.g. x86_64;
> >   
> 
> Point IV.

For this point I've only used make && make xcheck

> 
> > (b) one with 32-bit time and old kernel headers (any kernel version
> > at runtime);  
> 
> Point II.
> 
> > 
> > (c) one with 32-bit time and new kernel headers, old kernel at
> > runtime;
> > 
> > (d) one with 32-bit time and new kernel headers, new kernel at
> > runtime but no --enable-kernel;

Point c) and d) test scenario as described:

https://github.com/lmajewski/meta-y2038


> > 
> > (e) one with 32-bit time and new kernel at runtime and new 
> > --enable-kernel.  
> 
> Point II.
> 
> >   
> 
> I will double check if the above points are in sync with points a) to
> e).
> 
> > If some of these are problematic to test, you can ask for help
> > testing them.  For *this particular* patch you might not need to
> > test both (c) and (d), because they are identical as far as
> > compilation is concerned and the testsuite doesn't really cover
> > execution of clock_settime anyway.  But it's in the nature of the
> > Y2038 changes - involving lots of conditional code - that it's
> > necessary to test in several different configurations to cover the
> > conditionals adequately.  
> 
> QEMU (and OE/Yocto) helps here ...
> 
> > 
> > (For avoidance of doubt, it is *not* necessary to run
> > build-many-glibcs.py for this patch, and nor would running
> > build-many-glibcs.py be sufficient since it doesn't do execution
> > testing or cover the kernel headers version and --enable-kernel
> > variants.)
> >   
> 
> The build-many-glibcs helps with checking if there aren't some odd
> build breaks. It is one of many test approaches for testing the Y2038
> problem.
> 
> Note:
> 
> [1] - https://github.com/lmajewski/meta-y2038
> [2] -
> https://github.com/lmajewski/y2038_glibc/commit/1229b54508d0bb130a017a5b5591209167255665
> [3] - https://github.com/lmajewski/y2038-tests/commits/master
> 
> 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




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
diff mbox

Patch

diff --git a/include/time.h b/include/time.h
index 64b5468115..91f6280eb4 100644
--- a/include/time.h
+++ b/include/time.h
@@ -124,6 +124,14 @@  extern __time64_t __timegm64 (struct tm *__tp) __THROW;
 libc_hidden_proto (__timegm64)
 #endif
 
+#if __TIMESIZE == 64
+# define __clock_settime64 __clock_settime
+#else
+extern int __clock_settime64 (clockid_t clock_id,
+                              const struct __timespec64 *tp);
+libc_hidden_proto (__clock_settime64)
+#endif
+
 /* Compute the `struct tm' representation of T,
    offset OFFSET seconds east of UTC,
    and store year, yday, mon, mday, wday, hour, min, sec into *TP.
diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c
index f76178e0f6..1295bb5603 100644
--- a/sysdeps/unix/sysv/linux/clock_settime.c
+++ b/sysdeps/unix/sysv/linux/clock_settime.c
@@ -20,11 +20,9 @@ 
 #include <time.h>
 #include <shlib-compat.h>
 
-#include "kernel-posix-cpu-timers.h"
-
 /* Set CLOCK to value TP.  */
 int
-__clock_settime (clockid_t clock_id, const struct timespec *tp)
+__clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
 {
   /* Make sure the time cvalue is OK.  */
   if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000)
@@ -33,8 +31,40 @@  __clock_settime (clockid_t clock_id, const struct timespec *tp)
       return -1;
     }
 
-  return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
+#ifdef __ASSUME_TIME64_SYSCALLS
+# ifndef __NR_clock_settime64
+#  define __NR_clock_settime64 __NR_clock_settime
+# endif
+  return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
+#else
+# ifdef __NR_clock_settime64
+  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
+  if (ret == 0 || errno != ENOSYS)
+    return ret;
+# endif
+  if (! in_time_t_range (tp->tv_sec))
+    {
+      __set_errno (EOVERFLOW);
+      return -1;
+    }
+
+  struct timespec ts32;
+  valid_timespec64_to_timespec (tp, &ts32);
+  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
+#endif
 }
+
+#if __TIMESIZE != 64
+int
+__clock_settime (clockid_t clock_id, const struct timespec *tp)
+{
+  struct __timespec64 ts64;
+
+  valid_timespec_to_timespec64 (tp, &ts64);
+  return __clock_settime64 (clock_id, &ts64);
+}
+#endif
+
 libc_hidden_def (__clock_settime)
 
 versioned_symbol (libc, __clock_settime, clock_settime, GLIBC_2_17);