Message-ID: <s5hr2tcy849.wl-tiwai@suse.de>
Date:   Sun, 05 Nov 2017 17:59:18 +0100
From:   Takashi Iwai <tiwai@...e.de>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Baolin Wang <baolin.wang@...aro.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Fabian Frederick <fabf@...net.be>,
        Arvind Yadav <arvind.yadav.cs@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        alsa-devel@...a-project.org, Vinod Koul <vinod.koul@...el.com>,
        hardik.t.shah@...el.com, guneshwor.o.singh@...el.com,
        Liam Girdwood <lgirdwood@...il.com>,
        SF Markus Elfring <elfring@...rs.sourceforge.net>,
        gudishax.kranthikumar@...el.com, Mark Brown <broonie@...nel.org>,
        Bhumika Goyal <bhumirks@...il.com>,
        Naveen M <naveen.m@...el.com>, jeeja.kp@...el.com,
        Takashi Sakamoto <o-takashi@...amocchi.jp>,
        subhransu.s.prusty@...el.com, Ingo Molnar <mingo@...nel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [alsa-devel] [RFC PATCH v2 7/7] sound: core: Avoid using timespec for struct snd_timer_tread

On Sun, 05 Nov 2017 14:16:28 +0100,
Arnd Bergmann wrote:
> 
> On Sun, Nov 5, 2017 at 11:29 AM, Takashi Iwai <tiwai@...e.de> wrote:
> > On Thu, 02 Nov 2017 12:06:57 +0100,
> > Baolin Wang wrote:
> >>
> >> The struct snd_timer_tread will use 'timespec' type variables to record
> >> timestamp, which is not year 2038 safe on 32bits system.
> >>
> >> Since the struct snd_timer_tread is passed through read() rather than
> >> ioctl(), and the read syscall has no command number that lets us pick
> >> between the 32-bit or 64-bit version of this structure.
> >>
> >> Thus we introduced one new command SNDRV_TIMER_IOCTL_TREAD64 and new
> >> struct snd_timer_tread64 replacing timespec with s64 type to handle
> >> 64bit time_t. Also add one struct compat_snd_timer_tread64_x86_32 to
> >> handle 64bit time_t with 32bit alignment. That means we will set
> >> tu->tread = TREAD_FORMAT_64BIT when user space has a 64bit time_t,
> >> then we will copy to user with struct snd_timer_tread64. For x86_32
> >> mode, we will set tu->tread = TREAD_FORMAT_TIME32_X86 to copy
> >> struct compat_snd_timer_tread64_x86_32 for user. Otherwise we will
> >> use 32bit time_t variables when copying to user.
> >
> > We should introduce SNDRV_TIMER_IOCTL_USER_PVERSION instead, where
> > user-space can tell which protocol version it understands.  If the
> > protocol version is higher than some definition, we can assume it's
> > 64-bit ready.  The *_USER_PVERSION is issued from alsa-lib side.
> > In that way, we can extend the ABI more flexibly.  A similar trick was
> > already used in PCM ABI.  (Ditto for control and rawmidi API; we
> > should have the same mechanism for all relevant APIs).
> >
> > Moreover, once when the new protocol is used, we can use the standard
> > 64bit monotonic nsecs as a timestamp, so that we don't need to care
> > about 32/64bit compatibility.
> 
> I think that's fine, we can do that too, but I don't see how we get around
> to doing something like Baolin's patch first. Without this, we will get
> existing user space code compiling against our kernel headers using a
> new glibc release that will inadvertently change the structure layout
> on the read file descriptor.

But it won't work in anyway in multiple ways, e.g. this timer read
stuff and another the structs embedded in the mmappged page.  If you
do rebuild things with new glibc, it should tell kernel about the new
ABI in anyway more or less explicitly.  And if you need it, it means
that some source-code level API change would be possible.

Of course, passing which data type is another question.  Maybe 64bit
nsecs wouldn't fit all places, and timespec64 style would be still
required.  But still, the current patch looks still too unnecessarily
complex to me.  (Yeah I know that the problem is complex, but the code
can be simpler, I hope!)

> The trick with redefining SNDRV_TIMER_IOCTL_TREAD in that
> configuration lets the kernel know what API the user space expects
> without requiring source-level changes.

Right, it works for this case, but not always.
If jumping the API would give a cleaner way of implementation, I'd
prefer that over too hackeries, IMO.

> If you want to for all users of SNDRV_TIMER_IOCTL_TREAD to move
> to a new interface for y2038-safety, we'd have to redefined the structure
> to avoid the libc-provided 'struct timespec' on 32-bit architectures, like:
> 
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 299a822d2c4e..f93cace4cd24 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -801,7 +801,14 @@ enum {
> 
>  struct snd_timer_tread {
>         int event;
> +#if __BITS_PER_LONG == 32
> +       struct {
> +               __kernel_long_t tv_sec;
> +               __kernel_long_t tv_usec;
> +       };
> +#else
>         struct timespec tstamp;
> +#endif
>         unsigned int val;
>  };
> 
> This has a somewhat higher risk of breaking existing code (since the type
> changes), and it doesn't solve the overflow.

Hm, how to define the timestamp type is one of the biggest questions
indeed.  In general, there can't be any guarantee that just rebuilding
with the 64bit timespec would work for all existing codes.  In theory
it shouldn't break, but who knows...


thanks,

Takashi

Powered by blists - more mailing lists