Linux: Use 32-bit vDSO for clock_gettime, gettimeofday, time (bug 28071)
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
| dj/TryBot-32bit |
success
|
Build for i686
|
Commit Message
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(-)
Comments
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.
@@ -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
@@ -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)
@@ -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 */
@@ -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 */
new file mode 100644
@@ -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>
new file mode 100644
@@ -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>
new file mode 100644
@@ -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>