Re: [Y2038] [PATCH] usb: usbtest: Add new ioctl interface

[Arnd Bergmann <arnd@arndb.de>]

On Friday 23 October 2015 14:54:58 Deepa Dinamani wrote:
> >
> > > Since the original implementation is broken already, my first preference
> > > was to fix the interface with the new interface itself.
> >
> > I wouldn't call the original implementation broken, except for the
> > 64-bit compat problem. What makes it broken is that the ioctl data
> > structure changes in user space when that changes the defintion of
> > 'struct timeval', but since the data returned here is a difference
> > of two times, the current 32-bit tv_sec variable is actually good
> > enough.
> >
> >
> I don't think seconds granularity is sufficient here for usb operations.
> This page shows some tests which run for less than a second:
> http://blackfin.uclinux.org/doku.php?id=linux-kernel:usb-gadget:zero

That's not what I meant here. I mean using 32-bit tv_sec plus 32-bit
tv_usec is always sufficient, because no test can run for more than
68 years and overflow the 32 bit number.

> > > My intention was to not fix the broken interface, but to replace or
> > provide
> > > a new interface.
> > >
> > > Wouldn't the following be better?
> > >
> > > #ifdef CONFIG_COMPAT
> > >           old ioctl(code = 100)
> > >            use old timeval struct
> > >           #if CONFIG_COMPAT_TIME
> > >                  use compat_timeval instead of timeval
> > > #else
> > >           new ioctl(code = 101)
> > > #endif
> >
> > Sorry, I'm not really following here. Doesn't this break existing
> > 32-bit users that don't even care about the y2038 issue?
> >
> 
> Maybe I misunderstood what CONFIG_COMPAT_TIME was doing.
> What I meant was that we should keep the old interface only for older
> binaries.
> All new code should only use new interface.
> That was the eventual goal.

What I want COMPAT_TIME to mean eventually is to support old
binaries that are known to be broken in 2038. This one is not
does not suffer from the overflow, so we can leave it enabled
unconditionally.

> #ifdef CONFIG_COMPAT_TIME
>            old ioctl(code = 100)
>             support 64 bit and 32 bit timeval
> #endif
>          new ioctl(code = 101)
> 
> By the time all support for 32-bit time is removed, use of the old
> interface should be removed.
>
> Timeval was a bad struct to expose to userspace anyway.
> 
> In this regard, I was going to fix up the usbtest tool to use new interface
> also.

That would work too, and the USB maintainers might like this method.
My preference would still be to keep the existing interface as the
primary way and not force a change on the users, no matter how bad
the original decision was.

From looking at http://www.linux-usb.org/usbtest/, my impression
is that this tool was never packaged properly for distributions
and instead a simple .c file is offered for direct download.
This means we probably have lots copies of this file in random
places that are being used but never updated, even if we can change
the master copy to use another interface. The existing file will
still compile on future libc versions, but will fail to work correctly
for now apparent reason if we deprecate that ioctl command.

> > > > Ok. In my system call patch series, I introduce a new
> > CONFIG_COMPAT_TIME
> > > > symbol that specifically deals with compatibility mode for 32-bit
> > time_t
> > > > on both 32-bit and 64-bit architectures. This is the #ifdef we should
> > > > be using here as well in principle. My patches however are not merged
> > > > yet, but there is no harm in leaving that code active here, as both
> > > > versions
> > > > provide a correct API and do not overflow in 2038.
> > > >
> > > >
> > > Does it help to review a patch of this kind with a design document?
> > > Does the kernel review system allow something like this?
> > > Some of the questions that popped up are the ones I asked myself before.
> > > It might let the thought process be more evident.
> >
> > Most kernel developers work better with patches than with design documents.
> >
> 
> I was saying patches only communicate the final product.
> For instance, I did consider using .compat_ioctl, timespec64 etc.
> I cannot really tell the story as to why this is the best way to do it
> unless someone asks.

You can have a really long changelog text to describe this if you want.

> Summarizing the next steps:
> 
> 1. Figure out how many ioctl or uapi need 64-bit timeval.

Sounds good.

> 2. Should compat fix for existing usbtest ioctl be based on top of your
> COMPAT_TIME changes?

I would prefer to see a fix for the driver that does not depend on my
patches. Just do one that handles both variants of the data structure
on both 32-bit and 64-bit architectures, without any #ifdef.

> 3. Contact usb list about usbtest ioctl uses and about proposed new ioctl.

Yes, but don't make this one a priority. Adding a new ioctl is just the
icing on the cake, and that patch mainly serves to highlight the fact
that the original API was implemented poorly and how it should have
been done instead. It's more important for us to avoid making things
worse with the 64-bit time_t change.

	Arnd