Re: [alsa-devel] [RFC PATCH v2 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

[Arnd Bergmann]

On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@xxxxxxx> 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