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@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
