From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755153AbdLOMCp (ORCPT ); Fri, 15 Dec 2017 07:02:45 -0500 Received: from mail-oi0-f65.google.com ([209.85.218.65]:46689 "EHLO mail-oi0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754672AbdLOMCn (ORCPT ); Fri, 15 Dec 2017 07:02:43 -0500 X-Google-Smtp-Source: ACJfBos6/JIMxDqTEoed7hoHAiHxtqYnskht23mztyo2E6Gf9sWaY4qW+j7k8PfiTiQoHc8O/zy6lKG4aHt7h0ozM7g= MIME-Version: 1.0 In-Reply-To: <1513297310.18523.293.camel@codethink.co.uk> References: <20171127193037.8711-1-deepa.kernel@gmail.com> <20171127193037.8711-9-deepa.kernel@gmail.com> <1513297310.18523.293.camel@codethink.co.uk> From: Arnd Bergmann Date: Fri, 15 Dec 2017 13:02:41 +0100 X-Google-Sender-Auth: x7j7AJKoaRwo_EqaQGz8MOV7r5o Message-ID: Subject: Re: [Y2038] [PATCH v2 08/10] fix get_timespec64() for y2038 safe compat interfaces To: Ben Hutchings Cc: Deepa Dinamani , Thomas Gleixner , John Stultz , y2038 Mailman List , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 15, 2017 at 1:21 AM, Ben Hutchings 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