| Message ID | 1553586574-18608-5-git-send-email-twischer@de.adit-jv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: aloop: Support selection of snd_timer | expand |
On Tue, 26 Mar 2019 08:49:33 +0100, <twischer@de.adit-jv.com> wrote: > > From: Timo Wischer <twischer@de.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@de.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. thanks, Takashi
On 3/26/19 09:35, Takashi Iwai wrote: > On Tue, 26 Mar 2019 08:49:33 +0100, > <twischer@de.adit-jv.com> wrote: >> From: Timo Wischer <twischer@de.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@de.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 o loopback_snd_timer_link_changed() o loopback_snd_timer_open() o spin_lock_irqsave(&dpcm->cable->lock, flags); * snd_pcm_start() called for aloop sound card o loopback_trigger() o 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 o dpcm->cable->snd_timer.instance = NULL; o spin_unlock_irqrestore() * loopback_trigger() can enter the lock o 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. 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)? 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(). > > thanks, > > Takashi
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@de.adit-jv.com> wrote: > > From: Timo Wischer <twischer@de.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@de.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. > + 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. > 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. thanks, Takashi
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@de.adit-jv.com> wrote: >> >> From: Timo Wischer <twischer@de.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@de.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. > >> + 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. 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. > > > thanks, > > Takashi
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@de.adit-jv.com> wrote: > >> From: Timo Wischer <twischer@de.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@de.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. thanks, Takashi
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@de.adit-jv.com> wrote: >>>> From: Timo Wischer <twischer@de.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@de.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) 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. 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? Thanks Timo > > > thanks, > > Takashi
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@de.adit-jv.com> wrote: > >>>> From: Timo Wischer <twischer@de.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@de.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, Takashi
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@de.adit-jv.com> wrote: >>>>>> From: Timo Wischer <twischer@de.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@de.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
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@de.adit-jv.com> wrote: > >>>>>> From: Timo Wischer <twischer@de.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@de.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). 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
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@de.adit-jv.com> wrote: >>>>>>>> From: Timo Wischer <twischer@de.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@de.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
On Thu, 04 Apr 2019 12:18:33 +0200, Timo Wischer wrote: > > >>> 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? The patch 4 must be superfluous, at least. The trigger callback is called within the stream lock, so you really don't need to save flags. The patch 8 needs more consideration. I'm not fond of the current way to select the timer source. For example, the ALSA timer source isn't necessarily bound with cards. It can be a hrtimer, or system timer backend ALSA global timer, too. This can't be chosen in the suggested way. Also, the use of tasklet is superfluous. The timer instance can be called via tasklet by itself if you pass the flag properly. > 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. It depends on the implementation. Usually you "lock" such a control that is bound with a PCM stream at opening, so that other process won't be able to change. Such a trick is used for the dynamic routing via control element for emu10k1, for example. But, whether it's really usable, I don't know. Let's see. thanks, Takashi
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 18bd8c3..a7e5dd2 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -90,6 +90,7 @@ struct snd_pcm_ops { unsigned long offset); int (*mmap)(struct snd_pcm_substream *substream, struct vm_area_struct *vma); int (*ack)(struct snd_pcm_substream *substream); + int (*link_changed)(struct snd_pcm_substream *substream); }; /* diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index f731f90..57a8a66 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1981,6 +1981,27 @@ static bool is_pcm_file(struct file *file) /* * PCM link handling */ +/* Note: call with snd_pcm_link_rwsem locked */ +static int snd_pcm_link_changed(struct snd_pcm_substream * const substream) +{ + struct snd_pcm_substream *s; + + /* snd_pcm_link_rwsem is down whenever the link_list is changed. + * Therefore this lock is sufficent for the iteration. + */ + snd_pcm_group_for_each_entry(s, substream) { + int err; + + if (!s->ops->link_changed) + continue; + err = s->ops->link_changed(s); + if (err < 0) + return err; + } + + return 0; +} + static int snd_pcm_link(struct snd_pcm_substream *substream, int fd) { int res = 0; @@ -2030,6 +2051,9 @@ static int snd_pcm_link(struct snd_pcm_substream *substream, int fd) snd_pcm_group_assign(substream1, target_group); snd_pcm_stream_unlock(substream1); snd_pcm_group_unlock_irq(target_group, nonatomic); + + if (res >= 0) + res = snd_pcm_link_changed(substream); _end: up_write(&snd_pcm_link_rwsem); _nolock: @@ -2076,6 +2100,11 @@ static int snd_pcm_unlink(struct snd_pcm_substream *substream) snd_pcm_group_unlock_irq(group, nonatomic); if (do_free) kfree(group); + if (res >= 0) + res = snd_pcm_link_changed(substream); + /* Also signal substream which was removed */ + if (res >= 0 && substream->ops->link_changed) + res = substream->ops->link_changed(substream); _end: up_write(&snd_pcm_link_rwsem);