Re: [RFC v2 03/20] y2038: linux: Provide __clock_settime64 implementation

[Lukasz Majewski <lukma at denx dot de>]

Hi Arnd,

> On Tue, Jun 25, 2019 at 5:51 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > On Tue, Jun 25, 2019 at 2:11 AM Alistair Francis
> > > <alistair.francis@wdc.com> wrote:
> > >  
> > > >  weak_alias (__clock_settime, clock_settime)
> > > > +
> > > > +#if __TIMESIZE != 64
> > > > +int
> > > > +__clock_settime (clockid_t clock_id, const struct timespec *tp)
> > > > +{
> > > > +  struct __timespec64 ts64;
> > > > +
> > > > +  valid_timespec_to_timespec64 (tp, &ts64);
> > > > +  return __clock_settime64 (clock_id, &ts64);
> > > > +}
> > > > +#endif  
> > >
> > > I missed this when Lukasz first posted this, but I would still
> > > prefer this to be changed.
> > >
> > > Having clock_settime() (using the weak_alias) call into
> > > __clock_settime64() means that a kernel that warns about
> > > old user space calling into the old syscall never notices this.  
> >
> > Could you be more specific here?
> >
> >
> > I thought that we do not care about the legacy systems (i.e. those
> > which don't switch to 64 bit time syscalls before Y2038).
> >
> > The assumption is to not break anything and use 64 bit time related
> > syscalls after v5.1 kernel (i.e. clock_settime64 and friends on
> > systems with __WORDSIZE=32).
> >
> > Syscalls, which handle only 32 bit time would be used as fallback.  
> 
> I don't mind falling back on the 32-bit implementation from a time64
> syscall when running on the old kernel, that part is required to make
> new binaries run on pre-5.1 kernel.
> 
> Your __clock_settime however does the reverse: you have an application
> that calls clock_settime(), the alias redirects that to
> __clock_settime(), and that converts it into the 64-bit structure and
> passes it into __clock_settime64(), which then calls the time64
> syscall before falling back to calling the time32 syscall.

This patch focuses on the situation where we strive to provide/use 64
bit time support and try not to rely on 32 bit interfaces.

> 
> This is problematic in the scenario that you have an embedded system
> you deploy today, and turn off the time32 syscalls in the kernel.

I assume that then we would only have __NR_clock_settime64 defined (no
__NR_clock_settime available) on WORDSIZE==32 archs?

> All applications are built against the time64 glibc interfaces, except
> for one tool that someone forgot. This calls the old clock_settime()
> with a 32-bit time, which gets converted into and passed into
> clock_settime64 in the kernel where it successfully sets the time at
> boot.

Interesting use case. Thanks for sharing it.

> 
> In 2038, it stops working because of the time_t overflow that was
> not caught during validation. If we call the time32 interface here, it
> breaks immediately on kernels that return -ENOSYS from
> clock_gettime(), 

Maybe I'm not aware of something, but isn't the removal of
clock_settime syscall supporting only 32 bit time (on archs with
WORDSIZE==32) the ABI break?

Shouldn't those syscalls be kept until the minimal supported glibc
kernel version is 5.1?

> which makes the validation easier and more reliable.
> 
> > > Can you please leave the existing clock_settime()  
> >
> > I think that some pseudo code would shed some more light on your
> > idea.  
> 
> What I mean is to have
> 
> #ifdef __NR_clock_settime
> int
> __clock_settime (clockid_t clock_id, const struct timespec *tp)
> {
>   return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> }
> #endif
> 
> This will do the right thing on 64-bit architectures (which use time64
> always), on new 32-bit architectures (not provide __clock_settime
> at all, clock_settime() should be an alias for __clock_settime64())
> and on old 32-bit architectures (work as expected on existing
> kernels, fail with -ENOSYS when we want it to).
> 

The latest patch for clock_settime [1]:
Could be changed to:

#if __TIMESIZE != 64
int
__clock_settime (clockid_t clock_id, const struct timespec *tp)
{
/* For WORDSIZE==32 systems the headers could still have defined
__NR_clock_settime, but the kernel itself may not support it anymore */
#ifdef __NR_clock_settime
  return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
#endif

  struct __timespec64 ts64;

  valid_timespec_to_timespec64 (tp, &ts64);
  return __clock_settime64 (clock_id, &ts64);
}
#endif


However, there is the problem that in some point in time the glibc will
switch to 64 bit __TIMESIZE only (probably when minimal kernel version
for glibc would be grater than 5.1) and all __clock_settime syscalls
would be served with __clock_settime64 (as 64 bit time support is
always in place).

After this switch the "unconverted" program will setup wrong time.


> > and most notably from:
> > https://sourceware.org/glibc/wiki/Y2038ProofnessDesign#clock_gettime.28.29
> > recommended by glibc for the syscalls conversion for 64 bit.
> >
> > It also uses the __ASSUME_TIME64_SYSCALLS flag.  
> 
> I thought it was covered in there, but maybe that changed.

This flag is supposed to indicate if the kernel supports 64 bit time
interface (either with clock_settime syscall on WORDSIZE==64 or
clock_settime64 on WORDSIZE==32, including x32).

The newest proposition for it: [2] 

> 
>       Arnd


Note:

[1] - https://patchwork.ozlabs.org/patch/1107235/
[2] - https://patchwork.ozlabs.org/patch/1117100/



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: pgpsfpoCs6L9W.pgp
Description: OpenPGP digital signature