Re: Question regarding delayed trigger in dpcm_set_fe_update_state()

[Cezary Rojewski <cezary.rojewski@intel.com>]

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