On 15 Jun 03:33 "Arnd Bergmann" <[email protected]> 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 <[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.
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
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
