The y2038 issue of printer exist in the time_t of timeval in ioctl LPSETTIME. This patch try to convert it to y2038 safe by the following steps: 1. Remove timeval from lp_set_timeout in order to support 32bit and 64bit time_t in the same function without the new definition of timeval64 or something else. 2. Handle both 32bit and 64bit time in the same LPSETTIMEOUT switch case in order to support y2038 safe and non-safe cases. 3. Merge compat of LPSETTIMEOUT into non-comapt one.
I thought split these steps into three different patches. But I feel these changes are simple and direct.
Signed-off-by: Bamvor Jian Zhang bamvor.zhangjian@linaro.org --- drivers/char/lp.c | 94 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 52 insertions(+), 42 deletions(-)
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index c4094c4..a207e0c 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -136,6 +136,14 @@ #include <asm/irq.h> #include <asm/uaccess.h>
+/* + * FIXME: It should be removed after COMPAT_USE_64BIT_TIME is accessible for + * 32bit architecture. + */ +#ifndef COMPAT_USE_64BIT_TIME +#define COMPAT_USE_64BIT_TIME (0) +#endif /* COMPAT_USE_64BIT_TIME */ + /* if you have more than 8 printers, remember to increase LP_NO */ #define LP_NO 8
@@ -572,6 +580,22 @@ static int lp_release(struct inode * inode, struct file * file) return 0; }
+static int lp_set_timeout(unsigned int minor, s64 tv_sec, s64 tv_usec) +{ + long to_jiffies; + + if ((tv_sec < 0) || (tv_usec < 0)) + return -EINVAL; + + to_jiffies = usecs_to_jiffies(tv_usec); + to_jiffies += tv_sec * (long)HZ; + if (to_jiffies <= 0) + return -EINVAL; + + lp_table[minor].timeout = to_jiffies; + return 0; +} + static int lp_do_ioctl(unsigned int minor, unsigned int cmd, unsigned long arg, void __user *argp) { @@ -586,6 +610,9 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd, if ((LP_F(minor) & LP_EXIST) == 0) return -ENODEV; switch ( cmd ) { + s32 time32[2]; + s64 time64[2]; + case LPTIME: if (arg > UINT_MAX / HZ) return -EINVAL; @@ -647,58 +674,49 @@ static int lp_do_ioctl(unsigned int minor, unsigned int cmd, sizeof(struct lp_stats)); break; #endif - case LPGETFLAGS: - status = LP_F(minor); + case LPGETFLAGS: + status = LP_F(minor); if (copy_to_user(argp, &status, sizeof(int))) return -EFAULT; break;
+ case LPSETTIMEOUT: + /* + * For 64bit application or 32bit application with 64bit + * time_t + */ + if ((IS_ENABLED(CONFIG_64BIT) && !is_compat_task()) + || COMPAT_USE_64BIT_TIME) { + if (copy_from_user(time64, argp, + sizeof(time64))) + return -EFAULT; + + return lp_set_timeout(minor, time64[0], + time64[1]); + } else { + if (copy_from_user(time32, argp, + sizeof(time32))) + return -EFAULT; + + return lp_set_timeout(minor, time32[0], + time32[1]); + } + break; default: retval = -EINVAL; } return retval; }
-static int lp_set_timeout(unsigned int minor, struct timeval *par_timeout) -{ - long to_jiffies; - - /* Convert to jiffies, place in lp_table */ - if ((par_timeout->tv_sec < 0) || - (par_timeout->tv_usec < 0)) { - return -EINVAL; - } - to_jiffies = DIV_ROUND_UP(par_timeout->tv_usec, 1000000/HZ); - to_jiffies += par_timeout->tv_sec * (long) HZ; - if (to_jiffies <= 0) { - return -EINVAL; - } - lp_table[minor].timeout = to_jiffies; - return 0; -} - static long lp_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { unsigned int minor; - struct timeval par_timeout; int ret;
minor = iminor(file_inode(file)); mutex_lock(&lp_mutex); - switch (cmd) { - case LPSETTIMEOUT: - if (copy_from_user(&par_timeout, (void __user *)arg, - sizeof (struct timeval))) { - ret = -EFAULT; - break; - } - ret = lp_set_timeout(minor, &par_timeout); - break; - default: - ret = lp_do_ioctl(minor, cmd, arg, (void __user *)arg); - break; - } + ret = lp_do_ioctl(minor, cmd, arg, (void __user *)arg); mutex_unlock(&lp_mutex);
return ret; @@ -709,19 +727,11 @@ static long lp_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { unsigned int minor; - struct timeval par_timeout; int ret;
minor = iminor(file_inode(file)); mutex_lock(&lp_mutex); switch (cmd) { - case LPSETTIMEOUT: - if (compat_get_timeval(&par_timeout, compat_ptr(arg))) { - ret = -EFAULT; - break; - } - ret = lp_set_timeout(minor, &par_timeout); - break; #ifdef LP_STATS case LPGETSTATS: /* FIXME: add an implementation if you set LP_STATS */
On Friday 04 December 2015 23:11:58 Bamvor Jian Zhang wrote:
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index c4094c4..a207e0c 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -136,6 +136,14 @@ #include <asm/irq.h> #include <asm/uaccess.h> +/*
- FIXME: It should be removed after COMPAT_USE_64BIT_TIME is accessible for
- 32bit architecture.
- */
+#ifndef COMPAT_USE_64BIT_TIME +#define COMPAT_USE_64BIT_TIME (0) +#endif /* COMPAT_USE_64BIT_TIME */
Unfortunately, I think this can't work: the kernel has no idea whether user space uses a 32-bit or 64-bit time_t, and we need to be able to support both kinds of user space at the same time.
What we have to do instead is change the definition of LPSETTIMEOUT, like
#if __BITS_PER_SIZE_T >= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
We need to decide what the correct #if condition should be, but the idea is that all current user space remains unchanged, while user space built with a 64-bit time_t sees the modified definition of LPSETTIMEOUT and passes that into the kernel when using the larger structure.
/* if you have more than 8 printers, remember to increase LP_NO */ #define LP_NO 8 @@ -572,6 +580,22 @@ static int lp_release(struct inode * inode, struct file * file) return 0; } +static int lp_set_timeout(unsigned int minor, s64 tv_sec, s64 tv_usec)
tv_usec does not need to be 64-bit here, it has to be below 1 million.
+{
- long to_jiffies;
- if ((tv_sec < 0) || (tv_usec < 0))
return -EINVAL;
- to_jiffies = usecs_to_jiffies(tv_usec);
- to_jiffies += tv_sec * (long)HZ;
- if (to_jiffies <= 0)
return -EINVAL;
- lp_table[minor].timeout = to_jiffies;
- return 0;
+}
I think tv_sec can also be shorter, as we ignore the upper bits after the multiplication with HZ.
Arnd
Hi, Arnd
On 12/05/2015 12:44 AM, Arnd Bergmann wrote:
On Friday 04 December 2015 23:11:58 Bamvor Jian Zhang wrote:
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index c4094c4..a207e0c 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -136,6 +136,14 @@ #include <asm/irq.h> #include <asm/uaccess.h> +/*
- FIXME: It should be removed after COMPAT_USE_64BIT_TIME is accessible for
- 32bit architecture.
- */
+#ifndef COMPAT_USE_64BIT_TIME +#define COMPAT_USE_64BIT_TIME (0) +#endif /* COMPAT_USE_64BIT_TIME */
Unfortunately, I think this can't work: the kernel has no idea whether user space uses a 32-bit or 64-bit time_t, and we need to be able to support both kinds of user space at the same time.
What we have to do instead is change the definition of LPSETTIMEOUT, like
#if __BITS_PER_SIZE_T >= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
We need to decide what the correct #if condition should be, but the idea is that all current user space remains unchanged, while user space built with a 64-bit time_t sees the modified definition of LPSETTIMEOUT and passes that into the kernel when using the larger structure.
/* if you have more than 8 printers, remember to increase LP_NO */ #define LP_NO 8 @@ -572,6 +580,22 @@ static int lp_release(struct inode * inode, struct file * file) return 0; } +static int lp_set_timeout(unsigned int minor, s64 tv_sec, s64 tv_usec)
tv_usec does not need to be 64-bit here, it has to be below 1 million.
Oh, yes. I will update in my next version.
+{
- long to_jiffies;
- if ((tv_sec < 0) || (tv_usec < 0))
return -EINVAL;
- to_jiffies = usecs_to_jiffies(tv_usec);
- to_jiffies += tv_sec * (long)HZ;
- if (to_jiffies <= 0)
return -EINVAL;
- lp_table[minor].timeout = to_jiffies;
- return 0;
+}
I think tv_sec can also be shorter, as we ignore the upper bits after the multiplication with HZ.
Given that long is 64bit in 64bit architecture, shall I keep tv_sec as s64 and cast HZ to s64 as well? + to_jiffies += tv_sec * (s64)HZ;
Regards
Bamvor
Arnd
On Saturday 05 December 2015 23:57:59 Bamvor Jian Zhang wrote:
+{
- long to_jiffies;
- if ((tv_sec < 0) || (tv_usec < 0))
return -EINVAL;
- to_jiffies = usecs_to_jiffies(tv_usec);
- to_jiffies += tv_sec * (long)HZ;
- if (to_jiffies <= 0)
return -EINVAL;
- lp_table[minor].timeout = to_jiffies;
- return 0;
+}
I think tv_sec can also be shorter, as we ignore the upper bits after the multiplication with HZ.
Given that long is 64bit in 64bit architecture, shall I keep tv_sec as s64 and cast HZ to s64 as well?
to_jiffies += tv_sec * (s64)HZ;
I would just leave tv_sec as 'long' then.
You don't need a cast for HZ at all.
Neither of them is important, since it only matters if the timeout is more than 50 days, and that is not a setting we need to worry about.
Arnd
Hi, arnd
On 12/05/2015 12:44 AM, Arnd Bergmann wrote:
On Friday 04 December 2015 23:11:58 Bamvor Jian Zhang wrote:
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index c4094c4..a207e0c 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -136,6 +136,14 @@ #include <asm/irq.h> #include <asm/uaccess.h>
+/*
- FIXME: It should be removed after COMPAT_USE_64BIT_TIME is accessible for
- 32bit architecture.
- */
+#ifndef COMPAT_USE_64BIT_TIME +#define COMPAT_USE_64BIT_TIME (0) +#endif /* COMPAT_USE_64BIT_TIME */
Unfortunately, I think this can't work: the kernel has no idea whether user space uses a 32-bit or 64-bit time_t, and we need to be able to support both kinds of user space at the same time.
What we have to do instead is change the definition of LPSETTIMEOUT, like
#if __BITS_PER_SIZE_T >= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
We need to decide what the correct #if condition should be, but the idea is that all current user space remains unchanged, while user space built with a 64-bit time_t sees the modified definition of LPSETTIMEOUT and passes that into the kernel when using the larger structure.
I am trying to follow your suggestion. But I do not understand why `__BITS_PER_SIZE_T` is relative to sizeof(time_t). And I also tried to use `__BITS_PER_TIME_T` according to the following table. /* * userspace | kernel | `__BITS_PER_TIME_T` | `__BITS_PER_LONG` | `__BITS_PER_TIME_T` <= `__BITS_PER_LONG` * -----------|----------|---------------------|-------------------|------------------------------------------ * 32 | 32 | 32 | 32 | true * 32 | 32 | 64 | 32 | false * 32 | 64 | 32 | 32 | true * 32 | 64 | 64 | 32 | false * 64 | 64 | 64 | 64 | true */ #if __BITS_PER_TIME_T <= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
As you know, there is no `__BITS_PER_TIME_T` as well. Finally, I thought I could make use of `__USE_TIME_BITS64` which mentioned in https://sourceware.org/glibc/wiki/Y2038ProofnessDesign
How about something like this?
#if defined(__USE_TIME_BITS64) && __BITS_PER_LONG == 32 #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #else #define LPSETTIMEOUT 0x060f /* set parport timeout */ #endif
Regards
Bamvor
/* if you have more than 8 printers, remember to increase LP_NO */ #define LP_NO 8
@@ -572,6 +580,22 @@ static int lp_release(struct inode * inode, struct file * file) return 0; }
+static int lp_set_timeout(unsigned int minor, s64 tv_sec, s64 tv_usec)
tv_usec does not need to be 64-bit here, it has to be below 1 million.
+{
- long to_jiffies;
- if ((tv_sec < 0) || (tv_usec < 0))
- return -EINVAL;
- to_jiffies = usecs_to_jiffies(tv_usec);
- to_jiffies += tv_sec * (long)HZ;
- if (to_jiffies <= 0)
- return -EINVAL;
- lp_table[minor].timeout = to_jiffies;
- return 0;
+}
I think tv_sec can also be shorter, as we ignore the upper bits after the multiplication with HZ.
Arnd
On Thursday 24 December 2015, Bamvor Zhang Jian wrote:
Hi, arnd
On 12/05/2015 12:44 AM, Arnd Bergmann wrote:
On Friday 04 December 2015 23:11:58 Bamvor Jian Zhang wrote:
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index c4094c4..a207e0c 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -136,6 +136,14 @@ #include <asm/irq.h> #include <asm/uaccess.h>
+/*
- FIXME: It should be removed after COMPAT_USE_64BIT_TIME is accessible for
- 32bit architecture.
- */
+#ifndef COMPAT_USE_64BIT_TIME +#define COMPAT_USE_64BIT_TIME (0) +#endif /* COMPAT_USE_64BIT_TIME */
Unfortunately, I think this can't work: the kernel has no idea whether user space uses a 32-bit or 64-bit time_t, and we need to be able to support both kinds of user space at the same time.
What we have to do instead is change the definition of LPSETTIMEOUT, like
#if __BITS_PER_SIZE_T >= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
We need to decide what the correct #if condition should be, but the idea is that all current user space remains unchanged, while user space built with a 64-bit time_t sees the modified definition of LPSETTIMEOUT and passes that into the kernel when using the larger structure.
I am trying to follow your suggestion. But I do not understand why `__BITS_PER_SIZE_T` is relative to sizeof(time_t). And I also tried to use `__BITS_PER_TIME_T` according to the following table.
Sorry for the confusion, I meant to write __BITS_PER_TIME_T, not __BITS_PER_SIZE_T.
- userspace | kernel | `__BITS_PER_TIME_T` | `__BITS_PER_LONG` |
`__BITS_PER_TIME_T` <= `__BITS_PER_LONG`
- -----------|----------|---------------------|-------------------|------------------------------------------
32 | 32 | 32 | 32 | true
32 | 32 | 64 | 32 | false
32 | 64 | 32 | 32 | true
32 | 64 | 64 | 32 | false
64 | 64 | 64 | 64 | true
*/ #if __BITS_PER_TIME_T <= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
As you know, there is no `__BITS_PER_TIME_T` as well. Finally, I thought I could make use of `__USE_TIME_BITS64` which mentioned in https://sourceware.org/glibc/wiki/Y2038ProofnessDesign
How about something like this?
#if defined(__USE_TIME_BITS64) && __BITS_PER_LONG == 32 #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #else #define LPSETTIMEOUT 0x060f /* set parport timeout */ #endif
Yes, this is what I meant. Unfortunately, we have not agreed on what we are going to call that macro, but this is roughly how it should work. However, the definition you have here is only correct for user space, not for the kernel itself, which needs a slightly different definition when __KERNEL__ is defined and 'struct timeval' has been removed from the kernel.
Arnd
Hi, Arnd
On 12/24/2015 11:42 PM, Arnd Bergmann wrote:
On Thursday 24 December 2015, Bamvor Zhang Jian wrote:
Hi, arnd
On 12/05/2015 12:44 AM, Arnd Bergmann wrote:
On Friday 04 December 2015 23:11:58 Bamvor Jian Zhang wrote:
diff --git a/drivers/char/lp.c b/drivers/char/lp.c index c4094c4..a207e0c 100644 --- a/drivers/char/lp.c +++ b/drivers/char/lp.c @@ -136,6 +136,14 @@ #include <asm/irq.h> #include <asm/uaccess.h>
+/*
- FIXME: It should be removed after COMPAT_USE_64BIT_TIME is accessible for
- 32bit architecture.
- */
+#ifndef COMPAT_USE_64BIT_TIME +#define COMPAT_USE_64BIT_TIME (0) +#endif /* COMPAT_USE_64BIT_TIME */
Unfortunately, I think this can't work: the kernel has no idea whether user space uses a 32-bit or 64-bit time_t, and we need to be able to support both kinds of user space at the same time.
What we have to do instead is change the definition of LPSETTIMEOUT, like
#if __BITS_PER_SIZE_T >= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
We need to decide what the correct #if condition should be, but the idea is that all current user space remains unchanged, while user space built with a 64-bit time_t sees the modified definition of LPSETTIMEOUT and passes that into the kernel when using the larger structure.
I am trying to follow your suggestion. But I do not understand why `__BITS_PER_SIZE_T` is relative to sizeof(time_t). And I also tried to use `__BITS_PER_TIME_T` according to the following table.
Sorry for the confusion, I meant to write __BITS_PER_TIME_T, not __BITS_PER_SIZE_T.
- userspace | kernel | `__BITS_PER_TIME_T` | `__BITS_PER_LONG` |
`__BITS_PER_TIME_T` <= `__BITS_PER_LONG`
- -----------|----------|---------------------|-------------------|------------------------------------------
32 | 32 | 32 | 32 | true
32 | 32 | 64 | 32 | false
32 | 64 | 32 | 32 | true
32 | 64 | 64 | 32 | false
64 | 64 | 64 | 64 | true
*/ #if __BITS_PER_TIME_T <= __BITS_PER_LONG #define LPSETTIMEOUT 0x060f /* set parport timeout */ #else #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #endif
As you know, there is no `__BITS_PER_TIME_T` as well. Finally, I thought I could make use of `__USE_TIME_BITS64` which mentioned in https://sourceware.org/glibc/wiki/Y2038ProofnessDesign
How about something like this?
#if defined(__USE_TIME_BITS64) && __BITS_PER_LONG == 32 #define LPSETTIMEOUT _IOW(0x06, 0x0f, struct timeval) #else #define LPSETTIMEOUT 0x060f /* set parport timeout */ #endif
Yes, this is what I meant. Unfortunately, we have not agreed on what we are going to call that macro, but this is roughly how it should work.
Got you, could I use __USE_TIME_BITS64 in my patch right now? Or should I need to wait for this agreement?
However, the definition you have here is only correct for user space, not for the kernel itself, which needs a slightly different definition when __KERNEL__ is defined and 'struct timeval' has been removed from the kernel.
Yeap, I am thinking it is header for uapi. I will define LPSETTIMEOUT64 in driver/char/lp.c: #define LPSETTIMEOUT64 _IOW(0x06, 0x0f, s64[2])
Is it make sense?
Regards
Bamvor
Arnd