by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH 0/8] Fix year 2038 issue for sound subsystem

On Wed, Apr 25, 2018 at 9:23 AM, Jaroslav Kysela <[email protected]> wrote:
> Dne 24.4.2018 v 15:37 Arnd Bergmann napsal(a):
>> On Tue, Apr 24, 2018 at 3:29 PM, Jaroslav Kysela <[email protected]> wrote:
>>> Dne 24.4.2018 v 14:06 Baolin Wang napsal(a):
>>>> Since many structures will use timespec type variables to record time stamp
>>>> in uapi/asound.h, which are not year 2038 safe on 32bit system. This patchset
>>>> tries to introduce new structures removing timespec type to compatible native
>>>> mode and compat mode.
>>>>
>>>> Moreover this patchset also converts the internal structrures to use timespec64
>>>> type and related APIs.
>>>
>>> Thanks for your patchset. A few comments:
>>>
>>> It might be more nice to reuse the existing structures and put
>>> timespec64 to the reserved field and duplicate information (with the
>>> 32-bit wrapping for the old fields). It means that we do not need new
>>> ioctls and old libraries will be fine.
>>
>> The current approach is intended to make any user space work
>> without source-level changes, i.e. you can still build an old alsa-lib
>> package against a new glibc as long as you have the latest kernel
>> headers (which the glibc requires for using 64-bit time_t).
>>
>> If we try to extend the structures in a different way, that requires
>> user space changes, and existing source code would silently
>> break on a future glibc.
>> IMHO changing the source-level interface should only be done
>> as a last resort for y2038.
>
> We have almost everything hidden in the alsa-lib code for the
> applications and there is the protocol versioning, so we can detect the
> changes easily and handle the new fields in the library.

I think we are both misunderstanding each other here, let's try
to work out what changes are required in alsa-lib. The idea of
Baolin's patches that you can simply rebuild alsa-lib (or any other
library using the alsa kernel interface, if any exist) against a new
C library and still have working audio.

Unfortunately I had not looked at the alsa-lib source code before,
so I missed the fact that it uses its own copy of the kernel headers,
and that it also defines an incompatible 'timespec' structure itself,
so it seems it's not as easy.

The earlier patches that Baolin posted last November tried to
work with unmodified kernel headers, which would have avoided
breaking alsa-lib, but after discussing with Takashi, Baolin simplified
them to remove the special cases for i386 structure alignment,
and I added back the support for mmap(), which did not work
in the original series, and is impossible without updating at least
the header file.

> As you noted,
> the current code will be fine until 2038 even with my proposed change
> (which is more easy to be implemented in the kernel - less bloat) and
> there are 20 years to update alsa-lib remaining for the 32-bit systems.

I did not claim that it works fine -- in fact the current state is
completely broken once you upgrade your glibc. What I meant is that
there is no *overflow* of time_t as long as user space enforces the
use of monotonic timestamps.

From what I can tell, we have to fix these areas:

1. The kernel-internal interfaces in ALSA should be changed to avoid
using 'struct timespec' and use something else (nanoseconds,
ktime_t, timespec64, ...) so we can completely remove timespec
from the kernel for everything other than compatibility with old
user space.
2. alsa-lib must be changed to no longer define a 'struct timespec'
that is incompatible with the C library, to avoid silently breaking
ABIs between structures used inside alsa-lib and those in
code using alsa-lib with a 64-bit time_t.
3. We must create a change to either alsa-lib (and every other
implementation) or the kernel (including the uapi header) to
avoid breaking SNDRV_TIMER_IOCTL_TREAD, which
currently expects a 'timespec' to be read from a file descriptor
4. on 32-bit x86 and powerpc, we need to do something about
about the mmap() for status and control structures to
avoid breaking. My current patch implements a new binary
layout to avoid most problems, including the issue of compat
mode that never worked.
5. For all other ioctls we have the choice between fixing the
kernel to provide an interface that is compatible with
a future glibc, or to change the uapi header to move away
from timespec to a structure with a different name but
same binary layout and have alsa-lib convert between the
two. I still see Baolin's series as preferred because it matches
what we do for all other subsytems, implementing the
native ioctls using the 64-bit version of timespec that
match what the both kernel and future glibc use, and only
providing the 32-bit interfaces for compatibility with existing
binaries.
6. For completeness, it might be helpful to have alsa-lib
use symbol versioning for each exported API so a
single alsa-lib binary can work with both 32-bit time_t
and 64-bit time_t using applications. Without this, everything
will still keep working as long as you rebuild the entire
distro with 64-bit time_t at once, but it won't allow a gradual
migration of applications.

> Only the binary compatibility for the older binaries should be taken
> into account.
>
> Also, you expect that tv_nsec will be changed to the 's64' type. Do you
> have that confirmed from the glibc developers? From the current
> specification, the tv_nsec type is 'long'. It may cause some binary
> compatibility issues, too.

This is a complex sub-topic. Yes, we've had long discussions with the
glibc developers about it. glibc (and any other C99 compliant C
library) will use 'long' for tv_nsec, but they also have to use padding
after (or before, depending on endianess) tv_nsec to ensure that
the binary layout of timespec matches what we use in a 64-bit kernel.

For timestamps sent from the kernel to user space, we must initialize
the upper 32 bits of tv_nsec even on 32-bit kernels to avoid leaking
kernel stack data in the padding bits, so all interfaces are defined
in terms of __s64/__s64 rather than __s64/long timespec structures,
while both the kernel and user space use __s64/long internally.

Arnd

2018-04-25 11:58:15

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH 0/8] Fix year 2038 issue for sound subsystem

On Wed, Apr 25, 2018 at 01:26:23PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 25, 2018 at 9:23 AM, Jaroslav Kysela <[email protected]> wrote:

> > We have almost everything hidden in the alsa-lib code for the
> > applications and there is the protocol versioning, so we can detect the
> > changes easily and handle the new fields in the library.

> I think we are both misunderstanding each other here, let's try
> to work out what changes are required in alsa-lib. The idea of
> Baolin's patches that you can simply rebuild alsa-lib (or any other
> library using the alsa kernel interface, if any exist) against a new
> C library and still have working audio.

It's probably also worth mentioning that there are userspaces that don't
use alsa-lib at all, at least tinyalsa and salsa (if anyone still uses
salsa).

Attachments:

(No filename) (826.00 B)
signature.asc (499.00 B)
Download all attachments

2018-04-26 03:11:05

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 8/8] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control

Attachments:

(No filename) (2.00 kB)
.config.gz (28.65 kB)
Download all attachments

2018-04-26 06:22:55

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 8/8] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control

Hi Arnd,

I love your patch! Yet something to improve:

[auto build test ERROR on v4.17-rc2]
[cannot apply to sound/for-next asoc/for-next arm-soc/for-next next-20180424]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Baolin-Wang/Fix-year-2038-issue-for-sound-subsystem/20180426-010145
config: x86_64-acpi-redef (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64

All errors (new ones prefixed by >>):

In file included from sound/core/pcm_native.c:3728:0:
sound/core/pcm_compat.c: In function 'snd_pcm_ioctl_compat':
>> sound/core/pcm_compat.c:580:2: error: duplicate case value
case SNDRV_PCM_IOCTL_SYNC_PTR_X32:
^~~~
sound/core/pcm_compat.c:547:2: note: previously used here
case __SNDRV_PCM_IOCTL_SYNC_PTR64:
^~~~

vim +580 sound/core/pcm_compat.c

^1da177e Linus Torvalds 2005-04-16 507
^1da177e Linus Torvalds 2005-04-16 508 static long snd_pcm_ioctl_compat(struct file *file, unsigned int cmd, unsigned long arg)
^1da177e Linus Torvalds 2005-04-16 509 {
877211f5 Takashi Iwai 2005-11-17 510 struct snd_pcm_file *pcm_file;
877211f5 Takashi Iwai 2005-11-17 511 struct snd_pcm_substream *substream;
^1da177e Linus Torvalds 2005-04-16 512 void __user *argp = compat_ptr(arg);
^1da177e Linus Torvalds 2005-04-16 513
^1da177e Linus Torvalds 2005-04-16 514 pcm_file = file->private_data;
^1da177e Linus Torvalds 2005-04-16 515 if (! pcm_file)
^1da177e Linus Torvalds 2005-04-16 516 return -ENOTTY;
^1da177e Linus Torvalds 2005-04-16 517 substream = pcm_file->substream;
^1da177e Linus Torvalds 2005-04-16 518 if (! substream)
^1da177e Linus Torvalds 2005-04-16 519 return -ENOTTY;
^1da177e Linus Torvalds 2005-04-16 520
^1da177e Linus Torvalds 2005-04-16 521 /*
^1da177e Linus Torvalds 2005-04-16 522 * When PCM is used on 32bit mode, we need to disable
53cdcc38 Arnd Bergmann 2018-04-24 523 * mmap of the old PCM status/control records because
53cdcc38 Arnd Bergmann 2018-04-24 524 * of the size incompatibility.
^1da177e Linus Torvalds 2005-04-16 525 */
548a648b Takashi Iwai 2006-07-31 526 pcm_file->no_compat_mmap = 1;
^1da177e Linus Torvalds 2005-04-16 527
^1da177e Linus Torvalds 2005-04-16 528 switch (cmd) {
^1da177e Linus Torvalds 2005-04-16 529 case SNDRV_PCM_IOCTL_PVERSION:
^1da177e Linus Torvalds 2005-04-16 530 case SNDRV_PCM_IOCTL_INFO:
5a7f2619 Takashi Iwai 2007-12-17 531 case SNDRV_PCM_IOCTL_TSTAMP:
6b587ef9 Takashi Iwai 2007-12-14 532 case SNDRV_PCM_IOCTL_TTSTAMP:
4b671f57 Takashi Iwai 2017-06-19 533 case SNDRV_PCM_IOCTL_USER_PVERSION:
^1da177e Linus Torvalds 2005-04-16 534 case SNDRV_PCM_IOCTL_HWSYNC:
^1da177e Linus Torvalds 2005-04-16 535 case SNDRV_PCM_IOCTL_PREPARE:
^1da177e Linus Torvalds 2005-04-16 536 case SNDRV_PCM_IOCTL_RESET:
^1da177e Linus Torvalds 2005-04-16 537 case SNDRV_PCM_IOCTL_START:
^1da177e Linus Torvalds 2005-04-16 538 case SNDRV_PCM_IOCTL_DROP:
^1da177e Linus Torvalds 2005-04-16 539 case SNDRV_PCM_IOCTL_DRAIN:
^1da177e Linus Torvalds 2005-04-16 540 case SNDRV_PCM_IOCTL_PAUSE:
^1da177e Linus Torvalds 2005-04-16 541 case SNDRV_PCM_IOCTL_HW_FREE:
^1da177e Linus Torvalds 2005-04-16 542 case SNDRV_PCM_IOCTL_RESUME:
^1da177e Linus Torvalds 2005-04-16 543 case SNDRV_PCM_IOCTL_XRUN:
^1da177e Linus Torvalds 2005-04-16 544 case SNDRV_PCM_IOCTL_LINK:
^1da177e Linus Torvalds 2005-04-16 545 case SNDRV_PCM_IOCTL_UNLINK:
53cdcc38 Arnd Bergmann 2018-04-24 546 case __SNDRV_PCM_IOCTL_SYNC_PTR32:
53cdcc38 Arnd Bergmann 2018-04-24 547 case __SNDRV_PCM_IOCTL_SYNC_PTR64:
67616fed Takashi Iwai 2017-08-30 548 return snd_pcm_common_ioctl(file, substream, cmd, argp);
^1da177e Linus Torvalds 2005-04-16 549 case SNDRV_PCM_IOCTL_HW_REFINE32:
^1da177e Linus Torvalds 2005-04-16 550 return snd_pcm_ioctl_hw_params_compat(substream, 1, argp);
^1da177e Linus Torvalds 2005-04-16 551 case SNDRV_PCM_IOCTL_HW_PARAMS32:
^1da177e Linus Torvalds 2005-04-16 552 return snd_pcm_ioctl_hw_params_compat(substream, 0, argp);
^1da177e Linus Torvalds 2005-04-16 553 case SNDRV_PCM_IOCTL_SW_PARAMS32:
^1da177e Linus Torvalds 2005-04-16 554 return snd_pcm_ioctl_sw_params_compat(substream, argp);
de41e437 Baolin Wang 2018-04-24 555 case SNDRV_PCM_IOCTL_STATUS_COMPAT32:
de41e437 Baolin Wang 2018-04-24 556 return snd_pcm_status_user32(substream, argp, false);
de41e437 Baolin Wang 2018-04-24 557 case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT32:
de41e437 Baolin Wang 2018-04-24 558 return snd_pcm_status_user32(substream, argp, true);
^1da177e Linus Torvalds 2005-04-16 559 case SNDRV_PCM_IOCTL_CHANNEL_INFO32:
^1da177e Linus Torvalds 2005-04-16 560 return snd_pcm_ioctl_channel_info_compat(substream, argp);
^1da177e Linus Torvalds 2005-04-16 561 case SNDRV_PCM_IOCTL_WRITEI_FRAMES32:
^1da177e Linus Torvalds 2005-04-16 562 return snd_pcm_ioctl_xferi_compat(substream, SNDRV_PCM_STREAM_PLAYBACK, argp);
^1da177e Linus Torvalds 2005-04-16 563 case SNDRV_PCM_IOCTL_READI_FRAMES32:
^1da177e Linus Torvalds 2005-04-16 564 return snd_pcm_ioctl_xferi_compat(substream, SNDRV_PCM_STREAM_CAPTURE, argp);
^1da177e Linus Torvalds 2005-04-16 565 case SNDRV_PCM_IOCTL_WRITEN_FRAMES32:
^1da177e Linus Torvalds 2005-04-16 566 return snd_pcm_ioctl_xfern_compat(substream, SNDRV_PCM_STREAM_PLAYBACK, argp);
^1da177e Linus Torvalds 2005-04-16 567 case SNDRV_PCM_IOCTL_READN_FRAMES32:
^1da177e Linus Torvalds 2005-04-16 568 return snd_pcm_ioctl_xfern_compat(substream, SNDRV_PCM_STREAM_CAPTURE, argp);
^1da177e Linus Torvalds 2005-04-16 569 case SNDRV_PCM_IOCTL_DELAY32:
^1da177e Linus Torvalds 2005-04-16 570 return snd_pcm_ioctl_delay_compat(substream, argp);
^1da177e Linus Torvalds 2005-04-16 571 case SNDRV_PCM_IOCTL_REWIND32:
^1da177e Linus Torvalds 2005-04-16 572 return snd_pcm_ioctl_rewind_compat(substream, argp);
^1da177e Linus Torvalds 2005-04-16 573 case SNDRV_PCM_IOCTL_FORWARD32:
^1da177e Linus Torvalds 2005-04-16 574 return snd_pcm_ioctl_forward_compat(substream, argp);
de41e437 Baolin Wang 2018-04-24 575 case SNDRV_PCM_IOCTL_STATUS_COMPAT64:
de41e437 Baolin Wang 2018-04-24 576 return snd_pcm_status_user_compat64(substream, argp, false);
de41e437 Baolin Wang 2018-04-24 577 case SNDRV_PCM_IOCTL_STATUS_EXT_COMPAT64:
de41e437 Baolin Wang 2018-04-24 578 return snd_pcm_status_user_compat64(substream, argp, true);
513ace79 Takashi Iwai 2016-02-28 579 #ifdef CONFIG_X86_X32
513ace79 Takashi Iwai 2016-02-28 @580 case SNDRV_PCM_IOCTL_SYNC_PTR_X32:

:::::: The code at line 580 was first introduced by commit
:::::: 513ace79b657e2022a592e77f24074e088681ecc ALSA: pcm: Fix ioctls for X32 ABI

:::::: TO: Takashi Iwai <[email protected]>
:::::: CC: Takashi Iwai <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Attachments:

(No filename) (7.11 kB)
.config.gz (29.65 kB)
Download all attachments

2018-04-26 06:27:26

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 8/8] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control

2018-04-26 08:17:44

On Thu, Apr 26, 2018 at 12:53 PM, Baolin Wang <[email protected]> wrote:

>>
>> 971
>> 972 static int snd_pcm_status_user32(struct snd_pcm_substream *substream,
>> 973 struct snd_pcm_status32 __user * _status,
>> 974 bool ext)
>> 975 {
>> 976 struct snd_pcm_status64 status64;
>> 977 struct snd_pcm_status32 status32;
>> 978 int res;
>> 979
>> 980 memset(&status64, 0, sizeof(status64));
>> 981 memset(&status32, 0, sizeof(status32));
>> 982 /*
>> 983 * with extension, parameters are read/write,
>> 984 * get audio_tstamp_data from user,
>> 985 * ignore rest of status structure
>> 986 */
>> 987 if (ext && get_user(status64.audio_tstamp_data,
>> 988 (u32 __user *)(&_status->audio_tstamp_data)))
>> 989 return -EFAULT;
>> 990 res = snd_pcm_status64(substream, &status64);
>> 991 if (res < 0)
>> 992 return res;
>> 993
>> 994 status32 = (struct snd_pcm_status32) {
>> > 995 .state = status64.state,
>> 996 .trigger_tstamp_sec = status64.trigger_tstamp_sec,
>> 997 .trigger_tstamp_nsec = status64.trigger_tstamp_nsec,
>> 998 .tstamp_sec = status64.tstamp_sec,
>> 999 .tstamp_nsec = status64.tstamp_nsec,
>> 1000 .appl_ptr = status64.appl_ptr,
>> 1001 .hw_ptr = status64.hw_ptr,
>> 1002 .delay = status64.delay,
>> 1003 .avail = status64.avail,
>> 1004 .avail_max = status64.avail_max,
>> 1005 .overrange = status64.overrange,
>>> 1006 .suspended_state = status64.suspended_state,
>
> I am not sure for the warning here, we should change 'snd_pcm_state_t'
> to 's32' for struct snd_pcm_status64?
>
> typedef int __bitwise snd_pcm_state_t;

The problem is that snd_pcm_status32 uses 'u32' here instead of snd_pcm_state_t,
and the __bitwise annotation makes the two types incompatible. This is a
preexisting problem, the warning mail just appeared because you moved that
code to a different file.

If you want to avoid that warning, either use a type case with '__force',
or change the snd_pcm_status32 structure to also use snd_pcm_state_t.

That would be a useful change, but it should be separate from your
other changes since it's an unrelated problem.

Arnd

2018-04-26 15:16:34

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH 8/8] ALSA: add new 32-bit layout for snd_pcm_mmap_status/control

On Tue, Apr 24, 2018 at 2:06 PM, Baolin Wang <[email protected]> wrote:

> -struct snd_pcm_mmap_status {
> +/*
> + * For mmap operations, we need the 64-bit layout, both for compat mode,
> + * and for y2038 compatibility. For 64-bit applications, the two definitions
> + * are identical, so we keep the traditional version.
> + */
> +#ifdef __SND_STRUCT_TIME64
> +#define __snd_pcm_mmap_status64 snd_pcm_mmap_status
> +#define __snd_pcm_mmap_control64 snd_pcm_mmap_control
> +#define __snd_pcm_sync_ptr64 snd_pcm_sync_ptr
> +#else
> +#define __snd_pcm_mmap_status snd_pcm_mmap_status
> +#define __snd_pcm_mmap_control snd_pcm_mmap_control
> +#define __snd_pcm_sync_ptr snd_pcm_sync_ptr
> +#endif
> +
> +struct __snd_pcm_mmap_status {
> snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */
> int pad1; /* Needed for 64 bit alignment */
> snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */

One more thing here: this definition is slightly suboptimal because in
an alsa-lib that gets built with 64-bit time_t, we end up with an unusable
__snd_pcm_mmap_status structure (__snd_pcm_mmap_status64 works
fine, and that would be the normal thing to use). Just in case we want
to be able to build an alsa-lib that is capable of running both on
new kernels with 64-bit time_t interfaces exposed to applications and
also on old kernels that don't have the new ioctls, we probably want
another fixup merged in:

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 18fbdcb2c7b6..638c717d3eb9 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -496,19 +496,30 @@ struct snd_pcm_status {
#define __snd_pcm_mmap_status64 snd_pcm_mmap_status
#define __snd_pcm_mmap_control64 snd_pcm_mmap_control
#define __snd_pcm_sync_ptr64 snd_pcm_sync_ptr
+#define __snd_timespec64 timespec
+struct __snd_timespec {
+ __s32 tv_sec;
+ __s32 tv_nsec;
+};
#else
#define __snd_pcm_mmap_status snd_pcm_mmap_status
#define __snd_pcm_mmap_control snd_pcm_mmap_control
#define __snd_pcm_sync_ptr snd_pcm_sync_ptr
+#define __snd_timespec timespec
+struct __snd_timespec64 {
+ __s64 tv_sec;
+ __s64 tv_nsec;
+};
+
#endif

struct __snd_pcm_mmap_status {
snd_pcm_state_t state; /* RO: state - SNDRV_PCM_STATE_XXXX */
int pad1; /* Needed for 64 bit alignment */
snd_pcm_uframes_t hw_ptr; /* RO: hw ptr (0...boundary-1) */
- struct timespec tstamp; /* Timestamp */
+ struct __snd_timespec tstamp; /* Timestamp */
snd_pcm_state_t suspended_state; /* RO: suspended stream state */
- struct timespec audio_tstamp; /* from sample counter or wall clock */
+ struct __snd_timespec audio_tstamp; /* from sample counter or
wall clock */
};

struct __snd_pcm_mmap_control {
@@ -532,11 +543,6 @@ struct __snd_pcm_sync_ptr {
} c;
};

-struct __snd_timespec64 {
- __s64 tv_sec;
- __s64 tv_nsec;
-};
-
#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];

With this change, alsa-lib can either access whichever structure
matches the glibc 'timespec' definition, or it can ask for
__struct __snd_pcm_mmap_status and struct __snd_pcm_mmap_status64
explicitly, regardless of the time_t definition.

Arnd