Messages in this thread | | | Date | Tue, 23 Apr 2019 17:45:24 +0200 | From | Lukasz Majewski <> | Subject | Re: [PATCH 3/6] y2038: linux: Provide __clock_settime64 implementation |
| |
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 [unhandled content-type:application/pgp-signature] | |