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

  From: Lukasz Majewski <lukma@denx.de>

This patch provides new __clock_settime64 explicit 64 bit function for
setting the time. Moreover, a 32 bit version - __clock_settime - has been
refactored to internally use __clock_settime64.

The __clock_settime is now supposed to be used on systems still supporting
32 bit time (__TIMESIZE != 64) - hence the necessary conversion to 64 bit
struct timespec.

The new clock_settime64 syscall available from Linux 5.1+ has been used,
when applicable.

The __ASSUME_TIME64_SYSCALLS flag indicates if the Linux kernel supports
64 bit version of clock_settime (i.e. clock_settime64).
If this flag is not defined, the fallback with legacy clock_settime
supporting 32 bit time is used.

When working on 32 bit systems without Y2038 time support the
clock_settime64 returns error when one wants to set time with wrong
(overflowed) tv_sec value. Moreover, the correctness of tv_nsec is also
checked.

In this patch the internal padding (tv_pad) of struct __timespec64 is
left untouched (on systems with __WORDSIZE == 32) as Linux kernel ignores
upper 32 bits of tv_nsec.

Tests:
- The code has been tested with x86_64/x86 (native compilation):
make PARALLELMFLAGS="-j8" && make xcheck PARALLELMFLAGS="-j8"

- Run specific tests on ARM/x86 32bit systems (qemu):
https://github.com/lmajewski/meta-y2038
and run tests:
https://github.com/lmajewski/y2038-tests/commits/master
on kernels with and without 64 bit time support.

No regressions were observed.

* include/time.h (__clock_settime64):
  Add __clock_settime alias according to __TIMESIZE define
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime):
  Refactor this function to be used only on 32 bit machines as a wrapper
  on __clock_settime64.
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime64): Add
* sysdeps/unix/sysv/linux/clock_settime.c (__clock_settime64):
  Use clock_settime64 kernel syscall (available from 5.1-rc1+ Linux)
---
 include/time.h                          |  8 ++++++
 sysdeps/unix/sysv/linux/clock_settime.c | 37 ++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 4 deletions(-)
  

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.

Can you please leave the existing clock_settime()
untouched and instead just change the internal callers
(if any) over to __clock_settime64?

      Arnd
  

Hi Arnd,

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

> 
> Can you please leave the existing clock_settime()

I think that some pseudo code would shed some more light on your idea.

> untouched and instead just change the internal callers
> (if any) over to __clock_settime64?

The above approach (from this patch) was developed to comply with
following recommendations:

https://www.gnu.org/software/libc/manual/html_mono/libc.html
"D.2.1 64-bit time symbol handling in the GNU C Library"
to convert *clock_settime*.

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.

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

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
  

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
  

On Wed, Jun 26, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de> wrote:
> > 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:
> > 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?

No, the kernel header files are generated independently of the configuration.
The macro would still be there at compile-time, but depending on kernel
configuration, the system call would return -ENOSYS, same way we do
for other optional system calls.

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

We will probably keep them as an option in the kernel until 2038,
but leave it to the distro or embedded system design to turn them
on or off. Most of the remaining general-purpose distros (Ubuntu
just said they'd stop theirs, others are likely to follow) are likely to
leave them on for compatibility, while embedded systems with
projected life times beyond 2038 should turn them off.

The minimal kernel version supported by glibc doesn't matter
to the kernel, as kernels support older C libraries going back
to the beginning, just like newer glibc versions support
applications linked against earlier glibc versions.

> 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

The "#ifdef __NR_clock_settime" is always true when __TIMESIZE != 64,
so that could be simplified to the version I suggested.

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

I don't understand. What does it mean to switch to __TIMESIZE=64?
Would that not break all existing binaries regardless of the implementation?

I would expect that the only thing changing after the minimum
kernel version is updated is the fallback from the public time64
interfaces to the time32 system calls, but glibc cannot drop the
public time32 interfaces as long as someone might be using those,
i.e. just before the 2038 overflow. In no case would an existing
lilbrary symbol change the data type of its arguments though.

Between adding the time64 system calls (now) and removing
the time32 system calls (in y2038), there are a couple of
intermediate steps that can be years apart, or all happen
at the same time:

- drop support for building with pre-5.1 kernel headers
- stop running on pre-5.1 kernels
- change the default for newly compiled code to time64
- drop support for building with time32

      Arnd
  

Hi Arnd,

> On Wed, Jun 26, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de>
> wrote:
> > > 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:  
> > > 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?  
> 
> No, the kernel header files are generated independently of the
> configuration. The macro would still be there at compile-time, but
> depending on kernel configuration, the system call would return
> -ENOSYS, same way we do for other optional system calls.
> 
> > > 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?  
> 
> We will probably keep them as an option in the kernel until 2038,
> but leave it to the distro or embedded system design to turn them
> on or off.

Isn't this the ABI break on demand ?

> Most of the remaining general-purpose distros (Ubuntu
> just said they'd stop theirs, others are likely to follow) are likely
> to leave them on for compatibility, while embedded systems with
> projected life times beyond 2038 should turn them off.
> 
> The minimal kernel version supported by glibc doesn't matter
> to the kernel, as kernels support older C libraries going back
> to the beginning, just like newer glibc versions support
> applications linked against earlier glibc versions.
> 
> > 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  
> 
> The "#ifdef __NR_clock_settime" is always true when __TIMESIZE != 64,
> so that could be simplified to the version I suggested.

For following setup: WORDSIZE=32, TIMESIZE=32 and kernel with
__NR_clock_settime returning -ENOSYS (disabled by embedded system
designer in the kernel), but supporting __NR_clock_settime64 (for
example 32 bit ARM):

The policy question - shall the user space binary after calling
clock_settime:

1. Receive -ENOSYS as the __NR_clock_settime was used to fulfill the
request

or

2. Receive 0 (operation succeed) as the available __NR_clock_settime64
has been used to perform the requested operation.


For the proposed patch (clock_settime) - the option 2 is now performed.


> 
> > 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.  
> 
> I don't understand. What does it mean to switch to __TIMESIZE=64?
> Would that not break all existing binaries regardless of the
> implementation?

__TIMESIZE represents the size of time_t (the glibc internal time
representation). This change would be probably performed in the future
after the _TIME_BITS==64 is defined by default.

However, I do not feel competent enough to speak about glibc long term
plans.

> 
> I would expect that the only thing changing after the minimum
> kernel version is updated is the fallback from the public time64
> interfaces to the time32 system calls, but glibc cannot drop the
> public time32 interfaces as long as someone might be using those,
> i.e. just before the 2038 overflow. In no case would an existing
> lilbrary symbol change the data type of its arguments though.

Yes, correct. 

> 
> Between adding the time64 system calls (now) and removing
> the time32 system calls (in y2038), there are a couple of
> intermediate steps that can be years apart, or all happen
> at the same time:
> 
> - drop support for building with pre-5.1 kernel headers
> - stop running on pre-5.1 kernels
> - change the default for newly compiled code to time64
> - drop support for building with time32
> 
>       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
  

* Lukasz Majewski:

> Hi Arnd,
>
>> We will probably keep them as an option in the kernel until 2038,
>> but leave it to the distro or embedded system design to turn them
>> on or off.
>
> Isn't this the ABI break on demand ?

Yes, it is.  It happened with vsyscall, too.

Stuff like that is only feasible if you expect that a dynamically linked
libc is the ultimate ABI boundary for applications.  I don't think
that's realistic or actually intended, especially for things like
futexes.

> For following setup: WORDSIZE=32, TIMESIZE=32 and kernel with
> __NR_clock_settime returning -ENOSYS (disabled by embedded system
> designer in the kernel), but supporting __NR_clock_settime64 (for
> example 32 bit ARM):
>
> The policy question - shall the user space binary after calling
> clock_settime:
>
> 1. Receive -ENOSYS as the __NR_clock_settime was used to fulfill the
> request
>
> or
>
> 2. Receive 0 (operation succeed) as the available __NR_clock_settime64
> has been used to perform the requested operation.
>
>
> For the proposed patch (clock_settime) - the option 2 is now performed.

In my opinion, an existing 32-bit architecture which does not provide
the clock_settime system call (that it has provided before) is just
broken, and will be so at least until 2038.

We can add the fallback code in glibc, but that will only give people
the wrong idea that they can disable the 32-bit system calls.  It's just
not true.

Thanks,
Florian
  

On Wed, Jun 26, 2019 at 5:03 PM Lukasz Majewski <lukma@denx.de> wrote:
> > On Wed, Jun 26, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de> wrote:
> > > > On Tue, Jun 25, 2019 at 5:51 PM Lukasz Majewski <lukma@denx.de>
> > >
> > > Shouldn't those syscalls be kept until the minimal supported glibc
> > > kernel version is 5.1?
> >
> > We will probably keep them as an option in the kernel until 2038,
> > but leave it to the distro or embedded system design to turn them
> > on or off.
>
> Isn't this the ABI break on demand ?

It's removing an unused ABI in order to ensure that nothing uses it.
The policy is to never break things that users rely on, so you should
only disable the system calls if you control all of user space and want
to ensure that whatever works now doesn't break in the future.

> > The "#ifdef __NR_clock_settime" is always true when __TIMESIZE != 64,
> > so that could be simplified to the version I suggested.
>
> For following setup: WORDSIZE=32, TIMESIZE=32 and kernel with
> __NR_clock_settime returning -ENOSYS (disabled by embedded system
> designer in the kernel), but supporting __NR_clock_settime64 (for
> example 32 bit ARM):
>
> The policy question - shall the user space binary after calling
> clock_settime:
>
> 1. Receive -ENOSYS as the __NR_clock_settime was used to fulfill the
> request
>
> or
>
> 2. Receive 0 (operation succeed) as the available __NR_clock_settime64
> has been used to perform the requested operation.
>
>
> For the proposed patch (clock_settime) - the option 2 is now performed.

I would prefer to fail as early as possible, maybe even a failure from the
ld.so when loading a binary that links against clock_settime() on a kernel that
doesn't have the system call. That's probably too hard to implement,
so abort(), or return -ENOSYS.

Definitely don't return success, that would defeat the purpose of
disabling the syscall in the kernel, and just lead to silent incorrect
data.

> > > 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.
> >
> > I don't understand. What does it mean to switch to __TIMESIZE=64?
> > Would that not break all existing binaries regardless of the
> > implementation?
>
> __TIMESIZE represents the size of time_t (the glibc internal time
> representation). This change would be probably performed in the future
> after the _TIME_BITS==64 is defined by default.

I'm still not following. Which of steps listed below would that correspond
to? I guess the last one, right?

> > Between adding the time64 system calls (now) and removing
> > the time32 system calls (in y2038), there are a couple of
> > intermediate steps that can be years apart, or all happen
> > at the same time:
> >
> > - drop support for building with pre-5.1 kernel headers
> > - stop running on pre-5.1 kernels
> > - change the default for newly compiled code to time64
> > - drop support for building with time32

       Arnd
  

Hi Florian,

> * Lukasz Majewski:
> 
> > Hi Arnd,
> >  
> >> We will probably keep them as an option in the kernel until 2038,
> >> but leave it to the distro or embedded system design to turn them
> >> on or off.  
> >
> > Isn't this the ABI break on demand ?  
> 
> Yes, it is.  It happened with vsyscall, too.
> 
> Stuff like that is only feasible if you expect that a dynamically
> linked libc is the ultimate ABI boundary for applications.  I don't
> think that's realistic or actually intended, especially for things
> like futexes.
> 
> > For following setup: WORDSIZE=32, TIMESIZE=32 and kernel with
> > __NR_clock_settime returning -ENOSYS (disabled by embedded system
> > designer in the kernel), but supporting __NR_clock_settime64 (for
> > example 32 bit ARM):
> >
> > The policy question - shall the user space binary after calling
> > clock_settime:
> >
> > 1. Receive -ENOSYS as the __NR_clock_settime was used to fulfill the
> > request
> >
> > or
> >
> > 2. Receive 0 (operation succeed) as the available
> > __NR_clock_settime64 has been used to perform the requested
> > operation.
> >
> >
> > For the proposed patch (clock_settime) - the option 2 is now
> > performed.  
> 
> In my opinion, an existing 32-bit architecture which does not provide
> the clock_settime system call (that it has provided before) is just
> broken, and will be so at least until 2038.

Am I correct, that you opt for first option that glibc shall return
just -ENOSYS in that case?


> 
> We can add the fallback code in glibc, but that will only give people
> the wrong idea that they can disable the 32-bit system calls.  It's
> just not true.
> 
> Thanks,
> Florian




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
  

Hi Arnd,

> On Wed, Jun 26, 2019 at 5:03 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > On Wed, Jun 26, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > > > On Tue, Jun 25, 2019 at 5:51 PM Lukasz Majewski
> > > > > <lukma@denx.de>  
> > > >
> > > > Shouldn't those syscalls be kept until the minimal supported
> > > > glibc kernel version is 5.1?  
> > >
> > > We will probably keep them as an option in the kernel until 2038,
> > > but leave it to the distro or embedded system design to turn them
> > > on or off.  
> >
> > Isn't this the ABI break on demand ?  
> 
> It's removing an unused ABI in order to ensure that nothing uses it.
> The policy is to never break things that users rely on, so you should
> only disable the system calls if you control all of user space and
> want to ensure that whatever works now doesn't break in the future.

In other words use glibc to help with validation of the system by
disabling (by kernel option) the time32 syscalls and check what happens?

> 
> > > The "#ifdef __NR_clock_settime" is always true when __TIMESIZE !=
> > > 64, so that could be simplified to the version I suggested.  
> >
> > For following setup: WORDSIZE=32, TIMESIZE=32 and kernel with
> > __NR_clock_settime returning -ENOSYS (disabled by embedded system
> > designer in the kernel), but supporting __NR_clock_settime64 (for
> > example 32 bit ARM):
> >
> > The policy question - shall the user space binary after calling
> > clock_settime:
> >
> > 1. Receive -ENOSYS as the __NR_clock_settime was used to fulfill the
> > request
> >
> > or
> >
> > 2. Receive 0 (operation succeed) as the available
> > __NR_clock_settime64 has been used to perform the requested
> > operation.
> >
> >
> > For the proposed patch (clock_settime) - the option 2 is now
> > performed.  
> 
> I would prefer to fail as early as possible, maybe even a failure
> from the ld.so when loading a binary that links against
> clock_settime() on a kernel that doesn't have the system call. That's
> probably too hard to implement, so abort(), or return -ENOSYS.

I see your point. Let's wait for other glibc developers opinion.

> 
> Definitely don't return success, that would defeat the purpose of
> disabling the syscall in the kernel, and just lead to silent incorrect
> data.
> 
> > > > 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.  
> > >
> > > I don't understand. What does it mean to switch to __TIMESIZE=64?
> > > Would that not break all existing binaries regardless of the
> > > implementation?  
> >
> > __TIMESIZE represents the size of time_t (the glibc internal time
> > representation). This change would be probably performed in the
> > future after the _TIME_BITS==64 is defined by default.  
> 
> I'm still not following. Which of steps listed below would that
> correspond to? I guess the last one, right?
> 
> > > Between adding the time64 system calls (now) and removing
> > > the time32 system calls (in y2038), there are a couple of
> > > intermediate steps that can be years apart, or all happen
> > > at the same time:
> > >
> > > - drop support for building with pre-5.1 kernel headers
> > > - stop running on pre-5.1 kernels
        ^^^ __ASSUME_TIME32_SYSCALLS flag which will be always true then

> > > - change the default for newly compiled code to time64
        ^^^ The _TIME_BITS==64 set by default (in glibc) for all
        compiled programs


> > > - drop support for building with time32  
        ^^^ TIMESIZE == 64 (no support for time32)

(But this need ACK from e.g. Joseph if I understood it correctly).

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

* Lukasz Majewski:

>> In my opinion, an existing 32-bit architecture which does not provide
>> the clock_settime system call (that it has provided before) is just
>> broken, and will be so at least until 2038.
>
> Am I correct, that you opt for first option that glibc shall return
> just -ENOSYS in that case?

I think it's undefined.  For clock_settime, the only useful outcome is
-1 with errno set to ENOSYS.  However, there are some system calls which
cannot really fail (time, for instance), and there I would say that
behavior is simply undefined.

Thanks,
Florian
  

Hi Florian,

> * Lukasz Majewski:
> 
> >> In my opinion, an existing 32-bit architecture which does not
> >> provide the clock_settime system call (that it has provided
> >> before) is just broken, and will be so at least until 2038.  
> >
> > Am I correct, that you opt for first option that glibc shall return
> > just -ENOSYS in that case?  
> 
> I think it's undefined.  For clock_settime, the only useful outcome is
> -1 with errno set to ENOSYS.  However, there are some system calls
> which cannot really fail (time, for instance), and there I would say
> that behavior is simply undefined.

Ok. I see. So the clock_settime syscall shall return from glibc with -1
and errno set to ENOSYS (I shall NOT try to set the time with
clock_settime64 bit syscall in this case).

Thanks for explanation, appreciated.

> 
> Thanks,
> Florian




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
  

On Thu, Jun 27, 2019 at 12:55 AM Lukasz Majewski <lukma@denx.de> wrote:
> > On Wed, Jun 26, 2019 at 5:03 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > > On Wed, Jun 26, 2019 at 11:07 AM Lukasz Majewski <lukma@denx.de>
> > > > wrote:
> > > > > > On Tue, Jun 25, 2019 at 5:51 PM Lukasz Majewski
> > > > > > <lukma@denx.de>
> > > > >
> > > > > Shouldn't those syscalls be kept until the minimal supported
> > > > > glibc kernel version is 5.1?
> > > >
> > > > We will probably keep them as an option in the kernel until 2038,
> > > > but leave it to the distro or embedded system design to turn them
> > > > on or off.
> > >
> > > Isn't this the ABI break on demand ?
> >
> > It's removing an unused ABI in order to ensure that nothing uses it.
> > The policy is to never break things that users rely on, so you should
> > only disable the system calls if you control all of user space and
> > want to ensure that whatever works now doesn't break in the future.
>
> In other words use glibc to help with validation of the system by
> disabling (by kernel option) the time32 syscalls and check what happens?

Yes. [ Even better would be a compile-time option to just leave out
the time32 support from glibc entirely, but as I understand that will
not be possible until most users have moved over to time64,
and users can't wait for that to do validation on products shipping now. ]

Mostly this means to just not put in code to support a case that
nobody cares about (running time32 applications on pure time64
systems), so I would not expect this to cause extra effort in development,
just a little care with the implementation.


> > > > 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.
>
> I'm still not following. Which of steps listed below would that
> correspond to? I guess the last one, right?
>
> > > > - drop support for building with time32
>         ^^^ TIMESIZE == 64 (no support for time32)
>
> (But this need ACK from e.g. Joseph if I understood it correctly).

I still don't see how that would break anything: if applications
are all built with 64-bit time_t, they would not call into the old
__clock_settime() function, which is then only kept around
for compatibility with old binaries and would still operate on
32-bit time_t and use __NR_clock_settime.

      Arnd
  

Hi Arnd

> On Thu, Jun 27, 2019 at 12:55 AM Lukasz Majewski <lukma@denx.de>
> wrote:
> > > On Wed, Jun 26, 2019 at 5:03 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > > > On Wed, Jun 26, 2019 at 11:07 AM Lukasz Majewski
> > > > > <lukma@denx.de> wrote:  
> > > > > > > On Tue, Jun 25, 2019 at 5:51 PM Lukasz Majewski
> > > > > > > <lukma@denx.de>  
> > > > > >
> > > > > > Shouldn't those syscalls be kept until the minimal supported
> > > > > > glibc kernel version is 5.1?  
> > > > >
> > > > > We will probably keep them as an option in the kernel until
> > > > > 2038, but leave it to the distro or embedded system design to
> > > > > turn them on or off.  
> > > >
> > > > Isn't this the ABI break on demand ?  
> > >
> > > It's removing an unused ABI in order to ensure that nothing uses
> > > it. The policy is to never break things that users rely on, so
> > > you should only disable the system calls if you control all of
> > > user space and want to ensure that whatever works now doesn't
> > > break in the future.  
> >
> > In other words use glibc to help with validation of the system by
> > disabling (by kernel option) the time32 syscalls and check what
> > happens?  
> 
> Yes. [ Even better would be a compile-time option to just leave out
> the time32 support from glibc entirely, but as I understand that will
> not be possible until most users have moved over to time64,
> and users can't wait for that to do validation on products shipping
> now. ]
> 
> Mostly this means to just not put in code to support a case that
> nobody cares about (running time32 applications on pure time64
> systems), so I would not expect this to cause extra effort in
> development, just a little care with the implementation.
> 
> 
> > > > > 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.  
> >
> > I'm still not following. Which of steps listed below would that
> > correspond to? I guess the last one, right?
> >  
> > > > > - drop support for building with time32  
> >         ^^^ TIMESIZE == 64 (no support for time32)
> >
> > (But this need ACK from e.g. Joseph if I understood it correctly).  
> 
> I still don't see how that would break anything: if applications
> are all built with 64-bit time_t, they would not call into the old
> __clock_settime() function, which is then only kept around
> for compatibility with old binaries and would still operate on
> 32-bit time_t and use __NR_clock_settime.
> 
>       Arnd

After the discussion I do believe that it would be correct to change
the proposed patch [1] '__clock_settime' to:

#if __TIMESIZE != 64
int
__clock_settime (clockid_t clock_id, const struct timespec *tp)
{
/* For archs with WORDSIZE==32, which do not support clock_settime64
the clock_settime supporting 32 bit time ABI will be used */

return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
}
#endif  


The "__clock_settime64()" part could be left untouched.


Note:

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

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
  

On Thu, Jun 27, 2019 at 12:36 PM Lukasz Majewski <lukma@denx.de> wrote:
> > On Thu, Jun 27, 2019 at 12:55 AM Lukasz Majewski <lukma@denx.de>

> After the discussion I do believe that it would be correct to change
> the proposed patch [1] '__clock_settime' to:
>
> #if __TIMESIZE != 64

This would hide the function on 64-bit architectures, right?

Maybe
#if (__TIMESIZE == __WORDSIZE) || (defined __x86_64__ && defined __ILP32__)

or shortening that by adding a new macro

#if __ASSUME_TIME32_SYSCALLS

> int
> __clock_settime (clockid_t clock_id, const struct timespec *tp)
> {
> /* For archs with WORDSIZE==32, which do not support clock_settime64
> the clock_settime supporting 32 bit time ABI will be used */

Same for the comment: __clock_settime would be used both
on 64-bit architectures as the normal path, and for compatiblity
with applications built without _TIME_BITS=64.

> return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> }
> #endif
>
> The "__clock_settime64()" part could be left untouched.

Ok.

      Arnd
  

Hi Arnd,

> On Thu, Jun 27, 2019 at 12:36 PM Lukasz Majewski <lukma@denx.de>
> wrote:
> > > On Thu, Jun 27, 2019 at 12:55 AM Lukasz Majewski <lukma@denx.de>  
> 
> > After the discussion I do believe that it would be correct to change
> > the proposed patch [1] '__clock_settime' to:
> >
> > #if __TIMESIZE != 64  
> 
> This would hide the function on 64-bit architectures, right?

Yes, this function would be hidden on architectures with __WORDSIZE==64
(e.g. x86_64) (+ x32 ).

> 
> Maybe
> #if (__TIMESIZE == __WORDSIZE) || (defined __x86_64__ && defined
> __ILP32__)
> 
> or shortening that by adding a new macro
> 
> #if __ASSUME_TIME32_SYSCALLS

Please see below comment.

> 
> > int
> > __clock_settime (clockid_t clock_id, const struct timespec *tp)
> > {
> > /* For archs with WORDSIZE==32, which do not support clock_settime64
> > the clock_settime supporting 32 bit time ABI will be used */  
> 
> Same for the comment: __clock_settime would be used both
> on 64-bit architectures as the normal path, and for compatiblity
> with applications built without _TIME_BITS=64.

With the patch [1] - it would be used in such a way:

1. For archs with WORDSIZE==32 and __TIMESIZE==32 (legacy ones - e.g.
arm 32 bits)

#if __TIMESIZE != 64
int
__clock_settime (clockid_t clock_id, const struct timespec *tp)
{
	return clock_settime(....)
}
#endif


2. For arch with WORDSIZE==64 (and WORDSIZE_SYSCALL=64 -> x32) and
__TIMESIZE==64

__clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
{
#ifdef __ASSUME_TIME64_SYSCALLS
# ifndef __NR_clock_settime64
#  define __NR_clock_settime64 __NR_clock_settime
# endif
  return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
#else
....


}

The __ASSUME_TIME64_SYSCALLS is always defined in this case [2]

Here the assumption is that for such architectures (like aarch64 or
x86_64) the __NR_clock_settime64 (if ever introduced) has the same ABI
as __NR_clock_settime, so there will be no need to adjust the glibc code.


3. New archs added or ones being Y2038 safe (with __NR_clock_settime64
available in the kernel -> starting from 5.1+).

For them the __ASSUME_TIME64_SYSCALLS is defined [2] and execution
patch from point 2. is used with potential fallback to 32 bit version
of clock_settime:

__clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
{
#ifdef __ASSUME_TIME64_SYSCALLS
# ifndef __NR_clock_settime64
#  define __NR_clock_settime64 __NR_clock_settime
# endif
  return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
#else
# ifdef __NR_clock_settime64
  int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
  if (ret == 0 || errno != ENOSYS)
    return ret;
# endif
  if (! in_time_t_range (tp->tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }

  struct timespec ts32;
  valid_timespec64_to_timespec (tp, &ts32);
  return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
#endif
}

Note for the above code:

The __NR_clock_settime64 is used whenever possible and if not available
the fallback to 32 bit time supporting clock_settime is executed (with
overflow check).


> 
> > return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> > }
> > #endif
> >
> > The "__clock_settime64()" part could be left untouched.  
> 
> Ok.
> 
>       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
  

On Thu, Jun 27, 2019 at 4:08 PM Lukasz Majewski <lukma@denx.de> wrote:
> > On Thu, Jun 27, 2019 at 12:36 PM Lukasz Majewski <lukma@denx.de> wrote:
> 2. For arch with WORDSIZE==64 (and WORDSIZE_SYSCALL=64 -> x32) and
> __TIMESIZE==64
>
> __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
> {
> #ifdef __ASSUME_TIME64_SYSCALLS
> # ifndef __NR_clock_settime64
> #  define __NR_clock_settime64 __NR_clock_settime
> # endif
>   return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> #else
> ....

Ok, I see. I missed the fact that you also had
__ASSUME_TIME64_SYSCALLS on 64-bit architectures,
and define a local __NR_clock_settime64, and just
assumed that the 64-bit case would be unmodified from what
it is now.

Are you missing a

weak_alias (__clock_settime64, clock_settime)

in this case?

   Arnd
  

Hi Arnd,

> On Thu, Jun 27, 2019 at 4:08 PM Lukasz Majewski <lukma@denx.de> wrote:
> > > On Thu, Jun 27, 2019 at 12:36 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > 2. For arch with WORDSIZE==64 (and WORDSIZE_SYSCALL=64 -> x32) and
> > __TIMESIZE==64
> >
> > __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > *tp) {
> > #ifdef __ASSUME_TIME64_SYSCALLS
> > # ifndef __NR_clock_settime64
> > #  define __NR_clock_settime64 __NR_clock_settime
> > # endif
> >   return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > #else
> > ....  
> 
> Ok, I see. I missed the fact that you also had
> __ASSUME_TIME64_SYSCALLS on 64-bit architectures,
> and define a local __NR_clock_settime64, and just
> assumed that the 64-bit case would be unmodified from what
> it is now.

Yes. Exactly.

> 
> Are you missing a
> 
> weak_alias (__clock_settime64, clock_settime)
> 
> in this case?

I think that it is not necessary as the patch [1] has following code:

> 
>    Arnd

 
#if __TIMESIZE == 64
# define __clock_settime64 __clock_settime
#else
extern int __clock_settime64 (clockid_t clock_id,
                              const struct __timespec64 *tp);
libc_hidden_proto (__clock_settime64)
#endif

So for archs supporting 64 bit time syscalls (WORDSIZE==64 &&
WORDSIZE_SYSCALLS==64) we have the #define and the
weak_alias (__clock_settime, clock_settime) does the job.



If you are interested, please look into patch [2], which is a
proposition of using clock_settime as a part of Y2038 safe system.
This code corresponds to v3 of adding clock_settime syscall support,
which is a bit outdated as now we discuss v5. It gives a rough idea
though.


Note:

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

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
  

On Thu, Jun 27, 2019 at 5:23 PM Lukasz Majewski <lukma@denx.de> wrote:

>
> I think that it is not necessary as the patch [1] has following code:

> #if __TIMESIZE == 64
> # define __clock_settime64 __clock_settime
> #else
> extern int __clock_settime64 (clockid_t clock_id,
>                               const struct __timespec64 *tp);
> libc_hidden_proto (__clock_settime64)
> #endif
>
> So for archs supporting 64 bit time syscalls (WORDSIZE==64 &&
> WORDSIZE_SYSCALLS==64) we have the #define and the
> weak_alias (__clock_settime, clock_settime) does the job.

Ok, so you redirect both __clock_settime64 to __clock_settime
and __NR_settime64 to __NR_settime, in order to get back to the
same definition that you have today on 64-bit.

Sorry for taking so long to understand that.

> If you are interested, please look into patch [2], which is a
> proposition of using clock_settime as a part of Y2038 safe system.
> This code corresponds to v3 of adding clock_settime syscall support,
> which is a bit outdated as now we discuss v5. It gives a rough idea
> though.

Yes, that part looked fine to me from the beginning.

      Arnd
  

Hi Arnd,

> On Thu, Jun 27, 2019 at 5:23 PM Lukasz Majewski <lukma@denx.de> wrote:
> 
> >
> > I think that it is not necessary as the patch [1] has following
> > code:  
> 
> > #if __TIMESIZE == 64
> > # define __clock_settime64 __clock_settime
> > #else
> > extern int __clock_settime64 (clockid_t clock_id,
> >                               const struct __timespec64 *tp);
> > libc_hidden_proto (__clock_settime64)
> > #endif
> >
> > So for archs supporting 64 bit time syscalls (WORDSIZE==64 &&
> > WORDSIZE_SYSCALLS==64) we have the #define and the
> > weak_alias (__clock_settime, clock_settime) does the job.  
> 
> Ok, so you redirect both __clock_settime64 to __clock_settime
> and __NR_settime64 to __NR_settime, in order to get back to the
> same definition that you have today on 64-bit.
> 
> Sorry for taking so long to understand that.

Not only you had a hard time to understand it.

The approach presented in this patch steams from following guidelines:

https://www.gnu.org/software/libc/manual/html_mono/libc.html
"D.2.1 64-bit time symbol handling in the GNU C Library"


> 
> > If you are interested, please look into patch [2], which is a
> > proposition of using clock_settime as a part of Y2038 safe system.
> > This code corresponds to v3 of adding clock_settime syscall support,
> > which is a bit outdated as now we discuss v5. It gives a rough idea
> > though.  
> 
> Yes, that part looked fine to me from the beginning.

Ok. Good :-)

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

On Thu, Jun 27, 2019 at 7:08 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Arnd,
>
> > On Thu, Jun 27, 2019 at 12:36 PM Lukasz Majewski <lukma@denx.de>
> > wrote:
> > > > On Thu, Jun 27, 2019 at 12:55 AM Lukasz Majewski <lukma@denx.de>
> >
> > > After the discussion I do believe that it would be correct to change
> > > the proposed patch [1] '__clock_settime' to:
> > >
> > > #if __TIMESIZE != 64
> >
> > This would hide the function on 64-bit architectures, right?
>
> Yes, this function would be hidden on architectures with __WORDSIZE==64
> (e.g. x86_64) (+ x32 ).
>
> >
> > Maybe
> > #if (__TIMESIZE == __WORDSIZE) || (defined __x86_64__ && defined
> > __ILP32__)
> >
> > or shortening that by adding a new macro
> >
> > #if __ASSUME_TIME32_SYSCALLS
>
> Please see below comment.
>
> >
> > > int
> > > __clock_settime (clockid_t clock_id, const struct timespec *tp)
> > > {
> > > /* For archs with WORDSIZE==32, which do not support clock_settime64
> > > the clock_settime supporting 32 bit time ABI will be used */
> >
> > Same for the comment: __clock_settime would be used both
> > on 64-bit architectures as the normal path, and for compatiblity
> > with applications built without _TIME_BITS=64.
>
> With the patch [1] - it would be used in such a way:
>
> 1. For archs with WORDSIZE==32 and __TIMESIZE==32 (legacy ones - e.g.
> arm 32 bits)
>
> #if __TIMESIZE != 64
> int
> __clock_settime (clockid_t clock_id, const struct timespec *tp)
> {
>         return clock_settime(....)
> }
> #endif
>
>
> 2. For arch with WORDSIZE==64 (and WORDSIZE_SYSCALL=64 -> x32) and
> __TIMESIZE==64
>
> __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
> {
> #ifdef __ASSUME_TIME64_SYSCALLS
> # ifndef __NR_clock_settime64
> #  define __NR_clock_settime64 __NR_clock_settime
> # endif
>   return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> #else
> ....
>
>
> }
>
> The __ASSUME_TIME64_SYSCALLS is always defined in this case [2]
>
> Here the assumption is that for such architectures (like aarch64 or
> x86_64) the __NR_clock_settime64 (if ever introduced) has the same ABI
> as __NR_clock_settime, so there will be no need to adjust the glibc code.
>
>
> 3. New archs added or ones being Y2038 safe (with __NR_clock_settime64
> available in the kernel -> starting from 5.1+).
>
> For them the __ASSUME_TIME64_SYSCALLS is defined [2] and execution
> patch from point 2. is used with potential fallback to 32 bit version
> of clock_settime:
>
> __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
> {
> #ifdef __ASSUME_TIME64_SYSCALLS
> # ifndef __NR_clock_settime64
> #  define __NR_clock_settime64 __NR_clock_settime
> # endif
>   return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> #else
> # ifdef __NR_clock_settime64
>   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
>   if (ret == 0 || errno != ENOSYS)
>     return ret;
> # endif
>   if (! in_time_t_range (tp->tv_sec))
>     {
>       __set_errno (EOVERFLOW);
>       return -1;
>     }
>
>   struct timespec ts32;
>   valid_timespec64_to_timespec (tp, &ts32);
>   return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> #endif
> }
>
> Note for the above code:
>
> The __NR_clock_settime64 is used whenever possible and if not available
> the fallback to 32 bit time supporting clock_settime is executed (with
> overflow check).

So is this the general approach to use for all y2038 problematic syscalls?


Alistair

>
>
> >
> > > return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> > > }
> > > #endif
> > >
> > > The "__clock_settime64()" part could be left untouched.
> >
> > Ok.
> >
> >       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
  

Hi Alistair,

> On Thu, Jun 27, 2019 at 7:08 AM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Arnd,
> >  
> > > On Thu, Jun 27, 2019 at 12:36 PM Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > > > On Thu, Jun 27, 2019 at 12:55 AM Lukasz Majewski
> > > > > <lukma@denx.de>  
> > >  
> > > > After the discussion I do believe that it would be correct to
> > > > change the proposed patch [1] '__clock_settime' to:
> > > >
> > > > #if __TIMESIZE != 64  
> > >
> > > This would hide the function on 64-bit architectures, right?  
> >
> > Yes, this function would be hidden on architectures with
> > __WORDSIZE==64 (e.g. x86_64) (+ x32 ).
> >  
> > >
> > > Maybe
> > > #if (__TIMESIZE == __WORDSIZE) || (defined __x86_64__ && defined
> > > __ILP32__)
> > >
> > > or shortening that by adding a new macro
> > >
> > > #if __ASSUME_TIME32_SYSCALLS  
> >
> > Please see below comment.
> >  
> > >  
> > > > int
> > > > __clock_settime (clockid_t clock_id, const struct timespec *tp)
> > > > {
> > > > /* For archs with WORDSIZE==32, which do not support
> > > > clock_settime64 the clock_settime supporting 32 bit time ABI
> > > > will be used */  
> > >
> > > Same for the comment: __clock_settime would be used both
> > > on 64-bit architectures as the normal path, and for compatiblity
> > > with applications built without _TIME_BITS=64.  
> >
> > With the patch [1] - it would be used in such a way:
> >
> > 1. For archs with WORDSIZE==32 and __TIMESIZE==32 (legacy ones -
> > e.g. arm 32 bits)
> >
> > #if __TIMESIZE != 64
> > int
> > __clock_settime (clockid_t clock_id, const struct timespec *tp)
> > {
> >         return clock_settime(....)
> > }
> > #endif
> >
> >
> > 2. For arch with WORDSIZE==64 (and WORDSIZE_SYSCALL=64 -> x32) and
> > __TIMESIZE==64
> >
> > __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > *tp) {
> > #ifdef __ASSUME_TIME64_SYSCALLS
> > # ifndef __NR_clock_settime64
> > #  define __NR_clock_settime64 __NR_clock_settime
> > # endif
> >   return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > #else
> > ....
> >
> >
> > }
> >
> > The __ASSUME_TIME64_SYSCALLS is always defined in this case [2]
> >
> > Here the assumption is that for such architectures (like aarch64 or
> > x86_64) the __NR_clock_settime64 (if ever introduced) has the same
> > ABI as __NR_clock_settime, so there will be no need to adjust the
> > glibc code.
> >
> >
> > 3. New archs added or ones being Y2038 safe (with
> > __NR_clock_settime64 available in the kernel -> starting from 5.1+).
> >
> > For them the __ASSUME_TIME64_SYSCALLS is defined [2] and execution
> > patch from point 2. is used with potential fallback to 32 bit
> > version of clock_settime:
> >
> > __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > *tp) {
> > #ifdef __ASSUME_TIME64_SYSCALLS
> > # ifndef __NR_clock_settime64
> > #  define __NR_clock_settime64 __NR_clock_settime
> > # endif
> >   return INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > #else
> > # ifdef __NR_clock_settime64
> >   int ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> >   if (ret == 0 || errno != ENOSYS)
> >     return ret;
> > # endif
> >   if (! in_time_t_range (tp->tv_sec))
> >     {
> >       __set_errno (EOVERFLOW);
> >       return -1;
> >     }
> >
> >   struct timespec ts32;
> >   valid_timespec64_to_timespec (tp, &ts32);
> >   return INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> > #endif
> > }
> >
> > Note for the above code:
> >
> > The __NR_clock_settime64 is used whenever possible and if not
> > available the fallback to 32 bit time supporting clock_settime is
> > executed (with overflow check).  
> 
> So is this the general approach to use for all y2038 problematic
> syscalls?

There is still ongoing discussion regarding the
__ASSUME_TIME64_SYSCALLS flag semantics and the clock_settime
implementation (as an example of syscall conversion).

Until both are pulled upstream - there is still room for changes (and
of course discussion).

> 
> 
> Alistair
> 
> >
> >  
> > >  
> > > > return INLINE_SYSCALL_CALL (clock_settime, clock_id, tp);
> > > > }
> > > > #endif
> > > >
> > > > The "__clock_settime64()" part could be left untouched.  
> > >
> > > Ok.
> > >
> > >       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  




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
  

On Thu, Jun 27, 2019 at 6:17 PM Lukasz Majewski <lukma@denx.de> wrote:

> >
> > Ok, so you redirect both __clock_settime64 to __clock_settime
> > and __NR_settime64 to __NR_settime, in order to get back to the
> > same definition that you have today on 64-bit.
> >
> > Sorry for taking so long to understand that.
>
> Not only you had a hard time to understand it.

Here is how I had imagined it would be done in a way that I find
easier to understand:

#if __WORDSIZE == 32 /* yes, all of them including rv32 and x32 */
__clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
{
   int ret;

   /* Make sure the time cvalue is OK.  */
   if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000)
    {
       __set_errno (EINVAL);
       return -1;
     }
#ifdef __NR_clock_settime64
 ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
#endif

#ifndef __ASSUME_TIME64_SYSCALLS
  if (ret == 0 || errno != ENOSYS)
    return ret;

  if (! in_time_t_range (tp->tv_sec))
    {
      __set_errno (EOVERFLOW);
      return -1;
    }

  struct timespec ts32;
  valid_timespec64_to_timespec (tp, &ts32);
  ret = INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
#endif

  return ret;
}
#endif

This should be functionally identical to what you have, but avoid
some of the complexity, especially once linux-5.1 becomes the
minimum version.

      Arnd
  

Hi Arnd,

> On Thu, Jun 27, 2019 at 6:17 PM Lukasz Majewski <lukma@denx.de> wrote:
> 
> > >
> > > Ok, so you redirect both __clock_settime64 to __clock_settime
> > > and __NR_settime64 to __NR_settime, in order to get back to the
> > > same definition that you have today on 64-bit.
> > >
> > > Sorry for taking so long to understand that.  
> >
> > Not only you had a hard time to understand it.  
> 
> Here is how I had imagined it would be done in a way that I find
> easier to understand:
> 
> #if __WORDSIZE == 32 /* yes, all of them including rv32 and x32 */
> __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
> {
>    int ret;
> 
>    /* Make sure the time cvalue is OK.  */
>    if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000)
>     {
>        __set_errno (EINVAL);
>        return -1;
>      }
> #ifdef __NR_clock_settime64
>  ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> #endif
> 
> #ifndef __ASSUME_TIME64_SYSCALLS
>   if (ret == 0 || errno != ENOSYS)
>     return ret;
> 
>   if (! in_time_t_range (tp->tv_sec))
>     {
>       __set_errno (EOVERFLOW);
>       return -1;
>     }
> 
>   struct timespec ts32;
>   valid_timespec64_to_timespec (tp, &ts32);
>   ret = INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> #endif
> 
>   return ret;
> }
> #endif
> 
> This should be functionally identical to what you have, but avoid
> some of the complexity, especially once linux-5.1 becomes the
> minimum version.

The semantics for __ASSUME_TIME64_SYSCALLS is under the discussion for
some time [1].

Also the __clock_settime64() implementation from [2] takes into
account some corner cases (like support for all archs since 3.2
kernel, x32, legacy systems, etc). The implementation from [2] IMHO
seems to be more concise and fits into the 64 bit conversion paradigm
for glibc [3].

However, this is only my opinion and we shall wait for other community
members to express their opinions.

> 
>       Arnd


Note:

[1] - https://patchwork.ozlabs.org/patch/1117100/
[2] - https://patchwork.ozlabs.org/patch/1107235/
[3] - 
https://www.gnu.org/software/libc/manual/html_mono/libc.html
"D.2.1 64-bit time symbol handling in the GNU C Library"



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
  

On Thu, Jun 27, 2019 at 3:08 PM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Arnd,
>
> > On Thu, Jun 27, 2019 at 6:17 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > > >
> > > > Ok, so you redirect both __clock_settime64 to __clock_settime
> > > > and __NR_settime64 to __NR_settime, in order to get back to the
> > > > same definition that you have today on 64-bit.
> > > >
> > > > Sorry for taking so long to understand that.
> > >
> > > Not only you had a hard time to understand it.
> >
> > Here is how I had imagined it would be done in a way that I find
> > easier to understand:
> >
> > #if __WORDSIZE == 32 /* yes, all of them including rv32 and x32 */
> > __clock_settime64 (clockid_t clock_id, const struct __timespec64 *tp)
> > {
> >    int ret;
> >
> >    /* Make sure the time cvalue is OK.  */
> >    if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000)
> >     {
> >        __set_errno (EINVAL);
> >        return -1;
> >      }
> > #ifdef __NR_clock_settime64
> >  ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > #endif
> >
> > #ifndef __ASSUME_TIME64_SYSCALLS
> >   if (ret == 0 || errno != ENOSYS)
> >     return ret;
> >
> >   if (! in_time_t_range (tp->tv_sec))
> >     {
> >       __set_errno (EOVERFLOW);
> >       return -1;
> >     }
> >
> >   struct timespec ts32;
> >   valid_timespec64_to_timespec (tp, &ts32);
> >   ret = INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> > #endif
> >
> >   return ret;
> > }
> > #endif
> >
> > This should be functionally identical to what you have, but avoid
> > some of the complexity, especially once linux-5.1 becomes the
> > minimum version.

This implementation looks good to me. It shouldn't be too hard to
extend this to all the problematic y2038 syscalls.

>
> The semantics for __ASSUME_TIME64_SYSCALLS is under the discussion for
> some time [1].

I don't see any comments on this, do you know the status?

>
> Also the __clock_settime64() implementation from [2] takes into
> account some corner cases (like support for all archs since 3.2
> kernel, x32, legacy systems, etc). The implementation from [2] IMHO
> seems to be more concise and fits into the 64 bit conversion paradigm
> for glibc [3].

This one also works for RV32 :)

Alistair

>
> However, this is only my opinion and we shall wait for other community
> members to express their opinions.
>
> >
> >       Arnd
>
>
> Note:
>
> [1] - https://patchwork.ozlabs.org/patch/1117100/
> [2] - https://patchwork.ozlabs.org/patch/1107235/
> [3] -
> https://www.gnu.org/software/libc/manual/html_mono/libc.html
> "D.2.1 64-bit time symbol handling in the GNU C Library"
>
>
>
> 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
  

Hi Alistair,

> On Thu, Jun 27, 2019 at 3:08 PM Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Arnd,
> >  
> > > On Thu, Jun 27, 2019 at 6:17 PM Lukasz Majewski <lukma@denx.de>
> > > wrote: 
> > > > >
> > > > > Ok, so you redirect both __clock_settime64 to __clock_settime
> > > > > and __NR_settime64 to __NR_settime, in order to get back to
> > > > > the same definition that you have today on 64-bit.
> > > > >
> > > > > Sorry for taking so long to understand that.  
> > > >
> > > > Not only you had a hard time to understand it.  
> > >
> > > Here is how I had imagined it would be done in a way that I find
> > > easier to understand:
> > >
> > > #if __WORDSIZE == 32 /* yes, all of them including rv32 and x32 */
> > > __clock_settime64 (clockid_t clock_id, const struct __timespec64
> > > *tp) {
> > >    int ret;
> > >
> > >    /* Make sure the time cvalue is OK.  */
> > >    if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000)
> > >     {
> > >        __set_errno (EINVAL);
> > >        return -1;
> > >      }
> > > #ifdef __NR_clock_settime64
> > >  ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > > #endif
> > >
> > > #ifndef __ASSUME_TIME64_SYSCALLS
> > >   if (ret == 0 || errno != ENOSYS)
> > >     return ret;
> > >
> > >   if (! in_time_t_range (tp->tv_sec))
> > >     {
> > >       __set_errno (EOVERFLOW);
> > >       return -1;
> > >     }
> > >
> > >   struct timespec ts32;
> > >   valid_timespec64_to_timespec (tp, &ts32);
> > >   ret = INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> > > #endif
> > >
> > >   return ret;
> > > }
> > > #endif
> > >
> > > This should be functionally identical to what you have, but avoid
> > > some of the complexity, especially once linux-5.1 becomes the
> > > minimum version.  
> 
> This implementation looks good to me. It shouldn't be too hard to
> extend this to all the problematic y2038 syscalls.

This is the plan. After we agree on __ASSUME_TIME64_SYSCALLS semantics
(the other patch - I've add you to CC on the ping for it) it shall be
possible to follow a similar pattern as for __clock_settime for other
syscalls.

> 
> >
> > The semantics for __ASSUME_TIME64_SYSCALLS is under the discussion
> > for some time [1].  
> 
> I don't see any comments on this, do you know the status?

I'm waiting for Joseph's (and the community) review and final
acceptance (pulling to main tree). 

> 
> >
> > Also the __clock_settime64() implementation from [2] takes into
> > account some corner cases (like support for all archs since 3.2
> > kernel, x32, legacy systems, etc). The implementation from [2] IMHO
> > seems to be more concise and fits into the 64 bit conversion
> > paradigm for glibc [3].  
> 
> This one also works for RV32 :)

Good :-). Nice to have some users and testers :-).

> 
> Alistair
> 
> >
> > However, this is only my opinion and we shall wait for other
> > community members to express their opinions.
> >  
> > >
> > >       Arnd  
> >
> >
> > Note:
> >
> > [1] - https://patchwork.ozlabs.org/patch/1117100/
> > [2] - https://patchwork.ozlabs.org/patch/1107235/
> > [3] -
> > https://www.gnu.org/software/libc/manual/html_mono/libc.html
> > "D.2.1 64-bit time symbol handling in the GNU C Library"
> >
> >
> >
> > 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  




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
  

Hi,

> Hi Alistair,
> 
> > On Thu, Jun 27, 2019 at 3:08 PM Lukasz Majewski <lukma@denx.de>
> > wrote:  
> > >
> > > Hi Arnd,
> > >    
> > > > On Thu, Jun 27, 2019 at 6:17 PM Lukasz Majewski <lukma@denx.de>
> > > > wrote:   
> > > > > >
> > > > > > Ok, so you redirect both __clock_settime64 to
> > > > > > __clock_settime and __NR_settime64 to __NR_settime, in
> > > > > > order to get back to the same definition that you have
> > > > > > today on 64-bit.
> > > > > >
> > > > > > Sorry for taking so long to understand that.    
> > > > >
> > > > > Not only you had a hard time to understand it.    
> > > >
> > > > Here is how I had imagined it would be done in a way that I find
> > > > easier to understand:
> > > >
> > > > #if __WORDSIZE == 32 /* yes, all of them including rv32 and x32
> > > > */ __clock_settime64 (clockid_t clock_id, const struct
> > > > __timespec64 *tp) {
> > > >    int ret;
> > > >
> > > >    /* Make sure the time cvalue is OK.  */
> > > >    if (tp->tv_nsec < 0 || tp->tv_nsec >= 1000000000)
> > > >     {
> > > >        __set_errno (EINVAL);
> > > >        return -1;
> > > >      }
> > > > #ifdef __NR_clock_settime64
> > > >  ret = INLINE_SYSCALL_CALL (clock_settime64, clock_id, tp);
> > > > #endif
> > > >
> > > > #ifndef __ASSUME_TIME64_SYSCALLS
> > > >   if (ret == 0 || errno != ENOSYS)
> > > >     return ret;
> > > >
> > > >   if (! in_time_t_range (tp->tv_sec))
> > > >     {
> > > >       __set_errno (EOVERFLOW);
> > > >       return -1;
> > > >     }
> > > >
> > > >   struct timespec ts32;
> > > >   valid_timespec64_to_timespec (tp, &ts32);
> > > >   ret = INLINE_SYSCALL_CALL (clock_settime, clock_id, &ts32);
> > > > #endif
> > > >
> > > >   return ret;
> > > > }
> > > > #endif
> > > >
> > > > This should be functionally identical to what you have, but
> > > > avoid some of the complexity, especially once linux-5.1 becomes
> > > > the minimum version.    
> > 
> > This implementation looks good to me. It shouldn't be too hard to
> > extend this to all the problematic y2038 syscalls.  
> 
> This is the plan. After we agree on __ASSUME_TIME64_SYSCALLS semantics
> (the other patch - I've add you to CC on the ping for it) it shall be
> possible to follow a similar pattern as for __clock_settime for other
> syscalls.

The exact list of syscalls which should be eligible for similar to
__clock_settime conversion can be found here:

https://patchwork.ozlabs.org/patch/1117100/

(In the patch comment to be precise).

> 
> >   
> > >
> > > The semantics for __ASSUME_TIME64_SYSCALLS is under the discussion
> > > for some time [1].    
> > 
> > I don't see any comments on this, do you know the status?  
> 
> I'm waiting for Joseph's (and the community) review and final
> acceptance (pulling to main tree). 
> 
> >   
> > >
> > > Also the __clock_settime64() implementation from [2] takes into
> > > account some corner cases (like support for all archs since 3.2
> > > kernel, x32, legacy systems, etc). The implementation from [2]
> > > IMHO seems to be more concise and fits into the 64 bit conversion
> > > paradigm for glibc [3].    
> > 
> > This one also works for RV32 :)  
> 
> Good :-). Nice to have some users and testers :-).
> 
> > 
> > Alistair
> >   
> > >
> > > However, this is only my opinion and we shall wait for other
> > > community members to express their opinions.
> > >    
> > > >
> > > >       Arnd    
> > >
> > >
> > > Note:
> > >
> > > [1] - https://patchwork.ozlabs.org/patch/1117100/
> > > [2] - https://patchwork.ozlabs.org/patch/1107235/
> > > [3] -
> > > https://www.gnu.org/software/libc/manual/html_mono/libc.html
> > > "D.2.1 64-bit time symbol handling in the GNU C Library"
> > >
> > >
> > >
> > > 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    
> 
> 
> 
> 
> 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




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
  

On Tue, 25 Jun 2019, Arnd Bergmann wrote:

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

Yes, that's exactly what it's meant to do.

My position is as stated at 
<https://sourceware.org/ml/libc-alpha/2018-12/msg00136.html>.  Duplicating 
nontrivial functions at the source-code level for 32-bit / 64-bit time is 
unmaintainable; duplicating them at the binary level is unreasonable bloat 
(for avoidance of doubt, this is a sustained objection).  Thus calls to 
pthread_mutex_timedlock for 32-bit time must end up using syscalls with 
64-bit time, because implementation approaches other than thin wrappers 
round the 64-bit version are unreasonable.

I think the appropriate definition of a trivial interface for 32-bit time, 
for which it's OK not to use a thin wrapper, is a syscall wrapper 
generated through syscalls.list; anything requiring actual C code to 
implement the function should be a thin wrapper round the version using 
64-bit time rather than having a variant that just tries to use the 32-bit 
syscall without attempting the 64-bit one.

As I said in <https://sourceware.org/ml/libc-alpha/2018-12/msg00144.html>, 
the choice of syscalls behind a function in glibc has never been a stable 
interface in glibc, and I don't think it ever reasonably can be a stable 
interface; you have to use the dynamic symbol table to identify whether a 
binary is using interfaces for 32-bit time, rather than basing things on 
what syscalls it uses at runtime.