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 <ti...@suse.de> wrote: > > > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote: > > > > > > > > Arnd Bergmann <a...@arndb.de> 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 _______________________________________________ Y2038 mailing list Y2038@lists.linaro.org https://lists.linaro.org/mailman/listinfo/y2038