From: Arnd Bergmann <arnd@arndb.de>
To: Ben Hutchings <ben.hutchings@codethink.co.uk>
Cc: Deepa Dinamani <deepa.kernel@gmail.com>,
Thomas Gleixner <tglx@linutronix.de>,
John Stultz <john.stultz@linaro.org>,
y2038 Mailman List <y2038@lists.linaro.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [Y2038] [PATCH v2 08/10] fix get_timespec64() for y2038 safe compat interfaces
Date: Fri, 15 Dec 2017 13:02:41 +0100 [thread overview]
Message-ID: <CAK8P3a150Hw62O06Qfdty+9zTr1m_=jJ5PZRE0NZt1qpsTcJBg@mail.gmail.com> (raw)
In-Reply-To: <1513297310.18523.293.camel@codethink.co.uk>
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
next prev parent reply other threads:[~2017-12-15 12:02 UTC|newest]
Thread overview: 49+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-27 19:30 [PATCH v2 00/10] posix_clocks: Prepare syscalls for 64 bit time_t conversion Deepa Dinamani
2017-11-27 19:30 ` Deepa Dinamani
2017-11-27 19:30 ` Deepa Dinamani
2017-11-27 19:30 ` Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 01/10] compat: Make compat helpers independent of CONFIG_COMPAT Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 02/10] include: Move compat_timespec/ timeval to compat_time.h Deepa Dinamani
2017-11-27 19:30 ` Deepa Dinamani
2017-11-27 19:30 ` Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 03/10] compat: enable compat_get/put_timespec64 always Deepa Dinamani
2017-12-14 23:27 ` [Y2038] " Ben Hutchings
2018-01-07 16:28 ` Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 04/10] arch: introduce CONFIG_64BIT_TIME Deepa Dinamani
2017-12-14 23:22 ` [Y2038] " Ben Hutchings
2017-11-27 19:30 ` [PATCH v2 05/10] arch: Introduce CONFIG_COMPAT_32BIT_TIME Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 06/10] posix-clocks: Make compat syscalls depend on CONFIG_COMPAT_32BIT_TIME Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 07/10] include: Add new y2038 safe __kernel_timespec Deepa Dinamani
2017-12-15 0:11 ` [Y2038] " Ben Hutchings
2017-12-15 10:36 ` Arnd Bergmann
2017-12-15 10:36 ` Arnd Bergmann
2017-11-27 19:30 ` [PATCH v2 08/10] fix get_timespec64() for y2038 safe compat interfaces Deepa Dinamani
2017-12-15 0:21 ` [Y2038] " Ben Hutchings
2017-12-15 12:02 ` Arnd Bergmann [this message]
2017-12-17 23:51 ` Ben Hutchings
2017-12-18 5:11 ` Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 09/10] change time types to new y2038 safe __kernel_* types Deepa Dinamani
2017-11-27 19:30 ` [PATCH v2 10/10] nanosleep: change time types to " Deepa Dinamani
2017-11-27 19:30 ` Deepa Dinamani
2017-12-15 0:31 ` [Y2038] " Ben Hutchings
2017-12-15 0:31 ` Ben Hutchings
2017-11-27 21:58 ` [PATCH v2 00/10] posix_clocks: Prepare syscalls for 64 bit time_t conversion Arnd Bergmann
2017-11-27 21:58 ` Arnd Bergmann
2017-11-27 21:58 ` Arnd Bergmann
2017-11-27 21:58 ` Arnd Bergmann
2017-11-27 22:29 ` Deepa Dinamani
2017-11-27 22:29 ` Deepa Dinamani
2017-11-27 22:29 ` Deepa Dinamani
2017-11-27 22:29 ` Deepa Dinamani
2017-11-28 14:17 ` Arnd Bergmann
2017-11-28 14:17 ` Arnd Bergmann
2017-11-28 14:17 ` Arnd Bergmann
2017-11-28 14:17 ` Arnd Bergmann
2017-11-28 23:17 ` Deepa Dinamani
2017-11-28 23:17 ` Deepa Dinamani
2017-11-28 23:17 ` Deepa Dinamani
2017-11-28 23:17 ` Deepa Dinamani
2017-11-29 21:12 ` Arnd Bergmann
2017-11-29 21:12 ` Arnd Bergmann
2017-11-29 21:12 ` Arnd Bergmann
2017-11-29 21:12 ` Arnd Bergmann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAK8P3a150Hw62O06Qfdty+9zTr1m_=jJ5PZRE0NZt1qpsTcJBg@mail.gmail.com' \
--to=arnd@arndb.de \
--cc=ben.hutchings@codethink.co.uk \
--cc=deepa.kernel@gmail.com \
--cc=john.stultz@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tglx@linutronix.de \
--cc=y2038@lists.linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.