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 3E61BC433EF for ; Thu, 7 Oct 2021 11:49:08 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1DD3C610CC for ; Thu, 7 Oct 2021 11:49:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241113AbhJGLvA (ORCPT ); Thu, 7 Oct 2021 07:51:00 -0400 Received: from mout.kundenserver.de ([212.227.126.130]:44435 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230091AbhJGLu7 (ORCPT ); Thu, 7 Oct 2021 07:50:59 -0400 Received: from mail-wr1-f44.google.com ([209.85.221.44]) by mrelayeu.kundenserver.de (mreue009 [213.165.67.97]) with ESMTPSA (Nemesis) id 1N17gw-1mvukh11OU-012Ztg for ; Thu, 07 Oct 2021 13:49:01 +0200 Received: by mail-wr1-f44.google.com with SMTP id e12so18217612wra.4 for ; Thu, 07 Oct 2021 04:49:01 -0700 (PDT) X-Gm-Message-State: AOAM531zS13Pb2yvztLJcqq0X4nlIWveZO/5zgJ58yorqpeYNtQmevbA WugNO0DGeF0+ixcbfMUhrCwk+RM0RadyRCCL1J0= X-Google-Smtp-Source: ABdhPJwBqe32vcpMnUIo0FX0+1s94CEa3VaWPex61C+7oJ6HCqGwk1cCqJIBbEf/G+UjX5v7CY40bq/ji3zFDNEaLaM= X-Received: by 2002:a1c:2358:: with SMTP id j85mr15888675wmj.1.1633607340709; Thu, 07 Oct 2021 04:49:00 -0700 (PDT) MIME-Version: 1.0 References: <20191211212025.1981822-1-arnd@arndb.de> <20191211212025.1981822-9-arnd@arndb.de> <29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org> In-Reply-To: From: Arnd Bergmann Date: Thu, 7 Oct 2021 13:48:44 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control To: Takashi Iwai Cc: Michael Forney , Arnd Bergmann , ALSA Development Mailing List , Takashi Iwai , Baolin Wang , y2038 Mailman List , Linux Kernel Mailing List , Mark Brown , Baolin Wang Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:8MDCtwMRNx+9CH2pBL2nD+57DWuRhB410hDVp2fiXnIbcJ08YP8 qP/humq/+1H+dxMMjKQrbv92zdJl9sFyUAWoyvYMSC2Ibzc/yu+YzplzghP7DA2nfttYlip DMv9kP83cw8uU4zqT5dPmKgO/8zpg+e1lLOReeP9VDeGSEe347M4NSfKUxTOa//x5E9H8C+ f9m+WqpvZX+/328Jx9NDQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:bcQ0A5LTM7M=:7u1AzP0ocrxvT6TO+Rt8uV 8M0CEEyNvP3BiAThtMTI3hedIjiTnRWEi5MLOnN9HxrCkP+Cad+4kK8dJjUAlUMWr1X7kHZks bKhkouu7aMJXYbQB04Yvhuu1zWejrUidGsDGAPzQF8fF9+y8CinvQZbDBBj+QlOu+Gg/2CeXT G2viZZvXCwuXCKdGu94WOBzvjTqNNsa4xaJNZWfAMWsCqX4NLS7bBaY7xvALg5XXg4NWryBVp lUORLA4D3kc+kIS+A360TTGwbW92ARYUsbdvHOO4ytiiwjSpw1NT0qibzeXukCc9ZkFf0NitQ 0Z4CfRN0G8WVRzwg+GtN9eDloqzSc3EeaxOKgTwDBeMQsRQOU2YBp/V0u1YYb2dnoSgWCyncE lecKqmXCgdHNQi0+5kN/HX+2+OXmTfPusAxB4CSwx/UagCe40d85tWzMXn275hesoTbD4tlKQ fehQq84EZk6PKsKFknbYJ5b4G9mTwt0d/w5cyRsMLsWPRjmYTKw9quD97D6akFLP2Ssdk5d4t 3DFrumC3E/WEbfpthKhsCGx5eu37NaXzpASs5Bv3jBCejKBnQOkXuWT4T5zTNothqxoiLAXQE kkac5yVFdGYWdzQpIicG8kOcpqZvJhLK1oF12Y/QKrk3fiEgM5TK9mQp2LoOus9Ky4DrwIJ3B 22ycM8AulViSU6bnFoUst5a4c9msEhK6g9Z3dojGI4FjvseoUqRxsLk8me5Xp7LXRJS3HrWc4 HMRB5OHURdfh17qvgvAKpFNxR8pvALPgEy07PZydXKzBcojhZl/sKUcpSJTtQWhleoLuXPRLy FkXdTRfImp5mfLaPTIPYN1bEkpEgVsB5zCGzH/RDu+rtMrFFEkxAEXhdK9R/rPQHKRUGP3wED FL99qEQSwvfONG5Jz9Qg== Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai wrote: > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote: > > > > Arnd Bergmann 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 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 7F6C1C433EF for ; Thu, 7 Oct 2021 11:50:05 +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 C84D961175 for ; Thu, 7 Oct 2021 11:50:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org C84D961175 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arndb.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 984E415E0; Thu, 7 Oct 2021 13:49:11 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 984E415E0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1633607401; bh=yzWPrAUyStofkzAwGJ+4VZNn9Z7qEouLTwfWOeGYXtA=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=E4nybpzY6DyP31u/kxxhAtVVkWHgTWQW7ulFniSBxSC+05AdCUvxwdnv10qwNF9bI ZgtkTzQp2y0GFyS0K6YsLQwz1pfiqpmHIhMWFN+khg5VGUQd0G5R/DSqy9YnAggTdy eLJouaPrs5BL7fWznAoXsU7enWZfYv40Y+UXztyQ= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 112DDF80259; Thu, 7 Oct 2021 13:49:11 +0200 (CEST) Received: by alsa1.perex.cz (Postfix, from userid 50401) id DE592F8027D; Thu, 7 Oct 2021 13:49:09 +0200 (CEST) Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.126.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 31621F80130 for ; Thu, 7 Oct 2021 13:49:02 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 31621F80130 Received: from mail-wr1-f54.google.com ([209.85.221.54]) by mrelayeu.kundenserver.de (mreue012 [213.165.67.97]) with ESMTPSA (Nemesis) id 1MRVy9-1mBXKN2RrZ-00NUat for ; Thu, 07 Oct 2021 13:49:02 +0200 Received: by mail-wr1-f54.google.com with SMTP id r7so18162042wrc.10 for ; Thu, 07 Oct 2021 04:49:02 -0700 (PDT) X-Gm-Message-State: AOAM532KTmydtxxkybE4jIcHj8tArn5QwLdeBFNBCR465t5hGSaDl5He f6tqAoqkZSAp++IcEDUjFHWC/PMKwn/twVwUY5c= X-Google-Smtp-Source: ABdhPJwBqe32vcpMnUIo0FX0+1s94CEa3VaWPex61C+7oJ6HCqGwk1cCqJIBbEf/G+UjX5v7CY40bq/ji3zFDNEaLaM= X-Received: by 2002:a1c:2358:: with SMTP id j85mr15888675wmj.1.1633607340709; Thu, 07 Oct 2021 04:49:00 -0700 (PDT) MIME-Version: 1.0 References: <20191211212025.1981822-1-arnd@arndb.de> <20191211212025.1981822-9-arnd@arndb.de> <29QBMJU8DE71E.2YZSH8IHT5HMH@mforney.org> In-Reply-To: From: Arnd Bergmann Date: Thu, 7 Oct 2021 13:48:44 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [alsa-devel] [PATCH v7 8/9] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control To: Takashi Iwai Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:m/xQRbfKirZEJs1ki8YTSD+vkf93UCaOwRKmp121yZH6vcEmjKt oTduMZvm3Gm3/Oxp7GUtxSLNaq8wnQWO4rYaDMANVZyiOOxuxuydt8jGL4yNEVAGfisycMS F/ufmRAldCMDf4FXbmhiNmrBpKJd133+kRiCbifrg5y715piz1zQBRNuyDcmRKdtNuvAFZi dQVuqfnBSDu9viKJHJbkw== X-UI-Out-Filterresults: notjunk:1;V03:K0:bm8dRng4m6o=:y0WU/ZIgpt5Aa7P8Hsy9pf d+RQvtGfCsS55zqyPj9HO7eIJuPqPwu6tObW53pL8pvBOyTmKpSTZ0KshvwY3Asr9nXJ2zJIg XvkRUWkUG7rzU0u+VnqwZ2CD7oozSNdr7krxOHnH6//ywSwSHU7wOkj9nHXWdov1regdv2mTY 633C7TBY+yPQMz++FGsJKHJRyMkuXZR1iKyDq/mtfubdftnUngcZDl6jo6N6X2/a9+1gogE6a 1H7Ut+JHKLHNHghYQ8/pN9vD2LScXwqJmaFBe5KQovv+QXLrmUnSWnaHPZNYwA473yHgTEBVg 3NrgLzjr7BIAjzqxvo65tKCurUR5rWyXXh0hCoGxUKFgtCoNzqdW/SQF8vQC2K1InOO0Lyg4I cfQyynOhoZ/BoYrse1/7OuXztAgZctfsSfY9R81NzCAmS4srL4U0MfRyLCy7l3iNxiG4I9QFh U+/LtoHdAZ1o4tVCkPkX4km2P9fcuqTyBLOwhTFDMidIaUaqfzF3xM7E7+Mr9ZPAnEFnyX7bw SpOhJVresoKhhzlLJYB0pJ9hNe1Em5z6A768lXatvFxKKCXFSQ0OcUc0LMDQTYQBynNsPfabu xa7W6itQNBJVpv488BVV4T9n1BjaZQ76ytYP6bhOxPvpq9KhbBg9ci88M5G3hrOBAegkW1u7h H3nkyMidfxgiWjrbDVppxIu0smX36HeD/w0WK2poNJBMLFc+T4FLaoH429cHOVB2P57s3hwj0 uKLykW+bw3rkVFjFVlDKKJUqm7UlWLGiZU5VahM9/J1CgXNK66JO7Ax74+XOdFGnnHAGA/C4F hjYLmgft01WcoBYNIbek24+hFNBmzGl3ZtFWW+84K3B2u2plkGm3CeHstzgnbI8EfAZWG1NSk Ucjh5WC1YvqVCVPgvstw== Cc: ALSA Development Mailing List , Arnd Bergmann , Baolin Wang , y2038 Mailman List , Linux Kernel Mailing List , 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 Thu, Oct 7, 2021 at 12:53 PM Takashi Iwai wrote: > On Wed, 06 Oct 2021 19:49:17 +0200, Michael Forney wrote: > > > > Arnd Bergmann 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