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:
- Figure out how many ioctl or uapi need 64-bit timeval.
Sounds good.
- 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.
- 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