[RFC PATCH v2 0/4] Convert ppdev to y2038 safe

[RFC PATCH v2 0/4] Convert ppdev to y2038 safe

[Bamvor Zhang Jian]

Hi, guys

This is my second attempt to convert ppdev to y2038 safe. The first
version is here[1].

There are two parts in my patches.
01/02 migrate timeval relative struct to 64bit time_t types.
03/04 convert ppdev to y2038 safe in both native 32bit and compat
application.

My patches try to follow the idea from arnd y2038 syscalls patches[2],
but my patches not depend on them.

The reason why I choose ppdev is the ppdev use the timexxx directly
in ioctl compare with the other drivers embedded timexxx in their
own type.

Build pass on arm and arm64 on each patches(with and without
CONFIG_COMPAT_TIME). Unfortunately, there is no parport device
(printer) in my test environment. Hope others could help to test
it.

[1] https://lists.linaro.org/pipermail/y2038/2015-June/000522.html
[2] http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/log/?h=y2038-syscalls

Bamvor Zhang Jian (4):
  y2038: add 64bit time_t support in timeval for 32bit architecture
  time64: add timeval64 helper for compat syscalls
  ppdev: add compat ioctl
  y2038: convert ppdev to 2038 safe

 drivers/char/ppdev.c       | 41 ++++++++++++++++++++++++++++++++++-------
 include/linux/compat.h     |  3 +++
 include/linux/time64.h     | 20 ++++++++++++++++++--
 include/uapi/linux/ppdev.h | 14 ++++++++++++--
 include/uapi/linux/time.h  | 16 ++++++++++++++++
 kernel/compat.c            | 17 +++++++++++++++++
 kernel/time/time.c         | 36 ++++++++++++++++++++++++++++++++++++
 7 files changed, 136 insertions(+), 11 deletions(-)

-- 
2.1.4

[RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture

[Bamvor Zhang Jian]

Add timeval64 structure and copy_(from)|(to)_user functions. Add
__kernel_timeval for syscalls.

This changes follow the similar way in the followings commit:
361a3bf time64: Add time64.h header and define struct timespec64
49cd6f8 time: More core infrastructure for timespec64
ca2c9c5 y2038: introduce struct __kernel_timespec[1].

[1] http://git.kernel.org/cgit/linux/kernel/git/arnd/playground.git/commit/?h=y2038-syscalls&id=9005d4f4a44fc56bd0a1fe7c08e8e3f13eb75de7

Signed-off-by: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>
---
 include/linux/time64.h    | 20 ++++++++++++++++++--
 include/uapi/linux/time.h | 10 ++++++++++
 kernel/time/time.c        | 36 ++++++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/include/linux/time64.h b/include/linux/time64.h
index 77b5df2..2ca4f85 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -1,24 +1,35 @@
 #ifndef _LINUX_TIME64_H
 #define _LINUX_TIME64_H
 
-#include <uapi/linux/time.h>
-#include <linux/math64.h>
 
 typedef __s64 time64_t;
 
+#ifndef CONFIG_COMPAT_TIME
+# define __kernel_timeval timeval
+#endif
+
 /*
  * This wants to go into uapi/linux/time.h once we agreed about the
  * userspace interfaces.
  */
 #if __BITS_PER_LONG == 64
 # define timespec64 timespec
+# define timeval64 timeval
 #else
 struct timespec64 {
 	time64_t	tv_sec;			/* seconds */
 	long		tv_nsec;		/* nanoseconds */
 };
+
+struct timeval64 {
+	time64_t		tv_sec;		/* seconds */
+	__kernel_suseconds_t	tv_usec;	/* microseconds */
+};
 #endif
 
+#include <uapi/linux/time.h>
+#include <linux/math64.h>
+
 /* Parameters used to convert the timespec values: */
 #define MSEC_PER_SEC	1000L
 #define USEC_PER_MSEC	1000L
@@ -189,4 +200,9 @@ static __always_inline void timespec64_add_ns(struct timespec64 *a, u64 ns)
 
 #endif
 
+extern int get_timeval64(struct timeval64 *tv,
+                         const struct __kernel_timeval __user *utv);
+extern int put_timeval64(const struct timeval64 *tv,
+                         struct __kernel_timeval __user *utv);
+
 #endif /* _LINUX_TIME64_H */
diff --git a/include/uapi/linux/time.h b/include/uapi/linux/time.h
index e75e1b6..2ca6a31 100644
--- a/include/uapi/linux/time.h
+++ b/include/uapi/linux/time.h
@@ -66,4 +66,14 @@ struct itimerval {
  */
 #define TIMER_ABSTIME			0x01
 
+/* types based on 64-bit time_t */
+#ifndef __kernel_timeval
+typedef __s64 __kernel_time64_t;
+
+struct __kernel_timeval {
+       __kernel_time64_t       tv_sec;
+       __s64                   tv_usec;
+};
+#endif
+
 #endif /* _UAPI_LINUX_TIME_H */
diff --git a/kernel/time/time.c b/kernel/time/time.c
index 85d5bb1..adf0455 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -37,6 +37,7 @@
 #include <linux/fs.h>
 #include <linux/math64.h>
 #include <linux/ptrace.h>
+#include <linux/time64.h>
 
 #include <asm/uaccess.h>
 #include <asm/unistd.h>
@@ -759,3 +760,38 @@ struct timespec timespec_add_safe(const struct timespec lhs,
 
 	return res;
 }
+
+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
+	   )
+               ktv.tv_usec &= 0xfffffffful;
+       tv->tv_usec = ktv.tv_usec;
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(get_timeval64);
+
+int put_timeval64(const struct timeval64 *tv,
+                  struct __kernel_timeval __user *utv)
+{
+       struct __kernel_timeval ktv = {
+               .tv_sec = tv->tv_sec,
+               .tv_usec = tv->tv_usec
+       };
+       return copy_to_user(utv, &utv, sizeof(ktv)) ? -EFAULT : 0;
+}
+EXPORT_SYMBOL_GPL(put_timeval64);
+
-- 
2.1.4

Re: [RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture

[John Stultz]

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.

Otherwise this looks similar to a patch Baolin (cc'ed) has been working on.

thanks
-john

Re: [RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture

[Bamvor Zhang Jian]

Hi, John

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

> Otherwise this looks similar to a patch Baolin (cc'ed) has been working on.
Yes.

regards

bamvor
>
> thanks
> -john
>

Re: [RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture

[Arnd Bergmann]

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

Re: [RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture

[Bamvor Zhang Jian]

Hi, Arnd

On 07/09/2015 06:26 PM, Arnd Bergmann wrote:
> 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 do not familar with this type. I grep the suseconds_t in glibc, it
seems that suseconds_t(__SUSECONDS_T_TYPE) is defined as
__SYSCALL_SLONG_TYPE which is __SLONGWORD_TYPE(32bit on 32bit
architecture).
> 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.
Got it.

regards

bamvor
>
> 	Arnd
>

Re: [RFC PATCH v2 1/4] y2038: add 64bit time_t support in timeval for 32bit architecture

[Arnd Bergmann]

On Wednesday 15 July 2015 11:18:31 Bamvor Zhang Jian wrote:
> Hi, Arnd
> 
> On 07/09/2015 06:26 PM, Arnd Bergmann wrote:
> > 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 do not familar with this type. I grep the suseconds_t in glibc, it
> seems that suseconds_t(__SUSECONDS_T_TYPE) is defined as
> __SYSCALL_SLONG_TYPE which is __SLONGWORD_TYPE(32bit on 32bit
> architecture).

Correct, but POSIX allows it to be redefined along with time_t, so
timeval can be a pair of 64-bit values. In contrast, timespec is
required by POSIX (and C11) to be a time_t and a 'long', which is
why we need a hack to check the size of the second word of the
timespec structure.

	Arnd

[RFC PATCH v2 2/4] time64: add timeval64 helper for compat syscalls

[Bamvor Zhang Jian]

Add __kernel_compat_timeval in uapi in order to use it in ioctl
command because compat_timeval is invisible in uapi. Meanwhile
We could avoid to define it by using the __s32 array in ioctl
command definition. I am sure which one is the better way.
Any suggestion or input is welcome.

This patch also define compat_get_timeval64, compat_put_timeval64
 for converting between compat_timeval and timeval64.

Signed-off-by: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>
---
 include/linux/compat.h    |  3 +++
 include/uapi/linux/time.h |  6 ++++++
 kernel/compat.c           | 17 +++++++++++++++++
 3 files changed, 26 insertions(+)

diff --git a/include/linux/compat.h b/include/linux/compat.h
index ab25814..14569a7 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -154,6 +154,9 @@ extern int compat_get_timespec(struct timespec *, const void __user *);
 extern int compat_put_timespec(const struct timespec *, void __user *);
 extern int compat_get_timeval(struct timeval *, const void __user *);
 extern int compat_put_timeval(const struct timeval *, void __user *);
+struct timeval64;
+extern int compat_get_timeval64(struct timeval64 *tv, const struct compat_timeval __user *ctv);
+extern int compat_put_timeval64(const struct timeval64 *tv, struct compat_timeval __user *ctv);
 
 /*
  * This function convert a timespec if necessary and returns a *user
diff --git a/include/uapi/linux/time.h b/include/uapi/linux/time.h
index 2ca6a31..9f6093e 100644
--- a/include/uapi/linux/time.h
+++ b/include/uapi/linux/time.h
@@ -76,4 +76,10 @@ struct __kernel_timeval {
 };
 #endif
 
+typedef __s32 __kernel_time32_t;
+struct __kernel_compat_timeval {
+       __kernel_time32_t       tv_sec;
+       __s32                   tv_usec;
+};
+
 #endif /* _UAPI_LINUX_TIME_H */
diff --git a/kernel/compat.c b/kernel/compat.c
index 333d364..ebe45b4 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -172,6 +172,23 @@ int compat_put_timeval(const struct timeval *tv, void __user *utv)
 }
 EXPORT_SYMBOL_GPL(compat_put_timeval);
 
+int compat_get_timeval64(struct timeval64 *tv, const struct compat_timeval __user *ctv)
+{
+	return (!access_ok(VERIFY_READ, ctv, sizeof(*ctv)) ||
+			__get_user(tv->tv_sec, &ctv->tv_sec) ||
+			__get_user(tv->tv_usec, &ctv->tv_usec)) ? -EFAULT : 0;
+}
+EXPORT_SYMBOL_GPL(compat_get_timeval64);
+
+/* TODO: is it ok that just put to user without implicit cast? */
+int compat_put_timeval64(const struct timeval64 *tv, struct compat_timeval *ctv)
+{
+	return (!access_ok(VERIFY_WRITE, ctv, sizeof(*ctv)) ||
+			__put_user(tv->tv_sec, &ctv->tv_sec) ||
+			__put_user(tv->tv_usec, &ctv->tv_usec)) ? -EFAULT : 0;
+}
+EXPORT_SYMBOL_GPL(compat_put_timeval64);
+
 int compat_get_timespec(struct timespec *ts, const void __user *uts)
 {
 	if (COMPAT_USE_64BIT_TIME)
-- 
2.1.4

[RFC PATCH v2 3/4] ppdev: add compat ioctl

[Bamvor Zhang Jian]

Add compat ioctl in ppdev in order to solve the y2038 issue in
later patch.
This patch simply add pp_do_ioctl to compat_ioctl, because I found
that all the ioctl access the arg as a pointer.

Signed-off-by: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>
---
 drivers/char/ppdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index ae0b42b..9207658 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -69,6 +69,7 @@
 #include <linux/ppdev.h>
 #include <linux/mutex.h>
 #include <linux/uaccess.h>
+#include <linux/compat.h>
 
 #define PP_VERSION "ppdev: user-space parallel port driver"
 #define CHRDEV "ppdev"
@@ -635,6 +636,11 @@ static long pp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return ret;
 }
 
+static long pp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	return pp_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
+}
+
 static int pp_open (struct inode * inode, struct file * file)
 {
 	unsigned int minor = iminor(inode);
@@ -744,6 +750,9 @@ static const struct file_operations pp_fops = {
 	.write		= pp_write,
 	.poll		= pp_poll,
 	.unlocked_ioctl	= pp_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= pp_compat_ioctl,
+#endif
 	.open		= pp_open,
 	.release	= pp_release,
 };
-- 
2.1.4

Re: [RFC PATCH v2 3/4] ppdev: add compat ioctl

[John Stultz]

On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian
<bamvor.zhangjian@linaro.org> wrote:
> Add compat ioctl in ppdev in order to solve the y2038 issue in
> later patch.
> This patch simply add pp_do_ioctl to compat_ioctl, because I found
> that all the ioctl access the arg as a pointer.
>
> Signed-off-by: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>
> ---
>  drivers/char/ppdev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index ae0b42b..9207658 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -69,6 +69,7 @@
>  #include <linux/ppdev.h>
>  #include <linux/mutex.h>
>  #include <linux/uaccess.h>
> +#include <linux/compat.h>
>
>  #define PP_VERSION "ppdev: user-space parallel port driver"
>  #define CHRDEV "ppdev"
> @@ -635,6 +636,11 @@ static long pp_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>         return ret;
>  }
>
> +static long pp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> +{
> +       return pp_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> +}
> +
>  static int pp_open (struct inode * inode, struct file * file)
>  {
>         unsigned int minor = iminor(inode);
> @@ -744,6 +750,9 @@ static const struct file_operations pp_fops = {
>         .write          = pp_write,
>         .poll           = pp_poll,
>         .unlocked_ioctl = pp_ioctl,
> +#ifdef CONFIG_COMPAT
> +       .compat_ioctl   = pp_compat_ioctl,
> +#endif

Does adding this patch w/o the following patch break 32bit apps using
this on 64bit kernels?

thanks
-john

Re: [Y2038] [RFC PATCH v2 3/4] ppdev: add compat ioctl

[Arnd Bergmann]

On Wednesday 08 July 2015 13:17:18 John Stultz wrote:
> On Mon, Jun 29, 2015 at 7:23 AM, Bamvor Zhang Jian
> <bamvor.zhangjian@linaro.org> wrote:
> > Add compat ioctl in ppdev in order to solve the y2038 issue in
> > later patch.
> > This patch simply add pp_do_ioctl to compat_ioctl, because I found
> > that all the ioctl access the arg as a pointer.
> >
> > Signed-off-by: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>

I just saw this mail fly by when you replied, but I guess it would
have been better to reply when the original mail came.

The description above makes no sense: The problem for compat ioctl
is not whether the argument is a pointer or not, but rather what
data structure it points to. In this case, we already know that 
it is /not/ compatible between 32-bit and 64-bit user space, because
at least two commands need special handling for the timespec
argument, which gets added in patch 4 of the series.

This means patches 3 and 4 have to be swapped in order to allow
bisection and not introduce a bug when only this one gets applied
but patch 4 is missing.

Moreover, all other ioctl commands that are handled in pp_ioctl
need to be checked regarding what their arguments are, including
data structures pointed to by the arguments (recursively, if there
are again pointers in those structures).

> >         unsigned int minor = iminor(inode);
> > @@ -744,6 +750,9 @@ static const struct file_operations pp_fops = {
> >         .write          = pp_write,
> >         .poll           = pp_poll,
> >         .unlocked_ioctl = pp_ioctl,
> > +#ifdef CONFIG_COMPAT
> > +       .compat_ioctl   = pp_compat_ioctl,
> > +#endif

The #ifdef here is not necessary, but will cause a warning on kernels 
that do not define CONFIG_COMPAT, in particular all 32-bit ones.

> Does adding this patch w/o the following patch break 32bit apps using
> this on 64bit kernels?

Without the patch, those apps will all get -EINVAL from the ioctl
handler. With the patch, the kernel actually performs the ioctl
that was requested, but that may use the wrong data structure.

	Arnd

[RFC PATCH v2 4/4] y2038: convert ppdev to 2038 safe

[Bamvor Zhang Jian]

Convert ppdev use 64bit time_t internally by replacing timeval to
timeval64.

In order to migrate to y2038 safe, split ioctl command PP[SG]ETTIME
to PP[SG]ETTIME64(y2038 safe) and PP[SG]ETTIME32 (the legacy
behavior in 32bit application). The 32bit application should make
use of PP[SG]ETTIME64 and __kernel_timeval to migrate to time64_t.

Signed-off-by: Bamvor Zhang Jian <bamvor.zhangjian@linaro.org>
---
 drivers/char/ppdev.c       | 32 +++++++++++++++++++++++++-------
 include/uapi/linux/ppdev.h | 14 ++++++++++++--
 2 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
index 9207658..a103d3d 100644
--- a/drivers/char/ppdev.c
+++ b/drivers/char/ppdev.c
@@ -497,8 +497,9 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		unsigned char mask;
 		int mode;
 		int ret;
-		struct timeval par_timeout;
+		struct timeval64 par_timeout;
 		long to_jiffies;
+		int is_not_compat_timeval = 0;
 
 	case PPRSTATUS:
 		reg = parport_read_status (port);
@@ -593,9 +594,17 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		atomic_sub (ret, &pp->irqc);
 		return 0;
 
-	case PPSETTIME:
-		if (copy_from_user (&par_timeout, argp, sizeof(struct timeval))) {
-			return -EFAULT;
+	case PPSETTIME64:
+		is_not_compat_timeval = 1;
+	case PPSETTIME32:
+		if (is_not_compat_timeval) {
+			if (get_timeval64(&par_timeout, argp)) {
+				return -EFAULT;
+			}
+		} else {
+			if (compat_get_timeval64(&par_timeout, argp)) {
+				return -EFAULT;
+			}
 		}
 		/* Convert to jiffies, place in pp->pdev->timeout */
 		if ((par_timeout.tv_sec < 0) || (par_timeout.tv_usec < 0)) {
@@ -609,13 +618,22 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 		pp->pdev->timeout = to_jiffies;
 		return 0;
 
-	case PPGETTIME:
+	case PPGETTIME64:
+		is_not_compat_timeval = 1;
+	case PPGETTIME32:
 		to_jiffies = pp->pdev->timeout;
 		memset(&par_timeout, 0, sizeof(par_timeout));
 		par_timeout.tv_sec = to_jiffies / HZ;
 		par_timeout.tv_usec = (to_jiffies % (long)HZ) * (1000000/HZ);
-		if (copy_to_user (argp, &par_timeout, sizeof(struct timeval)))
-			return -EFAULT;
+		if (is_not_compat_timeval) {
+			if (put_timeval64(&par_timeout, argp)) {
+				return -EFAULT;
+			}
+		} else {
+			if (compat_put_timeval64(&par_timeout, argp)) {
+				return -EFAULT;
+			}
+		}
 		return 0;
 
 	default:
diff --git a/include/uapi/linux/ppdev.h b/include/uapi/linux/ppdev.h
index dc18c5d..d62a47d 100644
--- a/include/uapi/linux/ppdev.h
+++ b/include/uapi/linux/ppdev.h
@@ -74,8 +74,18 @@ struct ppdev_frob_struct {
 #define PPSETPHASE	_IOW(PP_IOCTL, 0x94, int)
 
 /* Set and get port timeout (struct timeval's) */
-#define PPGETTIME	_IOR(PP_IOCTL, 0x95, struct timeval)
-#define PPSETTIME	_IOW(PP_IOCTL, 0x96, struct timeval)
+/* Force application use 64 time_t ioctl */
+/* TODO: It is an open question about we should use a __xxx_timeval or an
+ * implicit array.
+ * replace struct __kernel_timeval with __s32[4]
+ * replace struct compat_timeval with __s32[2]
+ */
+#define PPGETTIME	PPGETTIME64
+#define PPSETTIME	PPSETTIME64
+#define PPGETTIME64	_IOR(PP_IOCTL, 0x95, struct __kernel_timeval)
+#define PPSETTIME64	_IOW(PP_IOCTL, 0x96, struct __kernel_timeval)
+#define PPGETTIME32	_IOR(PP_IOCTL, 0x9c, struct __kernel_compat_timeval)
+#define PPSETTIME32	_IOW(PP_IOCTL, 0x9d, struct __kernel_compat_timeval)
 
 /* Get available modes (what the hardware can do) */
 #define PPGETMODES	_IOR(PP_IOCTL, 0x97, unsigned int)
-- 
2.1.4

Re: [RFC PATCH v2 4/4] y2038: convert ppdev to 2038 safe

[Arnd Bergmann]

On Monday 29 June 2015 22:23:27 Bamvor Zhang Jian wrote:
> diff --git a/include/uapi/linux/ppdev.h b/include/uapi/linux/ppdev.h
> index dc18c5d..d62a47d 100644
> --- a/include/uapi/linux/ppdev.h
> +++ b/include/uapi/linux/ppdev.h
> @@ -74,8 +74,18 @@ struct ppdev_frob_struct {
>  #define PPSETPHASE     _IOW(PP_IOCTL, 0x94, int)
>  
>  /* Set and get port timeout (struct timeval's) */
> -#define PPGETTIME      _IOR(PP_IOCTL, 0x95, struct timeval)
> -#define PPSETTIME      _IOW(PP_IOCTL, 0x96, struct timeval)
> +/* Force application use 64 time_t ioctl */
> +/* TODO: It is an open question about we should use a __xxx_timeval or an
> + * implicit array.
> + * replace struct __kernel_timeval with __s32[4]
> + * replace struct compat_timeval with __s32[2]
> + */
> +#define PPGETTIME      PPGETTIME64
> +#define PPSETTIME      PPSETTIME64
> +#define PPGETTIME64    _IOR(PP_IOCTL, 0x95, struct __kernel_timeval)
> +#define PPSETTIME64    _IOW(PP_IOCTL, 0x96, struct __kernel_timeval)
> +#define PPGETTIME32    _IOR(PP_IOCTL, 0x9c, struct __kernel_compat_timeval)
> +#define PPSETTIME32    _IOW(PP_IOCTL, 0x9d, struct __kernel_compat_timeval)

As commented before, these definitions should probably not be part of the
user-visible header file.

The main reason for using an __s64[2] array instead of struct __kernel_timeval
is to avoid adding __kernel_timeval: 'timeval' is thoroughly deprecated
and we don't want to establish new interfaces with that.

In case of this driver, nobody would ever want to change their user
space to use a 64-bit __kernel_timeval instead of timeval and explicitly
call PPGETTIME64 instead of PPGETTIME, because we are only dealing with
an interval here, and a 32-bit second value is sufficient to represent
that. Instead, the purpose of your patch is to make the kernel cope with
user space that happens to use a 64-bit time_t based definition of 
'struct timeval' and passes that to the ioctl.

	Arnd

Re: [RFC PATCH v2 4/4] y2038: convert ppdev to 2038 safe

[Arnd Bergmann]

On Monday 29 June 2015 22:23:27 Bamvor Zhang Jian wrote:
> diff --git a/include/uapi/linux/ppdev.h b/include/uapi/linux/ppdev.h
> index dc18c5d..d62a47d 100644
> --- a/include/uapi/linux/ppdev.h
> +++ b/include/uapi/linux/ppdev.h
> @@ -74,8 +74,18 @@ struct ppdev_frob_struct {
>  #define PPSETPHASE     _IOW(PP_IOCTL, 0x94, int)
>  
>  /* Set and get port timeout (struct timeval's) */
> -#define PPGETTIME      _IOR(PP_IOCTL, 0x95, struct timeval)
> -#define PPSETTIME      _IOW(PP_IOCTL, 0x96, struct timeval)
> +/* Force application use 64 time_t ioctl */
> +/* TODO: It is an open question about we should use a __xxx_timeval or an
> + * implicit array.
> + * replace struct __kernel_timeval with __s32[4]
> + * replace struct compat_timeval with __s32[2]
> + */
> +#define PPGETTIME      PPGETTIME64
> +#define PPSETTIME      PPSETTIME64
> +#define PPGETTIME64    _IOR(PP_IOCTL, 0x95, struct __kernel_timeval)
> +#define PPSETTIME64    _IOW(PP_IOCTL, 0x96, struct __kernel_timeval)
> +#define PPGETTIME32    _IOR(PP_IOCTL, 0x9c, struct __kernel_compat_timeval)
> +#define PPSETTIME32    _IOW(PP_IOCTL, 0x9d, struct __kernel_compat_timeval)

As commented before, these definitions should probably not be part of the
user-visible header file.

The main reason for using an __s64[2] array instead of struct __kernel_timeval
is to avoid adding __kernel_timeval: 'timeval' is thoroughly deprecated
and we don't want to establish new interfaces with that.

In case of this driver, nobody would ever want to change their user
space to use a 64-bit __kernel_timeval instead of timeval and explicitly
call PPGETTIME64 instead of PPGETTIME, because we are only dealing with
an interval here, and a 32-bit second value is sufficient to represent
that. Instead, the purpose of your patch is to make the kernel cope with
user space that happens to use a 64-bit time_t based definition of 
'struct timeval' and passes that to the ioctl.

	Arnd

[re-sent with fixed y2038 list]