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

[Deepa Dinamani]

On Thu, Oct 22, 2015 at 1:45 PM, Arnd Bergmann  wrote:

> On Thursday 22 October 2015 09:43:25 Deepa Dinamani wrote:
> > On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann  wrote:
> > > On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
> > > > On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann 
> 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 */
> > > __u32test_num;   /* 0..(TEST_CASES-1) */
> > > __u32iterations;
> > > ...
> > > __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

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

[Arnd Bergmann]

On Thursday 22 October 2015 09:43:25 Deepa Dinamani wrote:
> On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann  wrote:
> > On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
> > > On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann  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 */
> > __u32test_num;   /* 0..(TEST_CASES-1) */
> > __u32iterations;
> > ...
> > __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.
 
> 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.
 
> 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.

> 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?
 
> 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

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

[Deepa Dinamani]

On Thu, Oct 22, 2015 at 2:36 AM, Arnd Bergmann  wrote:

> On Wednesday 21 October 2015 18:03:02 Deepa Dinamani wrote:
> > On Wed, Oct 21, 2015 at 4:17 PM, Arnd Bergmann  wrote:
> > > On Wednesday 21 October 2015 15:17:14 Deepa Dinamani wrote:
> > > > Rename old ioctl and data types to COMPAT_USBTEST_REQUEST and
> > > > usbtest_param_compat, respectively.
> > > >
> > > > The change also uses monotonic time instead of real time for both
> ioctls.
> > > > This ensures that the time duration is always positive.  Also use
> ktime
> > > > api's for time routines accomodating moving all of kernel to use 64
> bit
> > > > time eventually.
> > >
> > > Changing to monotonic time is good here. Using ktime_t is often
> > > a good idea, but I think in this case it causes more problems
> > > because we end up having to convert it back into timeval.
> > >
> >
> > If we are keeping timeval in the interface, then I agree we don't need
> > ktime_t.
> > It was my understanding from the y2038 summary that you were recommending
> > eliminating timeval.
>
> It's a little more complicated: We try to avoid breaking compilation of
> user space programs if at all possible, so if they use timeval today,
> it's better not to require an API change as far as user space is
> concerned.
>
> On the other hand, we know that we have to do significant changes to
> glibc, and any interfaces that are between the kernel and libc (i.e.
> most system calls) should stop passing timeval. Instead the libc can
> provide its own helper functions and e.g. implement gettimeofday()
> by calling clock_gettime() and then converting the result.
> > > The most important point to notice here is that the definition is in a
> .c
> > > file, where user space cannot access it. This means that any program
> > > calling it already has a copy of this definition, and changing the
> > > definition here will not help you the way it would if the definition
> > > was in include/uapi/linux/*.h
> > >
> >
> > This was going to be in a follow on patch.
> > But, I was not sure if it should be done as part of outreachy.
> > The driver in the current state is self contained in this .c file.
> > All definitions should really be moved to a uapi header file as the
> > original fixme comment indicates.
> >
> > But the only current user program that seems to be using it is in the
> > kernel tools.
> > The structures and the ioctl are indeed redefined there.
> >
> > Also, the previous struct and IOCTL definitions have not changed because
> of
> > the above reason.
> > I kept them intact for compatibility with existing binaries.
>
> 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 */
> __u32test_num;   /* 0..(TEST_CASES-1) */
> __u32iterations;
> ...
> __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.

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;
};

And, a similar struct for timeval_64.

This would also mean adding api's to fill the structures defined above.
Basically an entire layer.

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.

Since the original implementation is broken already, my first preference
was to fix the interface with the new interface itself.
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