From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 32361 invoked from network); 18 Oct 2021 14:43:18 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 18 Oct 2021 14:43:18 -0000 Received: (qmail 22464 invoked by uid 550); 18 Oct 2021 14:43:15 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 22429 invoked from network); 18 Oct 2021 14:43:13 -0000 Date: Mon, 18 Oct 2021 10:43:00 -0400 From: Rich Felker To: Takashi Iwai Cc: Arnd Bergmann , musl@lists.openwall.com, Michael Forney , ALSA Development Mailing List , Takashi Iwai , Baolin Wang , y2038 Mailman List , Linux Kernel Mailing List , Mark Brown , Baolin Wang Message-ID: <20211018144259.GK7074@brightrain.aerifal.cx> References: <20211007160634.GB7074@brightrain.aerifal.cx> <20211007165158.GC7074@brightrain.aerifal.cx> <20211008120736.GF7074@brightrain.aerifal.cx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control On Sun, Oct 10, 2021 at 09:53:38AM +0200, Takashi Iwai wrote: > 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 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. Am I correct in my understanding that the fix of keeping the "broken" definition (and having the 64-bit kernel honor it for 32-bit binaries) has been accepted? Since musl's translation for pre-time64 kernels seems to have been using the "non-broken" definition, I think completing the fix requires a change in musl too. Rich