From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 44E01C433F5 for ; Mon, 18 Oct 2021 14:58:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 318DC6103D for ; Mon, 18 Oct 2021 14:58:20 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233224AbhJRPAa (ORCPT ); Mon, 18 Oct 2021 11:00:30 -0400 Received: from smtp-out1.suse.de ([195.135.220.28]:59618 "EHLO smtp-out1.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233204AbhJRPAP (ORCPT ); Mon, 18 Oct 2021 11:00:15 -0400 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 6A16521A7A; Mon, 18 Oct 2021 14:58:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1634569083; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Fdl36cozsDtS8yyvRlbTRlUn0BaFR6UEoCc2BpTjRK0=; b=eNltiTMgnttXseUi0jZ0zeStJba7BHQCjtwhm9hSC4N7CgF2/7BooiOTJ3qUFRE7X0tLSo cj6wByJhUiKJnbPxtmcTcKK1IaOZA1Kr2br1CBEjnksujY7yRXQht8G3bBsQbL0pr3L7lr oxooz74ggNNWGfH5tiWppR3CFZPmZO8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1634569083; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Fdl36cozsDtS8yyvRlbTRlUn0BaFR6UEoCc2BpTjRK0=; b=gArgfIcqudVnEjDEOagd39rUEMRA+AdwNF0jVnvkoGsfJjvj5+KSwYzx+AsO7HMod02Uw0 r1lc2Hgwtw0eQRDg== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 2D190A3BEB; Mon, 18 Oct 2021 14:58:03 +0000 (UTC) Date: Mon, 18 Oct 2021 16:58:03 +0200 Message-ID: From: Takashi Iwai To: Rich Felker 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 Subject: Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control In-Reply-To: <20211018144259.GK7074@brightrain.aerifal.cx> References: <20211007160634.GB7074@brightrain.aerifal.cx> <20211007165158.GC7074@brightrain.aerifal.cx> <20211008120736.GF7074@brightrain.aerifal.cx> <20211018144259.GK7074@brightrain.aerifal.cx> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 18 Oct 2021 16:43:00 +0200, Rich Felker wrote: > > 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? Yes, as it was already set in stone, we accept the broken definition as is. > 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. Hm, musl translator contains the own definition of ioctl? If so, we may reconsider about renumbering ioctls altogether. Suppose musl having a fallback to the old ioctl, the possible breakage by old kernels (that don't support renewed ioctls) would be minimal, right? Takashi From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 46A79C433F5 for ; Mon, 18 Oct 2021 14:59:08 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id D23C661074 for ; Mon, 18 Oct 2021 14:59:06 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D23C661074 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 55926165E; Mon, 18 Oct 2021 16:58:14 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 55926165E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1634569144; bh=cbFy3d59PGwkMJsN4pr+xxTx1tIqP/1FBvHkZMVLEvc=; h=Date:From:To:Subject:In-Reply-To:References:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=KgEFnr8ZQUrxRdhZfHXoUdV3jW4Xbx6w7xFgieY9zSPATIOFk+R4YJLui6LhqjkMB dUkO9G3VOto9q4ng7JV/hIKNmfSUlZJYUqC5vfcdELiKF+61a3AHsr+ZsnmvmbGJhl 2O2V6J2Y9nKRHbTEXG4Gtbtz785kXeElYffAL/ig= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 87045F80217; Mon, 18 Oct 2021 16:58:13 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 9CED3F80224; Mon, 18 Oct 2021 16:58:11 +0200 (CEST) Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 62606F80111 for ; Mon, 18 Oct 2021 16:58:04 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 62606F80111 Authentication-Results: alsa1.perex.cz; dkim=pass (1024-bit key) header.d=suse.de header.i=@suse.de header.b="eNltiTMg"; dkim=permerror (0-bit key) header.d=suse.de header.i=@suse.de header.b="gArgfIcq" Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 6A16521A7A; Mon, 18 Oct 2021 14:58:03 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1634569083; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Fdl36cozsDtS8yyvRlbTRlUn0BaFR6UEoCc2BpTjRK0=; b=eNltiTMgnttXseUi0jZ0zeStJba7BHQCjtwhm9hSC4N7CgF2/7BooiOTJ3qUFRE7X0tLSo cj6wByJhUiKJnbPxtmcTcKK1IaOZA1Kr2br1CBEjnksujY7yRXQht8G3bBsQbL0pr3L7lr oxooz74ggNNWGfH5tiWppR3CFZPmZO8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1634569083; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=Fdl36cozsDtS8yyvRlbTRlUn0BaFR6UEoCc2BpTjRK0=; b=gArgfIcqudVnEjDEOagd39rUEMRA+AdwNF0jVnvkoGsfJjvj5+KSwYzx+AsO7HMod02Uw0 r1lc2Hgwtw0eQRDg== Received: from alsa1.suse.de (alsa1.suse.de [10.160.4.42]) by relay2.suse.de (Postfix) with ESMTP id 2D190A3BEB; Mon, 18 Oct 2021 14:58:03 +0000 (UTC) Date: Mon, 18 Oct 2021 16:58:03 +0200 Message-ID: From: Takashi Iwai To: Rich Felker Subject: Re: [musl] Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control In-Reply-To: <20211018144259.GK7074@brightrain.aerifal.cx> References: <20211007160634.GB7074@brightrain.aerifal.cx> <20211007165158.GC7074@brightrain.aerifal.cx> <20211008120736.GF7074@brightrain.aerifal.cx> <20211018144259.GK7074@brightrain.aerifal.cx> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Cc: ALSA Development Mailing List , Arnd Bergmann , Baolin Wang , y2038 Mailman List , Linux Kernel Mailing List , musl@lists.openwall.com, Takashi Iwai , Michael Forney , Mark Brown , Baolin Wang X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Mon, 18 Oct 2021 16:43:00 +0200, Rich Felker wrote: > > 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? Yes, as it was already set in stone, we accept the broken definition as is. > 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. Hm, musl translator contains the own definition of ioctl? If so, we may reconsider about renumbering ioctls altogether. Suppose musl having a fallback to the old ioctl, the possible breakage by old kernels (that don't support renewed ioctls) would be minimal, right? Takashi