From: Arnd Bergmann <arnd@arndb.de>
To: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>
Cc: John Stultz <john.stultz@linaro.org>, Thomas Gleixner <tglx@linutronix.de>,
        y2038@lists.linaro.org, lkml <linux-kernel@vger.kernel.org>,
        Baolin Wang <baolin.wang@linaro.org>
Subject: Re: [RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture
Date: Thu, 09 Jul 2015 12:26:16 +0200
Message-ID: <1819798.ecaiCVEJzg@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <559E38B7.2030201@linaro.org>
References: <1435587807-10008-1-git-send-email-bamvor.zhangjian@linaro.org> <CALAqxLU0XnQfmPLOVqRtv0z_jU6kDFdK6rkfBNh11Ri-q5gPqw@mail.gmail.com> <559E38B7.2030201@linaro.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2292
Lines: 57

On Thursday 09 July 2015 17:02:47 Bamvor Zhang Jian wrote:
> On 07/09/2015 04:09 AM, John Stultz wrote:
> > On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian
> > <bamvor.zhangjian@linaro.org> wrote:
> >> +int get_timeval64(struct timeval64 *tv,
> >> +                  const struct __kernel_timeval __user *utv)
> >> +{
> >> +       struct __kernel_timeval ktv;
> >> +       int ret;
> >> +
> >> +       ret = copy_from_user(&ktv, utv, sizeof(ktv));
> >> +       if (ret)
> >> +               return -EFAULT;
> >> +
> >> +       tv->tv_sec = ktv.tv_sec;
> >> +       if (!IS_ENABLED(CONFIG_64BIT)
> >> +#ifdef CONFIG_COMPAT
> >> +          || is_compat_task()
> >> +#endif
> >
> > These sorts of ifdefs are to be avoided inside of functions.
> 
> > Instead, it seems is_compat_task() should be defined to 0 in the
> > !CONFIG_COMPAT case, so you can avoid the ifdefs and the compiler can
> > still optimize it out.
> I add this ifdef because I got compile failure on arm platform. This
> file do not include the <linux/compat.h> directly. And in arm64,
> compat.h is included implicitily.
> So, I am not sure what I should do here. Include <linux/compat.h> in
> this file directly or add a this check at the beginning of this file?
> 
> #ifndef is_compat_task
> #define is_compat_task() (0)
> #endif
> 

Actually I think we can completely skip this test here: Unlike
timespec, timeval is defined in a way that always lets user space
use a 64-bit type for the microsecond portion (suseconds_t tv_usec).

I think we should simplify this case and just assume that user space
does exactly that, and treat a tv_usec value with a nonzero upper
half as an error.

I would also keep this function local to the ppdev driver, in order
to not proliferate this to generic kernel code, but that is something
we can debate, based on what other drivers need. For core kernel
code, we should not need a get_timeval64 function because all system
calls that pass a timeval structure are obsolete and we don't need
to provide 64-bit time_t variants of them.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/