| Message ID | 6781c7b4e5934ad65e3c5b401c0a1bbd7cb44db6.1505973912.git.baolin.wang@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > The struct snd_pcm_sync_ptr will use 'timespec' type variables to record > timestamp, which is not year 2038 safe on 32bits system. > > Thus we introduced 'struct snd_pcm_sync_ptr32' and 'struct snd_pcm_sync_ptr64' > to handle 32bit time_t and 64bit time_t in native mode, which replace > timespec with s64 type. > > In compat mode, we renamed or introduced new structures to handle 32bit/64bit > time_t in compatible mode. The 'struct compat_snd_pcm_sync_ptr32' and > snd_pcm_ioctl_sync_ptr_compat() are used to handle 32bit time_t in compat mode. > 'struct compat_snd_pcm_sync_ptr64' and snd_pcm_ioctl_sync_ptr_compat64() are used > to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_sync_ptr64_x86_32' > and snd_pcm_ioctl_sync_ptr_compat64_x86_32() are used to handle 64bit time_t with > 32bit alignment. > > When glibc changes time_t to 64bit, any recompiled program will issue ioctl > commands that the kernel does not understand without this patch. > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > --- > include/sound/pcm.h | 46 +++++++++- > sound/core/pcm.c | 6 +- > sound/core/pcm_compat.c | 228 ++++++++++++++++++++++++++++++++++++++--------- > sound/core/pcm_lib.c | 9 +- > sound/core/pcm_native.c | 113 ++++++++++++++++++----- > 5 files changed, 329 insertions(+), 73 deletions(-) > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h > index 114cc29..c253cbf 100644 > --- a/include/sound/pcm.h > +++ b/include/sound/pcm.h > @@ -64,6 +64,15 @@ struct snd_pcm_hardware { > struct snd_pcm_audio_tstamp_config; /* definitions further down */ > struct snd_pcm_audio_tstamp_report; > > +struct snd_pcm_mmap_status64 { > + snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ > + int pad1; /* Needed for 64 bit alignment */ > + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ > + struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */ > + snd_pcm_state_t suspended_state; /* RO: suspended stream state */ > + struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from sample counter or wall clock */ > +}; This looks correct, but there is a subtlety here to note about x86-32 that we discussed in a previous (private) review. To recall my earlier thoughts: Normal architectures insert 32 bit padding after 'suspended_state', and 32-bit architectures (including x32) also after hw_ptr, but x86-32 does not. You make that explicit in the compat code, this version just relies on the compiler using identical padding in user and kernel space. We could make that explicit using struct snd_pcm_mmap_status64 { snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ int pad1; /* Needed for 64 bit alignment */ snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32) int pad2; #endif struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */ snd_pcm_state_t suspended_state; /* RO: suspended stream state */ #if !defined(CONFIG_X86_32) int pad3; #endif struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from sample counter or wall clock */ }; To make it clear that the layout is architecture specific. As a third alternative, we could define binary version of the structure explicitly, and have handlers for each layout, then call the correct handlers for both native and compat mode. This could use e.g. struct snd_pcm_mmap_status_time32 { u32 state; u32 pad1; u32 hw_ptr; s32 tstamp_sec; s32 tstamp_usec; u32 suspended_state; s32 audio_tstamp_sec; s32 audio_tstamp_usec; }; struct snd_pcm_mmap_status_time64 { u32 state; u32 pad1; u32 hw_ptr; u32 pad2; s64 tstamp_sec; s64 tstamp_usec; u32 suspended_state; u32 pad3; s64 audio_tstamp_sec; s64 audio_tstamp_usec; }; struct snd_pcm_mmap_status_time64_i386 { u32 state; u32 pad1; u32 hw_ptr; s64 tstamp_sec; s64 tstamp_usec; u32 suspended_state; s64 audio_tstamp_sec; s64 audio_tstamp_usec; } __packed __aligned(4); struct snd_pcm_mmap_status_64bit { u32 state; u32 pad1; u64 hw_ptr; s64 tstamp_sec; s64 tstamp_usec; u32 suspended_state; u32 pad3; s64 audio_tstamp_sec; s64 audio_tstamp_usec; }; My personal preference would be the third approach, but I'd like to hear from Takashi if he has a preference. The downside of that is that it requires the most changes to the existing code. > @@ -1459,8 +1468,21 @@ struct snd_pcm_status64 { > unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */ > }; > > +struct snd_pcm_sync_ptr64 { > + unsigned int flags; > + union { > + struct snd_pcm_mmap_status64 status; > + unsigned char reserved[64]; > + } s; > + union { > + struct snd_pcm_mmap_control control; > + unsigned char reserved[64]; > + } c; > +}; > + > #define SNDRV_PCM_IOCTL_STATUS64 _IOR('A', 0x20, struct snd_pcm_status64) > #define SNDRV_PCM_IOCTL_STATUS_EXT64 _IOWR('A', 0x24, struct snd_pcm_status64) > +#define SNDRV_PCM_IOCTL_SYNC_PTR64 _IOWR('A', 0x23, struct snd_pcm_sync_ptr64) > > #if __BITS_PER_LONG == 32 > struct snd_pcm_status32 { > @@ -1481,8 +1503,30 @@ struct snd_pcm_status32 { > unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */ > }; > > +struct snd_pcm_mmap_status32 { > + snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ > + int pad1; /* Needed for 64 bit alignment */ > + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ > + struct { s32 tv_sec; s32 tv_nsec; } tstamp; /* Timestamp */ > + snd_pcm_state_t suspended_state; /* RO: suspended stream state */ > + struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp; /* from sample counter or wall clock */ > +}; > + > +struct snd_pcm_sync_ptr32 { > + unsigned int flags; > + union { > + struct snd_pcm_mmap_status32 status; > + unsigned char reserved[64]; > + } s; > + union { > + struct snd_pcm_mmap_control control; > + unsigned char reserved[64]; > + } c; > +}; > + > #define SNDRV_PCM_IOCTL_STATUS32 _IOR('A', 0x20, struct snd_pcm_status32) > #define SNDRV_PCM_IOCTL_STATUS_EXT32 _IOWR('A', 0x24, struct snd_pcm_status32) > +#define SNDRV_PCM_IOCTL_SYNC_PTR32 _IOWR('A', 0x23, struct snd_pcm_sync_ptr32) > #endif Unfortunately, this approach doesn't quite work in this particular case: it depends on snd_pcm_sync_ptr32 and snd_pcm_sync_ptr64 having different sizes in order to result in distinct command codes for SNDRV_PCM_IOCTL_SYNC_PTR64 and SNDRV_PCM_IOCTL_SYNC_PTR32. However, the 64-byte 'reserved' fields mean that the unions are always the same size, and only the padding between 'flags' and 's' is different. Again, I suppose this almost works: 64-bit architectures use only one possible layout in the .unlocked_ioctl handler, and on most 32-bit architectures the added padding will make the structure 4 bytes longer, but not on i386, which now doesn't know whether user space passed a snd_pcm_sync_ptr32 or a snd_pcm_sync_ptr64 structure. Fixing this will require changing the uapi header file, in one of two ways: a) make the command number conditional: #if __BITS_PER_LONG == 64 || !defined(__i386__) #define SNDRV_PCM_IOCTL_SYNC_PTR SNDRV_PCM_IOCTL_SYNC_PTR_OLD #else #define SNDRV_PCM_IOCTL_SYNC_PTR (sizeof(__kernel_long_t) == sizeof(time_t) \ ? SNDRV_PCM_IOCTL_SYNC_PTR_OLD : SNDRV_PCM_IOCTL_SYNC_PTR_64) #endif b) change the user-visible structure definition to contain the correct explicit padding on all architectures including i386 struct snd_pcm_mmap_status { snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ int pad1; /* Needed for 64 bit alignment */ snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ + char pad2[sizeof(time_t) - sizeof(snd_pcm_uframes_t)]; struct timespec tstamp; /* Timestamp */ snd_pcm_state_t suspended_state; /* RO: suspended stream state */ + char pad2[sizeof(time_t) - sizeof(snd_pcm_state_t)]; struct timespec audio_tstamp; /* from sample counter or wall clock */ }; struct snd_pcm_mmap_control { snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */ snd_pcm_uframes_t avail_min; /* RW: min available frames for wakeup */ }; struct snd_pcm_sync_ptr32 { unsigned int flags; + char __pad[sizeof(time_t) - sizeof(unsigned int)]; union { struct snd_pcm_mmap_status status; unsigned char reserved[64]; } s; union { struct snd_pcm_mmap_control control; unsigned char reserved[64]; } c; }; > @@ -2995,8 +3058,12 @@ static int snd_pcm_common_ioctl(struct file *file, > return -EFAULT; > return 0; > } > - case SNDRV_PCM_IOCTL_SYNC_PTR: > - return snd_pcm_sync_ptr(substream, arg); > +#if __BITS_PER_LONG == 32 > + case SNDRV_PCM_IOCTL_SYNC_PTR32: > + return snd_pcm_sync_ptr32(substream, arg); > +#endif > + case SNDRV_PCM_IOCTL_SYNC_PTR64: > + return snd_pcm_sync_ptr64(substream, arg); > #ifdef CONFIG_SND_SUPPORT_OLD_API > case SNDRV_PCM_IOCTL_HW_REFINE_OLD: > return snd_pcm_hw_refine_old_user(substream, arg); Without either of the two changes, this function should cause a build error on i386 since SNDRV_PCM_IOCTL_SYNC_PTR32 has the same value as SNDRV_PCM_IOCTL_SYNC_PTR64. Arnd
On 21 September 2017 at 20:50, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record >> timestamp, which is not year 2038 safe on 32bits system. >> >> Thus we introduced 'struct snd_pcm_sync_ptr32' and 'struct snd_pcm_sync_ptr64' >> to handle 32bit time_t and 64bit time_t in native mode, which replace >> timespec with s64 type. >> >> In compat mode, we renamed or introduced new structures to handle 32bit/64bit >> time_t in compatible mode. The 'struct compat_snd_pcm_sync_ptr32' and >> snd_pcm_ioctl_sync_ptr_compat() are used to handle 32bit time_t in compat mode. >> 'struct compat_snd_pcm_sync_ptr64' and snd_pcm_ioctl_sync_ptr_compat64() are used >> to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_sync_ptr64_x86_32' >> and snd_pcm_ioctl_sync_ptr_compat64_x86_32() are used to handle 64bit time_t with >> 32bit alignment. >> >> When glibc changes time_t to 64bit, any recompiled program will issue ioctl >> commands that the kernel does not understand without this patch. >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> --- >> include/sound/pcm.h | 46 +++++++++- >> sound/core/pcm.c | 6 +- >> sound/core/pcm_compat.c | 228 ++++++++++++++++++++++++++++++++++++++--------- >> sound/core/pcm_lib.c | 9 +- >> sound/core/pcm_native.c | 113 ++++++++++++++++++----- >> 5 files changed, 329 insertions(+), 73 deletions(-) >> >> diff --git a/include/sound/pcm.h b/include/sound/pcm.h >> index 114cc29..c253cbf 100644 >> --- a/include/sound/pcm.h >> +++ b/include/sound/pcm.h >> @@ -64,6 +64,15 @@ struct snd_pcm_hardware { >> struct snd_pcm_audio_tstamp_config; /* definitions further down */ >> struct snd_pcm_audio_tstamp_report; >> >> +struct snd_pcm_mmap_status64 { >> + snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ >> + int pad1; /* Needed for 64 bit alignment */ >> + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ >> + struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */ >> + snd_pcm_state_t suspended_state; /* RO: suspended stream state */ >> + struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from sample counter or wall clock */ >> +}; > > This looks correct, but there is a subtlety here to note about x86-32 > that we discussed in a previous (private) review. To recall my earlier > thoughts: > > Normal architectures insert 32 bit padding after 'suspended_state', > and 32-bit architectures (including x32) also after hw_ptr, > but x86-32 does not. You make that explicit in the compat code, > this version just relies on the compiler using identical padding > in user and kernel space. We could make that explicit using > > struct snd_pcm_mmap_status64 { > snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ > int pad1; /* Needed for 64 bit alignment */ > snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ > #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32) > int pad2; > #endif > struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */ > snd_pcm_state_t suspended_state; /* RO: suspended stream state */ > #if !defined(CONFIG_X86_32) > int pad3; > #endif > struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from > sample counter or wall clock */ > }; I am sorry I did not get you here, why we do not need pad2 and pad3 for x86_32? You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if condition? > > To make it clear that the layout is architecture specific. As a third > alternative, we could define binary version of the structure explicitly, > and have handlers for each layout, then call the correct handlers for > both native and compat mode. This could use e.g. > > struct snd_pcm_mmap_status_time32 { > u32 state; > u32 pad1; > u32 hw_ptr; > s32 tstamp_sec; > s32 tstamp_usec; > u32 suspended_state; > s32 audio_tstamp_sec; > s32 audio_tstamp_usec; > }; > > struct snd_pcm_mmap_status_time64 { > u32 state; > u32 pad1; > u32 hw_ptr; > u32 pad2; > s64 tstamp_sec; > s64 tstamp_usec; > u32 suspended_state; > u32 pad3; > s64 audio_tstamp_sec; > s64 audio_tstamp_usec; > }; > > struct snd_pcm_mmap_status_time64_i386 { > u32 state; > u32 pad1; > u32 hw_ptr; > s64 tstamp_sec; > s64 tstamp_usec; > u32 suspended_state; > s64 audio_tstamp_sec; > s64 audio_tstamp_usec; > } __packed __aligned(4); > > struct snd_pcm_mmap_status_64bit { > u32 state; > u32 pad1; > u64 hw_ptr; > s64 tstamp_sec; > s64 tstamp_usec; > u32 suspended_state; > u32 pad3; > s64 audio_tstamp_sec; > s64 audio_tstamp_usec; > }; > > My personal preference would be the third approach, but I'd like > to hear from Takashi if he has a preference. The downside of that > is that it requires the most changes to the existing code. OK. > >> @@ -1459,8 +1468,21 @@ struct snd_pcm_status64 { >> unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */ >> }; >> >> +struct snd_pcm_sync_ptr64 { >> + unsigned int flags; >> + union { >> + struct snd_pcm_mmap_status64 status; >> + unsigned char reserved[64]; >> + } s; >> + union { >> + struct snd_pcm_mmap_control control; >> + unsigned char reserved[64]; >> + } c; >> +}; >> + >> #define SNDRV_PCM_IOCTL_STATUS64 _IOR('A', 0x20, struct snd_pcm_status64) >> #define SNDRV_PCM_IOCTL_STATUS_EXT64 _IOWR('A', 0x24, struct snd_pcm_status64) >> +#define SNDRV_PCM_IOCTL_SYNC_PTR64 _IOWR('A', 0x23, struct snd_pcm_sync_ptr64) >> >> #if __BITS_PER_LONG == 32 >> struct snd_pcm_status32 { >> @@ -1481,8 +1503,30 @@ struct snd_pcm_status32 { >> unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */ >> }; >> >> +struct snd_pcm_mmap_status32 { >> + snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ >> + int pad1; /* Needed for 64 bit alignment */ >> + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ >> + struct { s32 tv_sec; s32 tv_nsec; } tstamp; /* Timestamp */ >> + snd_pcm_state_t suspended_state; /* RO: suspended stream state */ >> + struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp; /* from sample counter or wall clock */ >> +}; >> + >> +struct snd_pcm_sync_ptr32 { >> + unsigned int flags; >> + union { >> + struct snd_pcm_mmap_status32 status; >> + unsigned char reserved[64]; >> + } s; >> + union { >> + struct snd_pcm_mmap_control control; >> + unsigned char reserved[64]; >> + } c; >> +}; >> + >> #define SNDRV_PCM_IOCTL_STATUS32 _IOR('A', 0x20, struct snd_pcm_status32) >> #define SNDRV_PCM_IOCTL_STATUS_EXT32 _IOWR('A', 0x24, struct snd_pcm_status32) >> +#define SNDRV_PCM_IOCTL_SYNC_PTR32 _IOWR('A', 0x23, struct snd_pcm_sync_ptr32) >> #endif > > Unfortunately, this approach doesn't quite work in this particular > case: it depends on snd_pcm_sync_ptr32 and snd_pcm_sync_ptr64 > having different sizes in order to result in distinct command codes > for SNDRV_PCM_IOCTL_SYNC_PTR64 and > SNDRV_PCM_IOCTL_SYNC_PTR32. > > However, the 64-byte 'reserved' fields mean that the unions are always > the same size, and only the padding between 'flags' and 's' is different. Ah, make sense. > > Again, I suppose this almost works: 64-bit architectures use only > one possible layout in the .unlocked_ioctl handler, and on most > 32-bit architectures the added padding will make the structure 4 bytes > longer, but not on i386, which now doesn't know whether user > space passed a snd_pcm_sync_ptr32 or a snd_pcm_sync_ptr64 > structure. > > Fixing this will require changing the uapi header file, in one of two > ways: > > a) make the command number conditional: > > #if __BITS_PER_LONG == 64 || !defined(__i386__) > #define SNDRV_PCM_IOCTL_SYNC_PTR SNDRV_PCM_IOCTL_SYNC_PTR_OLD > #else > #define SNDRV_PCM_IOCTL_SYNC_PTR (sizeof(__kernel_long_t) == sizeof(time_t) \ > ? SNDRV_PCM_IOCTL_SYNC_PTR_OLD > : SNDRV_PCM_IOCTL_SYNC_PTR_64) > #endif > > b) change the user-visible structure definition to contain the > correct explicit padding on all architectures including i386 > > struct snd_pcm_mmap_status { > snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ > int pad1; /* Needed for 64 bit alignment */ > snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ > + char pad2[sizeof(time_t) - sizeof(snd_pcm_uframes_t)]; > struct timespec tstamp; /* Timestamp */ > snd_pcm_state_t suspended_state; /* RO: suspended stream state */ > + char pad2[sizeof(time_t) - sizeof(snd_pcm_state_t)]; > struct timespec audio_tstamp; /* from sample counter or wall clock */ > }; > > struct snd_pcm_mmap_control { > snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */ > snd_pcm_uframes_t avail_min; /* RW: min available frames > for wakeup */ > }; > > struct snd_pcm_sync_ptr32 { > unsigned int flags; > + char __pad[sizeof(time_t) - sizeof(unsigned int)]; > union { > struct snd_pcm_mmap_status status; > unsigned char reserved[64]; > } s; > union { > struct snd_pcm_mmap_control control; > unsigned char reserved[64]; > } c; > }; OK. I will check here again. > >> @@ -2995,8 +3058,12 @@ static int snd_pcm_common_ioctl(struct file *file, >> return -EFAULT; >> return 0; >> } >> - case SNDRV_PCM_IOCTL_SYNC_PTR: >> - return snd_pcm_sync_ptr(substream, arg); >> +#if __BITS_PER_LONG == 32 >> + case SNDRV_PCM_IOCTL_SYNC_PTR32: >> + return snd_pcm_sync_ptr32(substream, arg); >> +#endif >> + case SNDRV_PCM_IOCTL_SYNC_PTR64: >> + return snd_pcm_sync_ptr64(substream, arg); >> #ifdef CONFIG_SND_SUPPORT_OLD_API >> case SNDRV_PCM_IOCTL_HW_REFINE_OLD: >> return snd_pcm_hw_refine_old_user(substream, arg); > > Without either of the two changes, this function should cause > a build error on i386 since SNDRV_PCM_IOCTL_SYNC_PTR32 > has the same value as SNDRV_PCM_IOCTL_SYNC_PTR64. > Yes. Thanks for your useful comments.
On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 21 September 2017 at 20:50, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record >> >> This looks correct, but there is a subtlety here to note about x86-32 >> that we discussed in a previous (private) review. To recall my earlier >> thoughts: >> >> Normal architectures insert 32 bit padding after 'suspended_state', >> and 32-bit architectures (including x32) also after hw_ptr, >> but x86-32 does not. You make that explicit in the compat code, >> this version just relies on the compiler using identical padding >> in user and kernel space. We could make that explicit using >> >> struct snd_pcm_mmap_status64 { >> snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ >> int pad1; /* Needed for 64 bit alignment */ >> snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ >> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32) >> int pad2; >> #endif >> struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */ >> snd_pcm_state_t suspended_state; /* RO: suspended stream state */ >> #if !defined(CONFIG_X86_32) >> int pad3; >> #endif >> struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from >> sample counter or wall clock */ >> }; > > I am sorry I did not get you here, why we do not need pad2 and pad3 > for x86_32? This is again the x86-32 alignment quirk: the structure as defined in the uapi header does not have padding, and the new s64 fields have 32-bit alignment on x86, so the compiler does not add implicit padding in user space. On all other architectures, the fields do get padded implicitly in user space, I'm just listing the padding explicitly. > You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if > condition? No, that was intentional: snd_pcm_uframes_t is 'unsigned long', so on 64-bit architectures we have no padding between two 64-bit values (hw_ptr and tstamp), and on x86-32 we have no padding because both have 32-bit alignment. However, snd_pcm_state_t is 'int', which is always 32-bit wide, so we do have padding on both 32-bit and 64-bit architectures between syspended_state and audio_tstamp, with the exception of x86-32. Arnd
On 22 September 2017 at 16:48, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, Sep 22, 2017 at 8:47 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> On 21 September 2017 at 20:50, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Thu, Sep 21, 2017 at 8:18 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> The struct snd_pcm_sync_ptr will use 'timespec' type variables to record >>> >>> This looks correct, but there is a subtlety here to note about x86-32 >>> that we discussed in a previous (private) review. To recall my earlier >>> thoughts: >>> >>> Normal architectures insert 32 bit padding after 'suspended_state', >>> and 32-bit architectures (including x32) also after hw_ptr, >>> but x86-32 does not. You make that explicit in the compat code, >>> this version just relies on the compiler using identical padding >>> in user and kernel space. We could make that explicit using >>> >>> struct snd_pcm_mmap_status64 { >>> snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ >>> int pad1; /* Needed for 64 bit alignment */ >>> snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ >>> #if !defined(CONFIG_64BIT) && !defined(CONFIG_X86_32) >>> int pad2; >>> #endif >>> struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */ >>> snd_pcm_state_t suspended_state; /* RO: suspended stream state */ >>> #if !defined(CONFIG_X86_32) >>> int pad3; >>> #endif >>> struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from >>> sample counter or wall clock */ >>> }; >> >> I am sorry I did not get you here, why we do not need pad2 and pad3 >> for x86_32? > > This is again the x86-32 alignment quirk: the structure as defined > in the uapi header does not have padding, and the new s64 fields > have 32-bit alignment on x86, so the compiler does not add implicit > padding in user space. > > On all other architectures, the fields do get padded implicitly > in user space, I'm just listing the padding explicitly. Make sense. > >> You missed ‘#if !defined(CONFIG_64BIT)“ at the second #if >> condition? > > No, that was intentional: > > snd_pcm_uframes_t is 'unsigned long', so on 64-bit architectures > we have no padding between two 64-bit values (hw_ptr and tstamp), > and on x86-32 we have no padding because both have 32-bit > alignment. > > However, snd_pcm_state_t is 'int', which is always 32-bit wide, > so we do have padding on both 32-bit and 64-bit architectures > between syspended_state and audio_tstamp, with the exception > of x86-32. Thanks, I can understand now.
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 114cc29..c253cbf 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -64,6 +64,15 @@ struct snd_pcm_hardware { struct snd_pcm_audio_tstamp_config; /* definitions further down */ struct snd_pcm_audio_tstamp_report; +struct snd_pcm_mmap_status64 { + snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ + int pad1; /* Needed for 64 bit alignment */ + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ + struct { s64 tv_sec; s64 tv_nsec; } tstamp; /* Timestamp */ + snd_pcm_state_t suspended_state; /* RO: suspended stream state */ + struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; /* from sample counter or wall clock */ +}; + struct snd_pcm_ops { int (*open)(struct snd_pcm_substream *substream); int (*close)(struct snd_pcm_substream *substream); @@ -389,7 +398,7 @@ struct snd_pcm_runtime { union snd_pcm_sync_id sync; /* hardware synchronization ID */ /* -- mmap -- */ - struct snd_pcm_mmap_status *status; + struct snd_pcm_mmap_status64 *status; struct snd_pcm_mmap_control *control; /* -- locking / scheduling -- */ @@ -1459,8 +1468,21 @@ struct snd_pcm_status64 { unsigned char reserved[52-2*sizeof(struct { s64 tv_sec; s64 tv_nsec; })]; /* must be filled with zero */ }; +struct snd_pcm_sync_ptr64 { + unsigned int flags; + union { + struct snd_pcm_mmap_status64 status; + unsigned char reserved[64]; + } s; + union { + struct snd_pcm_mmap_control control; + unsigned char reserved[64]; + } c; +}; + #define SNDRV_PCM_IOCTL_STATUS64 _IOR('A', 0x20, struct snd_pcm_status64) #define SNDRV_PCM_IOCTL_STATUS_EXT64 _IOWR('A', 0x24, struct snd_pcm_status64) +#define SNDRV_PCM_IOCTL_SYNC_PTR64 _IOWR('A', 0x23, struct snd_pcm_sync_ptr64) #if __BITS_PER_LONG == 32 struct snd_pcm_status32 { @@ -1481,8 +1503,30 @@ struct snd_pcm_status32 { unsigned char reserved[52-2*sizeof(struct { s32 tv_sec; s32 tv_nsec; })]; /* must be filled with zero */ }; +struct snd_pcm_mmap_status32 { + snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */ + int pad1; /* Needed for 64 bit alignment */ + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */ + struct { s32 tv_sec; s32 tv_nsec; } tstamp; /* Timestamp */ + snd_pcm_state_t suspended_state; /* RO: suspended stream state */ + struct { s32 tv_sec; s32 tv_nsec; } audio_tstamp; /* from sample counter or wall clock */ +}; + +struct snd_pcm_sync_ptr32 { + unsigned int flags; + union { + struct snd_pcm_mmap_status32 status; + unsigned char reserved[64]; + } s; + union { + struct snd_pcm_mmap_control control; + unsigned char reserved[64]; + } c; +}; + #define SNDRV_PCM_IOCTL_STATUS32 _IOR('A', 0x20, struct snd_pcm_status32) #define SNDRV_PCM_IOCTL_STATUS_EXT32 _IOWR('A', 0x24, struct snd_pcm_status32) +#define SNDRV_PCM_IOCTL_SYNC_PTR32 _IOWR('A', 0x23, struct snd_pcm_sync_ptr32) #endif #endif /* __SOUND_PCM_H */ diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 2d990d9..ca8a7df 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -1001,7 +1001,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, if (runtime == NULL) return -ENOMEM; - size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status)); + size = PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64)); runtime->status = snd_malloc_pages(size, GFP_KERNEL); if (runtime->status == NULL) { kfree(runtime); @@ -1013,7 +1013,7 @@ int snd_pcm_attach_substream(struct snd_pcm *pcm, int stream, runtime->control = snd_malloc_pages(size, GFP_KERNEL); if (runtime->control == NULL) { snd_free_pages((void*)runtime->status, - PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status))); + PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64))); kfree(runtime); return -ENOMEM; } @@ -1044,7 +1044,7 @@ void snd_pcm_detach_substream(struct snd_pcm_substream *substream) if (runtime->private_free != NULL) runtime->private_free(runtime); snd_free_pages((void*)runtime->status, - PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status))); + PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64))); snd_free_pages((void*)runtime->control, PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control))); kfree(runtime->hw_constraints.rules); diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c index 79e7475..7d690b2 100644 --- a/sound/core/pcm_compat.c +++ b/sound/core/pcm_compat.c @@ -570,7 +570,7 @@ static int snd_pcm_ioctl_xfern_compat(struct snd_pcm_substream *substream, } -struct snd_pcm_mmap_status32 { +struct compat_snd_pcm_mmap_status32 { s32 state; s32 pad1; u32 hw_ptr; @@ -579,32 +579,33 @@ struct snd_pcm_mmap_status32 { struct compat_timespec audio_tstamp; } __attribute__((packed)); -struct snd_pcm_mmap_control32 { +struct compat_snd_pcm_mmap_control32 { u32 appl_ptr; u32 avail_min; }; -struct snd_pcm_sync_ptr32 { +struct compat_snd_pcm_sync_ptr32 { u32 flags; union { - struct snd_pcm_mmap_status32 status; + struct compat_snd_pcm_mmap_status32 status; unsigned char reserved[64]; } s; union { - struct snd_pcm_mmap_control32 control; + struct compat_snd_pcm_mmap_control32 control; unsigned char reserved[64]; } c; } __attribute__((packed)); static int snd_pcm_ioctl_sync_ptr_compat(struct snd_pcm_substream *substream, - struct snd_pcm_sync_ptr32 __user *src) + struct compat_snd_pcm_sync_ptr32 __user *src) { struct snd_pcm_runtime *runtime = substream->runtime; - volatile struct snd_pcm_mmap_status *status; + volatile struct snd_pcm_mmap_status64 *status; volatile struct snd_pcm_mmap_control *control; u32 sflags; struct snd_pcm_mmap_control scontrol; - struct snd_pcm_mmap_status sstatus; + struct snd_pcm_mmap_status64 sstatus; + struct compat_snd_pcm_sync_ptr32 sync_ptr; snd_pcm_uframes_t boundary; int err; @@ -637,63 +638,78 @@ static int snd_pcm_ioctl_sync_ptr_compat(struct snd_pcm_substream *substream, scontrol.avail_min = control->avail_min; sstatus.state = status->state; sstatus.hw_ptr = status->hw_ptr % boundary; - sstatus.tstamp = status->tstamp; + sstatus.tstamp.tv_sec = status->tstamp.tv_sec; + sstatus.tstamp.tv_nsec = status->tstamp.tv_nsec; sstatus.suspended_state = status->suspended_state; - sstatus.audio_tstamp = status->audio_tstamp; + sstatus.audio_tstamp.tv_sec = status->audio_tstamp.tv_sec; + sstatus.audio_tstamp.tv_nsec = status->audio_tstamp.tv_nsec; snd_pcm_stream_unlock_irq(substream); - if (put_user(sstatus.state, &src->s.status.state) || - put_user(sstatus.hw_ptr, &src->s.status.hw_ptr) || - compat_put_timespec(&sstatus.tstamp, &src->s.status.tstamp) || - put_user(sstatus.suspended_state, &src->s.status.suspended_state) || - compat_put_timespec(&sstatus.audio_tstamp, - &src->s.status.audio_tstamp) || - put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) || - put_user(scontrol.avail_min, &src->c.control.avail_min)) + + memset(&sync_ptr, 0, sizeof(sync_ptr)); + sync_ptr.s.status = (struct compat_snd_pcm_mmap_status32) { + .state = sstatus.state, + .hw_ptr = sstatus.hw_ptr, + .tstamp = { + .tv_sec = sstatus.tstamp.tv_sec, + .tv_nsec = sstatus.tstamp.tv_nsec, + }, + .suspended_state = sstatus.suspended_state, + .audio_tstamp = { + .tv_sec = sstatus.audio_tstamp.tv_sec, + .tv_nsec = sstatus.audio_tstamp.tv_nsec, + }, + }; + + sync_ptr.c.control = (struct compat_snd_pcm_mmap_control32) { + .appl_ptr = scontrol.appl_ptr, + .avail_min = scontrol.avail_min, + }; + + if (copy_to_user(src, &sync_ptr, sizeof(sync_ptr))) return -EFAULT; return 0; } -#ifdef CONFIG_X86_X32 -/* X32 ABI has 64bit timespec and 64bit alignment */ -struct snd_pcm_mmap_status_x32 { +struct compat_snd_pcm_mmap_status64 { s32 state; s32 pad1; u32 hw_ptr; u32 pad2; /* alignment */ - struct timespec tstamp; + struct { s64 tv_sec; s64 tv_nsec; } tstamp; s32 suspended_state; s32 pad3; - struct timespec audio_tstamp; + struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; } __packed; -struct snd_pcm_mmap_control_x32 { +struct compat_snd_pcm_mmap_control64 { u32 appl_ptr; u32 avail_min; }; -struct snd_pcm_sync_ptr_x32 { +struct compat_snd_pcm_sync_ptr64 { u32 flags; u32 rsvd; /* alignment */ union { - struct snd_pcm_mmap_status_x32 status; + struct compat_snd_pcm_mmap_status64 status; unsigned char reserved[64]; } s; union { - struct snd_pcm_mmap_control_x32 control; + struct compat_snd_pcm_mmap_control64 control; unsigned char reserved[64]; } c; } __packed; -static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream, - struct snd_pcm_sync_ptr_x32 __user *src) +static int snd_pcm_ioctl_sync_ptr_compat64(struct snd_pcm_substream *substream, + struct compat_snd_pcm_sync_ptr64 __user *src) { struct snd_pcm_runtime *runtime = substream->runtime; - volatile struct snd_pcm_mmap_status *status; + volatile struct snd_pcm_mmap_status64 *status; volatile struct snd_pcm_mmap_control *control; u32 sflags; struct snd_pcm_mmap_control scontrol; - struct snd_pcm_mmap_status sstatus; + struct snd_pcm_mmap_status64 sstatus; + struct compat_snd_pcm_sync_ptr64 sync_ptr; snd_pcm_uframes_t boundary; int err; @@ -726,22 +742,142 @@ static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream, scontrol.avail_min = control->avail_min; sstatus.state = status->state; sstatus.hw_ptr = status->hw_ptr % boundary; - sstatus.tstamp = status->tstamp; + sstatus.tstamp.tv_sec = status->tstamp.tv_sec; + sstatus.tstamp.tv_nsec = status->tstamp.tv_nsec; sstatus.suspended_state = status->suspended_state; - sstatus.audio_tstamp = status->audio_tstamp; + sstatus.audio_tstamp.tv_sec = status->audio_tstamp.tv_sec; + sstatus.audio_tstamp.tv_nsec = status->audio_tstamp.tv_nsec; snd_pcm_stream_unlock_irq(substream); - if (put_user(sstatus.state, &src->s.status.state) || - put_user(sstatus.hw_ptr, &src->s.status.hw_ptr) || - put_timespec(&sstatus.tstamp, &src->s.status.tstamp) || - put_user(sstatus.suspended_state, &src->s.status.suspended_state) || - put_timespec(&sstatus.audio_tstamp, &src->s.status.audio_tstamp) || - put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) || - put_user(scontrol.avail_min, &src->c.control.avail_min)) + + memset(&sync_ptr, 0, sizeof(sync_ptr)); + sync_ptr.s.status = (struct compat_snd_pcm_mmap_status64) { + .state = sstatus.state, + .hw_ptr = sstatus.hw_ptr, + .tstamp = { + .tv_sec = sstatus.tstamp.tv_sec, + .tv_nsec = sstatus.tstamp.tv_nsec, + }, + .suspended_state = sstatus.suspended_state, + .audio_tstamp = { + .tv_sec = sstatus.audio_tstamp.tv_sec, + .tv_nsec = sstatus.audio_tstamp.tv_nsec, + }, + }; + + sync_ptr.c.control = (struct compat_snd_pcm_mmap_control64) { + .appl_ptr = scontrol.appl_ptr, + .avail_min = scontrol.avail_min, + }; + + if (copy_to_user(src, &sync_ptr, sizeof(sync_ptr))) return -EFAULT; return 0; } -#endif /* CONFIG_X86_X32 */ + +#ifdef IA32_EMULATION +struct compat_snd_pcm_mmap_status64_x86_32 { + s32 state; + s32 pad1; + u32 hw_ptr; + struct { s64 tv_sec; s64 tv_nsec; } tstamp; + s32 suspended_state; + struct { s64 tv_sec; s64 tv_nsec; } audio_tstamp; +} __packed; + +struct compat_snd_pcm_mmap_control64_x86_32 { + u32 appl_ptr; + u32 avail_min; +}; + +struct compat_snd_pcm_sync_ptr64_x86_32 { + u32 flags; + union { + struct compat_snd_pcm_mmap_status64_x86_32 status; + unsigned char reserved[64]; + } s; + union { + struct compat_snd_pcm_mmap_control64_x86_32 control; + unsigned char reserved[64]; + } c; +} __packed; + +static int +snd_pcm_ioctl_sync_ptr_compat64_x86_32(struct snd_pcm_substream *substream, + struct compat_snd_pcm_sync_ptr64_x86_32 __user *src) +{ + struct snd_pcm_runtime *runtime = substream->runtime; + volatile struct snd_pcm_mmap_status64 *status; + volatile struct snd_pcm_mmap_control *control; + u32 sflags; + struct snd_pcm_mmap_control scontrol; + struct snd_pcm_mmap_status64 sstatus; + struct compat_snd_pcm_sync_ptr64_x86_32 sync_ptr; + snd_pcm_uframes_t boundary; + int err; + + if (snd_BUG_ON(!runtime)) + return -EINVAL; + + if (get_user(sflags, &src->flags) || + get_user(scontrol.appl_ptr, &src->c.control.appl_ptr) || + get_user(scontrol.avail_min, &src->c.control.avail_min)) + return -EFAULT; + if (sflags & SNDRV_PCM_SYNC_PTR_HWSYNC) { + err = snd_pcm_hwsync(substream); + if (err < 0) + return err; + } + status = runtime->status; + control = runtime->control; + boundary = recalculate_boundary(runtime); + if (!boundary) + boundary = 0x7fffffff; + snd_pcm_stream_lock_irq(substream); + /* FIXME: we should consider the boundary for the sync from app */ + if (!(sflags & SNDRV_PCM_SYNC_PTR_APPL)) + control->appl_ptr = scontrol.appl_ptr; + else + scontrol.appl_ptr = control->appl_ptr % boundary; + if (!(sflags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN)) + control->avail_min = scontrol.avail_min; + else + scontrol.avail_min = control->avail_min; + sstatus.state = status->state; + sstatus.hw_ptr = status->hw_ptr % boundary; + sstatus.tstamp.tv_sec = status->tstamp.tv_sec; + sstatus.tstamp.tv_nsec = status->tstamp.tv_nsec; + sstatus.suspended_state = status->suspended_state; + sstatus.audio_tstamp.tv_sec = status->audio_tstamp.tv_sec; + sstatus.audio_tstamp.tv_nsec = status->audio_tstamp.tv_nsec; + snd_pcm_stream_unlock_irq(substream); + + memset(&sync_ptr, 0, sizeof(sync_ptr)); + sync_ptr.s.status = (struct compat_snd_pcm_mmap_status64_x86_32) { + .state = sstatus.state, + .hw_ptr = sstatus.hw_ptr, + .tstamp = { + .tv_sec = sstatus.tstamp.tv_sec, + .tv_nsec = sstatus.tstamp.tv_nsec, + }, + .suspended_state = sstatus.suspended_state, + .audio_tstamp = { + .tv_sec = sstatus.audio_tstamp.tv_sec, + .tv_nsec = sstatus.audio_tstamp.tv_nsec, + }, + }; + + sync_ptr.c.control = (struct compat_snd_pcm_mmap_control64_x86_32) { + .appl_ptr = scontrol.appl_ptr, + .avail_min = scontrol.avail_min, + }; + + if (copy_to_user(src, &sync_ptr, sizeof(sync_ptr))) + return -EFAULT; + + return 0; +} +#endif /* */ @@ -759,12 +895,14 @@ enum { SNDRV_PCM_IOCTL_READI_FRAMES32 = _IOR('A', 0x51, struct snd_xferi32), SNDRV_PCM_IOCTL_WRITEN_FRAMES32 = _IOW('A', 0x52, struct snd_xfern32), SNDRV_PCM_IOCTL_READN_FRAMES32 = _IOR('A', 0x53, struct snd_xfern32), - SNDRV_PCM_IOCTL_SYNC_PTR32 = _IOWR('A', 0x23, struct snd_pcm_sync_ptr32), + SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT32 = _IOWR('A', 0x23, struct compat_snd_pcm_sync_ptr32), + SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64 = _IOWR('A', 0x23, struct compat_snd_pcm_sync_ptr64), SNDRV_PCM_IOCTL_STATUS_COMPAT64 = _IOR('A', 0x20, struct compat_snd_pcm_status64), SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64 = _IOWR('A', 0x24, struct compat_snd_pcm_status64), #ifdef IA32_EMULATION SNDRV_PCM_IOCTL_STATUS_COMPAT64_X86_32 = _IOR('A', 0x20, struct compat_snd_pcm_status64_x86_32), SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64_X86_32 = _IOWR('A', 0x24, struct compat_snd_pcm_status64_x86_32), + SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64_X86_32 = _IOWR('A', 0x23, struct compat_snd_pcm_sync_ptr64_x86_32), #endif #ifdef CONFIG_X86_X32 SNDRV_PCM_IOCTL_CHANNEL_INFO_X32 = _IOR('A', 0x32, struct snd_pcm_channel_info), @@ -821,8 +959,10 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l return snd_pcm_status_user_compat(substream, argp, false); case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT32: return snd_pcm_status_user_compat(substream, argp, true); - case SNDRV_PCM_IOCTL_SYNC_PTR32: + case SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT32: return snd_pcm_ioctl_sync_ptr_compat(substream, argp); + case SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64: + return snd_pcm_ioctl_sync_ptr_compat64(substream, argp); case SNDRV_PCM_IOCTL_CHANNEL_INFO32: return snd_pcm_ioctl_channel_info_compat(substream, argp); case SNDRV_PCM_IOCTL_WRITEI_FRAMES32: @@ -848,6 +988,8 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l return snd_pcm_status_user_compat64_x86_32(substream, argp, false); case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64_X86_32: return snd_pcm_status_user_compat64_x86_32(substream, argp, true); + case SNDRV_PCM_IOCTL_SYNC_PTR_COMPAT64_X86_32: + return snd_pcm_ioctl_sync_ptr_compat64_x86_32(substream, argp); #endif #ifdef CONFIG_X86_X32 case SNDRV_PCM_IOCTL_SYNC_PTR_X32: diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 5ca9dc3..96047cc 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -162,7 +162,8 @@ static void xrun(struct snd_pcm_substream *substream) struct timespec64 tstamp; snd_pcm_gettime(runtime, &tstamp); - runtime->status->tstamp = timespec64_to_timespec(tstamp); + runtime->status->tstamp.tv_sec = tstamp.tv_sec; + runtime->status->tstamp.tv_nsec = tstamp.tv_nsec; } snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN); if (xrun_debug(substream, XRUN_DEBUG_BASIC)) { @@ -252,8 +253,10 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream, runtime->rate); *audio_tstamp = ns_to_timespec64(audio_nsecs); } - runtime->status->audio_tstamp = timespec64_to_timespec(*audio_tstamp); - runtime->status->tstamp = timespec64_to_timespec(*curr_tstamp); + runtime->status->audio_tstamp.tv_sec = audio_tstamp->tv_sec; + runtime->status->audio_tstamp.tv_nsec = audio_tstamp->tv_nsec; + runtime->status->tstamp.tv_sec = curr_tstamp->tv_sec; + runtime->status->tstamp.tv_nsec = curr_tstamp->tv_nsec; /* * re-take a driver timestamp to let apps detect if the reference tstamp diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 7f1f60d..287f20a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2774,50 +2774,113 @@ static snd_pcm_sframes_t snd_pcm_delay(struct snd_pcm_substream *substream) snd_pcm_stream_unlock_irq(substream); return err < 0 ? err : n; } - + static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, - struct snd_pcm_sync_ptr __user *_sync_ptr) + struct snd_pcm_sync_ptr64 *sync_ptr) { struct snd_pcm_runtime *runtime = substream->runtime; - struct snd_pcm_sync_ptr sync_ptr; - volatile struct snd_pcm_mmap_status *status; + volatile struct snd_pcm_mmap_status64 *status; volatile struct snd_pcm_mmap_control *control; int err; - memset(&sync_ptr, 0, sizeof(sync_ptr)); - if (get_user(sync_ptr.flags, (unsigned __user *)&(_sync_ptr->flags))) - return -EFAULT; - if (copy_from_user(&sync_ptr.c.control, &(_sync_ptr->c.control), sizeof(struct snd_pcm_mmap_control))) - return -EFAULT; status = runtime->status; control = runtime->control; - if (sync_ptr.flags & SNDRV_PCM_SYNC_PTR_HWSYNC) { + if (sync_ptr->flags & SNDRV_PCM_SYNC_PTR_HWSYNC) { err = snd_pcm_hwsync(substream); if (err < 0) return err; } snd_pcm_stream_lock_irq(substream); - if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_APPL)) { + if (!(sync_ptr->flags & SNDRV_PCM_SYNC_PTR_APPL)) { err = pcm_lib_apply_appl_ptr(substream, - sync_ptr.c.control.appl_ptr); + sync_ptr->c.control.appl_ptr); if (err < 0) { snd_pcm_stream_unlock_irq(substream); return err; } } else { - sync_ptr.c.control.appl_ptr = control->appl_ptr; + sync_ptr->c.control.appl_ptr = control->appl_ptr; } - if (!(sync_ptr.flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN)) - control->avail_min = sync_ptr.c.control.avail_min; + if (!(sync_ptr->flags & SNDRV_PCM_SYNC_PTR_AVAIL_MIN)) + control->avail_min = sync_ptr->c.control.avail_min; else - sync_ptr.c.control.avail_min = control->avail_min; - sync_ptr.s.status.state = status->state; - sync_ptr.s.status.hw_ptr = status->hw_ptr; - sync_ptr.s.status.tstamp = status->tstamp; - sync_ptr.s.status.suspended_state = status->suspended_state; + sync_ptr->c.control.avail_min = control->avail_min; + sync_ptr->s.status.state = status->state; + sync_ptr->s.status.hw_ptr = status->hw_ptr; + sync_ptr->s.status.tstamp.tv_sec = status->tstamp.tv_sec; + sync_ptr->s.status.tstamp.tv_nsec = status->tstamp.tv_nsec; + sync_ptr->s.status.suspended_state = status->suspended_state; snd_pcm_stream_unlock_irq(substream); + + return 0; +} + +#if __BITS_PER_LONG == 32 +static int snd_pcm_sync_ptr32(struct snd_pcm_substream *substream, + struct snd_pcm_sync_ptr32 __user *_sync_ptr) +{ + struct snd_pcm_sync_ptr64 sync_ptr64; + struct snd_pcm_sync_ptr32 sync_ptr32; + int ret; + + memset(&sync_ptr64, 0, sizeof(sync_ptr64)); + memset(&sync_ptr32, 0, sizeof(sync_ptr32)); + if (get_user(sync_ptr64.flags, (unsigned __user *)&(_sync_ptr->flags))) + return -EFAULT; + if (copy_from_user(&sync_ptr64.c.control, &(_sync_ptr->c.control), + sizeof(struct snd_pcm_mmap_control))) + return -EFAULT; + + ret = snd_pcm_sync_ptr(substream, &sync_ptr64); + if (ret) + return ret; + + sync_ptr32.s.status = (struct snd_pcm_mmap_status32) { + .state = sync_ptr64.s.status.state, + .hw_ptr = sync_ptr64.s.status.hw_ptr, + .tstamp = { + .tv_sec = sync_ptr64.s.status.tstamp.tv_sec, + .tv_nsec = sync_ptr64.s.status.tstamp.tv_nsec, + }, + .suspended_state = sync_ptr64.s.status.suspended_state, + .audio_tstamp = { + .tv_sec = sync_ptr64.s.status.audio_tstamp.tv_sec, + .tv_nsec = sync_ptr64.s.status.audio_tstamp.tv_nsec, + }, + }; + + sync_ptr32.c.control = (struct snd_pcm_mmap_control) { + .appl_ptr = sync_ptr64.c.control.appl_ptr, + .avail_min = sync_ptr64.c.control.avail_min, + }; + + if (copy_to_user(_sync_ptr, &sync_ptr32, sizeof(sync_ptr32))) + return -EFAULT; + + return 0; +} +#endif + +static int snd_pcm_sync_ptr64(struct snd_pcm_substream *substream, + struct snd_pcm_sync_ptr64 __user *_sync_ptr) +{ + struct snd_pcm_sync_ptr64 sync_ptr; + int ret; + + memset(&sync_ptr, 0, sizeof(sync_ptr)); + if (get_user(sync_ptr.flags, (unsigned __user *)&(_sync_ptr->flags))) + return -EFAULT; + if (copy_from_user(&sync_ptr.c.control, &(_sync_ptr->c.control), + sizeof(struct snd_pcm_mmap_control))) + return -EFAULT; + + ret = snd_pcm_sync_ptr(substream, &sync_ptr); + if (ret) + return ret; + if (copy_to_user(_sync_ptr, &sync_ptr, sizeof(sync_ptr))) return -EFAULT; + return 0; } @@ -2995,8 +3058,12 @@ static int snd_pcm_common_ioctl(struct file *file, return -EFAULT; return 0; } - case SNDRV_PCM_IOCTL_SYNC_PTR: - return snd_pcm_sync_ptr(substream, arg); +#if __BITS_PER_LONG == 32 + case SNDRV_PCM_IOCTL_SYNC_PTR32: + return snd_pcm_sync_ptr32(substream, arg); +#endif + case SNDRV_PCM_IOCTL_SYNC_PTR64: + return snd_pcm_sync_ptr64(substream, arg); #ifdef CONFIG_SND_SUPPORT_OLD_API case SNDRV_PCM_IOCTL_HW_REFINE_OLD: return snd_pcm_hw_refine_old_user(substream, arg); @@ -3329,7 +3396,7 @@ static int snd_pcm_mmap_status(struct snd_pcm_substream *substream, struct file if (!(area->vm_flags & VM_READ)) return -EINVAL; size = area->vm_end - area->vm_start; - if (size != PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status))) + if (size != PAGE_ALIGN(sizeof(struct snd_pcm_mmap_status64))) return -EINVAL; area->vm_ops = &snd_pcm_vm_ops_status; area->vm_private_data = substream;
The struct snd_pcm_sync_ptr will use 'timespec' type variables to record timestamp, which is not year 2038 safe on 32bits system. Thus we introduced 'struct snd_pcm_sync_ptr32' and 'struct snd_pcm_sync_ptr64' to handle 32bit time_t and 64bit time_t in native mode, which replace timespec with s64 type. In compat mode, we renamed or introduced new structures to handle 32bit/64bit time_t in compatible mode. The 'struct compat_snd_pcm_sync_ptr32' and snd_pcm_ioctl_sync_ptr_compat() are used to handle 32bit time_t in compat mode. 'struct compat_snd_pcm_sync_ptr64' and snd_pcm_ioctl_sync_ptr_compat64() are used to handle 64bit time_t with 64bit alignment. 'struct compat_snd_pcm_sync_ptr64_x86_32' and snd_pcm_ioctl_sync_ptr_compat64_x86_32() are used to handle 64bit time_t with 32bit alignment. When glibc changes time_t to 64bit, any recompiled program will issue ioctl commands that the kernel does not understand without this patch. Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- include/sound/pcm.h | 46 +++++++++- sound/core/pcm.c | 6 +- sound/core/pcm_compat.c | 228 ++++++++++++++++++++++++++++++++++++++--------- sound/core/pcm_lib.c | 9 +- sound/core/pcm_native.c | 113 ++++++++++++++++++----- 5 files changed, 329 insertions(+), 73 deletions(-)