Message ID | 20190930133134.14771-1-lukma@denx.de |
---|---|
State | Committed |
Commit | 9c44c6a908339e6d6eafa3639e641d5caea5e1ee |
Headers |
Received: (qmail 9773 invoked by alias); 30 Sep 2019 13:32:12 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 9765 invoked by uid 89); 30 Sep 2019 13:32:11 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_NUMSUBJECT, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 spammy=H*r:192.168.8, HContent-Transfer-Encoding:8bit X-HELO: mail-out.m-online.net From: Lukasz Majewski <lukma@denx.de> To: Joseph Myers <joseph@codesourcery.com>, Paul Eggert <eggert@cs.ucla.edu> Cc: Alistair Francis <alistair23@gmail.com>, Arnd Bergmann <arnd@arndb.de>, Alistair Francis <alistair.francis@wdc.com>, GNU C Library <libc-alpha@sourceware.org>, Adhemerval Zanella <adhemerval.zanella@linaro.org>, Florian Weimer <fweimer@redhat.com>, Carlos O'Donell <carlos@redhat.com>, Stepan Golosunov <stepan@golosunov.pp.ru>, Florian Weimer <fw@deneb.enyo.de>, Zack Weinberg <zackw@panix.com>, Lukasz Majewski <lukma@denx.de> Subject: [PATCH v10] y2038: Provide conversion helpers for struct __timespec64 Date: Mon, 30 Sep 2019 15:31:34 +0200 Message-Id: <20190930133134.14771-1-lukma@denx.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Lukasz Majewski
Sept. 30, 2019, 1:31 p.m. UTC
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(+)
Comments
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
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