| Message ID | 20190426142531.1378357-1-arnd@arndb.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] y2038: make CONFIG_64BIT_TIME unconditional | expand |
Hi Arnd, > As Stepan Golosunov points out, we made a small mistake in the > get_timespec64() function in the kernel. It was originally added under > the assumption that CONFIG_64BIT_TIME would get enabled on all 32-bit > and 64-bit architectures, but when I did the conversion, I only turned > it on for 32-bit ones. > > The effect is that the get_timespec64() function never clears the > upper half of the tv_nsec field for 32-bit tasks in compat mode. > Clearing this is required for POSIX compliant behavior of functions > that pass a 'timespec' structure with a 64-bit tv_sec and a 32-bit > tv_nsec, plus uninitialized padding. On my 32 bit ARM setup (with CONFIG_COMPAT_32BIT_TIME=y) for Y2038 test: - I do use clock_settime64 [1] to set 64 bit tv_sec with 32 bit tv_nsec and 32 bit unnamed padding. - I also may use clock_settime32 as a fallback (but it will not work beyond Y2038) if the 64 bit version is not available for any reason. In the get_timespec64() the in_compat_syscall() returns 0 [2], so the padding bits are not cleared. This imposes the in glibc requirement to zero the padding before passing it to the Linux kernel. At least for my setup (32bit ARM with 64 bit time support) this patch is not fixing anything. Moreover, the described above problem doesn't matter on native 64 bit systems as there both fields are 64 bit (no padding required). Note: [1] - https://elixir.bootlin.com/linux/v5.1-rc6/source/arch/arm/tools/syscall.tbl#L421 [2] - include/linux/compat.h -> CONFIG_COMPAT is not defined - as a result in_compat_syscall() returns 0. > > The easiest fix for linux-5.1 is to just make the Kconfig symbol > unconditional, as it was originally intended. As a follow-up, > we should remove any #ifdef CONFIG_64BIT_TIME completely. > > Link: > https://lore.kernel.org/lkml/20190422090710.bmxdhhankurhafxq@sghpc.golosunov.pp.ru/ > Cc: Lukasz Majewski <lukma@denx.de> Cc: Stepan Golosunov > <stepan@golosunov.pp.ru> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Please apply this one as a bugfix for 5.1 > --- > arch/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index 33687dddd86a..9092e0ffe4d3 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -764,7 +764,7 @@ config COMPAT_OLD_SIGACTION > bool > > config 64BIT_TIME > - def_bool ARCH_HAS_64BIT_TIME > + def_bool y > help > This should be selected by all architectures that need to > support new system calls with a 64-bit time_t. This is relevant on > all 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
27.04.2019 в 00:46:53 +0200 Lukasz Majewski написал: > Hi Arnd, > > > As Stepan Golosunov points out, we made a small mistake in the > > get_timespec64() function in the kernel. It was originally added under > > the assumption that CONFIG_64BIT_TIME would get enabled on all 32-bit > > and 64-bit architectures, but when I did the conversion, I only turned > > it on for 32-bit ones. > > > > The effect is that the get_timespec64() function never clears the > > upper half of the tv_nsec field for 32-bit tasks in compat mode. > > Clearing this is required for POSIX compliant behavior of functions > > that pass a 'timespec' structure with a 64-bit tv_sec and a 32-bit > > tv_nsec, plus uninitialized padding. > At least for my setup (32bit ARM with 64 bit time support) this patch is > not fixing anything. The patch is not supposed to fix anything on 32-bit architectures as in-kernel struct timespec64 has 32-bit tv_nsec there. Thus truncation should happen automatically. I also missed that fact when I was reading get_timespec64 code. (I am wondering whether such trucation is undefined behaviour in C and whether there should be sign-extension instead of zeroing-out for the in_compat_syscall() case though.) > > The easiest fix for linux-5.1 is to just make the Kconfig symbol > > unconditional, as it was originally intended. As a follow-up, > > we should remove any #ifdef CONFIG_64BIT_TIME completely. > > > > Link: > > https://lore.kernel.org/lkml/20190422090710.bmxdhhankurhafxq@sghpc.golosunov.pp.ru/ > > Cc: Lukasz Majewski <lukma@denx.de> Cc: Stepan Golosunov > > <stepan@golosunov.pp.ru> Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > Please apply this one as a bugfix for 5.1 > > --- > > arch/Kconfig | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index 33687dddd86a..9092e0ffe4d3 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -764,7 +764,7 @@ config COMPAT_OLD_SIGACTION > > bool > > > > config 64BIT_TIME > > - def_bool ARCH_HAS_64BIT_TIME > > + def_bool y > > help > > This should be selected by all architectures that need to > > support new system calls with a 64-bit time_t. This is relevant on > > all 32-bit
Hi Stepan, > 27.04.2019 в 00:46:53 +0200 Lukasz Majewski написал: > > Hi Arnd, > > > > > As Stepan Golosunov points out, we made a small mistake in the > > > get_timespec64() function in the kernel. It was originally added > > > under the assumption that CONFIG_64BIT_TIME would get enabled on > > > all 32-bit and 64-bit architectures, but when I did the > > > conversion, I only turned it on for 32-bit ones. > > > > > > The effect is that the get_timespec64() function never clears the > > > upper half of the tv_nsec field for 32-bit tasks in compat mode. > > > Clearing this is required for POSIX compliant behavior of > > > functions that pass a 'timespec' structure with a 64-bit tv_sec > > > and a 32-bit tv_nsec, plus uninitialized padding. > > > At least for my setup (32bit ARM with 64 bit time support) this > > patch is not fixing anything. > > The patch is not supposed to fix anything on 32-bit architectures as > in-kernel struct timespec64 has 32-bit tv_nsec there. Thus truncation > should happen automatically. I also missed that fact when I was > reading get_timespec64 code. Yes. You are right. The tv_nsec is 32 bit. > > (I am wondering whether such trucation is undefined behaviour in C According to [1] - Chapter 6.3.1.3 - Point 3 it is implementation-defined. > and > whether there should be sign-extension instead of zeroing-out for the > in_compat_syscall() case though.) What I've found is that "typically" the high order bits are discarded. However, it is still "implementation dependent". > > > > The easiest fix for linux-5.1 is to just make the Kconfig symbol > > > unconditional, as it was originally intended. As a follow-up, > > > we should remove any #ifdef CONFIG_64BIT_TIME completely. > > > > > > Link: > > > https://lore.kernel.org/lkml/20190422090710.bmxdhhankurhafxq@sghpc.golosunov.pp.ru/ > > > Cc: Lukasz Majewski <lukma@denx.de> Cc: Stepan Golosunov > > > <stepan@golosunov.pp.ru> Signed-off-by: Arnd Bergmann > > > <arnd@arndb.de> --- > > > Please apply this one as a bugfix for 5.1 > > > --- > > > arch/Kconfig | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > > index 33687dddd86a..9092e0ffe4d3 100644 > > > --- a/arch/Kconfig > > > +++ b/arch/Kconfig > > > @@ -764,7 +764,7 @@ config COMPAT_OLD_SIGACTION > > > bool > > > > > > config 64BIT_TIME > > > - def_bool ARCH_HAS_64BIT_TIME > > > + def_bool y > > > help > > > This should be selected by all architectures that need > > > to support new system calls with a 64-bit time_t. This is > > > relevant on all 32-bit Note: [1] - http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf 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 Sat, Apr 27, 2019 at 5:06 PM Lukasz Majewski <lukma@denx.de> wrote: > > 27.04.2019 в 00:46:53 +0200 Lukasz Majewski написал: > > (I am wondering whether such trucation is undefined behaviour in C > > According to [1] - Chapter 6.3.1.3 - Point 3 it is > implementation-defined. The kernel relies on the sane behavior for integer overflow in many places already, and it is built with -fno-strict-overflow to also make sure gcc doesn't optimize cases that would be undefined otherwise. > > and > > whether there should be sign-extension instead of zeroing-out for the > > in_compat_syscall() case though.) > > What I've found is that "typically" the high order bits are discarded. > > However, it is still "implementation dependent". I think the question was whether we should use kts.tv_nsec = (int)kts.tv_nsec; instead of kts.tv_nsec &= 0xfffffffful; Both have a clearly defined meaning in the C dialect we use in the kernel, but differ in the upper 32 bits for negative input values. I would say that using sign-extension indeed makes more sense here, but we don't need to change it for linux-5.1, since none of the callers of get_timespec64() care -- any negative 32-bit tv_nsec value results in -EINVAL, including the utimensat() syscall that has two special cases outside of the 0...999999999 range. Arnd
On Fri, 26 Apr 2019, Arnd Bergmann wrote: > As Stepan Golosunov points out, we made a small mistake in the > get_timespec64() function in the kernel. It was originally added under > the assumption that CONFIG_64BIT_TIME would get enabled on all 32-bit > and 64-bit architectures, but when I did the conversion, I only turned > it on for 32-bit ones. > > The effect is that the get_timespec64() function never clears the upper > half of the tv_nsec field for 32-bit tasks in compat mode. Clearing this > is required for POSIX compliant behavior of functions that pass a > 'timespec' structure with a 64-bit tv_sec and a 32-bit tv_nsec, plus > uninitialized padding. > > The easiest fix for linux-5.1 is to just make the Kconfig symbol > unconditional, as it was originally intended. As a follow-up, > we should remove any #ifdef CONFIG_64BIT_TIME completely. > > Link: https://lore.kernel.org/lkml/20190422090710.bmxdhhankurhafxq@sghpc.golosunov.pp.ru/ > Cc: Lukasz Majewski <lukma@denx.de> > Cc: Stepan Golosunov <stepan@golosunov.pp.ru> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > Please apply this one as a bugfix for 5.1 Can you provide a 'Fixes: ....' tag please? Thanks, tglx
On Mon, Apr 29, 2019 at 9:34 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > On Fri, 26 Apr 2019, Arnd Bergmann wrote: > > > As Stepan Golosunov points out, we made a small mistake in the > > get_timespec64() function in the kernel. It was originally added under > > the assumption that CONFIG_64BIT_TIME would get enabled on all 32-bit > > and 64-bit architectures, but when I did the conversion, I only turned > > it on for 32-bit ones. > > > > The effect is that the get_timespec64() function never clears the upper > > half of the tv_nsec field for 32-bit tasks in compat mode. Clearing this > > is required for POSIX compliant behavior of functions that pass a > > 'timespec' structure with a 64-bit tv_sec and a 32-bit tv_nsec, plus > > uninitialized padding. > > > > The easiest fix for linux-5.1 is to just make the Kconfig symbol > > unconditional, as it was originally intended. As a follow-up, > > we should remove any #ifdef CONFIG_64BIT_TIME completely. > > > > Link: https://lore.kernel.org/lkml/20190422090710.bmxdhhankurhafxq@sghpc.golosunov.pp.ru/ > > Cc: Lukasz Majewski <lukma@denx.de> > > Cc: Stepan Golosunov <stepan@golosunov.pp.ru> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > Please apply this one as a bugfix for 5.1 > > Can you provide a 'Fixes: ....' tag please? Ok, resent both patches now. I also took the chance to add a clarification for the point that Lukasz missed on the first submission. Arnd
diff --git a/arch/Kconfig b/arch/Kconfig index 33687dddd86a..9092e0ffe4d3 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -764,7 +764,7 @@ config COMPAT_OLD_SIGACTION bool config 64BIT_TIME - def_bool ARCH_HAS_64BIT_TIME + def_bool y help This should be selected by all architectures that need to support new system calls with a 64-bit time_t. This is relevant on all 32-bit
As Stepan Golosunov points out, we made a small mistake in the get_timespec64() function in the kernel. It was originally added under the assumption that CONFIG_64BIT_TIME would get enabled on all 32-bit and 64-bit architectures, but when I did the conversion, I only turned it on for 32-bit ones. The effect is that the get_timespec64() function never clears the upper half of the tv_nsec field for 32-bit tasks in compat mode. Clearing this is required for POSIX compliant behavior of functions that pass a 'timespec' structure with a 64-bit tv_sec and a 32-bit tv_nsec, plus uninitialized padding. The easiest fix for linux-5.1 is to just make the Kconfig symbol unconditional, as it was originally intended. As a follow-up, we should remove any #ifdef CONFIG_64BIT_TIME completely. Link: https://lore.kernel.org/lkml/20190422090710.bmxdhhankurhafxq@sghpc.golosunov.pp.ru/ Cc: Lukasz Majewski <lukma@denx.de> Cc: Stepan Golosunov <stepan@golosunov.pp.ru> Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- Please apply this one as a bugfix for 5.1 --- arch/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.20.0