Date:   Thu, 4 Apr 2019 12:18:33 +0200
From:   Timo Wischer <twischer@...adit-jv.com>
To:     Takashi Iwai <tiwai@...e.de>
CC:     <broonie@...nel.org>, <perex@...ex.cz>,
        <alsa-devel@...a-project.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 09/10] ALSA: pcm: Add snd_pcm_ops for snd_pcm_link()

On 3/27/19 10:38, Takashi Iwai wrote:
> On Wed, 27 Mar 2019 10:26:56 +0100,
> Timo Wischer wrote:
>> On 3/27/19 10:11, Takashi Iwai wrote:
>>> On Wed, 27 Mar 2019 09:34:40 +0100,
>>> Timo Wischer wrote:
>>>> On 3/26/19 17:00, Takashi Iwai wrote:
>>>>> On Tue, 26 Mar 2019 16:16:54 +0100,
>>>>> Timo Wischer wrote:
>>>>>> On 3/26/19 15:23, Takashi Iwai wrote:
>>>>>>> On Tue, 26 Mar 2019 12:25:37 +0100,
>>>>>>> Timo Wischer wrote:
>>>>>>>> On 3/26/19 09:35, Takashi Iwai wrote:
>>>>>>>>
>>>>>>>>         On Tue, 26 Mar 2019 08:49:33 +0100,
>>>>>>>>         <twischer@...adit-jv.com> wrote:
>>>>>>>>                  From: Timo Wischer <twischer@...adit-jv.com>
>>>>>>>>                      snd_pcm_link() can be called by the user as long
>>>>>>>> as the device is not
>>>>>>>>             yet started. Therefore currently a driver which wants to iterate over
>>>>>>>>             the linked substreams has to do this at the start trigger. But the start
>>>>>>>>             trigger should not block for a long time. Therefore there is no callback
>>>>>>>>             which can be used to iterate over the linked substreams without delaying
>>>>>>>>             the start trigger.
>>>>>>>>             This patch introduces a new callback function which will be called after
>>>>>>>>             the linked substream list was updated by snd_pcm_link(). This callback
>>>>>>>>             function is allowed to block for a longer time without interfering the
>>>>>>>>             synchronized start up of linked substreams.
>>>>>>>>                      Signed-off-by: Timo Wischer
>>>>>>>> <twischer@...adit-jv.com>
>>>>>>>>                  Well, the idea appears interesting, but I'm afraid
>>>>>>>> that the
>>>>>>>>         implementation is still racy.  The place you're calling the new
>>>>>>>>         callback isn't protected, hence the stream can be triggered while
>>>>>>>>         calling it.  That is, even during operating your loopback link_changed
>>>>>>>>         callback, another thread is able to start the stream.
>>>>>>>>         Hi Takashi,
>>>>>>>>
>>>>>>>> As far as I got you mean the following scenario:
>>>>>>>>
>>>>>>>>       * snd_pcm_link() is called for a HW sound card
>>>>>>>>           + loopback_snd_timer_link_changed()
>>>>>>> The start may happen at this point.
>>>>>> In this case the last link status will be used and aloop will print a
>>>>>> warning "Another sound timer was requested but at least one device is
>>>>>> already running...".
>>>>>>
>>>>>> Without this patch set a similar issue already exists. When calling
>>>>>> snd_pcm_start() before snd_pcm_link() was done the additional device
>>>>>> linked by the snd_pcm_link() will not be started.
>>>>>> Therefore the application has already to take care about the order of
>>>>>> the calls.
>>>>> Yes, but it doesn't matter for now, just because other drivers do care
>>>>> the PCM links only for trigger callback.  Now you're trying to add
>>>>> something new but in an incomplete manner.
>>>>>
>>>>>>>>           + loopback_snd_timer_open()
>>>>>>>>           + spin_lock_irqsave(&dpcm->cable->lock, flags);
>>>>>>>>       * snd_pcm_start() called for aloop sound card
>>>>>>>>           + loopback_trigger()
>>>>>>>>           + spin_lock(&cable->lock) -> has to wait till loopback_snd_timer_open()
>>>>>>>>             calls spin_unlock_irqrestore()
>>>>>>>>
>>>>>>>> So far snd_pcm_start() has to wait for loopback_snd_timer_open().
>>>>>>>>
>>>>>>>>       * loopback_snd_timer_open() will continue with
>>>>>>>>           + dpcm->cable->snd_timer.instance = NULL;
>>>>>>>>           + spin_unlock_irqrestore()
>>>>>>>>       * loopback_trigger() can enter the lock
>>>>>>>>           + loopback_snd_timer_start() will fail with -EINVAL due to
>>>>>>>>             (loopback_trigger == NULL)
>>>>>>>>
>>>>>>>> At least this will not result into memory corruption due to race or any other
>>>>>>>> wired behavior.
>>>>>>> I don't expect the memory corruption, but my point is that dealing
>>>>>>> with linked streams is still tricky.  It was considered for the
>>>>>>> lightweight coupled start/stop operation, and something intensively
>>>>>>> depending on the linked status was out of the original design...
>>>>>>>
>>>>>>>> But my expectation is that snd_pcm_link(hw, aloop) or snd_pcm_link(aloop, hw)
>>>>>>>> is only called by the application calling snd_pcm_start(aloop)
>>>>>>>> because the same aloop device cannot be opened by multiple applications at the
>>>>>>>> same time.
>>>>>>>>
>>>>>>>> Do you see an use case where one application would call snd_pcm_start() in
>>>>>>>> parallel with snd_pcm_link() (somehow configuring the device)?
>>>>>>> It's not about the actual application usages but rather against the
>>>>>>> malicious attacks.  Especially aloop is a virtual device that is
>>>>>>> available allover the places, it may be deployed / attacked easily.
>>>>>> The attack we are identifying here can only be done by the application
>>>>>> opening the aloop device.
>>>>>> An application allowed to open the aloop device is anyway able to
>>>>>> manipulate the audio streaming.
>>>>> Right, and if it such a racy access may lead to a driver misbehavior,
>>>>> it's a big concern.  The proposed callback usage is racy, so some
>>>>> other implementation might be broken easily in future.
>>>>>
>>>>>> Or do you see an attack which would influence any other device/stream
>>>>>> not opened by this application?
>>>>>>
>>>>>>>> May be we should add an additional synchronization mechanism in pcm_native.c
>>>>>>>> to avoid call of snd_pcm_link() in parallel with snd_pcm_start().
>>>>>>> If it really matters...  Honestly speaking, I'm not fully convinced
>>>>>>> whether we want to deal with this using the PCM link mechanism.
>>>>>>>
>>>>>>> What's the motivation for using the linked streams at the first place?
>>>>>>> That's one of the biggest missing piece in the whole picture.
>>>>>> In general when the user uses snd_pcm_link() it expects that the
>>>>>> linked devices are somehow synchronized.
>>>>>> Any applications already using snd_pcm_link() do not need to be
>>>>>> adapted to use the new feature of aloop (for example JACK or ALSA
>>>>>> multi plugin)
>>>>>>
>>>>>> But when linking a HW sound card and aloop without this patch set,
>>>>>> both devices will be started in sync but
>>>>>> the snd_pcm_period_eleapsed() calls of the different devices will
>>>>>> drift. To avoid this the aloop plugin can automatically use the right
>>>>>> timer.
>>>>>> If this feature is not implemented the user has to use snd_pcm_link()
>>>>>> to trigger snd_pcm_start() in sync but also has to configure the aloop
>>>>>> plugin to use the right sound timer.
>>>>>> May be the linked cards can change during runtime of the
>>>>>> system. Without this feature the aloop kernel driver has to be loaded
>>>>>> with different kernel parameters
>>>>>> to select the right timer.
>>>>>>
>>>>>> ALSA controls cannot be used easily. Selecting the sound timer by the
>>>>>> card number could be error prone because the card ID could change
>>>>>> between system starts.
>>>>>> Therefore an ALSA control supporting the card name should be
>>>>>> used. This could be for example done via an ALSA enum control. But in
>>>>>> this case the names of all sound cards of the system has to be
>>>>>> available
>>>>>> on aloop probe() call. But at this point in time the sound cards
>>>>>> probed after aloop are not available. Therefore only the sound timers
>>>>>> of the sound cards probed before aloop are selectable.
>>>>> Hm.  For me this patch series looks very hackish.  As mentioned, the
>>>>> PCM link usage is rather just a synchronous trigger start/stop for
>>>>> multiple streams belonging to the same hardware; in that sense, it'd
>>>>> be possible to adapt some mechanism for aloop, but at most it should
>>>>> be much less intrusive change, e.g. just doing the multiple
>>>>> loopback_timer_start() in a single loop.
>>>> What do you mean by "just doing the multiple loopback_timer_start() in
>>>> a single loop."?
>>>>
>>>> The snd_pcm_link() call information are mainly used to select the
>>>> right sound timer. Starting the timer in sync is a nice add-on.
>>>> Therefore by simple starting all timers in a for loop I would not
>>>> select the right sound timer.
>>>> (May be I misunderstood your example)
>>> My example is only about the system timer.
>>>
>>>> I could call the link_changed() callback inside
>>>> snd_pcm_stream_lock_irq() same lock also hold by snd_pcm_start().
>>>> But an application could still call snd_pcm_link() and snd_pcm_start()
>>>> in parallel e.g.:
>>>>
>>>> snd_pcm_common_ioctl(IOCTL_LINK)
>>>>> task switch
>>>> snd_pcm_common_ioctl(IOCTL_START)
>>>> snd_pcm_start()
>>>> done
>>>>> continue previous task
>>>> The same could for example happen with snd_pcm_start() and snd_hw_free():
>>>>
>>>> snd_pcm_common_ioctl(IOCTL_START)
>>>>> task switch
>>>> snd_pcm_common_ioctl(IOCTL_HW_FREE)
>>>> snd_hw_free()
>>>> Done
>>>>> continue previous task
>>>> snd_pcm_start() will fail with an error code
>>>>
>>>> Therefore I do not think we have to synchronize the IOCTL calls (this
>>>> can only be done in the user application or ALSAlib). We only have to
>>>> return an proper error code in case of misusage.
>>>>
>>>> As far as I understand ALSA was not designed for multithread usage. So
>>>> the user (or ALSAlib) has to be aware of calling the functions in the
>>>> right order.
>>>> Otherwise the user could expect returned error codes.
>>> Well, your assumption is plain wrong.  ALSA is designed for
>>> multithread usage.  And the kernel works for multithread.
>>>
>>> It's not about the right usage.  It's about the robustness, about
>>> whether any malicious code may lead to a serious defunct.  The opened
>>> race in calls may easily introduce such a failure.
>>>
>>>> In case of my patch set snd_pcm_start() will always fail with a proper
>>>> error code as long as no sound timer is running. This could be the
>>>> case if snd_pcm_start() is called before the first snd_pcm_link()
>>>> call.
>>>> This is also the case if snd_pcm_start() is called when the
>>>> snd_pcm_link() call has closed the old timer but not yet opened the
>>>> new one.
>>>> In case of snd_pcm_link() is called after snd_pcm_start() aloop will
>>>> write a proper warning (that the snd_pcm_link() call will have no
>>>> effect because the stream is already running).
>>>> All member variables used in loopback_snd_timer_open(),
>>>> loopback_snd_timer_source_update() and loopback_snd_timer_start() are
>>>> always protected by cable->lock. Therefore no one can read an invalid
>>>> state of these variables and the misusage can always be detected.
>>>>
>>>> Therefore I am not sure what I should change. May be you could
>>>> reference to a source code line which looks hackish for you?
>>> In principle, the PCM ops is supposed to be safe for operating a
>>> certain stuff.  If a state change may happen during the operation,
>>> this should be called inside PCM stream lock.  So the design of the
>>> new callback itself is fragile.
>>>
>>> then, it comes to a point to re-setup the timer at the link change.
>>> The idea is interesting, but it's a wrong usage of PCM link feature,
>>> to be honest.
>>>
>>> That said, I find the changes up to patch 8 are acceptable (with some
>>> fixes expected), but the link feature isn't.

Hi Takashi,

could you provide your feedback to the other patches?

I would like to apply these 8 patches first and then I would provide an 
addition patch to get an ALSA control to select the sound timer.
I fear this would be the best idea (replacing the snd_pcm_link() 
feature) because the ALSA control is accessible by any audio user and
we could use ALSA hooks to select the right sound timer when opening an 
ALSA device.

>> Thanks for clarification. Do you think there is a chance to get an
>> acceptable snd_pcm_link() feature when using PCM stream lock for
>> snd_pcm_link()?
>> In this case I think snd_timer_open() has to be adapted to use
>> non-blocking calls only (do not use mutexs).
> This needs a good amount of consideration: currently the PCM link
> feature is designed for the synchronous start/stop, especially for the
> streams *on the same hardware*.  The link between different devices is
> merely a best-effort.  This is the current design.
>
> Your target is obviously above that level.  So we have to consider
> either extending the usage and the implementation, or introducing
> another API.  Both are things we'd like to avoid as much as possible
> from the maintainability POV.
>
>> Or do I have to use another API to configure the sound timer at
>> runtime (e.g. ALSA controls or RW kernel module parameters)?
> The kernel parameter would be handy.  An alternative would be a
> configuration via proc or sysfs file.  Even configfs or debugfs is
> another possibility, but the proc file would be easiest, I suppose.
>
> Of course, the disadvantage of these is that it's usually writable
> only by root.
>
>
> Takashi

Powered by blists - more mailing lists