Message ID | 20191018145720.11706-2-lukma@denx.de |
---|---|
State | New |
Headers | show |
Series | [1/2] y2038: Helper macro to convert struct __timespec64 to struct timespec | expand |
On Fri, 18 Oct 2019, Lukasz Majewski wrote: > When working on systems still supporting 32 bit time (__TIMESIZE != 64) > without Y2038 time support the clock_getres returns error when received > tv_sec excess time_t range (i.e. overflow 32 bit type). > Moreover, the correctness of tv_nsec is checked. It's definitely not necessary to check if the kernel returned a valid tv_nsec value; that can be assumed if the syscall succeeded at all. I'm doubtful about the check for overflow in this case (a clock whose resolution exceeds 68 years does not seem a very useful clock), but that check is at least theoretically relevant. A stronger justification for the helper macro in patch 1/2 would be if you have a patch that uses it to check a timespec64 value coming from the *user* rather than from the kernel (that is, coming from the caller to one of the __*64 functions that will end up being directly called by code built with _TIME_BITS=64). If a function needs to check the nanoseconds value (rather than deferring to a kernel check of that value), I'd expect it to need to so such a check regardless of whether it needs to convert 64-bit time to 32-bit time. For example, that's what __clock_settime64 does - checks nanoseconds before calling into the kernel at all. So it's not clear to me that there is actually a use case for a macro that combines a check of the nanoseconds value with a conversion from 64-bit to 32-bit time. There *is* definitely a use for a macro such as the IS_VALID_NANOSECONDS macro in the first patch. Perhaps it would make sense to factor out and send a patch that just adds that macro and makes existing code with such checks use it. (See io/ppoll.c, nptl/sem_clockwait.c, nptl/sem_timedwait.c, sysdeps/htl/pt-cond-timedwait.c, sysdeps/htl/pt-mutex-timedlock.c, sysdeps/htl/pt-rwlock-timedrdlock.c, sysdeps/htl/pt-rwlock-timedwrlock.c, sysdeps/htl/sem-timedwait.c, sysdeps/mach/nanosleep.c, sysdeps/pthread/timer_settime.c, sysdeps/sparc/sparc32/lowlevellock.c, sysdeps/unix/clock_nanosleep.c, sysdeps/unix/clock_settime.c, sysdeps/unix/sysv/linux/clock_settime.c, time/clock_nanosleep.c for examples of files with checks that look like they could use such a macro - not necessarily an exhaustive list. Note that a few of those files use __builtin_expect, thus suggesting that the macro definition could use __glibc_likely to reflect that nanoseconds values almost certainly are valid in real use.)
Hi Joseph, > On Fri, 18 Oct 2019, Lukasz Majewski wrote: > > > When working on systems still supporting 32 bit time (__TIMESIZE != > > 64) without Y2038 time support the clock_getres returns error when > > received tv_sec excess time_t range (i.e. overflow 32 bit type). > > Moreover, the correctness of tv_nsec is checked. > > It's definitely not necessary to check if the kernel returned a valid > tv_nsec value; that can be assumed if the syscall succeeded at all. Ok. > I'm doubtful about the check for overflow in this case (a clock whose > resolution exceeds 68 years does not seem a very useful clock), but > that check is at least theoretically relevant. We can skip those two checks altogether (for clock_getres) and instead use valid_timespec64_to_timespec() conversion helper function. (then we would rely on kernel providing valid range or return syscall with error). > > A stronger justification for the helper macro in patch 1/2 would be > if you have a patch that uses it to check a timespec64 value coming > from the *user* rather than from the kernel (that is, coming from the > caller to one of the __*64 functions that will end up being directly > called by code built with _TIME_BITS=64). Yes, I do agree. One most apparent example is the clock_nanosleep conversion, which would need such check on passed *request value (const struct __timespec64 *request). > > If a function needs to check the nanoseconds value (rather than > deferring to a kernel check of that value), I'd expect it to need to > so such a check regardless of whether it needs to convert 64-bit time > to 32-bit time. For example, that's what __clock_settime64 does - > checks nanoseconds before calling into the kernel at all. So it's > not clear to me that there is actually a use case for a macro that > combines a check of the nanoseconds value with a conversion from > 64-bit to 32-bit time. Ok. > > There *is* definitely a use for a macro such as the > IS_VALID_NANOSECONDS macro in the first patch. Perhaps it would make > sense to factor out and send a patch that just adds that macro and > makes existing code with such checks use it. (See io/ppoll.c, > nptl/sem_clockwait.c, nptl/sem_timedwait.c, > sysdeps/htl/pt-cond-timedwait.c, sysdeps/htl/pt-mutex-timedlock.c, > sysdeps/htl/pt-rwlock-timedrdlock.c, > sysdeps/htl/pt-rwlock-timedwrlock.c, sysdeps/htl/sem-timedwait.c, > sysdeps/mach/nanosleep.c, sysdeps/pthread/timer_settime.c, > sysdeps/sparc/sparc32/lowlevellock.c, sysdeps/unix/clock_nanosleep.c, > sysdeps/unix/clock_settime.c, > sysdeps/unix/sysv/linux/clock_settime.c, time/clock_nanosleep.c for > examples of files with checks that look like they could use such a > macro - not necessarily an exhaustive list. Note that a few of those > files use __builtin_expect, thus suggesting that the macro definition > could use __glibc_likely to reflect that nanoseconds values almost > certainly are valid in real use.) > If we rely on kernel, then the IS_VALID_NANOSECONDS is not required for clock_getres(). However, it would be beneficial to have it as static inline function to be used in glibc. Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
diff --git a/include/time.h b/include/time.h index c2b6c9b842..cefc27ebb9 100644 --- a/include/time.h +++ b/include/time.h @@ -133,6 +133,14 @@ extern int __clock_settime64 (clockid_t clock_id, libc_hidden_proto (__clock_settime64) #endif +#if __TIMESIZE == 64 +# define __clock_getres64 __clock_getres +#else +extern int __clock_getres64 (clockid_t clock_id, + struct __timespec64 *tp); +libc_hidden_proto (__clock_getres64); +#endif + /* Compute the `struct tm' representation of T, offset OFFSET seconds east of UTC, and store year, yday, mon, mday, wday, hour, min, sec into *TP. diff --git a/sysdeps/unix/sysv/linux/clock_getres.c b/sysdeps/unix/sysv/linux/clock_getres.c index 0b82759043..e5b4bf7cea 100644 --- a/sysdeps/unix/sysv/linux/clock_getres.c +++ b/sysdeps/unix/sysv/linux/clock_getres.c @@ -19,21 +19,53 @@ #include <sysdep.h> #include <errno.h> #include <time.h> -#include "kernel-posix-cpu-timers.h" #ifdef HAVE_CLOCK_GETRES_VSYSCALL # define HAVE_VSYSCALL #endif #include <sysdep-vdso.h> - #include <shlib-compat.h> +#include <kernel-features.h> /* Get resolution of clock. */ int -__clock_getres (clockid_t clock_id, struct timespec *res) +__clock_getres64 (clockid_t clock_id, struct __timespec64 *res) { +#ifdef __ASSUME_TIME64_SYSCALLS +# ifndef __NR_clock_getres_time64 return INLINE_VSYSCALL (clock_getres, 2, clock_id, res); +# else + return INLINE_SYSCALL (clock_getres_time64, 2, clock_id, res); +# endif +#else +# ifdef __NR_clock_getres_time64 + int ret = INLINE_SYSCALL (clock_getres_time64, 2, clock_id, res); + if (ret == 0 || errno != ENOSYS) + return ret; +# endif + struct timespec ts32; + int retval = INLINE_VSYSCALL (clock_getres, 2, clock_id, &ts32); + if (! retval && res) + *res = valid_timespec_to_timespec64 (ts32); + + return retval; +#endif +} + +#if __TIMESIZE != 64 +int +__clock_getres (clockid_t clock_id, struct timespec *res) +{ + struct __timespec64 ts64; + int retval; + + retval = __clock_getres64 (clock_id, &ts64); + if (! retval && res) + *res = timespec64_to_timespec (ts64); + + return retval; } +#endif versioned_symbol (libc, __clock_getres, clock_getres, GLIBC_2_17); /* clock_getres moved to libc in version 2.17;