lkml.org 
[lkml]   [2021]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control
    On Fri, 08 Oct 2021 14:07:39 +0200,
    Rich Felker wrote:
    >
    > On Fri, Oct 08, 2021 at 01:11:34PM +0200, Takashi Iwai wrote:
    > > On Fri, 08 Oct 2021 11:24:39 +0200,
    > > Arnd Bergmann wrote:
    > > >
    > > > On Fri, Oct 8, 2021 at 10:43 AM Takashi Iwai <tiwai@suse.de> wrote:
    > > > > On Thu, 07 Oct 2021 18:51:58 +0200, Rich Felker wrote:
    > > > > > On Thu, Oct 07, 2021 at 06:18:52PM +0200, Takashi Iwai wrote:
    > > > >
    > > > > @@ -557,11 +558,15 @@ struct __snd_pcm_sync_ptr {
    > > > > #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];
    > > > > +typedef char __pad_before_u32[4];
    > > > > +typedef char __pad_after_u32[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)];
    > > > > +typedef char __pad_before_u32[0];
    > > > > +typedef char __pad_after_u32[4];
    > > > > #endif
    > > >
    > > > I think these should remain unchanged, the complex expression was intentionally
    > > > done so the structures are laid out the same way on 64-bit
    > > > architectures, so that
    > > > the kernel can use the __SND_STRUCT_TIME64 path internally on both 32-bit
    > > > and 64-bit architectures.
    > >
    > > That was explicitly defined, but OK, this isn't necessarily defined
    > > here.
    > >
    > > > > @@ -2970,8 +2981,17 @@ static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream,
    > > > > 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;
    > > > > + if (buggy_control) {
    > > > > + if (copy_from_user(&sync_ptr.c.control_api_2_0_15,
    > > > > + &(_sync_ptr->c.control_api_2_0_15),
    > > > > + sizeof(sync_ptr.c.control_api_2_0_15)))
    > > > > + return -EFAULT;
    > > > > + } else {
    > > > > + if (copy_from_user(&sync_ptr.c.control,
    > > > > + &(_sync_ptr->c.control),
    > > > > + sizeof(sync_ptr.c.control)))
    > > > > + return -EFAULT;
    > > > > + }
    > > >
    > > > The problem I see with this is that it might break musl's ability to
    > > > emulate the new
    > > > interface on top of the old (time32) one for linux-4.x and older
    > > > kernels, as the conversion
    > > > function is no longer stateless but has to know the negotiated
    > > > interface version.
    > > >
    > > > It's probably fine as long as we can be sure that the 2.0.16+ API
    > > > version only gets
    > > > negotiated if both the kernel and user sides support it, and musl only emulates
    > > > the 2.0.15 API version from the current kernels.
    > > >
    > > > I've tried to understand this part of musl's convert_ioctl_struct(), but I just
    > > > can't figure out whether it does the conversion based the on the layout that
    > > > is currently used in the kernel, or based on the layout we should have been
    > > > using, and would use with the above fix. Rich, can you help me here?
    > >
    > > So, at this moment, I'm not sure whether we should correct the struct
    > > at all. This will lead to yet more breakage, and basically the struct
    > > itself *works* -- the only bug is in 32bit compat handling in the
    > > kernel (again).
    > >
    > > The below is a revised kernel patch (again untested), just correcting
    > > the behavior of 32bit compat mode. 32bit apps on 32bit kernel work
    > > fine as is, as well as 64bit apps on 64bit kernel.
    >
    > I'm perfectly okay with this if Arnd is! It's probably the least
    > invasive and has the least long-term maintenance cost and fallout on
    > other projects.

    OK, I'll submit a proper patch now, to be included in the next PR for
    5.15-rc. For further fixes, let's think carefully.


    thanks,

    Takashi

    \
     
     \ /
      Last update: 2021-10-10 09:54    [W:4.162 / U:0.216 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site