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