Message ID | 20190414220841.20243-4-lukma@denx.de |
---|---|
State | New |
Headers | show |
Series | y2038: Linux: Provide __clock_* functions supporting 64 bit time | expand |
On Mon, 15 Apr 2019, Lukasz Majewski wrote: > The new 64 bit syscall (clock_settime64) available from Linux > 5.1+ has been used when applicable on 32 bit systems. > The execution path on 64 bit systems has not been changed or affected in > any way. Is this unchanged specifically because of __TIMESIZE conditionals in the code (so that it doesn't matter whether the 64-bit systems define any new syscall names)? Also, I don't see any __ASSUME_* / ENOSYS handling in this patch. As usual, for any new syscall that might be used in code that could also use an older syscall: * If the appropriate __ASSUME_* is defined, the new syscall can simply be used unconditionally. * If the appropriate __ASSUME_* is not defined, there needs to be runtime fallback to the old syscall if the new one returns an ENOSYS error (of course, that runtime fallback needs to check for the time overflowing the 32-bit range and give an appropriate error in that case, before calling the 32-bit syscall - and it's necessary to figure out what the priority should be of errors for invalid nanoseconds values versus overflowing seconds). It is normal and expected for the kernel headers used to build glibc to be (much) newer than the oldest kernel version supported by the resulting glibc binaries at runtime. This __ASSUME_* macro definition in kernel-features.h would need a multi-paragraph comment discussing exactly what the semantics of such __ASSUME_* macros are in the context of the sets of syscall names and numbers present on different kinds of Linux kernel architectures. > +#if __TIMESIZE != 64 > +int > +__clock_settime (clockid_t clock_id, const struct timespec *tp) > +{ > + struct __timespec64 ts64; > + > + if (! in_time_t_range (tp->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } I don't see how an overflow check makes sense in this context (converting a 32-bit timespec to a 64-bit one).
Hi Joseph, > On Mon, 15 Apr 2019, Lukasz Majewski wrote: > > > The new 64 bit syscall (clock_settime64) available from Linux > > 5.1+ has been used when applicable on 32 bit systems. > > The execution path on 64 bit systems has not been changed or > > affected in any way. > > Is this unchanged specifically because of __TIMESIZE conditionals in > the code (so that it doesn't matter whether the 64-bit systems define > any new syscall names)? I think yes. When __TIMESIZE == 64 the clock_settime syscall is supporting 64 bit time. The goal is to not change execution path for e.g. x86_64. IMHO this shall be kept separate from Y2038 and 32 bit execution path. This patch tries to follow 64 bit conversion for __clock_* functions as presented in: https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29 __TIMESIZE is necessary to distinct the 32/64 bit case as __clock_settime64 is used for both cases. In this function I also need to check if __TIMESIZE is defined https://github.com/lmajewski/y2038_glibc/commit/80f3b6220d80a8579e88b63fb90ba8ea7b771c5c#diff-725737619c471831ff796099fbab5c0aR33 as __clock_settime64 is aliased to clock_settime on 64 bit systems and then aliased/linked to librt (for backward compatibility). > > Also, I don't see any __ASSUME_* / ENOSYS handling in this patch. As > usual, for any new syscall that might be used in code that could also > use an older syscall: I've followed some code pattern from glibc sources; for example: sysdeps/unix/sysv/linux/lseek.c Here the new syscall is only guarded by: # ifdef __NR__llseek There was no __ASSUME_* for it. However, the statx for example uses __ASSUME_STATX, which is defined when kernel is newer than specified version. I will rewrite the code to be similar to statx. > > * If the appropriate __ASSUME_* is defined, the new syscall can > simply be used unconditionally. > > * If the appropriate __ASSUME_* is not defined, there needs to be > runtime fallback to the old syscall if the new one returns an ENOSYS > error (of course, that runtime fallback needs to check for the time > overflowing the 32-bit range and give an appropriate error in that > case, before calling the 32-bit syscall - and it's necessary to > figure out what the priority should be of errors for invalid > nanoseconds values versus overflowing seconds). It is normal and > expected for the kernel headers used to build glibc to be (much) > newer than the oldest kernel version supported by the resulting glibc > binaries at runtime. Ach... I see your point. The __ASSUME_ prevents from nonfunctional glibc when one uses new headers to compile glibc (which have __NR__clock_settime64 defined) afterwards installed in a system with old kernel (so __clock_settime64 returns -ENOSYS). I do agree that with current patch there is no fallback to old syscall in that case. I will fix it in v2. > > This __ASSUME_* macro definition in kernel-features.h would need a > multi-paragraph comment discussing exactly what the semantics of such > __ASSUME_* macros are in the context of the sets of syscall names and > numbers present on different kinds of Linux kernel architectures. Ok. I will add it to v2. > > > +#if __TIMESIZE != 64 > > +int > > +__clock_settime (clockid_t clock_id, const struct timespec *tp) > > +{ > > + struct __timespec64 ts64; > > + > > + if (! in_time_t_range (tp->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > I don't see how an overflow check makes sense in this context > (converting a 32-bit timespec to a 64-bit one). The above code is for the case when _TIME_BITS is not set (on 32 bit system) and we are after Y2038. The data provided by *tp has wrong value and shall not be passed to the kernel (no matter if it supports Y2038 time or not). > 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
15.04.2019 в 00:08:38 +0200 Lukasz Majewski написал: > +# if defined __NR_clock_settime64 > + /* Make sure that passed __timespec64 struct pad is 0. */ > + struct __timespec64 ts = *tp; > + ts.tv_pad = 0; > + return INLINE_SYSCALL_CALL (clock_settime64, clock_id, &ts); Isn't kernel supposed to zero out padding on its own? At least comment in kernel's get_timespec64 says so: /* Zero out the padding for 32 bit systems or in compat mode */ if (IS_ENABLED(CONFIG_64BIT_TIME) && in_compat_syscall()) kts.tv_nsec &= 0xFFFFFFFFUL; The code looks buggy though. It fails to zero out the padding in 32-bit kernels. That part is probably broken since 98f76206b3350 ("compat: Cleanup in_compat_syscall() callers"). And, hmm, is CONFIG_64BIT_TIME enabled anywhere?
Hi Stepan, > 15.04.2019 в 00:08:38 +0200 Lukasz Majewski написал: > > +# if defined __NR_clock_settime64 > > + /* Make sure that passed __timespec64 struct pad is 0. */ > > + struct __timespec64 ts = *tp; > > + ts.tv_pad = 0; > > + return INLINE_SYSCALL_CALL (clock_settime64, clock_id, &ts); > > Isn't kernel supposed to zero out padding on its own? > At least comment in kernel's get_timespec64 says so: > > /* Zero out the padding for 32 bit systems or in compat mode > */ if (IS_ENABLED(CONFIG_64BIT_TIME) && in_compat_syscall()) > kts.tv_nsec &= 0xFFFFFFFFUL; > For ARM (and x86) 32 bit machines I do use following syscalls (like clock_settime64): https://elixir.bootlin.com/linux/v5.1-rc4/source/arch/arm/tools/syscall.tbl#L420 which are providing 64 bit time support on 32 bit systems. Yes. In those systems the upper part (32 bits) of tv_nsec is cleared up with mask in the kernel. However, I would prefer not to pass random data to the kernel, and hence I do clear it up explicitly in glibc. > The code looks buggy though. It fails to zero out the padding in > 32-bit kernels. For the 32 bit systems without Y2038 support enabled in glibc - the clock_settime would be used, which corresponds to sys_clock_settime32() in the kernel. > That part is probably broken since > 98f76206b3350 ("compat: Cleanup in_compat_syscall() callers"). > > And, hmm, is CONFIG_64BIT_TIME enabled anywhere? When I do use clock_settime64 on the glibc side (with _TIME_BITS=64), I do not need to enable such config in the kernel. If the kernel supports this call (5.1+), then use it, otherwise fallback to clock_settime(). For 64 bit systems, I do not change the execution path. If you are interested, please look on the following repo (which has some more commits than those posted to the mailing list): https://github.com/lmajewski/y2038_glibc/commits/Y2038-2.29-glibc-__clock-internal-struct-timespec-v1 And meta layer for testing. https://github.com/lmajewski/meta-y2038 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
20.04.2019 в 13:21:12 +0200 Lukasz Majewski написал: > Hi Stepan, > > > 15.04.2019 в 00:08:38 +0200 Lukasz Majewski написал: > > > +# if defined __NR_clock_settime64 > > > + /* Make sure that passed __timespec64 struct pad is 0. */ > > > + struct __timespec64 ts = *tp; > > > + ts.tv_pad = 0; > > > + return INLINE_SYSCALL_CALL (clock_settime64, clock_id, &ts); > > > > Isn't kernel supposed to zero out padding on its own? > > At least comment in kernel's get_timespec64 says so: > > > > /* Zero out the padding for 32 bit systems or in compat mode > > */ if (IS_ENABLED(CONFIG_64BIT_TIME) && in_compat_syscall()) > > kts.tv_nsec &= 0xFFFFFFFFUL; > > > > For ARM (and x86) 32 bit machines I do use following syscalls (like > clock_settime64): > https://elixir.bootlin.com/linux/v5.1-rc4/source/arch/arm/tools/syscall.tbl#L420 > > which are providing 64 bit time support on 32 bit systems. > > Yes. In those systems the upper part (32 bits) of tv_nsec is cleared up > with mask in the kernel. Is it? The kernel (5.1-rc6) code looks to me like /* Zero out the padding for 32 bit systems or in compat mode */ if (false && false) kts.tv_nsec &= 0xFFFFFFFFUL; in 32-bit kernels. And like if (false && true) kts.tv_nsec &= 0xFFFFFFFFUL; for COMPAT syscalls in 64-bit kernels. It should probably be changed into if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall()) kts.tv_nsec &= 0xFFFFFFFFUL; (Or into something like if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall() && !COMPAT_USE_64BIT_TIME) kts.tv_nsec &= 0xFFFFFFFFUL; if x32 should retain 64-bit tv_nsec.) > However, I would prefer not to pass random data > to the kernel, and hence I do clear it up explicitly in glibc. If the kernel does not ignore padding on its own, then zeroing it out is required everywhere timespec is passed to kernel, including via code not known to glibc. (Does anyone promise that there won't be any ioctls that accept timespec, for example?) That seems to be error-prone (and might requre copying larger structes). On the other hand, if kernel 5.1+ ignores padding as intended there is no need to create additional copy of structs in glibc code that calls into clock_settime64 (or into timer_settime64 that accepts larger struct, for example). > > The code looks buggy though. It fails to zero out the padding in > > 32-bit kernels. > > For the 32 bit systems without Y2038 support enabled in glibc - the > clock_settime would be used, which corresponds to sys_clock_settime32() > in the kernel. I am talking about kernels with Y2038 support. > > That part is probably broken since > > 98f76206b3350 ("compat: Cleanup in_compat_syscall() callers"). > > > > And, hmm, is CONFIG_64BIT_TIME enabled anywhere? I guess that the remaining CONFIG_64BIT_TIME in kernel should be replaced with CONFIG_COMPAT_32BIT_TIME or removed.
On Mon, Apr 22, 2019 at 11:07 AM Stepan Golosunov <stepan@golosunov.pp.ru> wrote: > 20.04.2019 в 13:21:12 +0200 Lukasz Majewski написал: > Is it? The kernel (5.1-rc6) code looks to me like > > /* Zero out the padding for 32 bit systems or in compat mode */ > if (false && false) > kts.tv_nsec &= 0xFFFFFFFFUL; > > in 32-bit kernels. And like > > if (false && true) > kts.tv_nsec &= 0xFFFFFFFFUL; > > for COMPAT syscalls in 64-bit kernels. > > It should probably be changed into > > if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall()) > kts.tv_nsec &= 0xFFFFFFFFUL; > > (Or into something like > > if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall() && !COMPAT_USE_64BIT_TIME) > kts.tv_nsec &= 0xFFFFFFFFUL; > > if x32 should retain 64-bit tv_nsec.) I think the problem is that at some point CONFIG_64BIT_TIME was meant to be enabled on both 32-bit and 64-bit kernels, but the definition got changed along the way. We probably just want if (in_compat_syscall() ) kts.tv_nsec &= 0xFFFFFFFFUL; here, which would then truncate the nanoseconds for all compat mode including x32. For native mode, we don't need to truncate it, since timespec64 has a 32-bit 'tv_nsec' field in the kernel. > > However, I would prefer not to pass random data > > to the kernel, and hence I do clear it up explicitly in glibc. > > If the kernel does not ignore padding on its own, then zeroing it out > is required everywhere timespec is passed to kernel, including via > code not known to glibc. (Does anyone promise that there won't be any > ioctls that accept timespec, for example?) That seems to be > error-prone (and might requre copying larger structes). > > On the other hand, if kernel 5.1+ ignores padding as intended there is > no need to create additional copy of structs in glibc code that calls > into clock_settime64 (or into timer_settime64 that accepts larger > struct, for example). The intention is that the kernel ignores the padding. If you find another place in the kernel that forget that, we should fix it. > > > And, hmm, is CONFIG_64BIT_TIME enabled anywhere? > > I guess that the remaining CONFIG_64BIT_TIME in kernel should be > replaced with CONFIG_COMPAT_32BIT_TIME or removed. We should remove CONFIG_64BIT_TIME. CONFIG_COMPAT_32BIT_TIME is still needed to identify architectures that don't have it, in particular riscv32. Arnd
Hi Arnd and Stepan, > On Mon, Apr 22, 2019 at 11:07 AM Stepan Golosunov > <stepan@golosunov.pp.ru> wrote: > > 20.04.2019 в 13:21:12 +0200 Lukasz Majewski написал: > > Is it? The kernel (5.1-rc6) code looks to me like > > > > /* Zero out the padding for 32 bit systems or in compat > > mode */ if (false && false) > > kts.tv_nsec &= 0xFFFFFFFFUL; > > > > in 32-bit kernels. And like > > > > if (false && true) > > kts.tv_nsec &= 0xFFFFFFFFUL; > > > > for COMPAT syscalls in 64-bit kernels. > > > > It should probably be changed into > > > > if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall()) > > kts.tv_nsec &= 0xFFFFFFFFUL; > > > > (Or into something like > > > > if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall() > > && !COMPAT_USE_64BIT_TIME) kts.tv_nsec &= 0xFFFFFFFFUL; > > > > if x32 should retain 64-bit tv_nsec.) > > I think the problem is that at some point CONFIG_64BIT_TIME was > meant to be enabled on both 32-bit and 64-bit kernels, but the > definition got changed along the way. > > We probably just want > > if (in_compat_syscall() ) > kts.tv_nsec &= 0xFFFFFFFFUL; > > here, which would then truncate the nanoseconds for all compat > mode including x32. For native mode, we don't need to truncate > it, since timespec64 has a 32-bit 'tv_nsec' field in the kernel. > > > > However, I would prefer not to pass random data > > > to the kernel, and hence I do clear it up explicitly in glibc. > > > > If the kernel does not ignore padding on its own, then zeroing it > > out is required everywhere timespec is passed to kernel, including > > via code not known to glibc. (Does anyone promise that there won't > > be any ioctls that accept timespec, for example?) That seems to be > > error-prone (and might requre copying larger structes). > > > > On the other hand, if kernel 5.1+ ignores padding as intended there > > is no need to create additional copy of structs in glibc code that > > calls into clock_settime64 (or into timer_settime64 that accepts > > larger struct, for example). Ok, I think I see your point: - As kernel is ignoring padding, there is no need to copy the structure and set the padding to 0. However, in patch: [PATCH 1/6] y2038: Introduce internal for glibc struct __timespec64 The internal (for glibc) structure has been introduced - it has 32 bit tv_nsec and 32 bit padding. As it is passed to the kernel - the padding can have random values and hence shall be zeroed before passing to the kernel. The rationale for 32 bit tv_nsec is to be as close as possible to what is exported by glibc (64 bit tv_sec and 32 bit tv_nsec) for Y2038. I'm now wondering if it would be better to have glibc internal struct __timespec64 having both fields 64 bit (as it would be easier to pass it to Linux). > > The intention is that the kernel ignores the padding. If you find > another place in the kernel that forget that, we should fix it. > Thanks Arnd for clarification. > > > > And, hmm, is CONFIG_64BIT_TIME enabled anywhere? > > > > I guess that the remaining CONFIG_64BIT_TIME in kernel should be > > replaced with CONFIG_COMPAT_32BIT_TIME or removed. > > We should remove CONFIG_64BIT_TIME. CONFIG_COMPAT_32BIT_TIME > is still needed to identify architectures that don't have it, in > particular riscv32. > > Arnd 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 9eed500cf1..33eee8eb9a 100644 --- a/include/time.h +++ b/include/time.h @@ -124,6 +124,14 @@ extern __time64_t __timegm64 (struct tm *__tp) __THROW; libc_hidden_proto (__timegm64) #endif +#if __TIMESIZE == 64 +# define __clock_settime64 __clock_settime +#else +extern int __clock_settime64 (clockid_t clock_id, + const struct __timespec64 *tp); +libc_hidden_proto (__clock_settime64) +#endif + /* Compute the `struct tm' representation of T, offset OFFSET seconds east of UTC, and store year, yday, mon, mday, wday, hour, min, sec into *TP. diff --git a/sysdeps/unix/sysv/linux/clock_settime.c b/sysdeps/unix/sysv/linux/clock_settime.c index d837e3019c..a1ea1cac28 100644 --- a/sysdeps/unix/sysv/linux/clock_settime.c +++ b/sysdeps/unix/sysv/linux/clock_settime.c @@ -19,11 +19,9 @@ #include <sysdep.h> #include <time.h> -#include "kernel-posix-cpu-timers.h" - /* Set CLOCK to value TP. */ int -__clock_settime (clockid_t clock_id, const struct timespec *tp) +__clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp) { /* Make sure the time cvalue is OK. */ if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000) @@ -32,6 +30,36 @@ __clock_settime (clockid_t clock_id, const struct timespec *tp) return -1; } +#if defined (__TIMESIZE) && __TIMESIZE != 64 +# if defined __NR_clock_settime64 + /* Make sure that passed __timespec64 struct pad is 0. */ + struct __timespec64 ts = *tp; + ts.tv_pad = 0; + return INLINE_SYSCALL_CALL (clock_settime64, clock_id, &ts); +# else + struct timespec ts32; + valid_timespec64_to_timespec(tp, &ts32); + return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32); +# endif +#else return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp); +#endif } weak_alias (__clock_settime, clock_settime) + +#if __TIMESIZE != 64 +int +__clock_settime (clockid_t clock_id, const struct timespec *tp) +{ + struct __timespec64 ts64; + + if (! in_time_t_range (tp->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + valid_timespec_to_timespec64 (tp, &ts64); + return __clock_settime64 (clock_id, &ts64); +} +#endif