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