Question regarding delayed trigger in dpcm_set_fe_update_state()
Hello,
Writing with question regarding dpcm_set_fe_update_state() function which is part of sound/soc/soc-pcm.c file and has been introduced with commit "ASoC: dpcm: Fix race between FE/BE updates and trigger" [1].
The part that concerns me is the invocation of dpcm_fe_dai_do_trigger() regardless of the actual state given DPCM is in (actual state, not the DPCM_UPDATE_XX). The conditional invocation of said _trigger() and addition of .trigger_pending field are here to address a race where PCM state is being modified from multiple locations simultaneously, at least judging by the commit's description.
Note that the dpcm_set_fe_update_state() is called from all the dai-ops i.e.: startup, shutdown, hw_params, prepare and hw_free. Now, given that knowledge, we could end up in scenario where dpcm_fe_dai_do_trigger() is invoked as a part of dpcm_fe_dai_hw_free(). dpcm_set_fe_update_state() is called there twice, once with DPCM_UPDATE_FE and once with DPCM_UPDATE_NO. The second case is the more interesting one since it's called **after** ->hw_free() callback is invoked for all the DAIs.
dpcm_fe_dai_hw_free() dpcm_set_fe_update_state(DPCM_UPDATE_FE) // fine (...) dpcm_be_dai_hw_free() // data allocated in hw_params // is freed here (...) dpcm_set_fe_update_state(DPCM_UPDATE_NO) // not fine
The last is *not fine* if the .trigger_pending is not a zero, and can lead to panic as code used during ->trigger() is often manipulating data allocated during ->hw_params() but that data has just been freed with ->hw_free().
Is what I'm looking at a bug? Or, perhaps there's something I'm missing in the picture. From my study, it seems that only dpcm_fe_dai_prepare() is a safe place for calling dpcm_fe_dai_do_trigger() - once .trigger_pending is taken into account. Any input is much appreciated.
[1]: https://lore.kernel.org/all/1415116348-11792-1-git-send-email-tiwai@suse.de/...
Regards, Czarek
On 2022-10-12 4:07 PM, Cezary Rojewski wrote:
Hello,
Writing with question regarding dpcm_set_fe_update_state() function which is part of sound/soc/soc-pcm.c file and has been introduced with commit "ASoC: dpcm: Fix race between FE/BE updates and trigger" [1].
The part that concerns me is the invocation of dpcm_fe_dai_do_trigger() regardless of the actual state given DPCM is in (actual state, not the DPCM_UPDATE_XX). The conditional invocation of said _trigger() and addition of .trigger_pending field are here to address a race where PCM state is being modified from multiple locations simultaneously, at least judging by the commit's description.
Note that the dpcm_set_fe_update_state() is called from all the dai-ops i.e.: startup, shutdown, hw_params, prepare and hw_free. Now, given that knowledge, we could end up in scenario where dpcm_fe_dai_do_trigger() is invoked as a part of dpcm_fe_dai_hw_free(). dpcm_set_fe_update_state() is called there twice, once with DPCM_UPDATE_FE and once with DPCM_UPDATE_NO. The second case is the more interesting one since it's called **after** ->hw_free() callback is invoked for all the DAIs.
dpcm_fe_dai_hw_free() dpcm_set_fe_update_state(DPCM_UPDATE_FE) // fine (...) dpcm_be_dai_hw_free() // data allocated in hw_params // is freed here (...) dpcm_set_fe_update_state(DPCM_UPDATE_NO) // not fine
The last is *not fine* if the .trigger_pending is not a zero, and can lead to panic as code used during ->trigger() is often manipulating data allocated during ->hw_params() but that data has just been freed with ->hw_free().
Is what I'm looking at a bug? Or, perhaps there's something I'm missing in the picture. From my study, it seems that only dpcm_fe_dai_prepare() is a safe place for calling dpcm_fe_dai_do_trigger() - once .trigger_pending is taken into account. Any input is much appreciated.
Given no responses, perhaps there's something in my message to be improved?
On Wed, 12 Oct 2022 16:07:49 +0200, Cezary Rojewski wrote:
Hello,
Writing with question regarding dpcm_set_fe_update_state() function which is part of sound/soc/soc-pcm.c file and has been introduced with commit "ASoC: dpcm: Fix race between FE/BE updates and trigger" [1].
The part that concerns me is the invocation of dpcm_fe_dai_do_trigger() regardless of the actual state given DPCM is in (actual state, not the DPCM_UPDATE_XX). The conditional invocation of said _trigger() and addition of .trigger_pending field are here to address a race where PCM state is being modified from multiple locations simultaneously, at least judging by the commit's description.
Note that the dpcm_set_fe_update_state() is called from all the dai-ops i.e.: startup, shutdown, hw_params, prepare and hw_free. Now, given that knowledge, we could end up in scenario where dpcm_fe_dai_do_trigger() is invoked as a part of dpcm_fe_dai_hw_free(). dpcm_set_fe_update_state() is called there twice, once with DPCM_UPDATE_FE and once with DPCM_UPDATE_NO. The second case is the more interesting one since it's called **after** ->hw_free() callback is invoked for all the DAIs.
dpcm_fe_dai_hw_free() dpcm_set_fe_update_state(DPCM_UPDATE_FE) // fine (...) dpcm_be_dai_hw_free() // data allocated in hw_params // is freed here (...) dpcm_set_fe_update_state(DPCM_UPDATE_NO) // not fine
The last is *not fine* if the .trigger_pending is not a zero, and can lead to panic as code used during ->trigger() is often manipulating data allocated during ->hw_params() but that data has just been freed with ->hw_free().
Is what I'm looking at a bug? Or, perhaps there's something I'm missing in the picture. From my study, it seems that only dpcm_fe_dai_prepare() is a safe place for calling dpcm_fe_dai_do_trigger() - once .trigger_pending is taken into account. Any input is much appreciated.
First off, each prepare, hw_params, hw_free, etc call is protected by a mutex, so they can't be run simultaneously. And the commit was only about the (atomic) trigger call during those operations (which may be delayed if happening during other operations). So, the case you suggested in the above can't happen in reality; PCM core won't fire such trigger.
Of course, if you observe a race by fuzzing or such, then it'd be worth for investigating further, though.
Takashi
On 2022-10-28 3:38 PM, Takashi Iwai wrote:
On Wed, 12 Oct 2022 16:07:49 +0200, Cezary Rojewski wrote:
Hello,
Writing with question regarding dpcm_set_fe_update_state() function which is part of sound/soc/soc-pcm.c file and has been introduced with commit "ASoC: dpcm: Fix race between FE/BE updates and trigger" [1].
The part that concerns me is the invocation of dpcm_fe_dai_do_trigger() regardless of the actual state given DPCM is in (actual state, not the DPCM_UPDATE_XX). The conditional invocation of said _trigger() and addition of .trigger_pending field are here to address a race where PCM state is being modified from multiple locations simultaneously, at least judging by the commit's description.
Note that the dpcm_set_fe_update_state() is called from all the dai-ops i.e.: startup, shutdown, hw_params, prepare and hw_free. Now, given that knowledge, we could end up in scenario where dpcm_fe_dai_do_trigger() is invoked as a part of dpcm_fe_dai_hw_free(). dpcm_set_fe_update_state() is called there twice, once with DPCM_UPDATE_FE and once with DPCM_UPDATE_NO. The second case is the more interesting one since it's called **after** ->hw_free() callback is invoked for all the DAIs.
dpcm_fe_dai_hw_free() dpcm_set_fe_update_state(DPCM_UPDATE_FE) // fine (...) dpcm_be_dai_hw_free() // data allocated in hw_params // is freed here (...) dpcm_set_fe_update_state(DPCM_UPDATE_NO) // not fine
The last is *not fine* if the .trigger_pending is not a zero, and can lead to panic as code used during ->trigger() is often manipulating data allocated during ->hw_params() but that data has just been freed with ->hw_free().
Is what I'm looking at a bug? Or, perhaps there's something I'm missing in the picture. From my study, it seems that only dpcm_fe_dai_prepare() is a safe place for calling dpcm_fe_dai_do_trigger() - once .trigger_pending is taken into account. Any input is much appreciated.
First off, each prepare, hw_params, hw_free, etc call is protected by a mutex, so they can't be run simultaneously. And the commit was only about the (atomic) trigger call during those operations (which may be delayed if happening during other operations). So, the case you suggested in the above can't happen in reality; PCM core won't fire such trigger.
Of course, if you observe a race by fuzzing or such, then it'd be worth for investigating further, though.
Hello,
First things first - thank you so much for the answer. One bug less on Intel's side :D
Did not manage to connect the dots myself, logic around dpcm_set_fe_upadte_state() is not straightforward one. And yes, I've managed to get race going, but perhaps only because of an unguarded snd_pcm_stop() in the avs-driver i.e. called without stream-lock held. The scenario is quite complicated, requires several ongoing streams, AudioDSP exception and then firmware-recovery attempt.
Will see if something still pops up.
Until we meet again! Czarek
participants (2)
-
Cezary Rojewski -
Takashi Iwai