Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control

[Arnd Bergmann]

On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai <tiwai@xxxxxxx> wrote:
> On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote:
> >
> > Arnd Bergmann <arnd@xxxxxxxx> 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