Firstly enable ioctl in ppdev and then Keep par_timeout as timeval in compat ioctl in order to use the 64bit time type.
Signed-off-by: Bamvor Zhang Jian bamvor.zhangjian@linaro.org --- This is my first time to try to upstream some code in kernel. Any commit and feedback is welcome.
drivers/char/ppdev.c | 30 +++++++++++++++++++++++++----- include/uapi/linux/ppdev.h | 20 ++++++++++++++++++-- 2 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c index ae0b42b..9e3c101 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" @@ -592,9 +593,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; +#ifdef PPSETTIME64 + case PPSETTIME64: +#endif /* PPSETTIME64 */ + case PPSETTIME32: + if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task()) { + if (compat_get_timeval(&par_timeout, compat_ptr(arg))) + return -EFAULT; + } else { + if (copy_from_user(&par_timeout, argp, + sizeof(par_timeout))) + return -EFAULT; } /* Convert to jiffies, place in pp->pdev->timeout */ if ((par_timeout.tv_sec < 0) || (par_timeout.tv_usec < 0)) { @@ -608,13 +617,23 @@ static int pp_do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) pp->pdev->timeout = to_jiffies; return 0;
- case PPGETTIME: +#ifdef PPGETTIME64 + case PPGETTIME64: +#endif /* PPGETTIME64 */ + 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_ENABLED(CONFIG_64BIT) || is_compat_task()) { + if (compat_put_timeval(&par_timeout, compat_ptr(arg))) + return -EFAULT; + } else { + if (copy_to_user(argp, &par_timeout, + sizeof(par_timeout))) + return -EFAULT; + } return 0;
default: @@ -744,6 +763,7 @@ static const struct file_operations pp_fops = { .write = pp_write, .poll = pp_poll, .unlocked_ioctl = pp_ioctl, + .compat_ioctl = pp_ioctl, .open = pp_open, .release = pp_release, }; diff --git a/include/uapi/linux/ppdev.h b/include/uapi/linux/ppdev.h index dc18c5d..f4c8fac 100644 --- a/include/uapi/linux/ppdev.h +++ b/include/uapi/linux/ppdev.h @@ -74,8 +74,24 @@ 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) +#ifdef CONFIG_64BIT +#define PPGETTIME PPGETTIME64 +#define PPSETTIME PPSETTIME64 +#define PPGETTIME64 _IOR(PP_IOCTL, 0x95, struct timeval) +#define PPSETTIME64 _IOW(PP_IOCTL, 0x96, struct timeval) +#define PPGETTIME32 _IOR(PP_IOCTL, 0x9c, struct compat_timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x9d, struct compat_timeval) +#elif defined(CONFIG_COMPAT_TIME) +#define PPGETTIME PPGETTIME32 +#define PPSETTIME PPSETTIME32 +#define PPGETTIME32 _IOR(PP_IOCTL, 0x95, struct compat_timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x96, struct compat_timeval) +#else +#define PPGETTIME PPGETTIME32 +#define PPSETTIME PPSETTIME32 +#define PPGETTIME32 _IOR(PP_IOCTL, 0x95, struct timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x96, struct timeval) +#endif /* CONFIG_64BIT */
/* Get available modes (what the hardware can do) */ #define PPGETMODES _IOR(PP_IOCTL, 0x97, unsigned int)
On Thursday 11 June 2015 17:47:48 Bamvor Zhang Jian wrote:
Firstly enable ioctl in ppdev and then Keep par_timeout as timeval in compat ioctl in order to use the 64bit time type.
Signed-off-by: Bamvor Zhang Jian bamvor.zhangjian@linaro.org
This is my first time to try to upstream some code in kernel. Any commit and feedback is welcome.
I'm officially on parental leave now, but let me try to get you started a bit as I still have time to look into things.
First of all, you need to explain in the changelog what the specific problem in this driver is and why you picked the solution at hand. This probably requires a couple of paragraphs here. Try to think of how someone who knows the driver but does not know of how the y2038 problem affects it yet.
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c index ae0b42b..9e3c101 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" @@ -592,9 +593,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;+#ifdef PPSETTIME64
- case PPSETTIME64:
+#endif /* PPSETTIME64 */
- case PPSETTIME32:
if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task()) {if (compat_get_timeval(&par_timeout, compat_ptr(arg)))return -EFAULT;} else {if (copy_from_user(&par_timeout, argp,sizeof(par_timeout)))return -EFAULT;
The logic that you add here seems wrong to me: The structure format really should not depend on whether you have a compat task or not, but only on whether you use PPSETTIME32 or PPSETTIME64.
In particular, we want both 32-bit and 64-bit tasks to use the same structures. With your current approach, I don't see how a 32-bit task could ever pass a 64-bit time_t value here.
default: @@ -744,6 +763,7 @@ static const struct file_operations pp_fops = { .write = pp_write, .poll = pp_poll, .unlocked_ioctl = pp_ioctl,
- .compat_ioctl = pp_ioctl, .open = pp_open, .release = pp_release,
};
This should be a separate patch, because the implications of this are much wider than the rest of the patch. In that patch, explain how you verified that all ioctl commands that might get called by 32-bit tasks are handled correctly on a 64-bit kernel. If some additional commands are handled by pp_ioctl and need conversion, then add another patch to handle those.
diff --git a/include/uapi/linux/ppdev.h b/include/uapi/linux/ppdev.h index dc18c5d..f4c8fac 100644 --- a/include/uapi/linux/ppdev.h +++ b/include/uapi/linux/ppdev.h @@ -74,8 +74,24 @@ 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) +#ifdef CONFIG_64BIT +#define PPGETTIME PPGETTIME64 +#define PPSETTIME PPSETTIME64 +#define PPGETTIME64 _IOR(PP_IOCTL, 0x95, struct timeval) +#define PPSETTIME64 _IOW(PP_IOCTL, 0x96, struct timeval) +#define PPGETTIME32 _IOR(PP_IOCTL, 0x9c, struct compat_timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x9d, struct compat_timeval) +#elif defined(CONFIG_COMPAT_TIME) +#define PPGETTIME PPGETTIME32 +#define PPSETTIME PPSETTIME32 +#define PPGETTIME32 _IOR(PP_IOCTL, 0x95, struct compat_timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x96, struct compat_timeval) +#else +#define PPGETTIME PPGETTIME32 +#define PPSETTIME PPSETTIME32 +#define PPGETTIME32 _IOR(PP_IOCTL, 0x95, struct timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x96, struct timeval) +#endif /* CONFIG_64BIT */
This has multiple problems:
- header files in include/uapi/ cannot use CONFIG_* symbols because the program that sees the header is supposed to run on kernels with any configuration.
- compat_timeval is not defined in a uapi header file and is used only internally in the kernel, so you cannot refer to that.
- Introducing new command names in a uapi header is pointless because there is no user space source code that refers to them.
- CONFIG_COMPAT_TIME only exists in a patch set I made that has not been merged yet. Try to define your patch in a way that works independent of my patch set.
We should really treat the two problems as separate issues with different fixes:
a) make the driver handle all user space independent of the definition it uses for 'struct timespec', which may not match what the kernel uses internally. b) define PPGETTIME/PPSETTIME in the header file in a way that does not refer to timespec at all. We still need to come up with a strategy for how to do this across the uapi headers, and it may take a longer discussion with the libc maintainers. Most importantly, we need to come up with a rule for when to expose the new command number to user space.
Arnd
On 15 Jun 03:33 "Arnd Bergmann" arnd@arndb.de wrote：
On Thursday 11 June 2015 17:47:48 Bamvor Zhang Jian wrote:
Firstly enable ioctl in ppdev and then Keep par_timeout as timeval in compat ioctl in order to use the 64bit time type.
Signed-off-by: Bamvor Zhang Jian bamvor.zhangjian@linaro.org
This is my first time to try to upstream some code in kernel. Any commit and feedback is welcome.
I'm officially on parental leave now, but let me try to get you started a bit as I still have time to look into things.
Congratulations and thanks for patience.
First of all, you need to explain in the changelog what the specific
problem
in this driver is and why you picked the solution at hand. This probably requires a couple of paragraphs here. Try to think of how someone who knows the driver but does not know of how the y2038 problem affects it
yet. Sorry, I will add it next time.
diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c index ae0b42b..9e3c101 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" @@ -592,9 +593,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;+#ifdef PPSETTIME64
case PPSETTIME64:+#endif /* PPSETTIME64 */
case PPSETTIME32:if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task()) {if (compat_get_timeval(&par_timeout,
compat_ptr(arg)))
return -EFAULT;} else {if (copy_from_user(&par_timeout, argp,sizeof(par_timeout)))return -EFAULT; }The logic that you add here seems wrong to me: The structure format really should not depend on whether you have a compat task or not, but only on whether you use PPSETTIME32 or PPSETTIME64.
I am not sure if this code is consistent with my ideas. The idea is
PP[SG]ETTIME32 show that it is 32 bit(compat on 64bit or naive 32 bit),
but we do not know that whether it is y2038. So, I need a way to ensure
it's y2038 safe.
In particular, we want both 32-bit and 64-bit tasks to use the same structures. With your current approach, I don't see how a 32-bit task could ever pass a 64-bit time_t value here.
Yes. This is what I'm puzzled. After study your patch series, I don't found any solutions. Maybe I should read your patches again carefully,
default:@@ -744,6 +763,7 @@ static const struct file_operations pp_fops = { .write = pp_write, .poll = pp_poll, .unlocked_ioctl = pp_ioctl,
.compat_ioctl = pp_ioctl, .open = pp_open, .release = pp_release,};
This should be a separate patch, because the implications of this are much wider than the rest of the patch. In that patch, explain how you verified that all ioctl commands that might get called by 32-bit tasks are handled correctly on a 64-bit kernel. If some additional commands are handled by pp_ioctl and need conversion, then add another patch to handle those.
diff --git a/include/uapi/linux/ppdev.h b/include/uapi/linux/ppdev.h index dc18c5d..f4c8fac 100644 --- a/include/uapi/linux/ppdev.h +++ b/include/uapi/linux/ppdev.h @@ -74,8 +74,24 @@ 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) +#ifdef CONFIG_64BIT +#define PPGETTIME PPGETTIME64 +#define PPSETTIME PPSETTIME64 +#define PPGETTIME64 _IOR(PP_IOCTL, 0x95, struct timeval) +#define PPSETTIME64 _IOW(PP_IOCTL, 0x96, struct timeval) +#define PPGETTIME32 _IOR(PP_IOCTL, 0x9c, struct compat_timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x9d, struct compat_timeval) +#elif defined(CONFIG_COMPAT_TIME) +#define PPGETTIME PPGETTIME32 +#define PPSETTIME PPSETTIME32 +#define PPGETTIME32 _IOR(PP_IOCTL, 0x95, struct compat_timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x96, struct compat_timeval) +#else +#define PPGETTIME PPGETTIME32 +#define PPSETTIME PPSETTIME32 +#define PPGETTIME32 _IOR(PP_IOCTL, 0x95, struct timeval) +#define PPSETTIME32 _IOW(PP_IOCTL, 0x96, struct timeval) +#endif /* CONFIG_64BIT */
This has multiple problems:
header files in include/uapi/ cannot use CONFIG_* symbols because the program that sees the header is supposed to run on kernels with any configuration.
compat_timeval is not defined in a uapi header file and is used only internally in the kernel, so you cannot refer to that.
Introducing new command names in a uapi header is pointless because there is no user space source code that refers to them.
CONFIG_COMPAT_TIME only exists in a patch set I made that has not been merged yet. Try to define your patch in a way that works independent of my patch set.
We should really treat the two problems as separate issues with different fixes:
a) make the driver handle all user space independent of the definition it uses for 'struct timespec', which may not match what the kernel uses internally. b) define PPGETTIME/PPSETTIME in the header file in a way that does not refer to timespec at all. We still need to come up with a strategy for how to do this across the uapi headers, and it may take a longer discussion with the libc maintainers. Most importantly, we need to come up with a rule for when to expose the new command number to user space.
Arnd
On Monday 15 June 2015 22:26:51 Bamvor Zhang Jian wrote:
On 15 Jun 03:33 "Arnd Bergmann" arnd@arndb.de wrote：
case PPSETTIME64:+#endif /* PPSETTIME64 */
case PPSETTIME32:if (!IS_ENABLED(CONFIG_64BIT) || is_compat_task()) {if (compat_get_timeval(&par_timeout,compat_ptr(arg)))
return -EFAULT;} else {if (copy_from_user(&par_timeout, argp,sizeof(par_timeout)))return -EFAULT; }The logic that you add here seems wrong to me: The structure format really should not depend on whether you have a compat task or not, but only on whether you use PPSETTIME32 or PPSETTIME64.
I am not sure if this code is consistent with my ideas. The idea is
PP[SG]ETTIME32 show that it is 32 bit(compat on 64bit or naive 32 bit), but we do not know that whether it is y2038. So, I need a way to ensure it's y2038 safe.
The problem for y2038 safety in this driver is not that the 32-bit time_t is insufficient because the tv_sec member here is only used to pass a timeout value that is at most a few seconds rather than an absolute time.
Instead, we need to modify the driver so it can work with new user space that has set time_t to 64-bit and passes an updated structure layout.
In particular, we want both 32-bit and 64-bit tasks to use the same structures. With your current approach, I don't see how a 32-bit task could ever pass a 64-bit time_t value here.
Yes. This is what I'm puzzled. After study your patch series, I don't found any solutions. Maybe I should read your patches again carefully,
I think the trick is to define the structures separately for the use within the driver and for the user space interface. In the uapi header, you must keep using the definition based on 'timeval' because user existing space uses that structure with the PPSETTIME command. Within the driver, you can define two local structures that are fixed-size and then handle both in the ioctl function separately.
Arnd
-
Arnd Bergmann -
Bamvor Zhang Jian