The snd_pcm_mmap_status and snd_pcm_mmap_control interfaces are one of the
trickiest areas to get right when moving to 64-bit time_t in user space.
The snd_pcm_mmap_status structure layout is incompatible with user space
that uses a 64-bit time_t, so we need a new layout for it. Since the
SNDRV_PCM_IOCTL_SYNC_PTR ioctl combines it with snd_pcm_mmap_control
into snd_pcm_sync_ptr, we need to change those two as well.
Both structures are also exported via an mmap() operation on certain
architectures, and this suffers from incompatibility between 32-bit
and 64-bit user space. As we have to change both structures anyway,
this is a good opportunity to fix the mmap() problem as well, so let's
standardize on the existing 64-bit layout of the structure where possible.
The downside is that we lose mmap() support for existing 32-bit x86 and
powerpc applications, adding that would introduce very noticeable runtime
overhead and complexity. My assumption here is that not too many people
will miss the removed feature, given that:
- Almost all x86 and powerpc users these days are on 64-bit kernels,
the majority of today's 32-bit users are on architectures that never
supported mmap (ARM, MIPS, ...).
- It never worked in compat mode (it was intentionally disabled there)
- The application already needs to work with a fallback to
SNDRV_PCM_IOCTL_SYNC_PTR, which will keep working with both the old
and new structure layout.
Both the ioctl() and mmap() based interfaces are changed at the same
time, as they are based on the same structures. Unlike other interfaces,
we change the uapi header to export both the traditional structure and
a version that is portable between 32-bit and 64-bit user space code
and that corresponds to the existing 64-bit layout. We further check the
__USE_TIME_BITS64 macro that will be defined by future C library versions
whenever we use the new time_t definition, so any existing user space
source code will not see any changes until it gets rebuilt against a new
C library. However, the new structures are all visible in addition to the
old ones, allowing applications to explicitly request the new structures.
In order to detect the difference between the old snd_pcm_mmap_status and
the new __snd_pcm_mmap_status64 structure from the ioctl command number,
we rely on one quirk in the structure definition: snd_pcm_mmap_status
must be aligned to alignof(time_t), which leads the compiler to insert
four bytes of padding in struct snd_pcm_sync_ptr after 'flags' and a
corresponding change in the size of snd_pcm_sync_ptr itself. On x86-32
(and only there), the compiler doesn't use 64-bit alignment in structure,
so I'm adding an explicit pad in the structure that has no effect on the
existing 64-bit architectures but ensures that the layout matches for x86.
The snd_pcm_uframes_t type compatibility requires another hack: we can't
easily make that 64 bit wide, so I leave the type as 'unsigned long',
but add padding before and after it, to ensure that the data is properly
aligned to the respective 64-bit field in the in-kernel structure.
For the SNDRV_PCM_MMAP_OFFSET_STATUS/CONTROL constants that are used
as the virtual file offset in the mmap() function, we also have to
introduce new constants that depend on hte __USE_TIME_BITS64 macro:
The existing macros are renamed to SNDRV_PCM_MMAP_OFFSET_STATUS_OLD
and SNDRV_PCM_MMAP_OFFSET_CONTROL_OLD, they continue to work fine on
64-bit architectures, but stop working on native 32-bit user space.
The replacement _NEW constants are now used by default for user space
built with __USE_TIME_BITS64, those now work on all new kernels for x86,
ppc and alpha (32 and 64 bit, native and compat). It might be a good idea
for a future alsa-lib to support both the _OLD and _NEW macros and use
the corresponding structures directly. Unmodified alsa-lib source code
will retain the current behavior, so it will no longer be able to use
mmap() for the status/control structures on 32-bit systems, until either
the C library gets updated to 64-bit time_t or alsa-lib gets updated to
support both mmap() layouts.
Co-developed-with: Baolin Wang <[email protected]>
Signed-off-by: Baolin Wang <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
include/uapi/sound/asound.h | 110 ++++++++++++++++++++++++++++++++----
sound/core/pcm_compat.c | 30 +++++-----
sound/core/pcm_lib.c | 10 ++--
sound/core/pcm_native.c | 38 ++++++++-----
4 files changed, 147 insertions(+), 41 deletions(-)
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index ad86c5a7a1e2..df9983e7ead5 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -35,6 +35,8 @@
#include <time.h>
#endif
+#include <asm/byteorder.h>
+
/*
* protocol version
*/
@@ -301,7 +303,9 @@ typedef int __bitwise snd_pcm_subformat_t;
#define SNDRV_PCM_INFO_DRAIN_TRIGGER 0x40000000 /* internal kernel flag - trigger in drain */
#define SNDRV_PCM_INFO_FIFO_IN_FRAMES 0x80000000 /* internal kernel flag - FIFO size is in frames */
-
+#if (__BITS_PER_LONG == 32 && defined(__USE_TIME_BITS64)) || defined __KERNEL__
+#define __SND_STRUCT_TIME64
+#endif
typedef int __bitwise snd_pcm_state_t;
#define SNDRV_PCM_STATE_OPEN ((__force snd_pcm_state_t) 0) /* stream is open */
@@ -317,8 +321,17 @@ typedef int __bitwise snd_pcm_state_t;
enum {
SNDRV_PCM_MMAP_OFFSET_DATA = 0x00000000,
- SNDRV_PCM_MMAP_OFFSET_STATUS = 0x80000000,
- SNDRV_PCM_MMAP_OFFSET_CONTROL = 0x81000000,
+ SNDRV_PCM_MMAP_OFFSET_STATUS_OLD = 0x80000000,
+ SNDRV_PCM_MMAP_OFFSET_CONTROL_OLD = 0x81000000,
+ SNDRV_PCM_MMAP_OFFSET_STATUS_NEW = 0x82000000,
+ SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW = 0x83000000,
+#ifdef __SND_STRUCT_TIME64
+ SNDRV_PCM_MMAP_OFFSET_STATUS = SNDRV_PCM_MMAP_OFFSET_STATUS_NEW,
+ SNDRV_PCM_MMAP_OFFSET_CONTROL = SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW,
+#else
+ SNDRV_PCM_MMAP_OFFSET_STATUS = SNDRV_PCM_MMAP_OFFSET_STATUS_OLD,
+ SNDRV_PCM_MMAP_OFFSET_CONTROL = SNDRV_PCM_MMAP_OFFSET_CONTROL_OLD,
+#endif
};
union snd_pcm_sync_id {
@@ -480,16 +493,46 @@ struct snd_pcm_status {
};
#endif
-struct snd_pcm_mmap_status {
+/*
+ * For mmap operations, we need the 64-bit layout, both for compat mode,
+ * and for y2038 compatibility. For 64-bit applications, the two definitions
+ * are identical, so we keep the traditional version.
+ */
+#ifdef __SND_STRUCT_TIME64
+#define __snd_pcm_mmap_status64 snd_pcm_mmap_status
+#define __snd_pcm_mmap_control64 snd_pcm_mmap_control
+#define __snd_pcm_sync_ptr64 snd_pcm_sync_ptr
+#ifdef __KERNEL__
+#define __snd_timespec64 __kernel_timespec
+#else
+#define __snd_timespec64 timespec
+#endif
+struct __snd_timespec {
+ __s32 tv_sec;
+ __s32 tv_nsec;
+};
+#else
+#define __snd_pcm_mmap_status snd_pcm_mmap_status
+#define __snd_pcm_mmap_control snd_pcm_mmap_control
+#define __snd_pcm_sync_ptr snd_pcm_sync_ptr
+#define __snd_timespec timespec
+struct __snd_timespec64 {
+ __s64 tv_sec;
+ __s64 tv_nsec;
+};
+
+#endif
+
+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) */
- struct timespec tstamp; /* Timestamp */
+ struct __snd_timespec tstamp; /* Timestamp */
snd_pcm_state_t suspended_state; /* RO: suspended stream state */
- struct timespec audio_tstamp; /* from sample counter or wall clock */
+ struct __snd_timespec audio_tstamp; /* from sample counter or wall clock */
};
-struct snd_pcm_mmap_control {
+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 */
};
@@ -498,14 +541,59 @@ struct snd_pcm_mmap_control {
#define SNDRV_PCM_SYNC_PTR_APPL (1<<1) /* get appl_ptr from driver (r/w op) */
#define SNDRV_PCM_SYNC_PTR_AVAIL_MIN (1<<2) /* get avail_min from driver */
-struct snd_pcm_sync_ptr {
+struct __snd_pcm_sync_ptr {
unsigned int flags;
union {
- struct snd_pcm_mmap_status status;
+ struct __snd_pcm_mmap_status status;
+ unsigned char reserved[64];
+ } s;
+ union {
+ struct __snd_pcm_mmap_control control;
+ unsigned char reserved[64];
+ } c;
+};
+
+#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
+typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
+typedef char __pad_after_uframe[0];
+#endif
+
+#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
+typedef char __pad_before_uframe[0];
+typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
+#endif
+
+struct __snd_pcm_mmap_status64 {
+ __s32 state; /* RO: state - SNDRV_PCM_STATE_XXXX */
+ __u32 pad1; /* Needed for 64 bit alignment */
+ __pad_before_uframe __pad1;
+ snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
+ __pad_after_uframe __pad2;
+ struct __snd_timespec64 tstamp; /* Timestamp */
+ __s32 suspended_state; /* RO: suspended stream state */
+ __u32 pad3; /* Needed for 64 bit alignment */
+ struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */
+};
+
+struct __snd_pcm_mmap_control64 {
+ __pad_before_uframe __pad1;
+ snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */
+ __pad_before_uframe __pad2;
+
+ __pad_before_uframe __pad3;
+ snd_pcm_uframes_t avail_min; /* RW: min available frames for wakeup */
+ __pad_after_uframe __pad4;
+};
+
+struct __snd_pcm_sync_ptr64 {
+ __u32 flags;
+ __u32 pad1;
+ union {
+ struct __snd_pcm_mmap_status64 status;
unsigned char reserved[64];
} s;
union {
- struct snd_pcm_mmap_control control;
+ struct __snd_pcm_mmap_control64 control;
unsigned char reserved[64];
} c;
};
@@ -590,6 +678,8 @@ enum {
#define SNDRV_PCM_IOCTL_STATUS _IOR('A', 0x20, struct snd_pcm_status)
#define SNDRV_PCM_IOCTL_DELAY _IOR('A', 0x21, snd_pcm_sframes_t)
#define SNDRV_PCM_IOCTL_HWSYNC _IO('A', 0x22)
+#define __SNDRV_PCM_IOCTL_SYNC_PTR _IOWR('A', 0x23, struct __snd_pcm_sync_ptr)
+#define __SNDRV_PCM_IOCTL_SYNC_PTR64 _IOWR('A', 0x23, struct __snd_pcm_sync_ptr64)
#define SNDRV_PCM_IOCTL_SYNC_PTR _IOWR('A', 0x23, struct snd_pcm_sync_ptr)
#define SNDRV_PCM_IOCTL_STATUS_EXT _IOWR('A', 0x24, struct snd_pcm_status)
#define SNDRV_PCM_IOCTL_CHANNEL_INFO _IOR('A', 0x32, struct snd_pcm_channel_info)
diff --git a/sound/core/pcm_compat.c b/sound/core/pcm_compat.c
index 6a2e5ea145e6..967c689fb8da 100644
--- a/sound/core/pcm_compat.c
+++ b/sound/core/pcm_compat.c
@@ -178,8 +178,6 @@ struct compat_snd_pcm_status64 {
unsigned char reserved[52-4*sizeof(s64)];
} __packed;
-#define put_timespec(src, dst) copy_to_user(dst, src, sizeof(*dst))
-
static int snd_pcm_status_user_compat64(struct snd_pcm_substream *substream,
struct compat_snd_pcm_status64 __user *src,
bool ext)
@@ -382,10 +380,12 @@ struct snd_pcm_mmap_status_x32 {
s32 pad1;
u32 hw_ptr;
u32 pad2; /* alignment */
- struct timespec tstamp;
+ s64 tstamp_sec;
+ s64 tstamp_nsec;
s32 suspended_state;
s32 pad3;
- struct timespec audio_tstamp;
+ s64 audio_tstamp_sec;
+ s64 audio_tstamp_nsec;
} __packed;
struct snd_pcm_mmap_control_x32 {
@@ -453,9 +453,11 @@ static int snd_pcm_ioctl_sync_ptr_x32(struct snd_pcm_substream *substream,
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.tstamp.tv_sec, &src->s.status.tstamp_sec) ||
+ put_user(sstatus.tstamp.tv_nsec, &src->s.status.tstamp_nsec) ||
put_user(sstatus.suspended_state, &src->s.status.suspended_state) ||
- put_timespec(&sstatus.audio_tstamp, &src->s.status.audio_tstamp) ||
+ put_user(sstatus.audio_tstamp.tv_sec, &src->s.status.audio_tstamp_sec) ||
+ put_user(sstatus.audio_tstamp.tv_nsec, &src->s.status.audio_tstamp_nsec) ||
put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) ||
put_user(scontrol.avail_min, &src->c.control.avail_min))
return -EFAULT;
@@ -480,7 +482,6 @@ 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_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 CONFIG_X86_X32
@@ -504,8 +505,8 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
/*
* When PCM is used on 32bit mode, we need to disable
- * mmap of PCM status/control records because of the size
- * incompatibility.
+ * mmap of the old PCM status/control records because
+ * of the size incompatibility.
*/
pcm_file->no_compat_mmap = 1;
@@ -527,6 +528,13 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
case SNDRV_PCM_IOCTL_XRUN:
case SNDRV_PCM_IOCTL_LINK:
case SNDRV_PCM_IOCTL_UNLINK:
+ case __SNDRV_PCM_IOCTL_SYNC_PTR32:
+ return snd_pcm_common_ioctl(file, substream, cmd, argp);
+ case __SNDRV_PCM_IOCTL_SYNC_PTR64:
+#ifdef CONFIG_X86_X32
+ if (in_x32_syscall())
+ return snd_pcm_ioctl_sync_ptr_x32(substream, argp);
+#endif /* CONFIG_X86_X32 */
return snd_pcm_common_ioctl(file, substream, cmd, argp);
case SNDRV_PCM_IOCTL_HW_REFINE32:
return snd_pcm_ioctl_hw_params_compat(substream, 1, argp);
@@ -538,8 +546,6 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
return snd_pcm_status_user32(substream, argp, false);
case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT32:
return snd_pcm_status_user32(substream, argp, true);
- case SNDRV_PCM_IOCTL_SYNC_PTR32:
- return snd_pcm_ioctl_sync_ptr_compat(substream, argp);
case SNDRV_PCM_IOCTL_CHANNEL_INFO32:
return snd_pcm_ioctl_channel_info_compat(substream, argp);
case SNDRV_PCM_IOCTL_WRITEI_FRAMES32:
@@ -561,8 +567,6 @@ static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned l
case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64:
return snd_pcm_status_user_compat64(substream, argp, true);
#ifdef CONFIG_X86_X32
- case SNDRV_PCM_IOCTL_SYNC_PTR_X32:
- return snd_pcm_ioctl_sync_ptr_x32(substream, argp);
case SNDRV_PCM_IOCTL_CHANNEL_INFO_X32:
return snd_pcm_ioctl_channel_info_x32(substream, argp);
#endif /* CONFIG_X86_X32 */
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index ea5518d44e66..0271802bfba9 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -148,7 +148,8 @@ void __snd_pcm_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)) {
@@ -238,9 +239,10 @@ static void update_audio_tstamp(struct snd_pcm_substream *substream,
if (runtime->status->audio_tstamp.tv_sec != audio_tstamp->tv_sec ||
runtime->status->audio_tstamp.tv_nsec != audio_tstamp->tv_nsec) {
- 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;
}
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index ba0636a2b437..5a1245509eac 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -2889,14 +2889,15 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
return 0;
}
-#ifdef CONFIG_COMPAT
struct snd_pcm_mmap_status32 {
s32 state;
s32 pad1;
u32 hw_ptr;
- struct compat_timespec tstamp;
+ s32 tstamp_sec;
+ s32 tstamp_nsec;
s32 suspended_state;
- struct compat_timespec audio_tstamp;
+ s32 audio_tstamp_sec;
+ s32 audio_tstamp_nsec;
} __attribute__((packed));
struct snd_pcm_mmap_control32 {
@@ -2976,17 +2977,18 @@ static int snd_pcm_ioctl_sync_ptr_compat(struct snd_pcm_substream *substream,
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.tstamp.tv_sec, &src->s.status.tstamp_sec) ||
+ put_user(sstatus.tstamp.tv_nsec, &src->s.status.tstamp_nsec) ||
put_user(sstatus.suspended_state, &src->s.status.suspended_state) ||
- compat_put_timespec(&sstatus.audio_tstamp,
- &src->s.status.audio_tstamp) ||
+ put_user(sstatus.audio_tstamp.tv_sec, &src->s.status.audio_tstamp_sec) ||
+ put_user(sstatus.audio_tstamp.tv_nsec, &src->s.status.audio_tstamp_nsec) ||
put_user(scontrol.appl_ptr, &src->c.control.appl_ptr) ||
put_user(scontrol.avail_min, &src->c.control.avail_min))
return -EFAULT;
return 0;
}
-#endif
+#define __SNDRV_PCM_IOCTL_SYNC_PTR32 _IOWR('A', 0x23, struct snd_pcm_sync_ptr32)
static int snd_pcm_tstamp(struct snd_pcm_substream *substream, int __user *_arg)
{
@@ -3156,7 +3158,9 @@ static int snd_pcm_common_ioctl(struct file *file,
return -EFAULT;
return 0;
}
- case SNDRV_PCM_IOCTL_SYNC_PTR:
+ case __SNDRV_PCM_IOCTL_SYNC_PTR32:
+ return snd_pcm_ioctl_sync_ptr_compat(substream, arg);
+ case __SNDRV_PCM_IOCTL_SYNC_PTR64:
return snd_pcm_sync_ptr(substream, arg);
#ifdef CONFIG_SND_SUPPORT_OLD_API
case SNDRV_PCM_IOCTL_HW_REFINE_OLD:
@@ -3494,8 +3498,6 @@ static int snd_pcm_mmap_control(struct snd_pcm_substream *substream, struct file
static bool pcm_status_mmap_allowed(struct snd_pcm_file *pcm_file)
{
- if (pcm_file->no_compat_mmap)
- return false;
/* See pcm_control_mmap_allowed() below.
* Since older alsa-lib requires both status and control mmaps to be
* coupled, we have to disable the status mmap for old alsa-lib, too.
@@ -3720,11 +3722,19 @@ static int snd_pcm_mmap(struct file *file, struct vm_area_struct *area)
offset = area->vm_pgoff << PAGE_SHIFT;
switch (offset) {
- case SNDRV_PCM_MMAP_OFFSET_STATUS:
+ case SNDRV_PCM_MMAP_OFFSET_STATUS_OLD:
+ if (pcm_file->no_compat_mmap || !IS_ENABLED(CONFIG_64BIT))
+ return -ENXIO;
+ /* fallthrough */
+ case SNDRV_PCM_MMAP_OFFSET_STATUS_NEW:
if (!pcm_status_mmap_allowed(pcm_file))
return -ENXIO;
return snd_pcm_mmap_status(substream, file, area);
- case SNDRV_PCM_MMAP_OFFSET_CONTROL:
+ case SNDRV_PCM_MMAP_OFFSET_CONTROL_OLD:
+ if (pcm_file->no_compat_mmap || !IS_ENABLED(CONFIG_64BIT))
+ return -ENXIO;
+ /* fallthrough */
+ case SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW:
if (!pcm_control_mmap_allowed(pcm_file))
return -ENXIO;
return snd_pcm_mmap_control(substream, file, area);
@@ -3884,9 +3894,9 @@ static unsigned long snd_pcm_get_unmapped_area(struct file *file,
unsigned long offset = pgoff << PAGE_SHIFT;
switch (offset) {
- case SNDRV_PCM_MMAP_OFFSET_STATUS:
+ case SNDRV_PCM_MMAP_OFFSET_STATUS_NEW:
return (unsigned long)runtime->status;
- case SNDRV_PCM_MMAP_OFFSET_CONTROL:
+ case SNDRV_PCM_MMAP_OFFSET_CONTROL_NEW:
return (unsigned long)runtime->control;
default:
return (unsigned long)runtime->dma_area + offset;
--
2.20.0
Arnd Bergmann <[email protected]> wrote:
> +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> +typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> +typedef char __pad_after_uframe[0];
> +#endif
> +
> +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> +typedef char __pad_before_uframe[0];
> +typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> +#endif
> +
> +struct __snd_pcm_mmap_status64 {
> + __s32 state; /* RO: state - SNDRV_PCM_STATE_XXXX */
> + __u32 pad1; /* Needed for 64 bit alignment */
> + __pad_before_uframe __pad1;
> + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
> + __pad_after_uframe __pad2;
> + struct __snd_timespec64 tstamp; /* Timestamp */
> + __s32 suspended_state; /* RO: suspended stream state */
> + __u32 pad3; /* Needed for 64 bit alignment */
> + struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */
> +};
> +
> +struct __snd_pcm_mmap_control64 {
> + __pad_before_uframe __pad1;
> + snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */
> + __pad_before_uframe __pad2;
I was looking through this header and happened to notice that this
padding is wrong. I believe it should be __pad_after_uframe here.
I'm not sure of the implications of this typo, but I suspect it
breaks something on 32-bit systems with 64-bit time (regardless of
the endianness, since it changes the offset of avail_min).
> +
> + __pad_before_uframe __pad3;
> + snd_pcm_uframes_t avail_min; /* RW: min available frames for wakeup */
> + __pad_after_uframe __pad4;
> +};
On Wed, 06 Oct 2021 19:49:17 +0200,
Michael Forney wrote:
>
> Arnd Bergmann <[email protected]> wrote:
> > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> > +typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > +typedef char __pad_after_uframe[0];
> > +#endif
> > +
> > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> > +typedef char __pad_before_uframe[0];
> > +typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > +#endif
> > +
> > +struct __snd_pcm_mmap_status64 {
> > + __s32 state; /* RO: state - SNDRV_PCM_STATE_XXXX */
> > + __u32 pad1; /* Needed for 64 bit alignment */
> > + __pad_before_uframe __pad1;
> > + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
> > + __pad_after_uframe __pad2;
> > + struct __snd_timespec64 tstamp; /* Timestamp */
> > + __s32 suspended_state; /* RO: suspended stream state */
> > + __u32 pad3; /* Needed for 64 bit alignment */
> > + struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */
> > +};
> > +
> > +struct __snd_pcm_mmap_control64 {
> > + __pad_before_uframe __pad1;
> > + snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */
> > + __pad_before_uframe __pad2;
>
> I was looking through this header and happened to notice that this
> padding is wrong. I believe it should be __pad_after_uframe here.
>
> I'm not sure of the implications of this typo, but I suspect it
> breaks something on 32-bit systems with 64-bit time (regardless of
> the endianness, since it changes the offset of avail_min).
Right, that's the expected breakage. It seems that the 64bit time on
32bit arch is still rare, so we haven't heard a regression by that, so
far...
In anyway, care to send a formal fix patch?
Thanks for catching this!
Takashi
On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <[email protected]> wrote:
> On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> >
> > Arnd Bergmann <[email protected]> wrote:
> > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> > > +typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > > +typedef char __pad_after_uframe[0];
> > > +#endif
> > > +
> > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> > > +typedef char __pad_before_uframe[0];
> > > +typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > > +#endif
> > > +
> > > +struct __snd_pcm_mmap_status64 {
> > > + __s32 state; /* RO: state - SNDRV_PCM_STATE_XXXX */
> > > + __u32 pad1; /* Needed for 64 bit alignment */
> > > + __pad_before_uframe __pad1;
> > > + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
> > > + __pad_after_uframe __pad2;
> > > + struct __snd_timespec64 tstamp; /* Timestamp */
> > > + __s32 suspended_state; /* RO: suspended stream state */
> > > + __u32 pad3; /* Needed for 64 bit alignment */
> > > + struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */
> > > +};
> > > +
> > > +struct __snd_pcm_mmap_control64 {
> > > + __pad_before_uframe __pad1;
> > > + snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */
> > > + __pad_before_uframe __pad2;
> >
> > I was looking through this header and happened to notice that this
> > padding is wrong. I believe it should be __pad_after_uframe here.
> >
> > I'm not sure of the implications of this typo, but I suspect it
> > breaks something on 32-bit systems with 64-bit time (regardless of
> > the endianness, since it changes the offset of avail_min).
Thanks a lot for the report! Yes, this is definitely broken in some ways.
> Right, that's the expected breakage. It seems that the 64bit time on
> 32bit arch is still rare, so we haven't heard a regression by that, so
> far...
It might actually be worse: on a native 32-bit kernel, both user space
and kernel see the same broken definition with a 64-bit time_t, which
would end up actually making it work as expected. However, in
compat mode, the layout seen on the 32-bit user space is now
different from what the 64-bit kernel has, which would in turn not
work, in both the SNDRV_PCM_IOCTL_SYNC_PTR ioctl and in
the mmap() interface.
Fixing the layout to look like the way we had intended would make
newly compiled applications work in compat mode, but would break
applications built against the old header on new kernels and also
newly built applications on old kernels.
I still hope I missed something and it's not quite that bad, but I
fear the best we can do in this case make the broken interface
the normative one and fixing compat mode to write
mmap_control64->avail_min in the wrong location for
SNDRV_PCM_IOCTL_SYNC_PTR, as well as disabling
the mmap() interface again for compat tasks.
As far as I can tell, the broken interface will always result in
user space seeing a zero value for "avail_min". Can you
make a prediction what that would mean for actual
applications? Will they have no audio output, run into
a crash, or be able to use recover and appear to work normally
here?
Arnd
On Thu, 07 Oct 2021 13:48:44 +0200,
Arnd Bergmann wrote:
>
> On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <[email protected]> wrote:
> > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> > >
> > > Arnd Bergmann <[email protected]> wrote:
> > > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> > > > +typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > > > +typedef char __pad_after_uframe[0];
> > > > +#endif
> > > > +
> > > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> > > > +typedef char __pad_before_uframe[0];
> > > > +typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > > > +#endif
> > > > +
> > > > +struct __snd_pcm_mmap_status64 {
> > > > + __s32 state; /* RO: state - SNDRV_PCM_STATE_XXXX */
> > > > + __u32 pad1; /* Needed for 64 bit alignment */
> > > > + __pad_before_uframe __pad1;
> > > > + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
> > > > + __pad_after_uframe __pad2;
> > > > + struct __snd_timespec64 tstamp; /* Timestamp */
> > > > + __s32 suspended_state; /* RO: suspended stream state */
> > > > + __u32 pad3; /* Needed for 64 bit alignment */
> > > > + struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */
> > > > +};
> > > > +
> > > > +struct __snd_pcm_mmap_control64 {
> > > > + __pad_before_uframe __pad1;
> > > > + snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */
> > > > + __pad_before_uframe __pad2;
> > >
> > > I was looking through this header and happened to notice that this
> > > padding is wrong. I believe it should be __pad_after_uframe here.
> > >
> > > I'm not sure of the implications of this typo, but I suspect it
> > > breaks something on 32-bit systems with 64-bit time (regardless of
> > > the endianness, since it changes the offset of avail_min).
>
> Thanks a lot for the report! Yes, this is definitely broken in some ways.
>
> > Right, that's the expected breakage. It seems that the 64bit time on
> > 32bit arch is still rare, so we haven't heard a regression by that, so
> > far...
>
> It might actually be worse: on a native 32-bit kernel, both user space
> and kernel see the same broken definition with a 64-bit time_t, which
> would end up actually making it work as expected. However, in
> compat mode, the layout seen on the 32-bit user space is now
> different from what the 64-bit kernel has, which would in turn not
> work, in both the SNDRV_PCM_IOCTL_SYNC_PTR ioctl and in
> the mmap() interface.
>
> Fixing the layout to look like the way we had intended would make
> newly compiled applications work in compat mode, but would break
> applications built against the old header on new kernels and also
> newly built applications on old kernels.
>
> I still hope I missed something and it's not quite that bad, but I
> fear the best we can do in this case make the broken interface
> the normative one and fixing compat mode to write
> mmap_control64->avail_min in the wrong location for
> SNDRV_PCM_IOCTL_SYNC_PTR, as well as disabling
> the mmap() interface again for compat tasks.
>
> As far as I can tell, the broken interface will always result in
> user space seeing a zero value for "avail_min". Can you
> make a prediction what that would mean for actual
> applications? Will they have no audio output, run into
> a crash, or be able to use recover and appear to work normally
> here?
No, fortunately it's only about control->avail_min, and fiddling this
value can't break severely (otherwise it'd be a security problem ;)
In the buggy condition, it's always zero, and the kernel treated as if
1, i.e. wake up as soon as data is available, which is OK-ish for most
applications. Apps usually don't care about the wake-up condition so
much. There are subtle difference and may influence on the stability
of stream processing, but the stability usually depends more strongly
on the hardware and software configurations.
That being said, the impact by this bug (from the application behavior
POV) is likely quite small, but the contamination is large; as you
pointed out, it's much larger than I thought.
The definition in uapi/sound/asound.h is a bit cryptic, but IIUC,
__snd_pcm_mmap_control64 is used for 64bit archs, right? If so, the
problem rather hits more widely on 64bit archs silently. Then, the
influence by this bug must be almost negligible, as we've had no bug
report about the behavior change.
We may just fix it in kernel and for new library with hoping that no
one sees the actual problem. Or, we may provide a complete new set of
mmap offsets and ioctl to cover both broken and fixed interfaces...
The decision depends on how perfectly we'd like to address the bug.
As of now, I'm inclined to go for the former, but I'm open for more
opinions.
thanks,
Takashi
On Thu, Oct 7, 2021 at 2:43 PM Takashi Iwai <[email protected]> wrote:
> On Thu, 07 Oct 2021 13:48:44 +0200, Arnd Bergmann wrote:
> > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <[email protected]> wrote:
> > > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> >
> > As far as I can tell, the broken interface will always result in
> > user space seeing a zero value for "avail_min". Can you
> > make a prediction what that would mean for actual
> > applications? Will they have no audio output, run into
> > a crash, or be able to use recover and appear to work normally
> > here?
>
> No, fortunately it's only about control->avail_min, and fiddling this
> value can't break severely (otherwise it'd be a security problem ;)
>
> In the buggy condition, it's always zero, and the kernel treated as if
> 1, i.e. wake up as soon as data is available, which is OK-ish for most
> applications. Apps usually don't care about the wake-up condition so
> much. There are subtle difference and may influence on the stability
> of stream processing, but the stability usually depends more strongly
> on the hardware and software configurations.
>
> That being said, the impact by this bug (from the application behavior
> POV) is likely quite small, but the contamination is large; as you
> pointed out, it's much larger than I thought.
Ok, got it.
> The definition in uapi/sound/asound.h is a bit cryptic, but IIUC,
> __snd_pcm_mmap_control64 is used for 64bit archs, right? If so, the
> problem rather hits more widely on 64bit archs silently. Then, the
> influence by this bug must be almost negligible, as we've had no bug
> report about the behavior change.
While __snd_pcm_mmap_control64 is only used on 32-bit
architectures when 64-bit time_t is used. At the moment, this
means all users of musl-1.2.x libc, but not glibc.
On 64-bit architectures, __snd_pcm_mmap_control and
__snd_pcm_mmap_control64 are meant to be identical,
and this is actually true regardless of the bug, since
__pad_before_uframe and __pad_after_uframe both
end up as zero-length arrays here.
> We may just fix it in kernel and for new library with hoping that no
> one sees the actual problem. Or, we may provide a complete new set of
> mmap offsets and ioctl to cover both broken and fixed interfaces...
> The decision depends on how perfectly we'd like to address the bug.
> As of now, I'm inclined to go for the former, but I'm open for more
> opinions.
Adding the musl list to Cc for additional testers, anyone interested
please see [1] for the original report.
It would be good to hear from musl users that are already using
audio support with 32-bit applications on 64-bit kernels, which
is the case that has the problem today. Have you noticed any
problems with audio support here? If not, we can probably
"fix" the kernel here and make the existing binaries behave
the same way on 32-bit kernels. If there are applications that
don't work in that environment today, I think we need to instead
change the kernel to accept the currently broken format on
both 32-bit and 64-bit kernels, possibly introducing yet another
format that works as originally intended but requires a newly
built kernel.
Arnd
[1] https://lore.kernel.org/all/[email protected]/
On Thu, 07 Oct 2021 15:11:00 +0200,
Arnd Bergmann wrote:
>
> On Thu, Oct 7, 2021 at 2:43 PM Takashi Iwai <[email protected]> wrote:
> > On Thu, 07 Oct 2021 13:48:44 +0200, Arnd Bergmann wrote:
> > > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <[email protected]> wrote:
> > > > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> > >
> > > As far as I can tell, the broken interface will always result in
> > > user space seeing a zero value for "avail_min". Can you
> > > make a prediction what that would mean for actual
> > > applications? Will they have no audio output, run into
> > > a crash, or be able to use recover and appear to work normally
> > > here?
> >
> > No, fortunately it's only about control->avail_min, and fiddling this
> > value can't break severely (otherwise it'd be a security problem ;)
> >
> > In the buggy condition, it's always zero, and the kernel treated as if
> > 1, i.e. wake up as soon as data is available, which is OK-ish for most
> > applications. Apps usually don't care about the wake-up condition so
> > much. There are subtle difference and may influence on the stability
> > of stream processing, but the stability usually depends more strongly
> > on the hardware and software configurations.
> >
> > That being said, the impact by this bug (from the application behavior
> > POV) is likely quite small, but the contamination is large; as you
> > pointed out, it's much larger than I thought.
>
> Ok, got it.
>
> > The definition in uapi/sound/asound.h is a bit cryptic, but IIUC,
> > __snd_pcm_mmap_control64 is used for 64bit archs, right? If so, the
> > problem rather hits more widely on 64bit archs silently. Then, the
> > influence by this bug must be almost negligible, as we've had no bug
> > report about the behavior change.
>
> While __snd_pcm_mmap_control64 is only used on 32-bit
> architectures when 64-bit time_t is used. At the moment, this
> means all users of musl-1.2.x libc, but not glibc.
>
> On 64-bit architectures, __snd_pcm_mmap_control and
> __snd_pcm_mmap_control64 are meant to be identical,
> and this is actually true regardless of the bug, since
> __pad_before_uframe and __pad_after_uframe both
> end up as zero-length arrays here.
>
> > We may just fix it in kernel and for new library with hoping that no
> > one sees the actual problem. Or, we may provide a complete new set of
> > mmap offsets and ioctl to cover both broken and fixed interfaces...
> > The decision depends on how perfectly we'd like to address the bug.
> > As of now, I'm inclined to go for the former, but I'm open for more
> > opinions.
>
> Adding the musl list to Cc for additional testers, anyone interested
> please see [1] for the original report.
>
> It would be good to hear from musl users that are already using
> audio support with 32-bit applications on 64-bit kernels, which
> is the case that has the problem today. Have you noticed any
> problems with audio support here? If not, we can probably
> "fix" the kernel here and make the existing binaries behave
> the same way on 32-bit kernels. If there are applications that
> don't work in that environment today, I think we need to instead
> change the kernel to accept the currently broken format on
> both 32-bit and 64-bit kernels, possibly introducing yet another
> format that works as originally intended but requires a newly
> built kernel.
Thanks!
And now, looking more deeply, I feel more desperate.
This bug makes the expected padding gone on little-endian.
On LE 32bit, the buggy definition is:
char __pad1[0];
u32 appl_ptr;
char __pad2[0]; // this should have been [4]
char __pad3[0];
u32 avail_min;
char __pad4[4];
When an application issues SYNC_PTR64 ioctl to submit appl_ptr and
avail_min updates, 64bit kernel (in compat mode) reads directly as:
u64 appl_ptr;
u64 avail_min;
Hence a bogus appl_ptr would be passed if avail_min != 0.
And usually application sets non-zero avail_min.
That is, the bug must hit more severely if the new API were really
used. It wouldn't crash, but some weird streaming behavior can
happen like noise, jumping or underruns.
(Reading back avail_min=0 to user-space is rather harmless. Ditto for
the case of BE, then at least there is no appl_ptr corruption.)
This made me wonder which way to go:
it's certainly possible to fix the new kernel to treat both buggy and
sane formats (disabling compat mmap and re-define ioctls, having the
code for old APIs). The problem is, however, in the case where the
application needs to run on the older kernel that expects the buggy
format. Then apps would still have to send in the old buggy format --
or maybe better in the older 32bit format that won't hit the bug
above. It makes situation more complicated.
Do we know how widely the __USE_TIME_BITS64 is deployed nowadays?
Takashi
On Thu, Oct 07, 2021 at 05:33:19PM +0200, Takashi Iwai wrote:
> On Thu, 07 Oct 2021 15:11:00 +0200,
> Arnd Bergmann wrote:
> >
> > On Thu, Oct 7, 2021 at 2:43 PM Takashi Iwai <[email protected]> wrote:
> > > On Thu, 07 Oct 2021 13:48:44 +0200, Arnd Bergmann wrote:
> > > > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <[email protected]> wrote:
> > > > > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> > > >
> > > > As far as I can tell, the broken interface will always result in
> > > > user space seeing a zero value for "avail_min". Can you
> > > > make a prediction what that would mean for actual
> > > > applications? Will they have no audio output, run into
> > > > a crash, or be able to use recover and appear to work normally
> > > > here?
> > >
> > > No, fortunately it's only about control->avail_min, and fiddling this
> > > value can't break severely (otherwise it'd be a security problem ;)
> > >
> > > In the buggy condition, it's always zero, and the kernel treated as if
> > > 1, i.e. wake up as soon as data is available, which is OK-ish for most
> > > applications. Apps usually don't care about the wake-up condition so
> > > much. There are subtle difference and may influence on the stability
> > > of stream processing, but the stability usually depends more strongly
> > > on the hardware and software configurations.
> > >
> > > That being said, the impact by this bug (from the application behavior
> > > POV) is likely quite small, but the contamination is large; as you
> > > pointed out, it's much larger than I thought.
> >
> > Ok, got it.
> >
> > > The definition in uapi/sound/asound.h is a bit cryptic, but IIUC,
> > > __snd_pcm_mmap_control64 is used for 64bit archs, right? If so, the
> > > problem rather hits more widely on 64bit archs silently. Then, the
> > > influence by this bug must be almost negligible, as we've had no bug
> > > report about the behavior change.
> >
> > While __snd_pcm_mmap_control64 is only used on 32-bit
> > architectures when 64-bit time_t is used. At the moment, this
> > means all users of musl-1.2.x libc, but not glibc.
> >
> > On 64-bit architectures, __snd_pcm_mmap_control and
> > __snd_pcm_mmap_control64 are meant to be identical,
> > and this is actually true regardless of the bug, since
> > __pad_before_uframe and __pad_after_uframe both
> > end up as zero-length arrays here.
> >
> > > We may just fix it in kernel and for new library with hoping that no
> > > one sees the actual problem. Or, we may provide a complete new set of
> > > mmap offsets and ioctl to cover both broken and fixed interfaces...
> > > The decision depends on how perfectly we'd like to address the bug.
> > > As of now, I'm inclined to go for the former, but I'm open for more
> > > opinions.
> >
> > Adding the musl list to Cc for additional testers, anyone interested
> > please see [1] for the original report.
> >
> > It would be good to hear from musl users that are already using
> > audio support with 32-bit applications on 64-bit kernels, which
> > is the case that has the problem today. Have you noticed any
> > problems with audio support here? If not, we can probably
> > "fix" the kernel here and make the existing binaries behave
> > the same way on 32-bit kernels. If there are applications that
> > don't work in that environment today, I think we need to instead
> > change the kernel to accept the currently broken format on
> > both 32-bit and 64-bit kernels, possibly introducing yet another
> > format that works as originally intended but requires a newly
> > built kernel.
>
> Thanks!
>
> And now, looking more deeply, I feel more desperate.
>
> This bug makes the expected padding gone on little-endian.
> On LE 32bit, the buggy definition is:
>
> char __pad1[0];
> u32 appl_ptr;
> char __pad2[0]; // this should have been [4]
> char __pad3[0];
> u32 avail_min;
> char __pad4[4];
>
> When an application issues SYNC_PTR64 ioctl to submit appl_ptr and
> avail_min updates, 64bit kernel (in compat mode) reads directly as:
>
> u64 appl_ptr;
> u64 avail_min;
>
> Hence a bogus appl_ptr would be passed if avail_min != 0.
> And usually application sets non-zero avail_min.
> That is, the bug must hit more severely if the new API were really
> used. It wouldn't crash, but some weird streaming behavior can
> happen like noise, jumping or underruns.
>
> (Reading back avail_min=0 to user-space is rather harmless. Ditto for
> the case of BE, then at least there is no appl_ptr corruption.)
>
> This made me wonder which way to go:
> it's certainly possible to fix the new kernel to treat both buggy and
> sane formats (disabling compat mmap and re-define ioctls, having the
> code for old APIs). The problem is, however, in the case where the
> application needs to run on the older kernel that expects the buggy
> format. Then apps would still have to send in the old buggy format --
> or maybe better in the older 32bit format that won't hit the bug
> above. It makes situation more complicated.
Can't an ioctl number just be redefined so that, on old kernels with
the buggy one, newly built applications get told that mmap is not
available and use the unaffected non-mmap fallback?
> Do we know how widely the __USE_TIME_BITS64 is deployed nowadays?
Anyone using musl on 32-bit archs who's not >=2 years behind current.
Rich
On Thu, 07 Oct 2021 14:43:53 +0200,
Takashi Iwai wrote:
>
> On Thu, 07 Oct 2021 13:48:44 +0200,
> Arnd Bergmann wrote:
> >
> > On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <[email protected]> wrote:
> > > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> > > >
> > > > Arnd Bergmann <[email protected]> wrote:
> > > > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __BIG_ENDIAN : defined(__BIG_ENDIAN)
> > > > > +typedef char __pad_before_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > > > > +typedef char __pad_after_uframe[0];
> > > > > +#endif
> > > > > +
> > > > > +#if defined(__BYTE_ORDER) ? __BYTE_ORDER == __LITTLE_ENDIAN : defined(__LITTLE_ENDIAN)
> > > > > +typedef char __pad_before_uframe[0];
> > > > > +typedef char __pad_after_uframe[sizeof(__u64) - sizeof(snd_pcm_uframes_t)];
> > > > > +#endif
> > > > > +
> > > > > +struct __snd_pcm_mmap_status64 {
> > > > > + __s32 state; /* RO: state - SNDRV_PCM_STATE_XXXX */
> > > > > + __u32 pad1; /* Needed for 64 bit alignment */
> > > > > + __pad_before_uframe __pad1;
> > > > > + snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
> > > > > + __pad_after_uframe __pad2;
> > > > > + struct __snd_timespec64 tstamp; /* Timestamp */
> > > > > + __s32 suspended_state; /* RO: suspended stream state */
> > > > > + __u32 pad3; /* Needed for 64 bit alignment */
> > > > > + struct __snd_timespec64 audio_tstamp; /* sample counter or wall clock */
> > > > > +};
> > > > > +
> > > > > +struct __snd_pcm_mmap_control64 {
> > > > > + __pad_before_uframe __pad1;
> > > > > + snd_pcm_uframes_t appl_ptr; /* RW: appl ptr (0...boundary-1) */
> > > > > + __pad_before_uframe __pad2;
> > > >
> > > > I was looking through this header and happened to notice that this
> > > > padding is wrong. I believe it should be __pad_after_uframe here.
> > > >
> > > > I'm not sure of the implications of this typo, but I suspect it
> > > > breaks something on 32-bit systems with 64-bit time (regardless of
> > > > the endianness, since it changes the offset of avail_min).
> >
> > Thanks a lot for the report! Yes, this is definitely broken in some ways.
> >
> > > Right, that's the expected breakage. It seems that the 64bit time on
> > > 32bit arch is still rare, so we haven't heard a regression by that, so
> > > far...
> >
> > It might actually be worse: on a native 32-bit kernel, both user space
> > and kernel see the same broken definition with a 64-bit time_t, which
> > would end up actually making it work as expected. However, in
> > compat mode, the layout seen on the 32-bit user space is now
> > different from what the 64-bit kernel has, which would in turn not
> > work, in both the SNDRV_PCM_IOCTL_SYNC_PTR ioctl and in
> > the mmap() interface.
> >
> > Fixing the layout to look like the way we had intended would make
> > newly compiled applications work in compat mode, but would break
> > applications built against the old header on new kernels and also
> > newly built applications on old kernels.
> >
> > I still hope I missed something and it's not quite that bad, but I
> > fear the best we can do in this case make the broken interface
> > the normative one and fixing compat mode to write
> > mmap_control64->avail_min in the wrong location for
> > SNDRV_PCM_IOCTL_SYNC_PTR, as well as disabling
> > the mmap() interface again for compat tasks.
> >
> > As far as I can tell, the broken interface will always result in
> > user space seeing a zero value for "avail_min". Can you
> > make a prediction what that would mean for actual
> > applications? Will they have no audio output, run into
> > a crash, or be able to use recover and appear to work normally
> > here?
>
> No, fortunately it's only about control->avail_min, and fiddling this
> value can't break severely (otherwise it'd be a security problem ;)
>
> In the buggy condition, it's always zero, and the kernel treated as if
> 1, i.e. wake up as soon as data is available, which is OK-ish for most
> applications. Apps usually don't care about the wake-up condition so
> much. There are subtle difference and may influence on the stability
> of stream processing, but the stability usually depends more strongly
> on the hardware and software configurations.
>
> That being said, the impact by this bug (from the application behavior
> POV) is likely quite small, but the contamination is large; as you
> pointed out, it's much larger than I thought.
>
> The definition in uapi/sound/asound.h is a bit cryptic, but IIUC,
> __snd_pcm_mmap_control64 is used for 64bit archs, right? If so, the
> problem rather hits more widely on 64bit archs silently. Then, the
> influence by this bug must be almost negligible, as we've had no bug
> report about the behavior change.
Erm, scratch this part: on 64bit arch, both __pad_before_uframe and
__pad_after_uframe is 0-size, so the bug doesn't hit. It's only about
32bit arch.
> We may just fix it in kernel and for new library with hoping that no
> one sees the actual problem. Or, we may provide a complete new set of
> mmap offsets and ioctl to cover both broken and fixed interfaces...
> The decision depends on how perfectly we'd like to address the bug.
> As of now, I'm inclined to go for the former, but I'm open for more
> opinions.
Takashi