On Fri, Dec 15, 2017 at 1:21 AM, Ben Hutchings ben.hutchings@codethink.co.uk wrote:
On Mon, 2017-11-27 at 11:30 -0800, Deepa Dinamani wrote:
get/put_timespec64() interfaces will eventually be used for conversions between the new y2038 safe struct __kernel_timespec and struct timespec64.
The new y2038 safe syscalls have a common entry for native and compat interfaces. On compat interfaces, the high order bits of nanoseconds should be zeroed out. This is because the application code or the libc do not garuntee zeroing of these. If used without
Spelling: "guarantee"
[...]
--- a/kernel/time/time.c +++ b/kernel/time/time.c
[...]
@@ -851,6 +851,11 @@ int get_timespec64(struct timespec64 *ts, return -EFAULT;
ts->tv_sec = kts.tv_sec;
/* Zero out the padding for 32 bit systems or in compat mode */if (IS_ENABLED(CONFIG_64BIT_TIME) || !IS_ENABLED(CONFIG_64BIT) || in_compat_syscall())kts.tv_nsec &= 0xFFFFFFFFUL;[...]
I don't understand the condition here. Suppose we're building for an architecture that enables the new syscalls and selects ARCH_64BIT_TIME, but we also enable 64BIT. Then the above condition ends up as: if (1 || 0 || in_compat_syscall()) so it's always true.
Should the condition be: if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall()) or is your intent that architectures only select ARCH_64BIT_TIME if 64BIT is not enabled?
My understanding was that we always enable CONFIG_64BIT_TIME when 64BIT is enabled.
For a 64-bit architecture, we must not clear the upper 32 bits of tv_nsec, but instead need later check them for being nonzero. I think the correct condition would be
if ((IS_ENABLED(CONFIG_64BIT_TIME) && !IS_ENABLED(CONFIG_64BIT)) || in_compat_syscall())
Two more thoughts:
- The temporary variable here is defined as 'struct timespec', this must be changed to __kernel_timespec for the function to work correctly once we switch a 32-bit architecture over. Doing it in this patch is probably the best time for that change.
- I had an idea to handle the copying of timespec/timeval with a one-size-fits-all function and multiple wrappers around it, such as
enum user_ts_type { USER_TS_TIMEVAL = 1, USER_TS_32 = 2, USER_TS_CLEARNSEC = 4, USER_TS_NOCHECK = 8, };
/* native handlers want to check on 64-bit but zero on 32-bit */ #define USER_TS_NATIVE (IS_ENABLED(CONFIG_64BIT) ? 0 : USER_TS_CLEARNSEC)
/* compat handlers accessing 64-bit time structs always want to clear the upper half */ #define USER_TS_COMPAT64 USER_TS_CLEARNSEC
/* on x32, we always use 64-bit time_t but want to clear the upper half */ #define USER_TS_COMPAT32 (COMPAT_USE_64BIT_TIME) ? USER_TS_CLEARNSEC : USER_TS_32)
int get_timestruct(struct timespec64 *ts, const void __user *uts, enum user_ts_type flags) { int ret;
if (flags & USER_TS_32) { struct compat_timespec ts32; ret = copy_from_user(&ts32, uts, sizeof(ts32)); if (ret) return -EFAULT; ts->tv_sec = ts32.tv_sec; ts->tv_nsec = ts32.tv_nsec; } else { ret = copy_from_user(&ts, uts, sizeof(*ts)); if (ret) return -EFAULT; if (flags & USER_TS_CLEARNSEC) ts->tv_nsec &= 0xFFFFFFFFUL; }
if (flags & USER_TS_TIMEVAL) { if (!(flags & USER_TS_NOCHECK) && ts->tv_nsec >= USEC_PER_SEC) return -EINVAL;
ts->tv_nsec *= NSEC_PER_USEC; } else { if (!(flags & USER_TS_NOCHECK) && ts->tv_nsec >= NSEC_PER_SEC) return -EINVAL; }
return 0; }
int get_timespec64(struct timespec64 *ts, const struct compat_timespec __user *uts) { return get_timestruct(ts. uts, USER_TS_NATIVE); }
int get_compat_timespec32(struct timespec64 *ts, const struct compat_timespec __user *uts) { return get_timestruct(ts. uts, USER_TS_COMPAT32); }
int get_compat_timespec64(struct timespec64 *ts, const struct __kernel_timespec __user *uts) { return get_timestruct(ts. uts, USER_TS_COMPAT64); }
While working on the driver patches I encountered lots of different combinations of those that might be interesting here, so we could have wrappers for the most common ones and call get_timestruct() and put_timestruct() directly for the less common variations. Am I taking it too far here, or would that make sense?
Arnd