diff mbox series

[v3,2/5] y2038: Introduce internal for glibc struct __timespec64

Message ID 20190507131848.30980-3-lukma@denx.de
State New
Headers show
Series y2038: Linux: Provide __clock_settime function supporting 64 bit time | expand

Commit Message

Lukasz Majewski May 7, 2019, 1:18 p.m. UTC 
This type is a glibc's type similar to struct timespec
but whose tv_sec field is a __time64_t rather than a time_t,
which makes it Y2038-proof and usable to pass between user
code and Y2038-proof kernel syscalls (e.g. clock_gettime()).

To support passing this structure to the kernel - the tv_pad,
32 bit padding field has been introduced. The placement of it
depends on endiannes of the SoC.

Tested on x86_64 and ARM.

* include/time.h: Add struct __timespec64 definition

---
Changes for v3:
- Replace __TIMESIZE with __WORDSIZE (as architectures with __TIMESIZE==64
  will need to use this struct with 32 bit tv_nsec field).
- Update in-code comment

Changes for v2:
- None
---
 include/time.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Comments

Andreas Schwab May 7, 2019, 1:30 p.m. UTC | #1 
On Mai 07 2019, Lukasz Majewski <lukma@denx.de> wrote:

> +struct __timespec64
> +{
> +  __time64_t tv_sec;         /* Seconds */
> +# if BYTE_ORDER == BIG_ENDIAN
> +  int tv_pad: 32;            /* Padding named for checking/setting */
> +  __int32_t tv_nsec;         /* Nanoseconds */
> +# else
> +  __int32_t tv_nsec;         /* Nanoseconds */
> +  int tv_pad: 32;            /* Padding named for checking/setting */
> +# endif

No need to use a bitfield, since the padding is not fractional.

Andreas.
Lukasz Majewski May 7, 2019, 2:07 p.m. UTC | #2 
Hi Andreas,

> On Mai 07 2019, Lukasz Majewski <lukma@denx.de> wrote:
> 
> > +struct __timespec64
> > +{
> > +  __time64_t tv_sec;         /* Seconds */
> > +# if BYTE_ORDER == BIG_ENDIAN
> > +  int tv_pad: 32;            /* Padding named for checking/setting
> > */
> > +  __int32_t tv_nsec;         /* Nanoseconds */
> > +# else
> > +  __int32_t tv_nsec;         /* Nanoseconds */
> > +  int tv_pad: 32;            /* Padding named for checking/setting
> > */ +# endif  
> 
> No need to use a bitfield, since the padding is not fractional.

That bitfield is for following reasons:

1. Have a backup plan in the case if we need to copy and clear the
padding anyway before passing this structure to the kernel in the future
(as recently we discovered that for example x32 has a kernel bug with
clearing it).

2. Kernel syscalls (e.g. clock_settime64) expects on those systems two
values - each 8 bytes. To avoid any nasty surprises the explicit
padding was added.

> 
> Andreas.
> 




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
Andreas Schwab May 7, 2019, 2:13 p.m. UTC | #3 
On Mai 07 2019, Lukasz Majewski <lukma@denx.de> wrote:

> Hi Andreas,
>
>> On Mai 07 2019, Lukasz Majewski <lukma@denx.de> wrote:
>> 
>> > +struct __timespec64
>> > +{
>> > +  __time64_t tv_sec;         /* Seconds */
>> > +# if BYTE_ORDER == BIG_ENDIAN
>> > +  int tv_pad: 32;            /* Padding named for checking/setting
>> > */
>> > +  __int32_t tv_nsec;         /* Nanoseconds */
>> > +# else
>> > +  __int32_t tv_nsec;         /* Nanoseconds */
>> > +  int tv_pad: 32;            /* Padding named for checking/setting
>> > */ +# endif  
>> 
>> No need to use a bitfield, since the padding is not fractional.
>
> That bitfield is for following reasons:
>
> 1. Have a backup plan in the case if we need to copy and clear the
> padding anyway before passing this structure to the kernel in the future
> (as recently we discovered that for example x32 has a kernel bug with
> clearing it).
>
> 2. Kernel syscalls (e.g. clock_settime64) expects on those systems two
> values - each 8 bytes. To avoid any nasty surprises the explicit
> padding was added.

None of that requires a bitfield.

Andreas.
Lukasz Majewski May 8, 2019, 8:32 a.m. UTC | #4 
Hi Andreas,

> On Mai 07 2019, Lukasz Majewski <lukma@denx.de> wrote:
> 
> > Hi Andreas,
> >  
> >> On Mai 07 2019, Lukasz Majewski <lukma@denx.de> wrote:
> >>   
> >> > +struct __timespec64
> >> > +{
> >> > +  __time64_t tv_sec;         /* Seconds */
> >> > +# if BYTE_ORDER == BIG_ENDIAN
> >> > +  int tv_pad: 32;            /* Padding named for
> >> > checking/setting */
> >> > +  __int32_t tv_nsec;         /* Nanoseconds */
> >> > +# else
> >> > +  __int32_t tv_nsec;         /* Nanoseconds */
> >> > +  int tv_pad: 32;            /* Padding named for
> >> > checking/setting */ +# endif    
> >> 
> >> No need to use a bitfield, since the padding is not fractional.  
> >
> > That bitfield is for following reasons:
> >
> > 1. Have a backup plan in the case if we need to copy and clear the
> > padding anyway before passing this structure to the kernel in the
> > future (as recently we discovered that for example x32 has a kernel
> > bug with clearing it).
> >
> > 2. Kernel syscalls (e.g. clock_settime64) expects on those systems
> > two values - each 8 bytes. To avoid any nasty surprises the explicit
> > padding was added.  
> 
> None of that requires a bitfield.

For 32 bit systems with 64 bit time_t we would have following
situation:

struct timespec:
	-> tv_sec (8 B)
	-> tv_nsec (4 B -> must be long as of POSIX) [*]

This would have 16B size (sizeof() would return 16B [1])


My point is if we do need at any point in time to clear this padding
(due to e.g. kernel bug when passing data to syscalls [2] or solve issue
as presented in [*]).


Note:

[*] - the x32 ABI of x86_64 has long 8B and there are some issues with
POSIX compliance
https://sourceware.org/bugzilla/show_bug.cgi?id=16437

[1] - as discussed:
http://www.catb.org/esr/structure-packing/#_structure_alignment_and_padding

[2] -
https://lore.kernel.org/lkml/20190429131951.471701-1-arnd@arndb.de/

> 
> Andreas.
> 




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
Andreas Schwab May 8, 2019, 8:38 a.m. UTC | #5 
On Mai 08 2019, Lukasz Majewski <lukma@denx.de> wrote:

> My point is if we do need at any point in time to clear this padding
> (due to e.g. kernel bug when passing data to syscalls [2] or solve issue
> as presented in [*]).

Yes, but you don't need to use a *bitfield*.

Andreas.
Lukasz Majewski May 8, 2019, 9:10 a.m. UTC | #6 
On Wed, 08 May 2019 10:38:20 +0200
Andreas Schwab <schwab@suse.de> wrote:

> On Mai 08 2019, Lukasz Majewski <lukma@denx.de> wrote:
> 
> > My point is if we do need at any point in time to clear this padding
> > (due to e.g. kernel bug when passing data to syscalls [2] or solve
> > issue as presented in [*]).  
> 
> Yes, but you don't need to use a *bitfield*.

Could you share your opinion about the type to use to replace the
bitfield?

The other option would be to use __int32_t tv_pad.


Another question - shall we use __int32_t for tv_nsec or
__syscall_slong_t for internal representation?

We would end up with the same type in the end, but I don't know what is
preferable.

> 
> Andreas.
> 




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
Andreas Schwab May 8, 2019, 9:12 a.m. UTC | #7 
On Mai 08 2019, Lukasz Majewski <lukma@denx.de> wrote:

> On Wed, 08 May 2019 10:38:20 +0200
> Andreas Schwab <schwab@suse.de> wrote:
>
>> On Mai 08 2019, Lukasz Majewski <lukma@denx.de> wrote:
>> 
>> > My point is if we do need at any point in time to clear this padding
>> > (due to e.g. kernel bug when passing data to syscalls [2] or solve
>> > issue as presented in [*]).  
>> 
>> Yes, but you don't need to use a *bitfield*.
>
> Could you share your opinion about the type to use to replace the
> bitfield?

Anything that fits, but not a bitfield.

Andreas.
Lukasz Majewski May 8, 2019, 9:37 a.m. UTC | #8 
On Wed, 08 May 2019 11:12:27 +0200
Andreas Schwab <schwab@suse.de> wrote:

> On Mai 08 2019, Lukasz Majewski <lukma@denx.de> wrote:
> 
> > On Wed, 08 May 2019 10:38:20 +0200
> > Andreas Schwab <schwab@suse.de> wrote:
> >  
> >> On Mai 08 2019, Lukasz Majewski <lukma@denx.de> wrote:
> >>   
> >> > My point is if we do need at any point in time to clear this
> >> > padding (due to e.g. kernel bug when passing data to syscalls
> >> > [2] or solve issue as presented in [*]).    
> >> 
> >> Yes, but you don't need to use a *bitfield*.  
> >
> > Could you share your opinion about the type to use to replace the
> > bitfield?  
> 
> Anything that fits, but not a bitfield.

Ok :-)

> 
> Andreas.
> 




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
diff mbox series

Patch

diff --git a/include/time.h b/include/time.h
index ac3163c2a5..814e927645 100644
--- a/include/time.h
+++ b/include/time.h
@@ -5,6 +5,7 @@ 
 # include <bits/types/locale_t.h>
 # include <stdbool.h>
 # include <time/mktime-internal.h>
+# include <endian.h>
 
 extern __typeof (strftime_l) __strftime_l;
 libc_hidden_proto (__strftime_l)
@@ -51,6 +52,30 @@  extern void __tzset_parse_tz (const char *tz) attribute_hidden;
 extern void __tz_compute (__time64_t timer, struct tm *tm, int use_localtime)
   __THROW attribute_hidden;
 
+#if __WORDSIZE == 64
+# 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 bitfield.
+
+   As a general rule the Linux kernel is ignoring upper 32 bits of
+   tv_nsec field. However, there are architectures with potential need
+   for clearing the padding (i.e. 'x32').  */
+struct __timespec64
+{
+  __time64_t tv_sec;         /* Seconds */
+# if BYTE_ORDER == BIG_ENDIAN
+  int tv_pad: 32;            /* Padding named for checking/setting */
+  __int32_t tv_nsec;         /* Nanoseconds */
+# else
+  __int32_t tv_nsec;         /* Nanoseconds */
+  int tv_pad: 32;            /* Padding named for checking/setting */
+# endif
+};
+#endif
+
 #if __TIMESIZE == 64
 # define __ctime64 ctime
 #else