Re: [PATCH 3/6] y2038: linux: Provide __clock_settime64 implementation

Re: [PATCH 3/6] y2038: linux: Provide __clock_settime64 implementation

[Stepan Golosunov]

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?

Re: [PATCH 3/6] y2038: linux: Provide __clock_settime64 implementation

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/6] y2038: linux: Provide __clock_settime64 implementation

[Stepan Golosunov]

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.

Re: [PATCH 3/6] y2038: linux: Provide __clock_settime64 implementation

[Arnd Bergmann]

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

Re: [PATCH 3/6] y2038: linux: Provide __clock_settime64 implementation

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 3764 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/7] y2038: Introduce __ASSUME_64BIT_TIME define

[Stepan Golosunov]

02.05.2019 в 15:04:18 +0000 Joseph Myers написал:
> On Tue, 30 Apr 2019, Lukasz Majewski wrote:
> 
> >  - The need for explicit clearing padding when calling syscalls (as to
> >    be better safe than sorry in the future - there was related
> >    discussion started by Stepan).
> 
> This really isn't a difficult question.  What it comes down to is whether 
> the Linux kernel, in the first release version with these syscalls (we 
> don't care about old -rc versions; what matters is the actual 5.1 
> release), ignores the padding.
> 
> If 5.1 *release* ignores the padding, that is part of the kernel/userspace 
> ABI, in accordance with the kernel principle of not breaking userspace.  
> Thus, it is something userspace can rely on, now and in the future.
> 
> If 5.1 release does not ignore the padding, syscall presence does not mean 
> the padding is ignored by the kernel and so glibc needs to clear padding.  
> Of course, it needs to clear padding in a *copy* of the value provided by 
> the user unless the glibc API in question requires the timespec value in 
> question to be in writable memory.
> 
> So, which is (or will be) the case in 5.1 release?  Padding ignored or 
> not?  If more complicated (ignored for some architectures / ABIs but not 
> for others, or depending on whether compat syscalls are in use), then say 
> so - give a precise description of the exact circumstances under which the 
> padding around a 32-bit tv_nsec will or will not be ignored by the kernel 
> on input from userspace.

In current linux git it looks like padding is correctly ignored in
32-bit kernels (because kernel itself has 32-bit tv_nsec there) but
the code to clear it on compat syscalls in 64-bit kernels seems to be
broken.

The patch to fix this is at

https://lore.kernel.org/lkml/20190429131951.471701-1-arnd@arndb.de/

but it doesn't seem like it has reached Linus yet.


(Hmm.  I think that old ipc and socketcall syscalls in 32-bit kernels
are broken without that patch too.  They would try to read
__kernel_timespec when callers are passing old_timespec32.)

Re: [PATCH v2 2/7] y2038: Introduce __ASSUME_64BIT_TIME define

[Lukasz Majewski]

[-- Attachment #1: Type: text/plain, Size: 2903 bytes --]

On Sun, 5 May 2019 18:10:54 +0400
Stepan Golosunov <stepan@golosunov.pp.ru> wrote:

> 02.05.2019 в 15:04:18 +0000 Joseph Myers написал:
> > On Tue, 30 Apr 2019, Lukasz Majewski wrote:
> >   
> > >  - The need for explicit clearing padding when calling syscalls
> > > (as to be better safe than sorry in the future - there was related
> > >    discussion started by Stepan).  
> > 
> > This really isn't a difficult question.  What it comes down to is
> > whether the Linux kernel, in the first release version with these
> > syscalls (we don't care about old -rc versions; what matters is the
> > actual 5.1 release), ignores the padding.
> > 
> > If 5.1 *release* ignores the padding, that is part of the
> > kernel/userspace ABI, in accordance with the kernel principle of
> > not breaking userspace. Thus, it is something userspace can rely
> > on, now and in the future.
> > 
> > If 5.1 release does not ignore the padding, syscall presence does
> > not mean the padding is ignored by the kernel and so glibc needs to
> > clear padding. Of course, it needs to clear padding in a *copy* of
> > the value provided by the user unless the glibc API in question
> > requires the timespec value in question to be in writable memory.
> > 
> > So, which is (or will be) the case in 5.1 release?  Padding ignored
> > or not?  If more complicated (ignored for some architectures / ABIs
> > but not for others, or depending on whether compat syscalls are in
> > use), then say so - give a precise description of the exact
> > circumstances under which the padding around a 32-bit tv_nsec will
> > or will not be ignored by the kernel on input from userspace.  
> 
> In current linux git it looks like padding is correctly ignored in
> 32-bit kernels (because kernel itself has 32-bit tv_nsec there) but
> the code to clear it on compat syscalls in 64-bit kernels seems to be
> broken.
> 
> The patch to fix this is at
> 
> https://lore.kernel.org/lkml/20190429131951.471701-1-arnd@arndb.de/
> 
> but it doesn't seem like it has reached Linus yet.
> 

I hope that this patch will be pulled soon (before final cut) - for that
reason we can assume that the padding is ignored by the kernel and
hence do not explicitly clear it in glibc (as it was done in sent
patches)

> 
> (Hmm.  I think that old ipc and socketcall syscalls in 32-bit kernels
> are broken without that patch too.  They would try to read
> __kernel_timespec when callers are passing old_timespec32.)

Please correct me if I'm wrong, but this problem is related to x32
machines (and not to ARM 32 bit ones with 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]