Message-ID: <00f4c8db-44cf-b555-f3ca-42916b1e7ceb@de.adit-jv.com>
Date:   Wed, 27 Mar 2019 10:26:56 +0100
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: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.
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).

Or do I have to use another API to configure the sound timer at runtime 
(e.g. ALSA controls or RW kernel module parameters)?


Best regards

Timo

Powered by blists - more mailing lists