Re: [Y2038] [RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture
Hi, John
On 07/09/2015 04:09 AM, John Stultz wrote:
On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian bamvor.zhangjian@linaro.org wrote:
+int get_timeval64(struct timeval64 *tv,
const struct __kernel_timeval __user *utv)+{
struct __kernel_timeval ktv;int ret;ret = copy_from_user(&ktv, utv, sizeof(ktv));if (ret)return -EFAULT;tv->tv_sec = ktv.tv_sec;if (!IS_ENABLED(CONFIG_64BIT)+#ifdef CONFIG_COMPAT
|| is_compat_task()+#endif
These sorts of ifdefs are to be avoided inside of functions.
Instead, it seems is_compat_task() should be defined to 0 in the !CONFIG_COMPAT case, so you can avoid the ifdefs and the compiler can still optimize it out.
I add this ifdef because I got compile failure on arm platform. This file do not include the <linux/compat.h> directly. And in arm64, compat.h is included implicitily. So, I am not sure what I should do here. Include <linux/compat.h> in this file directly or add a this check at the beginning of this file?
#ifndef is_compat_task #define is_compat_task() (0) #endif
Otherwise this looks similar to a patch Baolin (cc'ed) has been working on.
Yes.
regards
bamvor
thanks -john
On Thursday 09 July 2015 17:02:47 Bamvor Zhang Jian wrote:
On 07/09/2015 04:09 AM, John Stultz wrote:
On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian bamvor.zhangjian@linaro.org wrote:
+int get_timeval64(struct timeval64 *tv,
const struct __kernel_timeval __user *utv)+{
struct __kernel_timeval ktv;int ret;ret = copy_from_user(&ktv, utv, sizeof(ktv));if (ret)return -EFAULT;tv->tv_sec = ktv.tv_sec;if (!IS_ENABLED(CONFIG_64BIT)+#ifdef CONFIG_COMPAT
|| is_compat_task()+#endif
These sorts of ifdefs are to be avoided inside of functions.
Instead, it seems is_compat_task() should be defined to 0 in the !CONFIG_COMPAT case, so you can avoid the ifdefs and the compiler can still optimize it out.
I add this ifdef because I got compile failure on arm platform. This file do not include the <linux/compat.h> directly. And in arm64, compat.h is included implicitily. So, I am not sure what I should do here. Include <linux/compat.h> in this file directly or add a this check at the beginning of this file?
#ifndef is_compat_task #define is_compat_task() (0) #endif
Actually I think we can completely skip this test here: Unlike timespec, timeval is defined in a way that always lets user space use a 64-bit type for the microsecond portion (suseconds_t tv_usec).
I think we should simplify this case and just assume that user space does exactly that, and treat a tv_usec value with a nonzero upper half as an error.
I would also keep this function local to the ppdev driver, in order to not proliferate this to generic kernel code, but that is something we can debate, based on what other drivers need. For core kernel code, we should not need a get_timeval64 function because all system calls that pass a timeval structure are obsolete and we don't need to provide 64-bit time_t variants of them.
Arnd
Hi, Arnd
On 07/09/2015 06:26 PM, Arnd Bergmann wrote:
On Thursday 09 July 2015 17:02:47 Bamvor Zhang Jian wrote:
On 07/09/2015 04:09 AM, John Stultz wrote:
On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian bamvor.zhangjian@linaro.org wrote:
+int get_timeval64(struct timeval64 *tv,
const struct __kernel_timeval __user *utv)+{
struct __kernel_timeval ktv;int ret;ret = copy_from_user(&ktv, utv, sizeof(ktv));if (ret)return -EFAULT;tv->tv_sec = ktv.tv_sec;if (!IS_ENABLED(CONFIG_64BIT)+#ifdef CONFIG_COMPAT
|| is_compat_task()+#endif
These sorts of ifdefs are to be avoided inside of functions.
Instead, it seems is_compat_task() should be defined to 0 in the !CONFIG_COMPAT case, so you can avoid the ifdefs and the compiler can still optimize it out.
I add this ifdef because I got compile failure on arm platform. This file do not include the <linux/compat.h> directly. And in arm64, compat.h is included implicitily. So, I am not sure what I should do here. Include <linux/compat.h> in this file directly or add a this check at the beginning of this file?
#ifndef is_compat_task #define is_compat_task() (0) #endif
Actually I think we can completely skip this test here: Unlike timespec, timeval is defined in a way that always lets user space use a 64-bit type for the microsecond portion (suseconds_t tv_usec).
I do not familar with this type. I grep the suseconds_t in glibc, it seems that suseconds_t(__SUSECONDS_T_TYPE) is defined as __SYSCALL_SLONG_TYPE which is __SLONGWORD_TYPE(32bit on 32bit architecture).
I think we should simplify this case and just assume that user space does exactly that, and treat a tv_usec value with a nonzero upper half as an error.
I would also keep this function local to the ppdev driver, in order to not proliferate this to generic kernel code, but that is something we can debate, based on what other drivers need. For core kernel code, we should not need a get_timeval64 function because all system calls that pass a timeval structure are obsolete and we don't need to provide 64-bit time_t variants of them.
Got it.
regards
bamvor
Arnd
On Wednesday 15 July 2015 11:18:31 Bamvor Zhang Jian wrote:
Hi, Arnd
On 07/09/2015 06:26 PM, Arnd Bergmann wrote:
On Thursday 09 July 2015 17:02:47 Bamvor Zhang Jian wrote:
On 07/09/2015 04:09 AM, John Stultz wrote:
On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian bamvor.zhangjian@linaro.org wrote:
+int get_timeval64(struct timeval64 *tv,
const struct __kernel_timeval __user *utv)+{
struct __kernel_timeval ktv;int ret;ret = copy_from_user(&ktv, utv, sizeof(ktv));if (ret)return -EFAULT;tv->tv_sec = ktv.tv_sec;if (!IS_ENABLED(CONFIG_64BIT)+#ifdef CONFIG_COMPAT
|| is_compat_task()+#endif
These sorts of ifdefs are to be avoided inside of functions.
Instead, it seems is_compat_task() should be defined to 0 in the !CONFIG_COMPAT case, so you can avoid the ifdefs and the compiler can still optimize it out.
I add this ifdef because I got compile failure on arm platform. This file do not include the <linux/compat.h> directly. And in arm64, compat.h is included implicitily. So, I am not sure what I should do here. Include <linux/compat.h> in this file directly or add a this check at the beginning of this file?
#ifndef is_compat_task #define is_compat_task() (0) #endif
Actually I think we can completely skip this test here: Unlike timespec, timeval is defined in a way that always lets user space use a 64-bit type for the microsecond portion (suseconds_t tv_usec).
I do not familar with this type. I grep the suseconds_t in glibc, it seems that suseconds_t(__SUSECONDS_T_TYPE) is defined as __SYSCALL_SLONG_TYPE which is __SLONGWORD_TYPE(32bit on 32bit architecture).
Correct, but POSIX allows it to be redefined along with time_t, so timeval can be a pair of 64-bit values. In contrast, timespec is required by POSIX (and C11) to be a time_t and a 'long', which is why we need a hack to check the size of the second word of the timespec structure.
Arnd
-
Arnd Bergmann -
Bamvor Zhang Jian