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

[Stepan Golosunov <stepan@golosunov.pp.ru>]

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.