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 <[email protected]>
> ---
> 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
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
