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?
Ben.
--
Ben Hutchings
Software Developer, Codethink Ltd.
On Fri, Dec 15, 2017 at 1:21 AM, Ben Hutchings
<[email protected]> 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
On Fri, 2017-12-15 at 13:02 +0100, Arnd Bergmann wrote:
[...]
> - 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,
> };
[...]
> 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?
I don't think I've reviewed enough time-handling stuff to know, but I
can certainly believe that this could be worth doing.
Ben.
--
Ben Hutchings
Software Developer, Codethink Ltd.
On Fri, Dec 15, 2017 at 4:02 AM, Arnd Bergmann <[email protected]> wrote:
> On Fri, Dec 15, 2017 at 1:21 AM, Ben Hutchings
> <[email protected]> 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())
I haven't enabled this by default on all 64 bit architectures.
The reason I have the condition this way is that I haven't decided how
I want to handle 64 bit time on x32, and x86 is the first architecture
I plan to enable this for.
At that time, this will be reworked based on whatever solution we
agree on. I did not want to depend on COMPAT_USE_64BIT_TIME yet.
But, I did mean to do
if (IS_ENABLED(CONFIG_64BIT_TIME) && (!IS_ENABLED(CONFIG_64BIT)) ||
in_compat_syscall()))
I will update this.
> 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.
Thanks, will do. I will post an update.
> - 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?
I think this is a little confusing.
nanosleep_copyout() uses a similar strategy. But, I think it makes
sense in that case as it uses common functions for all versions of
nanosleep and because of the union.
-Deepa