Overflow in calculating audio timestamp

[Jaroslav Kysela]

On 03. 02. 23 17:11, Alan Young wrote:
> 
> On 03/02/2023 00:34, Takashi Sakamoto wrote:
>> Hi,
>>
>> Thank you for the report.
>>
>> On Thu, Feb 02, 2023 at 01:55:24PM +0000, Alan Young wrote:
>>> sound/core/pcm_lib.c:update_audio_tstamp() contains the following
>>> calculation:
>>>
>>>           audio_nsecs = div_u64(audio_frames * 1000000000LL,
>>>                   runtime->rate);
>>>
>>> This will result in a 64-bit overflow after 4.4 days at 48000 Hz, or 1.1
>>> days at 192000.
>>>
>>> Are you interested in a patch to improve this?
>>>
>>> The same calculation occurs in a couple of other places.
>> I'm interested in your patch. Would you please post it C.C.ed to the
>> list and me?  As you noted, we can see the issue in ALSA PCM core and
>> Intel HDA stuffs at least.
>>
>>    * sound/core/pcm_lib.c
>>    * sound/pci/hda/hda_controller.c
>>    * sound/soc/intel/skylake/skl-pcm.c
>>
>> I note that 'NSEC_PER_SEC' macro is available once including
>> 'linux/time.h'. It is better to use instead of the literal.
>> The macro is defined in 'include/vdso/time64.h'.
>>
>>
>> As another issue, the value of 'audio_frames' comes from the value of
>> 'struct snd_pcm_runtime.hw_ptr_wrap'. In ALSA PCM core, the value is
>> increased by the size of PCM buffer every time hw_ptr cross the boundary
>> of PCM buffer, thus multiples of the size is expected. Nevertheless,
>> there is no check for overflow within 64 bit storage. In my opinion, the
>> committer had less care of it since user does not practically playback or
>> capture PCM substream so long. But the additional check is preferable as
>> long as it does not break the fallback implementation of audio time stamp.
> 
> 
> I have not yet finished testing various alternatives. I want to extend
> the overflow by "enough" and also am conscious of the need to keep the
> overhead down.
> 
> I actually think, on reflection, that the only case that matters is the
> call from update_audio_tstamp(). The others only deal with codec delays
> which will be small (unless I misunderstand those drivers).
> 
> This is what I have so far but I'll submit a proper patch when I have it
> refined.
> 
> static u64 snd_pcm_lib_frames_to_nsecs(u64 frames, unsigned int rate)
> {
>       /*
>        *  Avoid 64-bit calculation overflow after:
>        *  - 4.8 days @ 44100
>        *  - 0.56 days @ 384000
>        *  extending these intervals by a factor of 100.
>        */
>       if (frames < 0xffffffffffffffffLLU / NSEC_PER_SEC)
>           return div_u64(frames * NSEC_PER_SEC, rate);
> 
>       if (rate % 100 == 0)
>           return div_u64(frames * (NSEC_PER_SEC/100), (rate/100));
> 
>       /* Fallback: reduce precision to approximately deci-micro-seconds: 1.28e-7 */
>       return div_u64(frames * (NSEC_PER_SEC >> 7), rate) << 7;
> }

Thank you for your suggestion, but I think that the *whole* code for 
!get_time_info in update_audio_tstamp() should be recoded. The calling of 
ns_to_timespec64() is not enough to handle the boundary wraps in a decent 
range (tenths years for 24x7 operation) and the bellow code is dangerous for 
32-bit apps / system:

      if (crossed_boundary) {
                 snd_BUG_ON(crossed_boundary != 1);
                 runtime->hw_ptr_wrap += runtime->boundary;
      }

I would probably propose to have just hw_ptr_wrap +1 counter (we can 
reconstruct the frame position back by multiplication and do range check 
later), remove snd_BUG_ON and improve the timespec64 calculation.

The calculation should be split to two parts (tv_sec / tv_nsec):

1) calculate seconds: (frames / rate)
2) calculate the remainder (ns): ((frames % rate) * NSEC_PER_SEC) / rate

With 64-bit integer range, we should go up to (for 384000Hz rate):

2**64 / 384000 / 3600 / 24 / 365 = ~1523287 years

Maybe I did a mistake somewhere. I'm open for comments.

					Jaroslav

-- 
Jaroslav Kysela <perex at perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.