Message ID | 87czrqf5un.fsf@oldenburg.str.redhat.com |
---|---|
State | New |
Headers | show |
Series | Linux: Use 32-bit vDSO for clock_gettime, gettimeofday, time (bug 28071) | expand |
On 10/07/2021 14:15, Florian Weimer via Libc-alpha wrote: > The previous approach defeats the vDSO optimization on older kernels > because a failing clock_gettime64 system call is performed on every > function call. It also results in a clobbered errno value, exposing > an OpenJDK bug (JDK-8270244). > > The existing layering is preserved for __ASSUME_TIME64_SYSCALLS. > Otherwise, 32-bit time and gettimeofday use 32-bit clock_gettime, > and clock_gettime is implemented using the vDSO. > > Tested on i686-linux-gnu (with a time64 and non-time64 kernel), > x86_64-linux-gnu. Built with build-many-glibcs.py. > > --- > sysdeps/unix/sysv/linux/Makefile | 9 +++- > sysdeps/unix/sysv/linux/clock_gettime.c | 8 +++ > sysdeps/unix/sysv/linux/gettimeofday.c | 21 +++++--- > sysdeps/unix/sysv/linux/time.c | 22 ++++++--- > .../unix/sysv/linux/tst-clock_gettime-clobber.c | 57 ++++++++++++++++++++++ > sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c | 37 ++++++++++++++ > sysdeps/unix/sysv/linux/tst-time-clobber.c | 36 ++++++++++++++ > 7 files changed, 173 insertions(+), 17 deletions(-) > > diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile > index 7d43dc95f2..9f00e53de4 100644 > --- a/sysdeps/unix/sysv/linux/Makefile > +++ b/sysdeps/unix/sysv/linux/Makefile > @@ -213,7 +213,14 @@ ifeq ($(subdir),time) > sysdep_headers += sys/timex.h bits/timex.h > > sysdep_routines += ntp_gettime ntp_gettimex > -endif > + > +tests += \ > + tst-clock_gettime-clobber \ > + tst-gettimeofday-clobber \ > + tst-time-clobber \ > + # tests > + > +endif# $(subdir) == time > > ifeq ($(subdir),signal) > tests-special += $(objpfx)tst-signal-numbers.out > diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c > index cfe9370455..86a72ecf5a 100644 > --- a/sysdeps/unix/sysv/linux/clock_gettime.c > +++ b/sysdeps/unix/sysv/linux/clock_gettime.c > @@ -64,6 +64,7 @@ libc_hidden_def (__clock_gettime64) > int > __clock_gettime (clockid_t clock_id, struct timespec *tp) > { > +# ifdef __ASSUME_TIME64_SYSCALLS > int ret; > struct __timespec64 tp64; > > @@ -81,6 +82,13 @@ __clock_gettime (clockid_t clock_id, struct timespec *tp) > } > > return ret; > +# else /* !__ASSUME_TIME64_SYSCALLS */ > +# ifdef HAVE_CLOCK_GETTIME_VSYSCALL > + return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); > +# else > + return INLINE_SYSCALL_CALL (clock_gettime, clock_id, tp); > +# endif > +# endif /* !__ASSUME_TIME64_SYSCALLS */ > } > #endif > libc_hidden_def (__clock_gettime) This does not fix the issue for __ASSUME_TIME64_SYSCALLS where it still uses INLINE_SYSCALL_CALL which might clobber the errno, besides adding another ifdef code path (which I really want to avoid). Instead I think we need to open-coded the INLINE_VSYSCALL macro and replace all INLINE_SYSCALL_CALL with INTERNAL_SYSCALL_CALLS: /* Get current value of CLOCK and store it in TP. */ int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp) { int r; #ifdef HAVE_CLOCK_GETTIME64_VSYSCALL int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp) = GLRO(dl_vdso_clock_gettime64); if (vdso_time64 != NULL) { r = INTERNAL_VSYSCALL_CALL (vdso_time64, 2, clock_id, tp); if (r == 0) return 0; return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); } #endif #ifdef HAVE_CLOCK_GETTIME_VSYSCALL int (*vdso_time)(clockid_t clock_id, struct timespec *tp) = GLRO(dl_vdso_clock_gettime); if (vdso_time != NULL) { struct timespec tp32; r = INTERNAL_VSYSCALL_CALL (vdso_time, 2, clock_id, &tp32); if (r == 0) { *tp = valid_timespec_to_timespec64 (tp32); return 0; } return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); } #endif r = INTERNAL_SYSCALL_CALL (clock_gettime64, clock_id, tp); if (r == 0) return 0; if (-r != ENOSYS) return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); #ifndef __ASSUME_TIME64_SYSCALLS /* Fallback code that uses 32-bit support. */ struct timespec tp32; r = INTERNAL_SYSCALL_CALL (clock_gettime, clock_id, &tp32); if (r == 0) { *tp = valid_timespec_to_timespec64 (tp32); return 0; } #endif return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); } Keep in mind the previous code preferred 64-bit syscall for the case where the kernel provides 64-bit time_t syscalls *and* also a 32-bit vDSO (in this case the *64-bit* syscall should be preferable over the vDSO). I do not know if this is still the case for most of architectures, maybe we should ignore this scenario and assume that if the architecture provided a clock_gettime vDSO and ahs 64-bit support it would also provide a 64-bit vDSO clock_gettime. I have tested the above with the clobber tests on this patch on a kernel v5.11 (64-bit vDSO), v4.4 (32-bit vDSO), and 3.10 (no vDSO). > diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c > index 4197be5656..1b353956ea 100644 > --- a/sysdeps/unix/sysv/linux/gettimeofday.c > +++ b/sysdeps/unix/sysv/linux/gettimeofday.c > @@ -16,18 +16,17 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > +#include <dl-vdso.h> > +#include <libc-vdso.h> > #include <time.h> > #include <string.h> > +#include <sysdep-vdso.h> > > /* Optimize the function call by setting the PLT directly to vDSO symbol. */ > #ifdef USE_IFUNC_GETTIMEOFDAY > # include <sysdep.h> > -# include <sysdep-vdso.h> > > # ifdef SHARED > -# include <dl-vdso.h> > -# include <libc-vdso.h> > - > static int > __gettimeofday_syscall (struct timeval *restrict tv, void *restrict tz) > { > @@ -54,7 +53,7 @@ __gettimeofday (struct timeval *restrict tv, void *restrict tz) > } > # endif > weak_alias (__gettimeofday, gettimeofday) > -#else /* USE_IFUNC_GETTIMEOFDAY */ > +#else /* !USE_IFUNC_GETTIMEOFDAY */ > /* Conversion of gettimeofday function to support 64 bit time on archs > with __WORDSIZE == 32 and __TIMESIZE == 32/64 */ > #include <errno.h> > @@ -73,9 +72,12 @@ __gettimeofday64 (struct __timeval64 *restrict tv, void *restrict tz) > return 0; > } > > -# if __TIMESIZE != 64 > +# if __TIMESIZE == 64 > +weak_alias (__gettimeofday, gettimeofday) > +# else /* __TIMESIZE != 64 */ > libc_hidden_def (__gettimeofday64) > > +# ifdef __ASSUME_TIME64_SYSCALL > int > __gettimeofday (struct timeval *restrict tv, void *restrict tz) > { > @@ -92,6 +94,9 @@ __gettimeofday (struct timeval *restrict tv, void *restrict tz) > *tv = valid_timeval64_to_timeval (tv64); > return 0; > } > -# endif > weak_alias (__gettimeofday, gettimeofday) > -#endif > +# else /* !__ASSUME_TIME64_SYSCALL */ > +# include <time/gettimeofday.c> > +# endif /* !__ASSUME_TIME64_SYSCALL */ > +# endif > +#endif /* !USE_IFUNC_GETTIMEOFDAY */ By making clock_gettime not clobbering the errno in case of success as above there is no need to change Linux gettimeofday (since it only sets errno for the case of EOVERFLOW). > diff --git a/sysdeps/unix/sysv/linux/time.c b/sysdeps/unix/sysv/linux/time.c > index 43fec7d3a7..b194127691 100644 > --- a/sysdeps/unix/sysv/linux/time.c > +++ b/sysdeps/unix/sysv/linux/time.c > @@ -16,16 +16,16 @@ > License along with the GNU C Library; if not, see > <https://www.gnu.org/licenses/>. */ > > +#include <dl-vdso.h> > +#include <libc-vdso.h> > +#include <sysdep-vdso.h> > + > /* Optimize the function call by setting the PLT directly to vDSO symbol. */ > #ifdef USE_IFUNC_TIME > # include <time.h> > # include <sysdep.h> > -# include <sysdep-vdso.h> > > #ifdef SHARED > -# include <dl-vdso.h> > -# include <libc-vdso.h> > - > static time_t > time_syscall (time_t *t) > { > @@ -46,7 +46,7 @@ time (time_t *t) > return INLINE_VSYSCALL (time, 1, t); > } > # endif /* !SHARED */ > -#else /* USE_IFUNC_TIME */ > +#else /* !USE_IFUNC_TIME */ > # include <time.h> > # include <time-clockid.h> > # include <errno.h> > @@ -63,10 +63,13 @@ __time64 (__time64_t *timer) > *timer = ts.tv_sec; > return ts.tv_sec; > } > +# if __TIMESIZE == 64 > +weak_alias (__time, time) > > -# if __TIMESIZE != 64 > +# else /* __TIMESIZE != 64 */ > libc_hidden_def (__time64) > > +# ifdef __ASSUME_TIME64_SYSCALL > time_t > __time (time_t *timer) > { > @@ -82,6 +85,9 @@ __time (time_t *timer) > *timer = t; > return t; > } > -# endif > weak_alias (__time, time) > -#endif > +# else /* !__ASSUME_TIME64_SYSCALL */ > +# include <time/time.c> > +# endif /* !__ASSUME_TIME64_SYSCALL */ > +# endif /* __TIMESIZE != 64 */ > +#endif /* !USE_IFUNC_TIME */ Same for time. > diff --git a/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c b/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c > new file mode 100644 > index 0000000000..81a32bd15a > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c > @@ -0,0 +1,57 @@ > +/* Check that clock_gettime does not clobber errno on success. > + Copyright (C) 2021 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <time.h> > +#include <support/check.h> > +#include <stdio.h> > + > +static void > +test_clock (clockid_t clk) > +{ > + printf ("info: testing clock: %d\n", (int) clk); > + > + for (int original_errno = 0; original_errno < 2; ++original_errno) > + { > + errno = original_errno; > + struct timespec ts; > + if (clock_gettime (clk, &ts) == 0) > + TEST_COMPARE (errno, original_errno); > + } > +} > + > +static int > +do_test (void) > +{ > + test_clock (CLOCK_BOOTTIME); > + test_clock (CLOCK_BOOTTIME_ALARM); > + test_clock (CLOCK_MONOTONIC); > + test_clock (CLOCK_MONOTONIC_COARSE); > + test_clock (CLOCK_MONOTONIC_RAW); > + test_clock (CLOCK_PROCESS_CPUTIME_ID); > + test_clock (CLOCK_REALTIME); > + test_clock (CLOCK_REALTIME_ALARM); > + test_clock (CLOCK_REALTIME_COARSE); > +#ifdef CLOCK_TAI > + test_clock (CLOCK_TAI); > +#endif > + test_clock (CLOCK_THREAD_CPUTIME_ID); > + return 0; > +} > + > +#include <support/test-driver.c> Ok. > diff --git a/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c b/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c > new file mode 100644 > index 0000000000..bb9bdb67b0 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c > @@ -0,0 +1,37 @@ > +/* Check that gettimeofday does not clobber errno. > + Copyright (C) 2021 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <stddef.h> > +#include <support/check.h> > +#include <sys/time.h> > + > +static int > +do_test (void) > +{ > + for (int original_errno = 0; original_errno < 2; ++original_errno) > + { > + errno = original_errno; > + struct timeval tv; > + gettimeofday (&tv, NULL); > + TEST_COMPARE (errno, original_errno); > + } > + return 0; > +} > + > +#include <support/test-driver.c> Ok. > diff --git a/sysdeps/unix/sysv/linux/tst-time-clobber.c b/sysdeps/unix/sysv/linux/tst-time-clobber.c > new file mode 100644 > index 0000000000..ad83be9be8 > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tst-time-clobber.c > @@ -0,0 +1,36 @@ > +/* Check that time does not clobber errno. > + Copyright (C) 2021 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +#include <errno.h> > +#include <stddef.h> > +#include <support/check.h> > +#include <time.h> > + > +static int > +do_test (void) > +{ > + for (int original_errno = 0; original_errno < 2; ++original_errno) > + { > + errno = original_errno; > + time (NULL); > + TEST_COMPARE (errno, original_errno); > + } > + return 0; > +} > + > +#include <support/test-driver.c> > Ok.
* Adhemerval Zanella: > This does not fix the issue for __ASSUME_TIME64_SYSCALLS where it still uses > INLINE_SYSCALL_CALL which might clobber the errno, besides adding another > ifdef code path (which I really want to avoid). Instead I think we need to > open-coded the INLINE_VSYSCALL macro and replace all INLINE_SYSCALL_CALL > with INTERNAL_SYSCALL_CALLS: But for __ASSUME_TIME64_SYSCALLS, clock_gettime64 will not fail. What am I missing? Is the issue that INLINE_VSYSCALL may set ENOSYS artificially? Thanks, Florian
On 10/07/2021 15:54, Florian Weimer wrote: > * Adhemerval Zanella: > >> This does not fix the issue for __ASSUME_TIME64_SYSCALLS where it still uses >> INLINE_SYSCALL_CALL which might clobber the errno, besides adding another >> ifdef code path (which I really want to avoid). Instead I think we need to >> open-coded the INLINE_VSYSCALL macro and replace all INLINE_SYSCALL_CALL >> with INTERNAL_SYSCALL_CALLS: > > But for __ASSUME_TIME64_SYSCALLS, clock_gettime64 will not fail. > > What am I missing? Is the issue that INLINE_VSYSCALL may set ENOSYS > artificially? I meant for __clock_gettime64, where it may still clobber the errno on older kernels (although it might be a fringe case). In any case, I still think making all time32 call to call time64 is a better implementation than add some specific calls for time32.
* Adhemerval Zanella: > On 10/07/2021 15:54, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> This does not fix the issue for __ASSUME_TIME64_SYSCALLS where it still uses >>> INLINE_SYSCALL_CALL which might clobber the errno, besides adding another >>> ifdef code path (which I really want to avoid). Instead I think we need to >>> open-coded the INLINE_VSYSCALL macro and replace all INLINE_SYSCALL_CALL >>> with INTERNAL_SYSCALL_CALLS: >> >> But for __ASSUME_TIME64_SYSCALLS, clock_gettime64 will not fail. >> >> What am I missing? Is the issue that INLINE_VSYSCALL may set ENOSYS >> artificially? > > I meant for __clock_gettime64, where it may still clobber the errno > on older kernels (although it might be a fringe case). In any case, > I still think making all time32 call to call time64 is a better > implementation than add some specific calls for time32. So do you want to send an alternative patch? I can add my tests on top of that. Thanks, Florian
On 10/07/2021 17:00, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 10/07/2021 15:54, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> This does not fix the issue for __ASSUME_TIME64_SYSCALLS where it still uses >>>> INLINE_SYSCALL_CALL which might clobber the errno, besides adding another >>>> ifdef code path (which I really want to avoid). Instead I think we need to >>>> open-coded the INLINE_VSYSCALL macro and replace all INLINE_SYSCALL_CALL >>>> with INTERNAL_SYSCALL_CALLS: >>> >>> But for __ASSUME_TIME64_SYSCALLS, clock_gettime64 will not fail. >>> >>> What am I missing? Is the issue that INLINE_VSYSCALL may set ENOSYS >>> artificially? >> >> I meant for __clock_gettime64, where it may still clobber the errno >> on older kernels (although it might be a fringe case). In any case, >> I still think making all time32 call to call time64 is a better >> implementation than add some specific calls for time32. > > So do you want to send an alternative patch? I can add my tests on top > of that. > Let me prepare one.
On 10/07/2021 17:00, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 10/07/2021 15:54, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> This does not fix the issue for __ASSUME_TIME64_SYSCALLS where it still uses >>>> INLINE_SYSCALL_CALL which might clobber the errno, besides adding another >>>> ifdef code path (which I really want to avoid). Instead I think we need to >>>> open-coded the INLINE_VSYSCALL macro and replace all INLINE_SYSCALL_CALL >>>> with INTERNAL_SYSCALL_CALLS: >>> >>> But for __ASSUME_TIME64_SYSCALLS, clock_gettime64 will not fail. >>> >>> What am I missing? Is the issue that INLINE_VSYSCALL may set ENOSYS >>> artificially? >> >> I meant for __clock_gettime64, where it may still clobber the errno >> on older kernels (although it might be a fringe case). In any case, >> I still think making all time32 call to call time64 is a better >> implementation than add some specific calls for time32. > > So do you want to send an alternative patch? I can add my tests on top > of that. Could you check the following? I checked on a 5.11 kernel (64-bit vDSO), 4.4 kernel (32-bit vDSO without CLOCK_MONOTONIC support) and on a 3.10 kernel (no vDSO support). Using the test (a slight modified one from the bug report): -- #include <time.h> #include <stdio.h> #include <errno.h> int main (void) { struct timespec ts; errno = 0; clock_gettime (CLOCK_REALTIME, &ts); printf ("errno = %m (%d)\n", errno); errno = 0; clock_gettime (CLOCK_MONOTONIC, &ts); printf ("errno = %m (%d)\n", errno); } -- I see no syscall on 5.11 kernel, only a clock_gettime (CLOCK_MONOTONIC) on the 4.4 and a clock_gettime_time64 plus a clock_gettime on the 3.10. --- [PATCH] Linux: Use 32-bit vDSO for clock_gettime, gettimeofday, time (BZ# 28071) The previous approach defeats the vDSO optimization on older kernels because a failing clock_gettime64 system call is performed on every function call. It also results in a clobbered errno value, exposing an OpenJDK bug (JDK-8270244). This patch fixes by open-code INLINE_VSYSCALL macro and replace all INLINE_SYSCALL_CALL with INTERNAL_SYSCALL_CALLS. Now for __clock_gettime64, the 64-bit vDSO is used and the 32-bit vDSO is tried before falling back to 64-bit syscalls. The previous code preferred 64-bit syscall for the case where the kernel provides 64-bit time_t syscalls *and* also a 32-bit vDSO (in this case the *64-bit* syscall should be preferable over the vDSO). All architectures that provides 32-bit vDSO (i386, mips, powerpc, s390) modulo sparc; but I am not sure if some kernels versions do provide only 32-bit vDSO while still providing 64-bit time_t syscall. Regardless, for such cases the 64-bit time_t syscall is used if the vDSO returns overflowed 32-bit time_t. Tested on i686-linux-gnu (with a time64 and non-time64 kernel), x86_64-linux-gnu. Built with build-many-glibcs.py. Co-authored-by: Florian Weimer <fweimer@redhat.com> --- sysdeps/unix/sysv/linux/Makefile | 6 ++ sysdeps/unix/sysv/linux/clock_gettime.c | 50 ++++++++++++---- .../sysv/linux/tst-clock_gettime-clobber.c | 57 +++++++++++++++++++ .../sysv/linux/tst-gettimeofday-clobber.c | 37 ++++++++++++ sysdeps/unix/sysv/linux/tst-time-clobber.c | 36 ++++++++++++ 5 files changed, 174 insertions(+), 12 deletions(-) create mode 100644 sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c create mode 100644 sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c create mode 100644 sysdeps/unix/sysv/linux/tst-time-clobber.c diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 7d43dc95f2..a4769aa9db 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -213,6 +213,12 @@ ifeq ($(subdir),time) sysdep_headers += sys/timex.h bits/timex.h sysdep_routines += ntp_gettime ntp_gettimex + +tests += \ + tst-clock_gettime-clobber \ + tst-gettimeofday-clobber \ + tst-time-clobber \ + # tests endif ifeq ($(subdir),signal) diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c index cfe9370455..59b146702f 100644 --- a/sysdeps/unix/sysv/linux/clock_gettime.c +++ b/sysdeps/unix/sysv/linux/clock_gettime.c @@ -35,27 +35,53 @@ __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp) #endif #ifdef HAVE_CLOCK_GETTIME64_VSYSCALL - r = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp); -#else - r = INLINE_SYSCALL_CALL (clock_gettime64, clock_id, tp); + int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp) + = GLRO(dl_vdso_clock_gettime64); + if (vdso_time64 != NULL) + { + r = INTERNAL_VSYSCALL_CALL (vdso_time64, 2, clock_id, tp); + if (r == 0) + return 0; + return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); + } #endif - if (r == 0 || errno != ENOSYS) - return r; +#ifdef HAVE_CLOCK_GETTIME_VSYSCALL + int (*vdso_time)(clockid_t clock_id, struct timespec *tp) + = GLRO(dl_vdso_clock_gettime); + if (vdso_time != NULL) + { + struct timespec tp32; + r = INTERNAL_VSYSCALL_CALL (vdso_time, 2, clock_id, &tp32); + if (r == 0 && tp32.tv_sec > 0) + { + *tp = valid_timespec_to_timespec64 (tp32); + return 0; + } + else if (r != 0) + return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); + /* Fallback to syscall if time32 overflow. */ + } +#endif + + r = INTERNAL_SYSCALL_CALL (clock_gettime64, clock_id, tp); + if (r == 0) + return 0; + if (-r != ENOSYS) + return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); #ifndef __ASSUME_TIME64_SYSCALLS /* Fallback code that uses 32-bit support. */ struct timespec tp32; -# ifdef HAVE_CLOCK_GETTIME_VSYSCALL - r = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32); -# else - r = INLINE_SYSCALL_CALL (clock_gettime, clock_id, &tp32); -# endif + r = INTERNAL_SYSCALL_CALL (clock_gettime, clock_id, &tp32); if (r == 0) - *tp = valid_timespec_to_timespec64 (tp32); + { + *tp = valid_timespec_to_timespec64 (tp32); + return 0; + } #endif - return r; + return INLINE_SYSCALL_ERROR_RETURN_VALUE (-r); } #if __TIMESIZE != 64 diff --git a/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c b/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c new file mode 100644 index 0000000000..81a32bd15a --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c @@ -0,0 +1,57 @@ +/* Check that clock_gettime does not clobber errno on success. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <time.h> +#include <support/check.h> +#include <stdio.h> + +static void +test_clock (clockid_t clk) +{ + printf ("info: testing clock: %d\n", (int) clk); + + for (int original_errno = 0; original_errno < 2; ++original_errno) + { + errno = original_errno; + struct timespec ts; + if (clock_gettime (clk, &ts) == 0) + TEST_COMPARE (errno, original_errno); + } +} + +static int +do_test (void) +{ + test_clock (CLOCK_BOOTTIME); + test_clock (CLOCK_BOOTTIME_ALARM); + test_clock (CLOCK_MONOTONIC); + test_clock (CLOCK_MONOTONIC_COARSE); + test_clock (CLOCK_MONOTONIC_RAW); + test_clock (CLOCK_PROCESS_CPUTIME_ID); + test_clock (CLOCK_REALTIME); + test_clock (CLOCK_REALTIME_ALARM); + test_clock (CLOCK_REALTIME_COARSE); +#ifdef CLOCK_TAI + test_clock (CLOCK_TAI); +#endif + test_clock (CLOCK_THREAD_CPUTIME_ID); + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c b/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c new file mode 100644 index 0000000000..bb9bdb67b0 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c @@ -0,0 +1,37 @@ +/* Check that gettimeofday does not clobber errno. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <stddef.h> +#include <support/check.h> +#include <sys/time.h> + +static int +do_test (void) +{ + for (int original_errno = 0; original_errno < 2; ++original_errno) + { + errno = original_errno; + struct timeval tv; + gettimeofday (&tv, NULL); + TEST_COMPARE (errno, original_errno); + } + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/tst-time-clobber.c b/sysdeps/unix/sysv/linux/tst-time-clobber.c new file mode 100644 index 0000000000..ad83be9be8 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-time-clobber.c @@ -0,0 +1,36 @@ +/* Check that time does not clobber errno. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <stddef.h> +#include <support/check.h> +#include <time.h> + +static int +do_test (void) +{ + for (int original_errno = 0; original_errno < 2; ++original_errno) + { + errno = original_errno; + time (NULL); + TEST_COMPARE (errno, original_errno); + } + return 0; +} + +#include <support/test-driver.c>
* Adhemerval Zanella: > Using the test (a slight modified one from the bug report): > > -- > #include <time.h> > #include <stdio.h> > #include <errno.h> > > int > main (void) > { > struct timespec ts; > errno = 0; > clock_gettime (CLOCK_REALTIME, &ts); > printf ("errno = %m (%d)\n", errno); > errno = 0; > clock_gettime (CLOCK_MONOTONIC, &ts); > printf ("errno = %m (%d)\n", errno); > } > -- > > I see no syscall on 5.11 kernel, only a clock_gettime (CLOCK_MONOTONIC) > on the 4.4 and a clock_gettime_time64 plus a clock_gettime on the 3.10. This still introduces a severe performance regression on older kernels. It may well make some exsting 32-bit applications unusable until the kernel is upgraded. I'm not sure if this is a good idea. At least I can see that the clobbering of errno is gone. Thanks, Florian
On 12/07/2021 07:40, Florian Weimer wrote: > * Adhemerval Zanella: > >> Using the test (a slight modified one from the bug report): >> >> -- >> #include <time.h> >> #include <stdio.h> >> #include <errno.h> >> >> int >> main (void) >> { >> struct timespec ts; >> errno = 0; >> clock_gettime (CLOCK_REALTIME, &ts); >> printf ("errno = %m (%d)\n", errno); >> errno = 0; >> clock_gettime (CLOCK_MONOTONIC, &ts); >> printf ("errno = %m (%d)\n", errno); >> } >> -- >> >> I see no syscall on 5.11 kernel, only a clock_gettime (CLOCK_MONOTONIC) >> on the 4.4 and a clock_gettime_time64 plus a clock_gettime on the 3.10. > > This still introduces a severe performance regression on older kernels. > It may well make some exsting 32-bit applications unusable until the > kernel is upgraded. I'm not sure if this is a good idea. > > At least I can see that the clobbering of errno is gone. Yes and this how we initially decided to provide 64-bit time_t support, where 32-bit implementations are done on top of 64-bit ones. I am not very fond of starting to adding 32-bit specific implementations, the complexity to fix some specific cases do not really pay in the long term imho. We might also add back the global time64 internal variable that indicates if the kernel supports 64-bit (removed by 9465c3a9fb557), but it also has its ow issues (live migration like CRIU).
* Adhemerval Zanella: > On 12/07/2021 07:40, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> Using the test (a slight modified one from the bug report): >>> >>> -- >>> #include <time.h> >>> #include <stdio.h> >>> #include <errno.h> >>> >>> int >>> main (void) >>> { >>> struct timespec ts; >>> errno = 0; >>> clock_gettime (CLOCK_REALTIME, &ts); >>> printf ("errno = %m (%d)\n", errno); >>> errno = 0; >>> clock_gettime (CLOCK_MONOTONIC, &ts); >>> printf ("errno = %m (%d)\n", errno); >>> } >>> -- >>> >>> I see no syscall on 5.11 kernel, only a clock_gettime (CLOCK_MONOTONIC) >>> on the 4.4 and a clock_gettime_time64 plus a clock_gettime on the 3.10. >> >> This still introduces a severe performance regression on older kernels. >> It may well make some exsting 32-bit applications unusable until the >> kernel is upgraded. I'm not sure if this is a good idea. >> >> At least I can see that the clobbering of errno is gone. > > Yes and this how we initially decided to provide 64-bit time_t support, > where 32-bit implementations are done on top of 64-bit ones. I am not > very fond of starting to adding 32-bit specific implementations, the > complexity to fix some specific cases do not really pay in the long > term imho. > > We might also add back the global time64 internal variable that indicates > if the kernel supports 64-bit (removed by 9465c3a9fb557), but it also has > its ow issues (live migration like CRIU). Hmm. I see. Let's fix the ENOSYS issue then because it breaks OpenJDK. I'm a bit surprised that we still see the extra syscalls with your patch, but I suppose that's just the way the INTERNAL_VSYSCALL_CALL macro works. Regarding the actual patch, there are a few missing spaces before parenthesis: + int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp) + = GLRO(dl_vdso_clock_gettime64); + int (*vdso_time)(clockid_t clock_id, struct timespec *tp) + = GLRO(dl_vdso_clock_gettime); Thanks, Florian
On 12/07/2021 10:15, Florian Weimer wrote: > * Adhemerval Zanella: > >> On 12/07/2021 07:40, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> Using the test (a slight modified one from the bug report): >>>> >>>> -- >>>> #include <time.h> >>>> #include <stdio.h> >>>> #include <errno.h> >>>> >>>> int >>>> main (void) >>>> { >>>> struct timespec ts; >>>> errno = 0; >>>> clock_gettime (CLOCK_REALTIME, &ts); >>>> printf ("errno = %m (%d)\n", errno); >>>> errno = 0; >>>> clock_gettime (CLOCK_MONOTONIC, &ts); >>>> printf ("errno = %m (%d)\n", errno); >>>> } >>>> -- >>>> >>>> I see no syscall on 5.11 kernel, only a clock_gettime (CLOCK_MONOTONIC) >>>> on the 4.4 and a clock_gettime_time64 plus a clock_gettime on the 3.10. >>> >>> This still introduces a severe performance regression on older kernels. >>> It may well make some exsting 32-bit applications unusable until the >>> kernel is upgraded. I'm not sure if this is a good idea. >>> >>> At least I can see that the clobbering of errno is gone. >> >> Yes and this how we initially decided to provide 64-bit time_t support, >> where 32-bit implementations are done on top of 64-bit ones. I am not >> very fond of starting to adding 32-bit specific implementations, the >> complexity to fix some specific cases do not really pay in the long >> term imho. >> >> We might also add back the global time64 internal variable that indicates >> if the kernel supports 64-bit (removed by 9465c3a9fb557), but it also has >> its ow issues (live migration like CRIU). > > Hmm. I see. Let's fix the ENOSYS issue then because it breaks OpenJDK. > > I'm a bit surprised that we still see the extra syscalls with your > patch, but I suppose that's just the way the INTERNAL_VSYSCALL_CALL > macro works. The INTERNAL_VSYSCALL_CALL issues the syscall if the vDSO is not present as a fallback mechanism. It should not be really necessary on most implementation currently, but there are some architectures and kernel version where the vDSO does not actually does it (I think mips on kernel 4.x version). However, I think open-coding is should be simple; specially now for the case we do not really want it to fallback to syscall. > > Regarding the actual patch, there are a few missing spaces before > parenthesis: > > + int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp) > + = GLRO(dl_vdso_clock_gettime64); > + int (*vdso_time)(clockid_t clock_id, struct timespec *tp) > + = GLRO(dl_vdso_clock_gettime); My understanding is for GLRO the space is not really required because the macro is not used a function call. I followed the same idea for the function pointer definition. Regardless of the missing space, are you ok with my patch then? > > Thanks, > Florian >
* Adhemerval Zanella: >> I'm a bit surprised that we still see the extra syscalls with your >> patch, but I suppose that's just the way the INTERNAL_VSYSCALL_CALL >> macro works. > > The INTERNAL_VSYSCALL_CALL issues the syscall if the vDSO is not present > as a fallback mechanism. It should not be really necessary on most > implementation currently, but there are some architectures and kernel > version where the vDSO does not actually does it (I think mips on kernel > 4.x version). But we check the function pointer before that, so we should never hit the fallback path. That's what confuses me. Here's what I see on s390-linux-gnu (7.9.z). System glibc (glibc-2.17-324.el7_9.s390): munmap(0x7d4e3000, 35579) = 0 fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0 mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7d4eb000 write(1, "errno = Success (0)\n", 20errno = Success (0) ) = 20 write(1, "errno = Success (0)\n", 20errno = Success (0) ) = 20 exit_group(1) = ? Current development glibc with your patch applied on top: ugetrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0 syscall_0x193(0, 0x7f916310, 0x7f91652c, 0x7f9163fc, 0x400638, 0x7f916518) = -1 ENOSYS (Function not implemented) clock_gettime(CLOCK_REALTIME, {tv_sec=1626099916, tv_nsec=791410856}) = 0 statx(1, "", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT|AT_EMPTY_PATH, STATX_BASIC_STATS, 0x7f915a90) = -1 ENOSYS (Function not implemented) fstatat64(1, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}, AT_EMPTY_PATH) = 0 getrandom("\x16\x60\xeb\x47", 4, GRND_NONBLOCK) = 4 brk(NULL) = 0x7d9e9000 brk(0x7da0a000) = 0x7da0a000 write(1, "errno = Success (0)\n", 20errno = Success (0) ) = 20 syscall_0x193(0x1, 0x7f916310, 0x14, 0xfd46054a, 0x400638, 0x7f916518) = -1 ENOSYS (Function not implemented) clock_gettime(CLOCK_MONOTONIC, {tv_sec=29888, tv_nsec=604781810}) = 0 write(1, "errno = Success (0)\n", 20errno = Success (0) ) = 20 The vdso has this (according to /lib/modules/3.10.0-1160.31.1.el7.s390x/vdso/vdso32.so, have not checked at run time): 2: 00000300 84 FUNC GLOBAL DEFAULT 7 __kernel_clock_getres@@LINUX_2.6.29 3: 00000230 208 FUNC GLOBAL DEFAULT 7 __kernel_gettimeofday@@LINUX_2.6.29 5: 00000354 410 FUNC GLOBAL DEFAULT 7 __kernel_clock_gettime@@LINUX_2.6.29 >> Regarding the actual patch, there are a few missing spaces before >> parenthesis: >> >> + int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp) >> + = GLRO(dl_vdso_clock_gettime64); >> + int (*vdso_time)(clockid_t clock_id, struct timespec *tp) >> + = GLRO(dl_vdso_clock_gettime); > > My understanding is for GLRO the space is not really required because the > macro is not used a function call. I followed the same idea for the > function pointer definition. I think the the ( in the pointer type still qualifies for the space because it is related to function application. We use the space in all prototypes, after all. > Regardless of the missing space, are you ok with my patch then? It gets rid of the ENOSYS error, so it is a step forward. Thanks, Florian
On 12/07/2021 11:29, Florian Weimer wrote: > * Adhemerval Zanella: > >>> I'm a bit surprised that we still see the extra syscalls with your >>> patch, but I suppose that's just the way the INTERNAL_VSYSCALL_CALL >>> macro works. >> >> The INTERNAL_VSYSCALL_CALL issues the syscall if the vDSO is not present >> as a fallback mechanism. It should not be really necessary on most >> implementation currently, but there are some architectures and kernel >> version where the vDSO does not actually does it (I think mips on kernel >> 4.x version). > > But we check the function pointer before that, so we should never hit > the fallback path. That's what confuses me. > > Here's what I see on s390-linux-gnu (7.9.z). System glibc > (glibc-2.17-324.el7_9.s390): > > munmap(0x7d4e3000, 35579) = 0 > fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0 > mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7d4eb000 > write(1, "errno = Success (0)\n", 20errno = Success (0) > ) = 20 > write(1, "errno = Success (0)\n", 20errno = Success (0) > ) = 20 > exit_group(1) = ? > > Current development glibc with your patch applied on top: > > ugetrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0 > syscall_0x193(0, 0x7f916310, 0x7f91652c, 0x7f9163fc, 0x400638, 0x7f916518) = -1 ENOSYS (Function not implemented) > clock_gettime(CLOCK_REALTIME, {tv_sec=1626099916, tv_nsec=791410856}) = 0 > statx(1, "", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT|AT_EMPTY_PATH, STATX_BASIC_STATS, 0x7f915a90) = -1 ENOSYS (Function not implemented) > fstatat64(1, "", {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}, AT_EMPTY_PATH) = 0 > getrandom("\x16\x60\xeb\x47", 4, GRND_NONBLOCK) = 4 > brk(NULL) = 0x7d9e9000 > brk(0x7da0a000) = 0x7da0a000 > write(1, "errno = Success (0)\n", 20errno = Success (0) > ) = 20 > syscall_0x193(0x1, 0x7f916310, 0x14, 0xfd46054a, 0x400638, 0x7f916518) = -1 ENOSYS (Function not implemented) > clock_gettime(CLOCK_MONOTONIC, {tv_sec=29888, tv_nsec=604781810}) = 0 > write(1, "errno = Success (0)\n", 20errno = Success (0) > ) = 20 > > The vdso has this (according to > /lib/modules/3.10.0-1160.31.1.el7.s390x/vdso/vdso32.so, have not checked > at run time): > > 2: 00000300 84 FUNC GLOBAL DEFAULT 7 __kernel_clock_getres@@LINUX_2.6.29 > 3: 00000230 208 FUNC GLOBAL DEFAULT 7 __kernel_gettimeofday@@LINUX_2.6.29 > 5: 00000354 410 FUNC GLOBAL DEFAULT 7 __kernel_clock_gettime@@LINUX_2.6.29 I am almost sure it is a kernel issue specific to s390, where kernel does not map the vDSO is there is no interpreter (it happens for static case and for the loader directly). I think it was fixed upstream, although s390 vDSO is also now gone on 5.5: s390> uname -a Linux 4.12.14-197.78-default #1 SMP Tue Jan 5 17:10:44 UTC 2021 (a30d048) s390x s390x s390x GNU/Linux s390> cat dump_vdso.c #include <stdio.h> #include <string.h> #include <assert.h> int main (int argc, char *argv[]) { FILE *maps = fopen ("/proc/self/maps", "r"); assert (maps != NULL); char *line = NULL; size_t s = 0; while (getline (&line, &s, maps) != -1) { if (strstr (line, "[vdso]") == NULL) continue; size_t start, end; sscanf (line, "%zx-%zx", &start, &end); fwrite ((void *) start, end - start, 1, stdout); } fclose (maps); return 0; } s390> gcc-8 -m31 dump_vdso.c -o dump_vdso s390> ./dump_vdso > vdso.so s390> readelf -Ws vdso.so | grep __kernel_clock_gettime 5: 000003e4 496 FUNC GLOBAL DEFAULT 8 __kernel_clock_gettime@@LINUX_2.6.29 s390> ./ld.so.1 --library-path . ./dump_vdso > vdso.so s390> readelf -Ws vdso.so | grep __kernel_clock_gettime readelf: Error: vdso.so: Failed to read file's magic number s390> file vdso.so vdso.so: empty With --enable-hardcoded-path-in-tests you can actually it does work as intended: s390> ./testrun.sh --tool=strace ./time/tst-time-clobber --direct [...] read(3, "\177ELF\1\2\1\3\0\0\0\0\0\0\0\0\0\3\0\26\0\0\0\1\0\2eH\0\0\0004"..., 512) = 512 statx(3, "", AT_STATX_SYNC_AS_STAT|AT_NO_AUTOMOUNT|AT_EMPTY_PATH, STATX_BASIC_STATS, {stx_mask=STATX_BASIC_STATS, stx_attributes=0, stx_mode=S_IFREG|0755, stx_size=17013776, ...}) = 0 mmap2(NULL, 1773140, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7d0c8000 mmap2(0x7d26c000, 16384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1a3000) = 0x7d26c000 mmap2(0x7d270000, 36436, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x7d270000 close(3) = 0 mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7d0c6000 set_tid_address(0x7d0c6ce8) = 2030 set_robust_list(0x7d0c6cf0, 12) = 0 mprotect(0x7d26c000, 8192, PROT_READ) = 0 mprotect(0x404000, 4096, PROT_READ) = 0 mprotect(0x7d2ab000, 4096, PROT_READ) = 0 ugetrlimit(RLIMIT_STACK, {rlim_cur=8192*1024, rlim_max=RLIM_INFINITY}) = 0 mmap2(NULL, 8, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_ANONYMOUS, -1, 0) = 0x7d2aa000 getrandom("\xa7\x99\xbb\xee", 4, GRND_NONBLOCK) = 4 exit_group(0) = ? +++ exited with 0 +++ > >>> Regarding the actual patch, there are a few missing spaces before >>> parenthesis: >>> >>> + int (*vdso_time64)(clockid_t clock_id, struct __timespec64 *tp) >>> + = GLRO(dl_vdso_clock_gettime64); >>> + int (*vdso_time)(clockid_t clock_id, struct timespec *tp) >>> + = GLRO(dl_vdso_clock_gettime); >> >> My understanding is for GLRO the space is not really required because the >> macro is not used a function call. I followed the same idea for the >> function pointer definition. > > I think the the ( in the pointer type still qualifies for the space > because it is related to function application. We use the space in all > prototypes, after all. Ack. > >> Regardless of the missing space, are you ok with my patch then? > > It gets rid of the ENOSYS error, so it is a step forward
* Adhemerval Zanella: > I am almost sure it is a kernel issue specific to s390, where kernel does > not map the vDSO is there is no interpreter (it happens for static case > and for the loader directly). I think it was fixed upstream, although > s390 vDSO is also now gone on 5.5: Oh, that's it indeed. With a hard-coded dynamic linker path, there is again no system call. So this patch is good functionality-wise, too. Thanks, Florian
On 12/07/2021 14:55, Florian Weimer wrote: > * Adhemerval Zanella: > >> I am almost sure it is a kernel issue specific to s390, where kernel does >> not map the vDSO is there is no interpreter (it happens for static case >> and for the loader directly). I think it was fixed upstream, although >> s390 vDSO is also now gone on 5.5: > > Oh, that's it indeed. With a hard-coded dynamic linker path, there is > again no system call. So this patch is good functionality-wise, too. > > Thanks, > Florian > Ok, I will push the patch upstream with the missing spaces before parenthesis fixed.
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile index 7d43dc95f2..9f00e53de4 100644 --- a/sysdeps/unix/sysv/linux/Makefile +++ b/sysdeps/unix/sysv/linux/Makefile @@ -213,7 +213,14 @@ ifeq ($(subdir),time) sysdep_headers += sys/timex.h bits/timex.h sysdep_routines += ntp_gettime ntp_gettimex -endif + +tests += \ + tst-clock_gettime-clobber \ + tst-gettimeofday-clobber \ + tst-time-clobber \ + # tests + +endif# $(subdir) == time ifeq ($(subdir),signal) tests-special += $(objpfx)tst-signal-numbers.out diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c index cfe9370455..86a72ecf5a 100644 --- a/sysdeps/unix/sysv/linux/clock_gettime.c +++ b/sysdeps/unix/sysv/linux/clock_gettime.c @@ -64,6 +64,7 @@ libc_hidden_def (__clock_gettime64) int __clock_gettime (clockid_t clock_id, struct timespec *tp) { +# ifdef __ASSUME_TIME64_SYSCALLS int ret; struct __timespec64 tp64; @@ -81,6 +82,13 @@ __clock_gettime (clockid_t clock_id, struct timespec *tp) } return ret; +# else /* !__ASSUME_TIME64_SYSCALLS */ +# ifdef HAVE_CLOCK_GETTIME_VSYSCALL + return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); +# else + return INLINE_SYSCALL_CALL (clock_gettime, clock_id, tp); +# endif +# endif /* !__ASSUME_TIME64_SYSCALLS */ } #endif libc_hidden_def (__clock_gettime) diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c index 4197be5656..1b353956ea 100644 --- a/sysdeps/unix/sysv/linux/gettimeofday.c +++ b/sysdeps/unix/sysv/linux/gettimeofday.c @@ -16,18 +16,17 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ +#include <dl-vdso.h> +#include <libc-vdso.h> #include <time.h> #include <string.h> +#include <sysdep-vdso.h> /* Optimize the function call by setting the PLT directly to vDSO symbol. */ #ifdef USE_IFUNC_GETTIMEOFDAY # include <sysdep.h> -# include <sysdep-vdso.h> # ifdef SHARED -# include <dl-vdso.h> -# include <libc-vdso.h> - static int __gettimeofday_syscall (struct timeval *restrict tv, void *restrict tz) { @@ -54,7 +53,7 @@ __gettimeofday (struct timeval *restrict tv, void *restrict tz) } # endif weak_alias (__gettimeofday, gettimeofday) -#else /* USE_IFUNC_GETTIMEOFDAY */ +#else /* !USE_IFUNC_GETTIMEOFDAY */ /* Conversion of gettimeofday function to support 64 bit time on archs with __WORDSIZE == 32 and __TIMESIZE == 32/64 */ #include <errno.h> @@ -73,9 +72,12 @@ __gettimeofday64 (struct __timeval64 *restrict tv, void *restrict tz) return 0; } -# if __TIMESIZE != 64 +# if __TIMESIZE == 64 +weak_alias (__gettimeofday, gettimeofday) +# else /* __TIMESIZE != 64 */ libc_hidden_def (__gettimeofday64) +# ifdef __ASSUME_TIME64_SYSCALL int __gettimeofday (struct timeval *restrict tv, void *restrict tz) { @@ -92,6 +94,9 @@ __gettimeofday (struct timeval *restrict tv, void *restrict tz) *tv = valid_timeval64_to_timeval (tv64); return 0; } -# endif weak_alias (__gettimeofday, gettimeofday) -#endif +# else /* !__ASSUME_TIME64_SYSCALL */ +# include <time/gettimeofday.c> +# endif /* !__ASSUME_TIME64_SYSCALL */ +# endif +#endif /* !USE_IFUNC_GETTIMEOFDAY */ diff --git a/sysdeps/unix/sysv/linux/time.c b/sysdeps/unix/sysv/linux/time.c index 43fec7d3a7..b194127691 100644 --- a/sysdeps/unix/sysv/linux/time.c +++ b/sysdeps/unix/sysv/linux/time.c @@ -16,16 +16,16 @@ License along with the GNU C Library; if not, see <https://www.gnu.org/licenses/>. */ +#include <dl-vdso.h> +#include <libc-vdso.h> +#include <sysdep-vdso.h> + /* Optimize the function call by setting the PLT directly to vDSO symbol. */ #ifdef USE_IFUNC_TIME # include <time.h> # include <sysdep.h> -# include <sysdep-vdso.h> #ifdef SHARED -# include <dl-vdso.h> -# include <libc-vdso.h> - static time_t time_syscall (time_t *t) { @@ -46,7 +46,7 @@ time (time_t *t) return INLINE_VSYSCALL (time, 1, t); } # endif /* !SHARED */ -#else /* USE_IFUNC_TIME */ +#else /* !USE_IFUNC_TIME */ # include <time.h> # include <time-clockid.h> # include <errno.h> @@ -63,10 +63,13 @@ __time64 (__time64_t *timer) *timer = ts.tv_sec; return ts.tv_sec; } +# if __TIMESIZE == 64 +weak_alias (__time, time) -# if __TIMESIZE != 64 +# else /* __TIMESIZE != 64 */ libc_hidden_def (__time64) +# ifdef __ASSUME_TIME64_SYSCALL time_t __time (time_t *timer) { @@ -82,6 +85,9 @@ __time (time_t *timer) *timer = t; return t; } -# endif weak_alias (__time, time) -#endif +# else /* !__ASSUME_TIME64_SYSCALL */ +# include <time/time.c> +# endif /* !__ASSUME_TIME64_SYSCALL */ +# endif /* __TIMESIZE != 64 */ +#endif /* !USE_IFUNC_TIME */ diff --git a/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c b/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c new file mode 100644 index 0000000000..81a32bd15a --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-clock_gettime-clobber.c @@ -0,0 +1,57 @@ +/* Check that clock_gettime does not clobber errno on success. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <time.h> +#include <support/check.h> +#include <stdio.h> + +static void +test_clock (clockid_t clk) +{ + printf ("info: testing clock: %d\n", (int) clk); + + for (int original_errno = 0; original_errno < 2; ++original_errno) + { + errno = original_errno; + struct timespec ts; + if (clock_gettime (clk, &ts) == 0) + TEST_COMPARE (errno, original_errno); + } +} + +static int +do_test (void) +{ + test_clock (CLOCK_BOOTTIME); + test_clock (CLOCK_BOOTTIME_ALARM); + test_clock (CLOCK_MONOTONIC); + test_clock (CLOCK_MONOTONIC_COARSE); + test_clock (CLOCK_MONOTONIC_RAW); + test_clock (CLOCK_PROCESS_CPUTIME_ID); + test_clock (CLOCK_REALTIME); + test_clock (CLOCK_REALTIME_ALARM); + test_clock (CLOCK_REALTIME_COARSE); +#ifdef CLOCK_TAI + test_clock (CLOCK_TAI); +#endif + test_clock (CLOCK_THREAD_CPUTIME_ID); + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c b/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c new file mode 100644 index 0000000000..bb9bdb67b0 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-gettimeofday-clobber.c @@ -0,0 +1,37 @@ +/* Check that gettimeofday does not clobber errno. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <stddef.h> +#include <support/check.h> +#include <sys/time.h> + +static int +do_test (void) +{ + for (int original_errno = 0; original_errno < 2; ++original_errno) + { + errno = original_errno; + struct timeval tv; + gettimeofday (&tv, NULL); + TEST_COMPARE (errno, original_errno); + } + return 0; +} + +#include <support/test-driver.c> diff --git a/sysdeps/unix/sysv/linux/tst-time-clobber.c b/sysdeps/unix/sysv/linux/tst-time-clobber.c new file mode 100644 index 0000000000..ad83be9be8 --- /dev/null +++ b/sysdeps/unix/sysv/linux/tst-time-clobber.c @@ -0,0 +1,36 @@ +/* Check that time does not clobber errno. + Copyright (C) 2021 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +#include <errno.h> +#include <stddef.h> +#include <support/check.h> +#include <time.h> + +static int +do_test (void) +{ + for (int original_errno = 0; original_errno < 2; ++original_errno) + { + errno = original_errno; + time (NULL); + TEST_COMPARE (errno, original_errno); + } + return 0; +} + +#include <support/test-driver.c>