Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag

From: Takashi Iwai <tiwai@suse.de>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
	P9ter Ujfalusi <peter.ujfalusi@linux.intel.com>,
	Ranjani Sridharan <ranjani.sridharan@linux.intel.com>,
	Kai Vehmanen <kai.vehmanen@linux.intel.com>
Subject: Re: [PATCH v2 1/3] ALSA: pcm: introduce INFO_NO_REWINDS flag
Date: Tue, 05 Oct 2021 15:35:03 +0200	[thread overview]
Message-ID: <s5hbl43egs8.wl-tiwai@suse.de> (raw)
In-Reply-To: <c62b3749-c5ea-7b1e-2831-272c8a14d3ac@linux.intel.com>

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@linux.intel.com/

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