Re: [PATCH v7 0/3] y2038: Linux: Introduce __clock_settime64 function

[Lukasz Majewski <lukma at denx dot de>]

Hi Alistair, Joseph,

> On Fri, Sep 6, 2019 at 2:28 PM Joseph Myers <joseph@codesourcery.com>
> wrote:
> >
> > On Fri, 6 Sep 2019, Alistair Francis wrote:
> >  
> > > Which I can fix with this diff:  
> >
> > This diff is not the right way to fix this build failure.
> >
> > One of the design principles in the Y2038 support is that is
> > __TIMESIZE == 64, the time functions *aren't* trivial wrappers of
> > time64 functions; rather, the time64 function definitions are
> > remapped (via macros) so they define the function name with no
> > "64".  For each case where there is a pair of functions (for
> > different time_t types) in the __TIMESIZE == 32 case, there should
> > just be the one function when __TIMESIZE == 64.
> >
> > This ensures that the Y2038 changes don't add any overhead at all
> > in the glibc binaries on existing platforms with __TIMESIZE == 64.
> >
> > You should look at exactly what the types in question are, that are
> > being reported as conflicting in your build (probably by looking at
> > preprocessed source).  __timespec64 and timespec are supposed to be
> > the same type (via #define) when __TIMESIZE == 64, to avoid such
> > incompatibilities.  
> 
> Looking at the place where the__timespec64 is defined they aren't the
> same for __TIMESIZE == 64 on a 32-bit system.
> 
> The code is below:
> 
> #if __WORDSIZE == 64 \
>   || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
   ^^^^^^^^^^^^^ - [1]

> # define __timespec64 timespec
> #else
> /* The glibc Y2038-proof struct __timespec64 structure for a time
> value. To keep things Posix-ish, we keep the nanoseconds field a
> 32-bit signed long, but since the Linux field is a 64-bit signed int,
> we pad our tv_nsec with a 32-bit int.
> 
>    As a general rule the Linux kernel is ignoring upper 32 bits of
>    tv_nsec field.  */
> struct __timespec64
         ^^^^^^^^^^ - [3]

> {
>   __time64_t tv_sec;         /* Seconds */
> # if BYTE_ORDER == BIG_ENDIAN
>   __int32_t tv_pad;          /* Padding */
>   __int32_t tv_nsec;         /* Nanoseconds */
> # else
>   __int32_t tv_nsec;         /* Nanoseconds */
>   __int32_t tv_pad;          /* Padding */
> # endif
> };
> #endif
> 
> So looking at that with __TIMESIZE == 64 but __WORDSIZE == 32 (as on
> RV32) we will specify a __timespec64 struct that is different to the
> timespec struct.
> 
> Would the correct fix be to add __TIMESIZE == 64 to the #if mentioned
> above?

I've replaced the condition [1] with plain #if __TIMESIZE == 64 [2], but
then there was a concern that: 

Replace __TIMESIZE with __WORDSIZE (as architectures with
__TIMESIZE==64 will need to use this struct with 32 bit tv_nsec field).


Alistair - I guess that the size of RISC-V's struct timespec tv_nsec
is 4? (as it shall be long tv_nsec; [4]) ?

Then if you replace the condition [1] with #if __TIMESIZE == 64 you
would have:

struct timespec
{
  __time_t tv_sec;		/* Seconds.  */
  __syscall_slong_t tv_nsec;    /* Nanoseconds.  */
}

And then,

You would have tv_sec (8 bytes) and tv_nsec (4 bytes) [*]. Kernel's
clock_settime64 expects to receive two 8 bytes values.

In the best case you would leak glibc data to the kenel. In the worst
case kernel will modify this data, which may be some other struct's
field.

The condition [1] prevents from this for machines with __WORDSIZE == 32
(excluding x32).

Joseph is the above concern valid ?

> 
> This diff fixes the build for me:
> 
> diff --git a/include/time.h b/include/time.h
> index 5f7529f10e9..ff5f18ec56c 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -51,7 +51,8 @@ extern void __tz_compute (__time64_t timer, struct
> tm *tm, int use_localtime)
>    __THROW attribute_hidden;
> 
>  #if __WORDSIZE == 64 \
> -  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64)
> +  || (defined __SYSCALL_WORDSIZE && __SYSCALL_WORDSIZE == 64) || \
> +  __TIMESIZE == 64
>  # define __timespec64 timespec
>  #else
>  /* The glibc Y2038-proof struct __timespec64 structure for a time
> value.
> 
> Alistair
> 
> >
> > --
> > Joseph S. Myers
> > joseph@codesourcery.com  

Note:

[2] - https://patchwork.ozlabs.org/patch/1092580/
[4] - https://linux.die.net/man/3/clock_gettime
[*] - what is the output of sizeof(__syscall_slong_t) on RISCV 32 bit?

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

Attachment: pgp7QanD_vNch.pgp
Description: OpenPGP digital signature