Overflow in calculating audio timestamp

[Jaroslav Kysela]

On 04. 02. 23 10:11, Alan Young wrote:
> 
> On 03/02/2023 18:02, Jaroslav Kysela wrote:
>> 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) 
> 
> Yes, indeed. My ambition was unnecessarily short.
> 
> 
>> 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 don't understand why?
 >
>> 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), 
> 
> Would that really help that much? It would extend the total possible duration 
> but perhaps ~1523287 years(below) is sufficient.
> 
>> remove snd_BUG_ON
> 
> Again, why?

For 32-bit apps the boundary is near to UINT32_MAX (see recalculate_boundary() 
function). So only one crossing point is not enough to cover a decent time range.

There should be a better check, if the add operation crosses the U64 range for 
snd_BUG_ON. In my eyes, it looks safer to use counter here and do the checks
in the function which uses this value.

>> 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
> 
> 
> Yes indeed. How about this?
> 
> static inline void snd_pcm_lib_frames_to_timespec64(u64 frames, unsigned int rate, struct timespec64 *audio_tstamp)
> {
> 	u32 remainder;
> 	audio_tstamp->tv_sec = div_u64_rem(frames, rate, &remainder);
> 	audio_tstamp->tv_nsec = div_u64(mul_u32_u32(remainder, NSEC_PER_SEC), rate);

Yes, this looks fine.

					Jaroslav

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