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 <arnd@arndb.de>
Date: Fri, 15 Dec 2017 13:02:41 +0100
Message-ID: <CAK8P3a150Hw62O06Qfdty+9zTr1m_=jJ5PZRE0NZt1qpsTcJBg@mail.gmail.com>
Subject: Re: [Y2038] [PATCH v2 08/10] fix get_timespec64() for y2038 safe
 compat interfaces
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4670
Lines: 140

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