[v10] y2038: Provide conversion helpers for struct __timespec64

  Those functions allow easy conversion between Y2038 safe struct
__timespec64 and other time related data structures (like struct timeval
or struct timespec).

* include/time.h (valid_timeval_to_timespec64): Add.
* include/time.h (valid_timespec_to_timespec64): Likewise.
* include/time.h (valid_timespec64_to_timespec): Likewise.
* include/time.h (valid_timespec64_to_timeval): Likewise.

---
Changes for v10:
- Provide only conversion functions to work on "valid" time structures
  (no need to provide any excessive checks)
- Return values instead of pointers

Changes for v9:
- Update comments to be more concise and describe return values from
  conversion functions (according to Joseph's request).

Changes for v8:
- None

Changes for v7:
- None

Changes for v6:
- Remove the #ifdef guard on __ASSUME_TIME64_SYSCALLS as those functions
  may be needed for fallback execution paths (on e.g. __clock_settime64).

Changes for v5:
- This code is now only compiled in when __ASSUME_TIME64_SYSCALLS is NOT
  defined. Previously it was depending on #if __TIMESIZE != 64.

Changes for v4:
- None

Changes for v3:
- Remove misleading comments regarding clearing tv_pad values during
  conversion (as Linux kernel on its own ignores upper 32 bits of tv_nsec).

Changes for v3:
- Remove timespec64_clear_padding function - as kernel ignores upper 32
  bits of tv_nsec when passed via syscall to the Linux kernel

Changes for v2:
- Add timespec64_clear_padding function
---
 include/time.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
  

On Mon, 30 Sep 2019, Lukasz Majewski wrote:

> Those functions allow easy conversion between Y2038 safe struct
> __timespec64 and other time related data structures (like struct timeval
> or struct timespec).
> 
> * include/time.h (valid_timeval_to_timespec64): Add.
> * include/time.h (valid_timespec_to_timespec64): Likewise.
> * include/time.h (valid_timespec64_to_timespec): Likewise.
> * include/time.h (valid_timespec64_to_timeval): Likewise.
> 
> ---
> Changes for v10:
> - Provide only conversion functions to work on "valid" time structures
>   (no need to provide any excessive checks)
> - Return values instead of pointers

This patch is OK, please commit.
  

This has broken the build of glibc for i686-gnu as struct timeval isn't 
defined at this point in the build.

In file included from /scratch/jmyers/glibc-bot/install/compilers/i686-gnu/sysroot/include/hurd/hurd_types.h:24,
                 from ../hurd/hurd.h:30,
                 from ../sysdeps/hurd/include/hurd.h:2,
                 from hurdid.c:18:
../include/time.h:183:51: error: parameter 1 ('tv') has incomplete type
 valid_timeval_to_timespec64 (const struct timeval tv)
                              ~~~~~~~~~~~~~~~~~~~~~^~
../include/time.h:221:1: error: return type is an incomplete type
 valid_timespec64_to_timeval (const struct __timespec64 ts64)
 ^~~~~~~~~~~~~~~~~~~~~~~~~~~
../include/time.h: In function 'valid_timespec64_to_timeval':
../include/time.h:223:18: error: storage size of 'tv' isn't known
   struct timeval tv;
                  ^~
  

Hi Joseph,

> This has broken the build of glibc for i686-gnu as struct timeval
> isn't defined at this point in the build.

I've been using glibc-many.py script and the diff between builds w/
this patch was the same.

I had the UNRESOLVED: for i686-gnu glibc (with and without this patch)
And this seems like the one for hurd...

As there are already patches applied on top of this one - I suppose that
I shall provide incremental fix for it.

> 
> In file included from
> /scratch/jmyers/glibc-bot/install/compilers/i686-gnu/sysroot/include/hurd/hurd_types.h:24,
> from ../hurd/hurd.h:30, from ../sysdeps/hurd/include/hurd.h:2,
>                  from hurdid.c:18:
> ../include/time.h:183:51: error: parameter 1 ('tv') has incomplete
> type valid_timeval_to_timespec64 (const struct timeval tv)
>                               ~~~~~~~~~~~~~~~~~~~~~^~
> ../include/time.h:221:1: error: return type is an incomplete type
>  valid_timespec64_to_timeval (const struct __timespec64 ts64)
>  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../include/time.h: In function 'valid_timespec64_to_timeval':
> ../include/time.h:223:18: error: storage size of 'tv' isn't known
>    struct timeval tv;
>                   ^~
> 

I will submit fix ASAP.


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, 1 Oct 2019, Lukasz Majewski wrote:

> I had the UNRESOLVED: for i686-gnu glibc (with and without this patch)
> And this seems like the one for hurd...

If you had UNRESOLVED rather than FAIL that means the build was broken at 
the time you did the "compilers" build.  You need to do the "compilers" 
build at a time when all glibc configurations are building OK, in order 
for it to be a good basis for testing the "glibcs" build.
  

Hi Joseph,

> Hi Joseph,
> 
> > This has broken the build of glibc for i686-gnu as struct timeval
> > isn't defined at this point in the build.  
> 
> I've been using glibc-many.py script and the diff between builds w/
> this patch was the same.
> 
> I had the UNRESOLVED: for i686-gnu glibc (with and without this patch)
> And this seems like the one for hurd...
> 
> As there are already patches applied on top of this one - I suppose
> that I shall provide incremental fix for it.
> 
> > 
> > In file included from
> > /scratch/jmyers/glibc-bot/install/compilers/i686-gnu/sysroot/include/hurd/hurd_types.h:24,
> > from ../hurd/hurd.h:30, from ../sysdeps/hurd/include/hurd.h:2,
> >                  from hurdid.c:18:
> > ../include/time.h:183:51: error: parameter 1 ('tv') has incomplete
> > type valid_timeval_to_timespec64 (const struct timeval tv)
> >                               ~~~~~~~~~~~~~~~~~~~~~^~
> > ../include/time.h:221:1: error: return type is an incomplete type
> >  valid_timespec64_to_timeval (const struct __timespec64 ts64)
> >  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> > ../include/time.h: In function 'valid_timespec64_to_timeval':
> > ../include/time.h:223:18: error: storage size of 'tv' isn't known
> >    struct timeval tv;
> >                   ^~
> >   
> 
> I will submit fix ASAP.

It seems to me like it would be best to add the missing
#include <sys/time.h> to HURD's #include <hurd/hurd_types.h> as it also
includes for example struct timespec header. However, is is not so
simple as this file is in hurd compiler includes 
(glibc-many-build/install/compilers/i686-gnu/sysroot/include/hurd/hurd_types.h).

The other option would be to add #include <sys/time.h> just before
struct __timespec64 conversion in ./include/time.h

I'm going to submit such patch for review.

> 
> 
> 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, 1 Oct 2019, Lukasz Majewski wrote:

> It seems to me like it would be best to add the missing
> #include <sys/time.h> to HURD's #include <hurd/hurd_types.h> as it also
> includes for example struct timespec header. However, is is not so
> simple as this file is in hurd compiler includes 
> (glibc-many-build/install/compilers/i686-gnu/sysroot/include/hurd/hurd_types.h).
> 
> The other option would be to add #include <sys/time.h> just before
> struct __timespec64 conversion in ./include/time.h
> 
> I'm going to submit such patch for review.

How about including <bits/types/struct_timeval.h> in include/time.h 
(inside the !_ISOMAC conditional)?  It already includes 
<bits/types/locale_t.h>, for example; since it's using struct timeval, it 
seems natural for it to include the corresponding header (rather than a 
more general sys/time.h include).
  

Hi Joseph,

> On Tue, 1 Oct 2019, Lukasz Majewski wrote:
> 
> > It seems to me like it would be best to add the missing
> > #include <sys/time.h> to HURD's #include <hurd/hurd_types.h> as it
> > also includes for example struct timespec header. However, is is
> > not so simple as this file is in hurd compiler includes 
> > (glibc-many-build/install/compilers/i686-gnu/sysroot/include/hurd/hurd_types.h).
> > 
> > The other option would be to add #include <sys/time.h> just before
> > struct __timespec64 conversion in ./include/time.h
> > 
> > I'm going to submit such patch for review.  
> 
> How about including <bits/types/struct_timeval.h> in include/time.h 
> (inside the !_ISOMAC conditional)?  It already includes 
> <bits/types/locale_t.h>, for example; since it's using struct
> timeval, it seems natural for it to include the corresponding header
> (rather than a more general sys/time.h include).
> 

Thanks for the hint. I will prepare and submit (after some testing)
patch using the above approach.


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 Mon, Sep 30, 2019 at 03:31:34PM +0200, Lukasz Majewski wrote:
> Those functions allow easy conversion between Y2038 safe struct
> __timespec64 and other time related data structures (like struct timeval
> or struct timespec).
> 
> * include/time.h (valid_timeval_to_timespec64): Add.
> * include/time.h (valid_timespec_to_timespec64): Likewise.
> * include/time.h (valid_timespec64_to_timespec): Likewise.
> * include/time.h (valid_timespec64_to_timeval): Likewise.
> 
> ---
> Changes for v10:
> - Provide only conversion functions to work on "valid" time structures
>   (no need to provide any excessive checks)
> - Return values instead of pointers
> 
> Changes for v9:
> - Update comments to be more concise and describe return values from
>   conversion functions (according to Joseph's request).
> 
> Changes for v8:
> - None
> 
> Changes for v7:
> - None
> 
> Changes for v6:
> - Remove the #ifdef guard on __ASSUME_TIME64_SYSCALLS as those functions
>   may be needed for fallback execution paths (on e.g. __clock_settime64).
> 
> Changes for v5:
> - This code is now only compiled in when __ASSUME_TIME64_SYSCALLS is NOT
>   defined. Previously it was depending on #if __TIMESIZE != 64.
> 
> Changes for v4:
> - None
> 
> Changes for v3:
> - Remove misleading comments regarding clearing tv_pad values during
>   conversion (as Linux kernel on its own ignores upper 32 bits of tv_nsec).
> 
> Changes for v3:
> - Remove timespec64_clear_padding function - as kernel ignores upper 32
>   bits of tv_nsec when passed via syscall to the Linux kernel
> 
> Changes for v2:
> - Add timespec64_clear_padding function
> ---
>  include/time.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/include/time.h b/include/time.h
> index 9727786634..9878c2b2ca 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -178,5 +178,54 @@ in_time_t_range (__time64_t t)
>    return s == t;
>  }
>  
> +/* Convert a known valid struct timeval into a struct __timespec64.  */
> +static inline struct __timespec64
> +valid_timeval_to_timespec64 (const struct timeval tv)
> +{
> +  struct __timespec64 ts64;
> +
> +  ts64.tv_sec = tv.tv_sec;
> +  ts64.tv_nsec = tv.tv_usec * 1000;
> +
> +  return ts64;
> +}
> +
> +/* Convert a known valid struct timespec into a struct __timespec64.  */
> +static inline struct __timespec64
> +valid_timespec_to_timespec64 (const struct timespec ts)
> +{
> +  struct __timespec64 ts64;
> +
> +  ts64.tv_sec = ts.tv_sec;
> +  ts64.tv_nsec = ts.tv_nsec;
> +
> +  return ts64;
> +}
> +
> +/* Convert a valid and within range of struct timespec, struct
> +   __timespec64 into a struct timespec.  */
> +static inline struct timespec
> +valid_timespec64_to_timespec (const struct __timespec64 ts64)
> +{
> +  struct timespec ts;
> +
> +  ts.tv_sec = (time_t) ts64.tv_sec;
> +  ts.tv_nsec = ts64.tv_nsec;
> +
> +  return ts;
> +}
> +
> +/* Convert a valid and within range of struct timeval struct
> +   __timespec64 into a struct timeval.  */
> +static inline struct timeval
> +valid_timespec64_to_timeval (const struct __timespec64 ts64)
> +{
> +  struct timeval tv;
> +
> +  tv.tv_sec = (time_t) ts64.tv_sec;
> +  tv.tv_usec = ts64.tv_nsec / 1000;
> +
> +  return tv;
> +}
>  #endif
>  #endif

Unfortunately, this didn't work out as intended because Linux kernel prior
to commit 7b8474466ed97be458c825f34a85f2c2b84c3f95 (including released
version v5.4) did not zero the upper 32-bits of tv_nsec on 32-bit
architectures, which means that we have to do it in
valid_timeval_to_timespec64 and valid_timespec_to_timespec64 instead.
  

* Dmitry V. Levin:

> Unfortunately, this didn't work out as intended because Linux kernel prior
> to commit 7b8474466ed97be458c825f34a85f2c2b84c3f95 (including released
> version v5.4) did not zero the upper 32-bits of tv_nsec on 32-bit
> architectures, which means that we have to do it in
> valid_timeval_to_timespec64 and valid_timespec_to_timespec64 instead.

Can't we treat this as a critical kernel bug that needs fixing before
the kernels become usable?

I'm concern that this will make initializers for struct timespec64
invalid.  glibc can't intercept everything that goes into the kernel.

Thanks,
Florian
  

Hi Florian,

> * Dmitry V. Levin:
> 
> > Unfortunately, this didn't work out as intended because Linux
> > kernel prior to commit 7b8474466ed97be458c825f34a85f2c2b84c3f95
> > (including released version v5.4) did not zero the upper 32-bits of
> > tv_nsec on 32-bit architectures, which means that we have to do it
> > in valid_timeval_to_timespec64 and valid_timespec_to_timespec64
> > instead.  
> 
> Can't we treat this as a critical kernel bug that needs fixing before
> the kernels become usable?
> 
> I'm concern that this will make initializers for struct timespec64
> invalid.  glibc can't intercept everything that goes into the kernel.
> 

There was a discussion regarding explicit clearing of data passed /
read from the kernel during development of this patch.

The agreement was that we rely on the kernel in this matter and no
explicit clearing is necessary in glibc.

> 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 Wed, Dec 4, 2019 at 11:19 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
> On Mon, Sep 30, 2019 at 03:31:34PM +0200, Lukasz Majewski wrote:
>
> Unfortunately, this didn't work out as intended because Linux kernel prior
> to commit 7b8474466ed97be458c825f34a85f2c2b84c3f95 (including released
> version v5.4) did not zero the upper 32-bits of tv_nsec on 32-bit
> architectures, which means that we have to do it in
> valid_timeval_to_timespec64 and valid_timespec_to_timespec64 instead.

Are you sure that patch actually makes a difference? On compat mode
in 64-bit architectures, the mask was still applied, and on native
32-bit modes, we get into a signed integer overflow when assigning the
tv_nsec from a 64-bit member in __kernel_timespec to a 32-bit member
in timespec64, but as the kernel is built with -fno-strict-overflow that
should still produce the correct result (truncating to 32 bit).

The patch is still useful for clarity but I fear I misread it originally
and thought it made a difference when in practice it does not actually
change the behavior, so the patch description ended overstating
the impact.

       Arnd
  

On Wed, 4 Dec 2019, Dmitry V. Levin wrote:

> Unfortunately, this didn't work out as intended because Linux kernel prior
> to commit 7b8474466ed97be458c825f34a85f2c2b84c3f95 (including released
> version v5.4) did not zero the upper 32-bits of tv_nsec on 32-bit
> architectures, which means that we have to do it in
> valid_timeval_to_timespec64 and valid_timespec_to_timespec64 instead.

"have to do it in valid_timeval_to_timespec64 and 
valid_timespec_to_timespec64" can't be the answer.

Remember that all the functions using 64-bit time are intended in future 
to be user-visible functions that will be called directly by user code 
with _TIME_BITS=64.  That is, it is those functions that have to handle 
the padding being uninitialized.  And so it is those functions, receiving 
64-bit-time structures on input, that have to copy the structures (many 
can legitimately be called with inputs in read-only memory, they can't 
write to the inputs unless the definition of the function semantics says 
they can) and zero the padding if supporting kernels that don't do so 
themselves - not any callers that convert from 32-bit.
  

On Wed, Dec 04, 2019 at 12:04:32PM +0100, Lukasz Majewski wrote:
> Hi Florian,
> 
> > * Dmitry V. Levin:
> > 
> > > Unfortunately, this didn't work out as intended because Linux
> > > kernel prior to commit 7b8474466ed97be458c825f34a85f2c2b84c3f95
> > > (including released version v5.4) did not zero the upper 32-bits of
> > > tv_nsec on 32-bit architectures, which means that we have to do it
> > > in valid_timeval_to_timespec64 and valid_timespec_to_timespec64
> > > instead.  
> > 
> > Can't we treat this as a critical kernel bug that needs fixing before
> > the kernels become usable?
> > 
> > I'm concern that this will make initializers for struct timespec64
> > invalid.  glibc can't intercept everything that goes into the kernel.
> > 
> 
> There was a discussion regarding explicit clearing of data passed /
> read from the kernel during development of this patch.
> 
> The agreement was that we rely on the kernel in this matter and no
> explicit clearing is necessary in glibc.

So the agreement was to push the garbage to the kernel assuming it will
handle the matter?

Has anybody considered what it will look like in a tracer's output?

clock_nanosleep_time64(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=13185818429535213454}, {tv_sec=0, tv_nsec=111245472}) = ? ERESTART_RESTARTBLOCK (Interrupted by signal)

Confusing, isn't it?

This happens because the type of syscall argument on the kernel side is a
pointer to struct __kernel_timespec where the type of tv_nsec is long long.
  

* Dmitry V. Levin:

> So the agreement was to push the garbage to the kernel assuming it will
> handle the matter?

Yes.

> Has anybody considered what it will look like in a tracer's output?
>
> clock_nanosleep_time64(CLOCK_REALTIME, 0, {tv_sec=0, tv_nsec=13185818429535213454}, {tv_sec=0, tv_nsec=111245472}) = ? ERESTART_RESTARTBLOCK (Interrupted by signal)
>
> Confusing, isn't it?
>
> This happens because the type of syscall argument on the kernel side is a
> pointer to struct __kernel_timespec where the type of tv_nsec is long long.

Looks like strace needs to mask the values in its output.  I really
don't see another way.

Thanks,
Florian