diff mbox series

[RFC,v2,7/7] sound: core: Avoid using timespec for struct snd_timer_tread

Message ID b920cd22144c6136abf0c82df6e2f26deea38cfc.1509612176.git.baolin.wang@linaro.org
State New
Headers show
Series Fix year 2038 issue for sound subsystem | expand

Commit Message

(Exiting) Baolin Wang Nov. 2, 2017, 11:06 a.m. UTC 
The struct snd_timer_tread will use 'timespec' type variables to record
timestamp, which is not year 2038 safe on 32bits system.

Since the struct snd_timer_tread is passed through read() rather than
ioctl(), and the read syscall has no command number that lets us pick
between the 32-bit or 64-bit version of this structure.

Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new
struct snd_timer_tread64 replacing timespec with s64 type to handle
64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to
handle 64bit time_t with 32bit alignment. That means we will set
tu->tread = TREAD_FORMAT_64BIT when user space has a 64bit time_t,
then we will copy to user with struct snd_timer_tread64. For x86_32
mode, we will set tu->tread = TREAD_FORMAT_TIME32_X86 to copy
struct compat_snd_timer_tread64_x86_32 for user. Otherwise we will
use 32bit time_t variables when copying to user.

Moreover this patch replaces timespec type with timespec64 type and
related y2038 safe APIs.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>

---
 include/uapi/sound/asound.h |   11 ++-
 sound/core/timer.c          |  173 ++++++++++++++++++++++++++++++++++---------
 sound/core/timer_compat.c   |   10 ++-
 3 files changed, 157 insertions(+), 37 deletions(-)

-- 
1.7.9.5

Comments

Arnd Bergmann Nov. 5, 2017, 1:16 p.m. UTC | #1 
On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@suse.de> wrote:
> On Thu, 02 Nov 2017 12:06:57 +0100,

> Baolin Wang wrote:

>>

>> The struct snd_timer_tread will use 'timespec' type variables to record

>> timestamp, which is not year 2038 safe on 32bits system.

>>

>> Since the struct snd_timer_tread is passed through read() rather than

>> ioctl(), and the read syscall has no command number that lets us pick

>> between the 32-bit or 64-bit version of this structure.

>>

>> Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new

>> struct snd_timer_tread64 replacing timespec with s64 type to handle

>> 64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to

>> handle 64bit time_t with 32bit alignment. That means we will set

>> tu->tread = TREAD_FORMAT_64BIT when user space has a 64bit time_t,

>> then we will copy to user with struct snd_timer_tread64. For x86_32

>> mode, we will set tu->tread = TREAD_FORMAT_TIME32_X86 to copy

>> struct compat_snd_timer_tread64_x86_32 for user. Otherwise we will

>> use 32bit time_t variables when copying to user.

>

> We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where

> user-space can tell which protocol version it understands.  If the

> protocol version is higher than some definition, we can assume it's

> 64-bit ready.  The *_USER_PVERSION is issued from alsa-lib side.

> In that way, we can extend the ABI more flexibly.  A similar trick was

> already used in PCM ABI.  (Ditto for control and rawmidi API; we

> should have the same mechanism for all relevant APIs).

>

> Moreover, once when the new protocol is used, we can use the standard

> 64bit monotonic nsecs as a timestamp, so that we don't need to care

> about 32/64bit compatibility.


I think that's fine, we can do that too, but I don't see how we get around
to doing something like Baolin's patch first. Without this, we will get
existing user space code compiling against our kernel headers using a
new glibc release that will inadvertently change the structure layout
on the read file descriptor.

The trick with redefining SNDRV_TIMER_IOCTL_TREAD in that
configuration lets the kernel know what API the user space expects
without requiring source-level changes.

If you want to for all users of SNDRV_TIMER_IOCTL_TREAD to move
to a new interface for y2038-safety, we'd have to redefined the structure
to avoid the libc-provided 'struct timespec' on 32-bit architectures, like:

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 299a822d2c4e..f93cace4cd24 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -801,7 +801,14 @@ enum {

 struct snd_timer_tread {
        int event;
+#if __BITS_PER_LONG == 32
+       struct {
+               __kernel_long_t tv_sec;
+               __kernel_long_t tv_usec;
+       };
+#else
        struct timespec tstamp;
+#endif
        unsigned int val;
 };

This has a somewhat higher risk of breaking existing code (since the type
changes), and it doesn't solve the overflow.

      Arnd
Takashi Iwai Nov. 5, 2017, 4:59 p.m. UTC | #2 
On Sun, 05 Nov 2017 14:16:28 +0100,
Arnd Bergmann wrote:
> 

> On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@suse.de> wrote:

> > On Thu, 02 Nov 2017 12:06:57 +0100,

> > Baolin Wang wrote:

> >>

> >> The struct snd_timer_tread will use 'timespec' type variables to record

> >> timestamp, which is not year 2038 safe on 32bits system.

> >>

> >> Since the struct snd_timer_tread is passed through read() rather than

> >> ioctl(), and the read syscall has no command number that lets us pick

> >> between the 32-bit or 64-bit version of this structure.

> >>

> >> Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new

> >> struct snd_timer_tread64 replacing timespec with s64 type to handle

> >> 64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to

> >> handle 64bit time_t with 32bit alignment. That means we will set

> >> tu->tread = TREAD_FORMAT_64BIT when user space has a 64bit time_t,

> >> then we will copy to user with struct snd_timer_tread64. For x86_32

> >> mode, we will set tu->tread = TREAD_FORMAT_TIME32_X86 to copy

> >> struct compat_snd_timer_tread64_x86_32 for user. Otherwise we will

> >> use 32bit time_t variables when copying to user.

> >

> > We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where

> > user-space can tell which protocol version it understands.  If the

> > protocol version is higher than some definition, we can assume it's

> > 64-bit ready.  The *_USER_PVERSION is issued from alsa-lib side.

> > In that way, we can extend the ABI more flexibly.  A similar trick was

> > already used in PCM ABI.  (Ditto for control and rawmidi API; we

> > should have the same mechanism for all relevant APIs).

> >

> > Moreover, once when the new protocol is used, we can use the standard

> > 64bit monotonic nsecs as a timestamp, so that we don't need to care

> > about 32/64bit compatibility.

> 

> I think that's fine, we can do that too, but I don't see how we get around

> to doing something like Baolin's patch first. Without this, we will get

> existing user space code compiling against our kernel headers using a

> new glibc release that will inadvertently change the structure layout

> on the read file descriptor.


But it won't work in anyway in multiple ways, e.g. this timer read
stuff and another the structs embedded in the mmappged page.  If you
do rebuild things with new glibc, it should tell kernel about the new
ABI in anyway more or less explicitly.  And if you need it, it means
that some source-code level API change would be possible.

Of course, passing which data type is another question.  Maybe 64bit
nsecs wouldn't fit all places, and timespec64 style would be still
required.  But still, the current patch looks still too unnecessarily
complex to me.  (Yeah I know that the problem is complex, but the code
can be simpler, I hope!)

> The trick with redefining SNDRV_TIMER_IOCTL_TREAD in that

> configuration lets the kernel know what API the user space expects

> without requiring source-level changes.


Right, it works for this case, but not always.
If jumping the API would give a cleaner way of implementation, I'd
prefer that over too hackeries, IMO.

> If you want to for all users of SNDRV_TIMER_IOCTL_TREAD to move

> to a new interface for y2038-safety, we'd have to redefined the structure

> to avoid the libc-provided 'struct timespec' on 32-bit architectures, like:

> 

> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h

> index 299a822d2c4e..f93cace4cd24 100644

> --- a/include/uapi/sound/asound.h

> +++ b/include/uapi/sound/asound.h

> @@ -801,7 +801,14 @@ enum {

> 

>  struct snd_timer_tread {

>         int event;

> +#if __BITS_PER_LONG == 32

> +       struct {

> +               __kernel_long_t tv_sec;

> +               __kernel_long_t tv_usec;

> +       };

> +#else

>         struct timespec tstamp;

> +#endif

>         unsigned int val;

>  };

> 

> This has a somewhat higher risk of breaking existing code (since the type

> changes), and it doesn't solve the overflow.


Hm, how to define the timestamp type is one of the biggest questions
indeed.  In general, there can't be any guarantee that just rebuilding
with the 64bit timespec would work for all existing codes.  In theory
it shouldn't break, but who knows...


thanks,

Takashi
Arnd Bergmann Nov. 6, 2017, 4:33 p.m. UTC | #3 
On Sun, Nov 5, 2017 at 5:59 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Sun, 05 Nov 2017 14:16:28 +0100,

>> On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@suse.de> wrote:

>> > On Thu, 02 Nov 2017 12:06:57 +0100,

>> > We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where

>> > user-space can tell which protocol version it understands.  If the

>> > protocol version is higher than some definition, we can assume it's

>> > 64-bit ready.  The *_USER_PVERSION is issued from alsa-lib side.

>> > In that way, we can extend the ABI more flexibly.  A similar trick was

>> > already used in PCM ABI.  (Ditto for control and rawmidi API; we

>> > should have the same mechanism for all relevant APIs).

>> >

>> > Moreover, once when the new protocol is used, we can use the standard

>> > 64bit monotonic nsecs as a timestamp, so that we don't need to care

>> > about 32/64bit compatibility.

>>

>> I think that's fine, we can do that too, but I don't see how we get around

>> to doing something like Baolin's patch first. Without this, we will get

>> existing user space code compiling against our kernel headers using a

>> new glibc release that will inadvertently change the structure layout

>> on the read file descriptor.

>

> But it won't work in anyway in multiple ways, e.g. this timer read

> stuff and another the structs embedded in the mmappged page.  If you

> do rebuild things with new glibc, it should tell kernel about the new

> ABI in anyway more or less explicitly.  And if you need it, it means

> that some source-code level API change would be possible.


Right, you mentioned the mmap interface at the kernel summit. This
is certainly the most tricky part and will probably require source-level
changes.

Can you clarify a few things about the mmap() interface?
Is this specifically about "struct snd_pcm_mmap_status" on the
pcm device, or are there others?

From what I can see, it's already fairly limited:
- on most architectures, it's completely disabled, only x86, ppc and
  alpha allow it to start with, and user space can work around
  the mmap not being available by falling back to ioctl if I read
  the comments correctly
- alpha is not affected since time_t is always 64-bit
- x86 and ppc disable the mmap() in compat mode already because
  of the same issue. If it comes to the worst, we can probably do
  the same for x86-32 and ppc32, disabling the existing status mmap
  for them as well, and change SNDRV_PCM_MMAP_OFFSET_STATUS
  to a new value for 32-bit kernels that exposes the same structure
  as 64-bit kernels.
- I think that since we always use an offset that is defined in the
  header file, we can use the same trick for mmap that we have
  for the ioctl command number:

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index c227ccba60ae..bcdbdac097d9 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t;

 enum {
        SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,
-       SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000,
+       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000,
        SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000,
+       SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000,
 };

+#if __BITS_PER_LONG == 64
+#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD
+#else
+#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) >
sizeof(__kernel_long_t)) ? \
+                                       SNDRV_PCM_MMAP_OFFSET_STATUS64 : \
+                                       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD)
+#endif
+
 union snd_pcm_sync_id {
        unsigned char id[16];
        unsigned short id16[8];

Does that make sense?

> Of course, passing which data type is another question.  Maybe 64bit

> nsecs wouldn't fit all places, and timespec64 style would be still

> required.  But still, the current patch looks still too unnecessarily

> complex to me.  (Yeah I know that the problem is complex, but the code

> can be simpler, I hope!)


I think we can simplify the x86_32 case, but probably not much beyond
that. The trick above however can fix 32-bit compat mode for mmap
if we want to do that.

>> The trick with redefining SNDRV_TIMER_IOCTL_TREAD in that

>> configuration lets the kernel know what API the user space expects

>> without requiring source-level changes.

>

> Right, it works for this case, but not always.

> If jumping the API would give a cleaner way of implementation, I'd

> prefer that over too hackeries, IMO.


Generally speaking I try to avoid being incompatible since that causes
more problems for users when they either fail to build existing source
code, or get silent interface breakage after recompiling against a new
glibc. If the kernel can make it work, that should be the first priority.

>> If you want to for all users of SNDRV_TIMER_IOCTL_TREAD to move

>> to a new interface for y2038-safety, we'd have to redefined the structure

>> to avoid the libc-provided 'struct timespec' on 32-bit architectures, like:

>>

>> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h

>> index 299a822d2c4e..f93cace4cd24 100644

>> --- a/include/uapi/sound/asound.h

>> +++ b/include/uapi/sound/asound.h

>> @@ -801,7 +801,14 @@ enum {

>>

>>  struct snd_timer_tread {

>>         int event;

>> +#if __BITS_PER_LONG == 32

>> +       struct {

>> +               __kernel_long_t tv_sec;

>> +               __kernel_long_t tv_usec;

>> +       };

>> +#else

>>         struct timespec tstamp;

>> +#endif

>>         unsigned int val;

>>  };

>>

>> This has a somewhat higher risk of breaking existing code (since the type

>> changes), and it doesn't solve the overflow.

>

> Hm, how to define the timestamp type is one of the biggest questions

> indeed.  In general, there can't be any guarantee that just rebuilding

> with the 64bit timespec would work for all existing codes.  In theory

> it shouldn't break, but who knows...


Right we can capture most cases then user space gets the kernel
headers from /usr/include/linux and that gets created from a new enough
kernel, or when the ioctl command number is defined in terms of
the variable structure sizes that actually differ. This covers almost all
interfaces, but I've seen some exceptions that will be silently broken
no matter what we do. For all I can see, ALSA ioctls are fine, it's just a lot
of work to get right. The mmap() problem might be fairly easy to solve
in the end, or it may be very hard.

      Arnd
Takashi Iwai Nov. 9, 2017, 4:52 p.m. UTC | #4 
On Mon, 06 Nov 2017 17:33:26 +0100,
Arnd Bergmann wrote:
> 

> On Sun, Nov 5, 2017 at 5:59 PM, Takashi Iwai <tiwai@suse.de> wrote:

> > On Sun, 05 Nov 2017 14:16:28 +0100,

> >> On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@suse.de> wrote:

> >> > On Thu, 02 Nov 2017 12:06:57 +0100,

> >> > We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where

> >> > user-space can tell which protocol version it understands.  If the

> >> > protocol version is higher than some definition, we can assume it's

> >> > 64-bit ready.  The *_USER_PVERSION is issued from alsa-lib side.

> >> > In that way, we can extend the ABI more flexibly.  A similar trick was

> >> > already used in PCM ABI.  (Ditto for control and rawmidi API; we

> >> > should have the same mechanism for all relevant APIs).

> >> >

> >> > Moreover, once when the new protocol is used, we can use the standard

> >> > 64bit monotonic nsecs as a timestamp, so that we don't need to care

> >> > about 32/64bit compatibility.

> >>

> >> I think that's fine, we can do that too, but I don't see how we get around

> >> to doing something like Baolin's patch first. Without this, we will get

> >> existing user space code compiling against our kernel headers using a

> >> new glibc release that will inadvertently change the structure layout

> >> on the read file descriptor.

> >

> > But it won't work in anyway in multiple ways, e.g. this timer read

> > stuff and another the structs embedded in the mmappged page.  If you

> > do rebuild things with new glibc, it should tell kernel about the new

> > ABI in anyway more or less explicitly.  And if you need it, it means

> > that some source-code level API change would be possible.

> 

> Right, you mentioned the mmap interface at the kernel summit. This

> is certainly the most tricky part and will probably require source-level

> changes.

> 

> Can you clarify a few things about the mmap() interface?

> Is this specifically about "struct snd_pcm_mmap_status" on the

> pcm device, or are there others?

> 

> From what I can see, it's already fairly limited:

> - on most architectures, it's completely disabled, only x86, ppc and

>   alpha allow it to start with, and user space can work around

>   the mmap not being available by falling back to ioctl if I read

>   the comments correctly

> - alpha is not affected since time_t is always 64-bit

> - x86 and ppc disable the mmap() in compat mode already because

>   of the same issue. If it comes to the worst, we can probably do

>   the same for x86-32 and ppc32, disabling the existing status mmap

>   for them as well, and change SNDRV_PCM_MMAP_OFFSET_STATUS

>   to a new value for 32-bit kernels that exposes the same structure

>   as 64-bit kernels.

> - I think that since we always use an offset that is defined in the

>   header file, we can use the same trick for mmap that we have

>   for the ioctl command number:

> 

> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h

> index c227ccba60ae..bcdbdac097d9 100644

> --- a/include/uapi/sound/asound.h

> +++ b/include/uapi/sound/asound.h

> @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t;

> 

>  enum {

>         SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,

> -       SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000,

> +       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000,

>         SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000,

> +       SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000,

>  };

> 

> +#if __BITS_PER_LONG == 64

> +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD

> +#else

> +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) >

> sizeof(__kernel_long_t)) ? \

> +                                       SNDRV_PCM_MMAP_OFFSET_STATUS64 : \

> +                                       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD)

> +#endif

> +

>  union snd_pcm_sync_id {

>         unsigned char id[16];

>         unsigned short id16[8];

> 

> Does that make sense?


Yeah, that should work.

But can we make the flip without the dynamic sizeof() comparison but
some ifdef?  The above doesn't allow the usage with switch(), for
example.

IOW, is there any macro indicating the 64bit user time_t?

In theory we can have the shadow mmap for the compat timespec, and
convert it always when the status gets changed.  But I guess disabling
the mmap should work simply as is, judging from the 64bit compat
status.


So, basically speaking, I find all fine with the suggested
conversions.  But, some details look fairly ugly, like the dynamic
const evaluation in the above.  If we can tidy up these devils, the
code will become more readable.


thanks,

Takashi
Arnd Bergmann Nov. 9, 2017, 5:01 p.m. UTC | #5 
On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Mon, 06 Nov 2017 17:33:26 +0100,


>> --- a/include/uapi/sound/asound.h

>> +++ b/include/uapi/sound/asound.h

>> @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t;

>>

>>  enum {

>>         SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,

>> -       SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000,

>> +       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000,

>>         SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000,

>> +       SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000,

>>  };

>>

>> +#if __BITS_PER_LONG == 64

>> +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD

>> +#else

>> +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) >

>> sizeof(__kernel_long_t)) ? \

>> +                                       SNDRV_PCM_MMAP_OFFSET_STATUS64 : \

>> +                                       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD)

>> +#endif

>> +

>>  union snd_pcm_sync_id {

>>         unsigned char id[16];

>>         unsigned short id16[8];

>>

>> Does that make sense?

>

> Yeah, that should work.

>

> But can we make the flip without the dynamic sizeof() comparison but

> some ifdef?  The above doesn't allow the usage with switch(), for

> example.

>

> IOW, is there any macro indicating the 64bit user time_t?


There is a macro defined by the C library, but so far we have not
started relying on it in kernel headers, because there is no guarantee
that this symbol is visible before sys/time.h has been included,
and there are some cases where it's possible to include a kernel
header before sys/time.h.

In case of sound/asound.h, that should be no problem since we rely
on having seen the definition on 'struct timeval' already today, and
that must come from sys/time.h. Then we just need to make sure that
all C libraries define the same macro.

Are you sure about the switch()/case problem? I thought that worked
in C99, the only problem would be using the macro outside of a
function, e.g. as initalizer for a variable

> In theory we can have the shadow mmap for the compat timespec, and

> convert it always when the status gets changed.  But I guess disabling

> the mmap should work simply as is, judging from the 64bit compat

> status.


Ok.

       Arnd
Takashi Iwai Nov. 9, 2017, 6:11 p.m. UTC | #6 
On Thu, 09 Nov 2017 18:01:47 +0100,
Arnd Bergmann wrote:
> 

> On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote:

> > On Mon, 06 Nov 2017 17:33:26 +0100,

> 

> >> --- a/include/uapi/sound/asound.h

> >> +++ b/include/uapi/sound/asound.h

> >> @@ -306,10 +306,19 @@ typedef int __bitwise snd_pcm_state_t;

> >>

> >>  enum {

> >>         SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,

> >> -       SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000,

> >> +       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000,

> >>         SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000,

> >> +       SNDRV_PCM_MMAP_OFFSET_STATUS64 = 0x82000000,

> >>  };

> >>

> >> +#if __BITS_PER_LONG == 64

> >> +#define SNDRV_PCM_MMAP_OFFSET_STATUS SNDRV_PCM_MMAP_OFFSET_STATUS_OLD

> >> +#else

> >> +#define SNDRV_PCM_MMAP_OFFSET_STATUS ((sizeof(time_t) >

> >> sizeof(__kernel_long_t)) ? \

> >> +                                       SNDRV_PCM_MMAP_OFFSET_STATUS64 : \

> >> +                                       SNDRV_PCM_MMAP_OFFSET_STATUS_OLD)

> >> +#endif

> >> +

> >>  union snd_pcm_sync_id {

> >>         unsigned char id[16];

> >>         unsigned short id16[8];

> >>

> >> Does that make sense?

> >

> > Yeah, that should work.

> >

> > But can we make the flip without the dynamic sizeof() comparison but

> > some ifdef?  The above doesn't allow the usage with switch(), for

> > example.

> >

> > IOW, is there any macro indicating the 64bit user time_t?

> 

> There is a macro defined by the C library, but so far we have not

> started relying on it in kernel headers, because there is no guarantee

> that this symbol is visible before sys/time.h has been included,

> and there are some cases where it's possible to include a kernel

> header before sys/time.h.

> 

> In case of sound/asound.h, that should be no problem since we rely

> on having seen the definition on 'struct timeval' already today, and

> that must come from sys/time.h. Then we just need to make sure that

> all C libraries define the same macro.

> 

> Are you sure about the switch()/case problem? I thought that worked

> in C99, the only problem would be using the macro outside of a

> function, e.g. as initalizer for a variable


Hmm, OK it seems working.

But, honestly speaking, it's too scaring.  I'm OK if it were only in
the kernel local code.  But it's the API/ABI definition, which is
referred by user-space...

A more solid condition would be really appreciated.


thanks,

Takashi
Arnd Bergmann Nov. 9, 2017, 11:20 p.m. UTC | #7 
On Thu, Nov 9, 2017 at 7:11 PM, Takashi Iwai <tiwai@suse.de> wrote:
> On Thu, 09 Nov 2017 18:01:47 +0100,

> Arnd Bergmann wrote:

>>

>> On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote:

>> >

>> > IOW, is there any macro indicating the 64bit user time_t?

>>

>> There is a macro defined by the C library, but so far we have not

>> started relying on it in kernel headers, because there is no guarantee

>> that this symbol is visible before sys/time.h has been included,

>> and there are some cases where it's possible to include a kernel

>> header before sys/time.h.

>>

>> In case of sound/asound.h, that should be no problem since we rely

>> on having seen the definition on 'struct timeval' already today, and

>> that must come from sys/time.h. Then we just need to make sure that

>> all C libraries define the same macro.

>>

>> Are you sure about the switch()/case problem? I thought that worked

>> in C99, the only problem would be using the macro outside of a

>> function, e.g. as initalizer for a variable

>

> Hmm, OK it seems working.

>

> But, honestly speaking, it's too scaring.  I'm OK if it were only in

> the kernel local code.  But it's the API/ABI definition, which is

> referred by user-space...

>

> A more solid condition would be really appreciated.


I understand your concern here and agree it's really ugly. It did take us
many attempts to come up with this trick for other cases, so my initial
reaction would be to use the same thing everywhere since I know
it works,  but we can use #ifdef instead if you prefer that. I think we
can use a single #ifdef variant to cover all cases, but I'd have to think
about the x32 and x86-32 some more here. With this trick, we can
make user space with new glibc use data structures that are compatible
with 64-bit kernels and avoid the additional translation helpers:

enum {
      SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,
      SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000,
#if (__BITS_PER_LONG == 64) || !defined(__USE_TIME_BITS64)
      SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000,
#else
      /* New snd_pcm_mmap_status layout on 32-bit architectures */
      SNDRV_PCM_MMAP_OFFSET_STATUS = 0x82000000,
#endif
};

struct snd_pcm_mmap_status {
        snd_pcm_state_t state;          /* RO: state - SNDRV_PCM_STATE_XXXX */
        int pad1;                                 /* Needed for 64 bit
alignment */
#if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64)
        __u64 hw_ptr;                        /* for compatibility with
64-bit layout */
#else
       snd_pcm_uframes_t hw_ptr;   /* RO: hw ptr (0...boundary-1) */
#endif
        struct timespec tstamp;          /* Timestamp */
        snd_pcm_state_t suspended_state; /* RO: suspended stream state */
#if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64)
        int pad2;
#endif
        struct timespec audio_tstamp;   /* from sample counter or wall clock */
};
#endif

struct snd_pcm_mmap_control {
#if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64)
       __u64 appl_ptr;     /* RW: appl ptr (0...boundary-1) */
        __u64 avail_min;    /* RW: min available frames for wakeup */
#else
        snd_pcm_uframes_t appl_ptr;     /* RW: appl ptr (0...boundary-1) */
        snd_pcm_uframes_t avail_min;    /* RW: min available frames
for wakeup */
#endif
};

struct snd_pcm_sync_ptr {
        unsigned int flags;
#if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64)
        __u32 __pad; /* for x86-32 */
#endif
       union {
                struct snd_pcm_mmap_status status;
                unsigned char reserved[64];
        } s;
        union {
                struct snd_pcm_mmap_control control;
                unsigned char reserved[64];
        } c;
};

struct snd_timer_tread {
        int event;
#if (__BITS_PER_LONG == 32) && defined(__USE_TIME_BITS64)
        int __pad0; /* make alignment sane on x86_32 */
#endif
       struct timespec tstamp;
        unsigned int val;
#if (__BITS_PER_LONG == 32) && defined(__USE_TIME_BITS64)
        int __pad1;
#endif
};

/* note: on x32, the number will change with a new glibc, but the
behavior will not */
#if (__BITS_PER_LONG == 32) && defined(__USE_TIME_BITS64)
#define SNDRV_TIMER_IOCTL_TREAD         _IOW('T', 0x62, int)
#else
#define SNDRV_TIMER_IOCTL_TREAD         _IOW('T', 0x02, int)
#endif
Takashi Iwai Nov. 10, 2017, 7:19 a.m. UTC | #8 
On Fri, 10 Nov 2017 00:20:10 +0100,
Arnd Bergmann wrote:
> 

> On Thu, Nov 9, 2017 at 7:11 PM, Takashi Iwai <tiwai@suse.de> wrote:

> > On Thu, 09 Nov 2017 18:01:47 +0100,

> > Arnd Bergmann wrote:

> >>

> >> On Thu, Nov 9, 2017 at 5:52 PM, Takashi Iwai <tiwai@suse.de> wrote:

> >> >

> >> > IOW, is there any macro indicating the 64bit user time_t?

> >>

> >> There is a macro defined by the C library, but so far we have not

> >> started relying on it in kernel headers, because there is no guarantee

> >> that this symbol is visible before sys/time.h has been included,

> >> and there are some cases where it's possible to include a kernel

> >> header before sys/time.h.

> >>

> >> In case of sound/asound.h, that should be no problem since we rely

> >> on having seen the definition on 'struct timeval' already today, and

> >> that must come from sys/time.h. Then we just need to make sure that

> >> all C libraries define the same macro.

> >>

> >> Are you sure about the switch()/case problem? I thought that worked

> >> in C99, the only problem would be using the macro outside of a

> >> function, e.g. as initalizer for a variable

> >

> > Hmm, OK it seems working.

> >

> > But, honestly speaking, it's too scaring.  I'm OK if it were only in

> > the kernel local code.  But it's the API/ABI definition, which is

> > referred by user-space...

> >

> > A more solid condition would be really appreciated.

> 

> I understand your concern here and agree it's really ugly. It did take us

> many attempts to come up with this trick for other cases, so my initial

> reaction would be to use the same thing everywhere since I know

> it works,  but we can use #ifdef instead if you prefer that. I think we

> can use a single #ifdef variant to cover all cases, but I'd have to think

> about the x32 and x86-32 some more here. With this trick, we can

> make user space with new glibc use data structures that are compatible

> with 64-bit kernels and avoid the additional translation helpers:

> 

> enum {

>       SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,

>       SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000,

> #if (__BITS_PER_LONG == 64) || !defined(__USE_TIME_BITS64)


Yeah, it's definitely better, more understandable!


thanks,

Takashi
diff mbox series

Patch

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index fabb283..4c74f52 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -760,7 +760,7 @@  struct snd_timer_status {
 
 #define SNDRV_TIMER_IOCTL_PVERSION	_IOR('T', 0x00, int)
 #define SNDRV_TIMER_IOCTL_NEXT_DEVICE	_IOWR('T', 0x01, struct snd_timer_id)
-#define SNDRV_TIMER_IOCTL_TREAD		_IOW('T', 0x02, int)
+#define SNDRV_TIMER_IOCTL_TREAD_OLD	_IOW('T', 0x02, int)
 #define SNDRV_TIMER_IOCTL_GINFO		_IOWR('T', 0x03, struct snd_timer_ginfo)
 #define SNDRV_TIMER_IOCTL_GPARAMS	_IOW('T', 0x04, struct snd_timer_gparams)
 #define SNDRV_TIMER_IOCTL_GSTATUS	_IOWR('T', 0x05, struct snd_timer_gstatus)
@@ -773,6 +773,15 @@  struct snd_timer_status {
 #define SNDRV_TIMER_IOCTL_STOP		_IO('T', 0xa1)
 #define SNDRV_TIMER_IOCTL_CONTINUE	_IO('T', 0xa2)
 #define SNDRV_TIMER_IOCTL_PAUSE		_IO('T', 0xa3)
+#define SNDRV_TIMER_IOCTL_TREAD64	_IOW('T', 0xa4, int)
+
+#if __BITS_PER_LONG == 64
+#define SNDRV_TIMER_IOCTL_TREAD SNDRV_TIMER_IOCTL_TREAD_OLD
+#else
+#define SNDRV_TIMER_IOCTL_TREAD ((sizeof(__kernel_long_t) > sizeof(time_t)) ? \
+				 SNDRV_TIMER_IOCTL_TREAD_OLD : \
+				 SNDRV_TIMER_IOCTL_TREAD64)
+#endif
 
 struct snd_timer_read {
 	unsigned int resolution;
diff --git a/sound/core/timer.c b/sound/core/timer.c
index 9168365..ba6c09e 100644
--- a/sound/core/timer.c
+++ b/sound/core/timer.c
@@ -58,6 +58,30 @@ 
 MODULE_ALIAS_CHARDEV(CONFIG_SND_MAJOR, SNDRV_MINOR_TIMER);
 MODULE_ALIAS("devname:snd/timer");
 
+enum timer_tread_format {
+	TREAD_FORMAT_NONE = 0,
+	TREAD_FORMAT_64BIT,
+	TREAD_FORMAT_TIME64,
+	TREAD_FORMAT_TIME32_X86,
+	TREAD_FORMAT_TIME32,
+};
+
+struct snd_timer_tread64 {
+	int event;
+	u32 pad1;
+	s64 tstamp_sec;
+	s64 tstamp_nsec;
+	unsigned int val;
+	u32 pad2;
+};
+
+struct compat_snd_timer_tread64_x86_32 {
+	int event;
+	s64 tstamp_sec;
+	s64 tstamp_nsec;
+	unsigned int val;
+} __packed;
+
 struct snd_timer_user {
 	struct snd_timer_instance *timeri;
 	int tread;		/* enhanced read with timestamps and events */
@@ -69,7 +93,7 @@  struct snd_timer_user {
 	int queue_size;
 	bool disconnected;
 	struct snd_timer_read *queue;
-	struct snd_timer_tread *tqueue;
+	struct snd_timer_tread64 *tqueue;
 	spinlock_t qlock;
 	unsigned long last_resolution;
 	unsigned int filter;
@@ -1262,7 +1286,7 @@  static void snd_timer_user_interrupt(struct snd_timer_instance *timeri,
 }
 
 static void snd_timer_user_append_to_tqueue(struct snd_timer_user *tu,
-					    struct snd_timer_tread *tread)
+					    struct snd_timer_tread64 *tread)
 {
 	if (tu->qused >= tu->queue_size) {
 		tu->overrun++;
@@ -1279,7 +1303,7 @@  static void snd_timer_user_ccallback(struct snd_timer_instance *timeri,
 				     unsigned long resolution)
 {
 	struct snd_timer_user *tu = timeri->callback_data;
-	struct snd_timer_tread r1;
+	struct snd_timer_tread64 r1;
 	unsigned long flags;
 
 	if (event >= SNDRV_TIMER_EVENT_START &&
@@ -1289,7 +1313,8 @@  static void snd_timer_user_ccallback(struct snd_timer_instance *timeri,
 		return;
 	memset(&r1, 0, sizeof(r1));
 	r1.event = event;
-	r1.tstamp = timespec64_to_timespec(*tstamp);
+	r1.tstamp_sec = tstamp->tv_sec;
+	r1.tstamp_nsec = tstamp->tv_nsec;
 	r1.val = resolution;
 	spin_lock_irqsave(&tu->qlock, flags);
 	snd_timer_user_append_to_tqueue(tu, &r1);
@@ -1311,7 +1336,7 @@  static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 				      unsigned long ticks)
 {
 	struct snd_timer_user *tu = timeri->callback_data;
-	struct snd_timer_tread *r, r1;
+	struct snd_timer_tread64 *r, r1;
 	struct timespec64 tstamp;
 	int prev, append = 0;
 
@@ -1332,7 +1357,8 @@  static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 	if ((tu->filter & (1 << SNDRV_TIMER_EVENT_RESOLUTION)) &&
 	    tu->last_resolution != resolution) {
 		r1.event = SNDRV_TIMER_EVENT_RESOLUTION;
-		r1.tstamp = timespec64_to_timespec(tstamp);
+		r1.tstamp_sec = tstamp.tv_sec;
+		r1.tstamp_nsec = tstamp.tv_nsec;
 		r1.val = resolution;
 		snd_timer_user_append_to_tqueue(tu, &r1);
 		tu->last_resolution = resolution;
@@ -1346,14 +1372,16 @@  static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 		prev = tu->qtail == 0 ? tu->queue_size - 1 : tu->qtail - 1;
 		r = &tu->tqueue[prev];
 		if (r->event == SNDRV_TIMER_EVENT_TICK) {
-			r->tstamp = timespec64_to_timespec(tstamp);
+			r->tstamp_sec = tstamp.tv_sec;
+			r->tstamp_nsec = tstamp.tv_nsec;
 			r->val += ticks;
 			append++;
 			goto __wake;
 		}
 	}
 	r1.event = SNDRV_TIMER_EVENT_TICK;
-	r1.tstamp = timespec64_to_timespec(tstamp);
+	r1.tstamp_sec = tstamp.tv_sec;
+	r1.tstamp_nsec = tstamp.tv_nsec;
 	r1.val = ticks;
 	snd_timer_user_append_to_tqueue(tu, &r1);
 	append++;
@@ -1368,7 +1396,7 @@  static void snd_timer_user_tinterrupt(struct snd_timer_instance *timeri,
 static int realloc_user_queue(struct snd_timer_user *tu, int size)
 {
 	struct snd_timer_read *queue = NULL;
-	struct snd_timer_tread *tqueue = NULL;
+	struct snd_timer_tread64 *tqueue = NULL;
 
 	if (tu->tread) {
 		tqueue = kcalloc(size, sizeof(*tqueue), GFP_KERNEL);
@@ -1797,11 +1825,11 @@  static int snd_timer_user_params(struct file *file,
 	tu->qhead = tu->qtail = tu->qused = 0;
 	if (tu->timeri->flags & SNDRV_TIMER_IFLG_EARLY_EVENT) {
 		if (tu->tread) {
-			struct snd_timer_tread tread;
+			struct snd_timer_tread64 tread;
 			memset(&tread, 0, sizeof(tread));
 			tread.event = SNDRV_TIMER_EVENT_EARLY;
-			tread.tstamp.tv_sec = 0;
-			tread.tstamp.tv_nsec = 0;
+			tread.tstamp_sec = 0;
+			tread.tstamp_nsec = 0;
 			tread.val = 0;
 			snd_timer_user_append_to_tqueue(tu, &tread);
 		} else {
@@ -1919,6 +1947,39 @@  static int snd_timer_user_pause(struct file *file)
 	return (err = snd_timer_pause(tu->timeri)) < 0 ? err : 0;
 }
 
+static int snd_timer_user_tread(void __user *argp, struct snd_timer_user *tu,
+				unsigned int cmd, bool compat)
+{
+	int __user *p = argp;
+	int xarg, old_tread;
+
+	if (tu->timeri)	/* too late */
+		return -EBUSY;
+	if (get_user(xarg, p))
+		return -EFAULT;
+
+	old_tread = tu->tread;
+
+	if (!xarg)
+		tu->tread = TREAD_FORMAT_NONE;
+	else if (!IS_ENABLED(CONFIG_64BITS) && !compat)
+		tu->tread = TREAD_FORMAT_64BIT;
+	else if (cmd == SNDRV_TIMER_IOCTL_TREAD64)
+		tu->tread = TREAD_FORMAT_TIME64;
+	else if (IS_ENABLED(CONFIG_X86))
+		tu->tread = TREAD_FORMAT_TIME32_X86;
+	else
+		tu->tread = TREAD_FORMAT_TIME32;
+
+	if (tu->tread != old_tread &&
+	    realloc_user_queue(tu, tu->queue_size) < 0) {
+		tu->tread = old_tread;
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
 enum {
 	SNDRV_TIMER_IOCTL_START_OLD = _IO('T', 0x20),
 	SNDRV_TIMER_IOCTL_STOP_OLD = _IO('T', 0x21),
@@ -1927,7 +1988,7 @@  enum {
 };
 
 static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
-				 unsigned long arg)
+				 unsigned long arg, bool compat)
 {
 	struct snd_timer_user *tu;
 	void __user *argp = (void __user *)arg;
@@ -1939,23 +2000,9 @@  static long __snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 		return put_user(SNDRV_TIMER_VERSION, p) ? -EFAULT : 0;
 	case SNDRV_TIMER_IOCTL_NEXT_DEVICE:
 		return snd_timer_user_next_device(argp);
-	case SNDRV_TIMER_IOCTL_TREAD:
-	{
-		int xarg, old_tread;
-
-		if (tu->timeri)	/* too late */
-			return -EBUSY;
-		if (get_user(xarg, p))
-			return -EFAULT;
-		old_tread = tu->tread;
-		tu->tread = xarg ? 1 : 0;
-		if (tu->tread != old_tread &&
-		    realloc_user_queue(tu, tu->queue_size) < 0) {
-			tu->tread = old_tread;
-			return -ENOMEM;
-		}
-		return 0;
-	}
+	case SNDRV_TIMER_IOCTL_TREAD_OLD:
+	case SNDRV_TIMER_IOCTL_TREAD64:
+		return snd_timer_user_tread(argp, tu, cmd, compat);
 	case SNDRV_TIMER_IOCTL_GINFO:
 		return snd_timer_user_ginfo(file, argp);
 	case SNDRV_TIMER_IOCTL_GPARAMS:
@@ -1995,7 +2042,7 @@  static long snd_timer_user_ioctl(struct file *file, unsigned int cmd,
 	long ret;
 
 	mutex_lock(&tu->ioctl_lock);
-	ret = __snd_timer_user_ioctl(file, cmd, arg);
+	ret = __snd_timer_user_ioctl(file, cmd, arg, false);
 	mutex_unlock(&tu->ioctl_lock);
 	return ret;
 }
@@ -2017,7 +2064,24 @@  static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 	int err = 0;
 
 	tu = file->private_data;
-	unit = tu->tread ? sizeof(struct snd_timer_tread) : sizeof(struct snd_timer_read);
+	switch (tu->tread) {
+	case TREAD_FORMAT_TIME32_X86:
+		unit = sizeof(struct compat_snd_timer_tread64_x86_32);
+		break;
+	case TREAD_FORMAT_64BIT:
+	case TREAD_FORMAT_TIME64:
+		unit = sizeof(struct snd_timer_tread64);
+		break;
+	case TREAD_FORMAT_TIME32:
+		unit = sizeof(struct snd_timer_tread);
+		break;
+	case TREAD_FORMAT_NONE:
+		unit = sizeof(struct snd_timer_read);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
 	mutex_lock(&tu->ioctl_lock);
 	spin_lock_irq(&tu->qlock);
 	while ((long)count - result >= unit) {
@@ -2057,9 +2121,48 @@  static ssize_t snd_timer_user_read(struct file *file, char __user *buffer,
 		spin_unlock_irq(&tu->qlock);
 
 		if (tu->tread) {
-			if (copy_to_user(buffer, &tu->tqueue[qhead],
-					 sizeof(struct snd_timer_tread)))
-				err = -EFAULT;
+			struct snd_timer_tread64 *tread = &tu->tqueue[qhead];
+			struct snd_timer_tread tread32;
+			struct compat_snd_timer_tread64_x86_32 compat_tread64;
+
+			switch (tu->tread) {
+			case TREAD_FORMAT_TIME32_X86:
+				memset(&compat_tread64, 0, sizeof(compat_tread64));
+				compat_tread64 = (struct compat_snd_timer_tread64_x86_32) {
+					.event = tread->event,
+					.tstamp_sec = tread->tstamp_sec,
+					.tstamp_nsec = tread->tstamp_nsec,
+					.val = tread->val,
+				};
+
+				if (copy_to_user(buffer, &compat_tread64,
+						 sizeof(compat_tread64)))
+					err = -EFAULT;
+				break;
+			case TREAD_FORMAT_64BIT:
+			case TREAD_FORMAT_TIME64:
+				if (copy_to_user(buffer, tread,
+						 sizeof(struct snd_timer_tread64)))
+					err = -EFAULT;
+				break;
+			case TREAD_FORMAT_TIME32:
+				memset(&tread32, 0, sizeof(tread32));
+				tread32 = (struct snd_timer_tread) {
+					.event = tread->event,
+					.tstamp = {
+						.tv_sec = tread->tstamp_sec,
+						.tv_nsec = tread->tstamp_nsec,
+					},
+					.val = tread->val,
+				};
+
+				if (copy_to_user(buffer, &tread32, sizeof(tread32)))
+					err = -EFAULT;
+				break;
+			default:
+				err = -ENOTSUPP;
+				break;
+			}
 		} else {
 			if (copy_to_user(buffer, &tu->queue[qhead],
 					 sizeof(struct snd_timer_read)))
diff --git a/sound/core/timer_compat.c b/sound/core/timer_compat.c
index 3e09548..866e090 100644
--- a/sound/core/timer_compat.c
+++ b/sound/core/timer_compat.c
@@ -110,7 +110,15 @@  static long snd_timer_user_ioctl_compat(struct file *file, unsigned int cmd, uns
 	case SNDRV_TIMER_IOCTL_PAUSE:
 	case SNDRV_TIMER_IOCTL_PAUSE_OLD:
 	case SNDRV_TIMER_IOCTL_NEXT_DEVICE:
-		return snd_timer_user_ioctl(file, cmd, (unsigned long)argp);
+	{
+		struct snd_timer_user *tu = file->private_data;
+		long ret;
+
+		mutex_lock(&tu->ioctl_lock);
+		ret = __snd_timer_user_ioctl(file, cmd, arg, false);
+		mutex_unlock(&tu->ioctl_lock);
+		return ret;
+	}
 	case SNDRV_TIMER_IOCTL_GPARAMS32:
 		return snd_timer_user_gparams_compat(file, argp);
 	case SNDRV_TIMER_IOCTL_INFO32: