On Sun, Nov 5, 2017 at 11:23 AM, Takashi Iwai tiwai@suse.de wrote:
On Thu, 02 Nov 2017 12:06:52 +0100, Baolin Wang wrote:
The struct snd_pcm_status will use 'timespec' type variables to record timestamp, which is not year 2038 safe on 32bits system.
Userspace will use SNDRV_PCM_IOCTL_STATUS and SNDRV_PCM_IOCTL_STATUS_EXT as commands to issue ioctl() to fill the 'snd_pcm_status' structure in userspace. The command number is always defined through _IOR/_IOW/IORW, so when userspace changes the definition of 'struct timespec' to use 64-bit types, the command number also changes.
Thus in the kernel, we now need to define two versions of each such ioctl and corresponding ioctl commands to handle 32bit time_t and 64bit time_t in native mode: struct snd_pcm_status32 { ...... struct { s32 tv_sec; s32 tv_nsec; } trigger_tstamp; struct { s32 tv_sec; s32 tv_nsec; } tstamp; ...... }
struct snd_pcm_status64 { ...... struct { s64 tv_sec; s64 tv_nsec; } trigger_tstamp; struct { s64 tv_sec; s64 tv_nsec; } tstamp; ...... }
Moreover in compat file, we renamed or introduced new structures to handle 32bit/64bit time_t in compatible mode. 'struct compat_snd_pcm_status32' and snd_pcm_status_user_compat() are used to handle 32bit time_t in compat mode. 'struct compat_snd_pcm_status64' and snd_pcm_status_user_compat64() are used to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_status64_x86_32' and snd_pcm_status_user_compat64_x86_32() are used to handle 64bit time_t with 32bit alignment.
Hmm... I don't get why you need to redefine these in include/sound/pcm.h and define even IOCTLs there. These are the things for ABI, no? If yes, we don't need to have it globally in the public header but define/convert them locally.
The problem is that the ABI is currently defined in terms of a mix of data structures from kernel and glibc. We have to be very careful about changing the public header files to avoid breaking applications that were built against new headers but run on old kernels, so we can't just make the time_t fields 64-bit wide in the header and change the ioctl number for everyone.
The new structure definitions in the internal are representations of the possible binary layouts that user space will actually see with the possible combinations of 32-bit and 64-bit toolchains, the x86-32 quirk (64-bit variables being misaligned), the x32 hack (makeing __kernel_long_t 64-bit) and new glibc using 64-bit time_t.
And, renaming snd_pcm_status64 allover the places for internal doesn't look good.
Would you prefer adding another distinct type for the kernel-internal structure? That would probably be cleaner overall, but also complicate the one case in which the user-space representation is actually the same as the one in the kernel. Note that we can't use snd_pcm_status here, because that is based on 'time_t', which in the kernel has to remain defined as 'long', so we'd still suffer from the overflow.
One more thing: as we discussed in Prague, I think the additional compat_snd_pcm_status64_x86_32 structure can be avoided if we make slight changes to the user-visible definition of snd_pcm_status that only take effect on x86-32 with a modified libc, i.e.
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index 299a822d2c4e..40be757de124 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -447,6 +447,7 @@ enum {
struct snd_pcm_status { snd_pcm_state_t state; /* stream state */ + __u8 __pad0[sizeof(time_t) - sizeof(__kernel_long_t)]; struct timespec trigger_tstamp; /* time when stream was started/stopped/paused */ struct timespec tstamp; /* reference timestamp */ snd_pcm_uframes_t appl_ptr; /* appl ptr */
With that change, some of the other changes should get simpler too.
Arnd