Re: Overflow in calculating audio timestamp

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 2/3/23 12:02, Jaroslav Kysela wrote:
> 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.

I am not following how the boundary comes into play for cases where the
timestamp comes directly from a link counter, and is not related to the
DMA hw_ptr at all.