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

[Arnd Bergmann <arnd@arndb.de>]

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 is problematic in the scenario that you have an embedded system
you deploy today, and turn off the time32 syscalls in the kernel.
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.

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(), 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).

> 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.

      Arnd