Re: Overflow in calculating audio timestamp

[Alan Young <consult.awy@gmail.com>]

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;
}

Alan.