On Thu, Oct 22, 2015 at 1:45 PM, Arnd Bergmann arnd@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 arnd@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 arnd@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:
- 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_...
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.
- 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