On Tue, 05 Oct 2021 15:10:40 +0200, Pierre-Louis Bossart wrote:
On 10/5/21 1:59 AM, Takashi Iwai wrote:
On Mon, 04 Oct 2021 18:24:21 +0200, Pierre-Louis Bossart wrote:
When the hardware can only deal with a monotonically increasing appl_ptr, this flag can be set. In case the application requests a rewind, snd_pcm_rewind() will not return an error code but signal that the appl_ptr was not modified.
This modification itself is fine, but I guess that application may still move the appl_ptr directly via SNDRV_PCM_IOCTL_SYNC_PTR ioctl. We need to verify the backward move there, I suppose?
Sorry Takashi, I wasn't able to fully follow your question.
In the previous version, I had an explicit check to see if the appl_ptr was modified by a rewind, but you mentioned this would be broken for 32-bit devices due to the use of the 'boundary'. I really have no idea how we can detect a rewind in this configuration, so if you are asking to detect when the appl_ptr is modified through some other means (which I didn't get) we will have the same problem, won't we?
Which same problem are you referring to?
What I suggested here is that there is still a way to modify the appl_ptr from user-space even if you plug the way by disabling the status/control mmap and disabling the rewind ioctl. You have to cover the sync_ptr ioctl call path as well, and it'd need a backward check.
see the initial thread here:
https://lore.kernel.org/alsa-devel/de5e91c6-5f0e-9866-a1c2-0943b4342359@linu...
And, what I meant in the previous thread was that the check in the given patch wasn't "enough", i.e. it needs more careful checks considering the boundary crossing. That is, you can't simply compare appl_ptr vs old_appl_ptr as a single condition for the backward move.
For example, check snd_pcm_playback_avail() and co. That contains a couple of more condition checks and corrections due to the possible boundary crossing. (Here, runtime->boundary may differ depending on 32 or 64bit context.)
The actual implementation of the backward move check would be slightly different from those, but I hope you get my idea.
Takashi