On Thu, Oct 22, 2015 at 1:45 PM, Arnd Bergmann <a...@arndb.de> wrote:
> On Thursday 22 October 2015 09:43:25 Deepa Dinamani wrote:
> > On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann <a...@arndb.de> wrote:
> > > On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
> > > > On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann <a...@arndb.de>
> wrote:
> > > Yes, and that is the right idea. I suspect we will have to provide
> > > different definitions for kernel and user space though, in one
> > > form or another. You are right that we want to remove 'timeval'
> > > from the kernel, and in order to keep user space unchanged, this
> > > means defining the structure like
> > >
> > > struct usbtest_param_32 {
> > > /* inputs */
> > > __u32 test_num; /* 0..(TEST_CASES-1) */
> > > __u32 iterations;
> > > ...
> > > __s32 duration_sec;
> > > __s32 duration_usec;
> > > };
> > >
> > > which is a well-defined binary version of what 32-bit user space
> > > sees today. We also need the 64-bit version of that, for both
> > > normal 64-bit tasks and future 32-bit tasks that are built with
> > > the old structure definition.
> > >
> > > Optionally, we can introduce a better interface with a new command
> > > number as your patch does, but that should be a separate patch,
> > > and we have to see if the USB maintainers want to do this or not.
> > >
> >
> > There are two problems with the original ioctl interface:
> >
> > 1. Because of the use of timeval, compatibility is broken between 32 bit
> > and 64 bit binaries.
> >
> > This has nothing to do with y2038 problem at all.
> > This is the case with all interfaces using timeval itself and has nothing
> > to do with this one
> > particular bad interface design.
>
> Right.
>
> > The struct you suggested above will work to map to two separate ioctls.
> > But, if this is a generic problem, shouldn't the above solution be in
> some
> > common file?
> > For instance we could have this:
> >
> > struct timeval_32 {
> > __s32 duration_sec;
> > __s32 duration_usec;
> > };
>
> This is essentially 'struct compat_timeval', which I intend to keep
> around in the kernel for backwards compatibility and will make available
> to 32-bit kernels that currently can't use it. The patches still need
> a few more review rounds though.
>
> > And, a similar struct for timeval_64.
> >
> > This would also mean adding api's to fill the structures defined above.
> > Basically an entire layer.
>
> We should really see how many drivers need this. I have shown that the
> core kernel does not need it with my patches, as all system calls that
> use timeval are already deprecated. I have a list of drivers that need
> to be converted at
>
>
> https://docs.google.com/spreadsheets/d/1HCYwHXxs48TsTb6IGUduNjQnmfRvMPzCN6T_0YiQwis/edit#gid=346406462
>
> on the second sheet. All the ones that have an ABI exposed to user space
> are marked 'uapi' or 'ioctl', and we could check which ones of these
> would be helped by having a generic set of helpers for timeval_64.
>
Ok. I can do this. I can get back to you early next week.
>
> > This is not necessary for this driver as the struct's are not exposed.
> > My guess is also that there aren't many applications using this because
> of
> > the way it needs redeclaring everything in the application.
>
> Agreed. My guess is that there is only one application using it at all,
> but it's hard to know for sure. If we could prove that there is only one,
> and we can change it to use a new ioctl command if that is present in some
> header file, all this could be simplified.
>
Ok. I can contact the usb linux mailing list if they know of any other or
just to fish around for their opinion.
I'll keep you in the cc. Let me know if you would prefer the whole 2038/
outreachy mailing list instead.
>
> > 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
> > 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.
#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.
> > The old ioctl support to be eventually removed completely.
>
> We should only ever plan to break existing user space if there
> is no other option at all. Remember that most users don't care
> about the y2038 problem because they are on 64-bit systems that
> have a couple of bugs we need to fix in the next few years, or
> because they are on 32-bit systems that they will stop using
> before it becomes a problem. The implied rules that result from
> this are:
>
> - nothing we do should require changes for 64-bit tasks
>
> - all 32-bit tasks should keep working on future kernels (both 32-bit
> and 64-bit) unless they explicitly disable CONFIG_COMPAT_TIME
>
> - CONFIG_COMPAT_TIME and all code under it will be removed in 2038
> at the latest, but perhaps not much earlier.
>
> - if possible, source code that produces a working 32-bit binary today
> should not need modifications to work when compiled against a libc
> with 64-bit time_t and run on a kernel without CONFIG_COMPAT_TIME.
>
> > 2. The y2038 problem in the driver is quite straight forward as to
> picking
> > the right calls to fill in whatever data types we choose above.
>
> I would argue that it's exactly the same problem for 32-bit and 64-bit
> kernels in this driver: you can have user space binaries with different
> definitions of timeval and we want both of them to work.
>
> > > > USB framework's higher level ioctl framework already supports a
> > > > .compat_ioctl and pointers are fixed before coming into this usbtest
> > > driver.
> > > > Are you suggesting .compat_ioctl extension to the usb_driver, or
> > > rewriting
> > > > the driver as a cdev?
> > >
> > > Once the handler is changed to handle both versions of the structure,
> > > we don't need a separate .compat_ioctl any more, we just need to make
> > > sure that the handler gets called for both. I haven't checked this but
> > > according to your description I expect that is what happens already/
> > >
> > > > The other ioctls take care of individual compat data types by
> internally
> > > > using #ifdef CONFIG_COMPAT.
> > >
> > > 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.
Summarizing the next steps:
1. Figure out how many ioctl or uapi need 64-bit timeval.
2. Should compat fix for existing usbtest ioctl be based on top of your
COMPAT_TIME changes?
3. Contact usb list about usbtest ioctl uses and about proposed new ioctl.
>
> Arnd
>
- Deepa
_______________________________________________
Y2038 mailing list
Y2038@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/y2038
