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: [email protected]
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
<[email protected]> 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
> <[email protected]> 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: [email protected]