lkml.org 
[lkml]   [2019]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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]
\
 
 \ /
  Last update: 2019-04-23 17:46    [W:0.115 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site