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