Question regarding delayed trigger in dpcm_set_fe_update_state()

Question regarding delayed trigger in dpcm_set_fe_update_state()

[Cezary Rojewski]

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/T/


Regards,
Czarek

Re: Question regarding delayed trigger in dpcm_set_fe_update_state()

[Cezary Rojewski]

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?

Re: Question regarding delayed trigger in dpcm_set_fe_update_state()

[Takashi Iwai]

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

Re: Question regarding delayed trigger in dpcm_set_fe_update_state()

[Cezary Rojewski]

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