[PATCH] ASoC: dpcm: fix race condition to dpcm links in dpcm_be_dai_trigger

[Pierre-Louis Bossart]

On 9/29/21 12:49 AM, Gyeongtaek Lee wrote:
> If routing change and underrun stop is run at the same time,
> data abort can be occurred by the following sequence.
> 
> CPU0: Processing underrun 	CPU1: Processing routing change
> dpcm_be_dai_trigger():		dpcm_be_disconnect():
> 
> for_each_dpcm_be(fe, stream, dpcm) {
> 
> 				spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> 				list_del(&dpcm->list_be);
> 				list_del(&dpcm->list_fe);
> 				spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
> 				kfree(dpcm);
> 
> struct snd_soc_pcm_runtime *be = dpcm->be; <-- Accessing freed memory
> 
> To prevent this situation, dpcm_lock is needed during accessing
> the lists for dpcm links.

Isn't there still a possible inconsistency here introduced by the
duplication of the BE list?

You protect the list creation, but before you use it in
dpcm_be_dai_trigger(), there's a time window where the function could be
pre-empted and a disconnect event might have happened. As a result you
could trigger a BE that's no longer connected.

What you identified as a race is likely valid, but how to fix it isn't
clear to me - the DPCM code isn't self-explanatory at all with its use
in various places of the dpcm_lock spinlock, the pcm mutex, the card mutex.

Ideally we would need to find a way to prevent changes in connections
while we are doing the triggers, but triggers can take a bit of time if
they involve any sort of communication over a bus. I really wonder if
this dpcm_lock should be a mutex and if the model for DPCM really
involves interrupt contexts as the irqsave/irqrestore mentions hint at.

> Signed-off-by: Gyeongtaek Lee <gt82.lee at samsung.com>
> Cc: stable at vger.kernel.org
> ---
>  sound/soc/soc-pcm.c | 53 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 48f71bb81a2f..df2cd4c0dabe 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1993,17 +1993,63 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
>  	return ret;
>  }
>  
> +struct dpcm_be_list {
> +	unsigned int num;
> +	struct snd_soc_pcm_runtime *be[];
> +};
> +
> +static int dpcm_create_be_list(struct snd_soc_pcm_runtime *fe, int stream,
> +		struct dpcm_be_list **be_list)
> +{
> +	struct snd_soc_dpcm *dpcm;
> +	struct dpcm_be_list *be;
> +	int size = 0;
> +	int ret = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> +
> +	for_each_dpcm_be(fe, stream, dpcm)
> +		size++;
> +
> +	be = kzalloc(struct_size(be, be, size), GFP_ATOMIC);
> +	if (!be) {
> +		ret = -ENOMEM;
> +	} else {
> +		unsigned int i = 0;
> +
> +		for_each_dpcm_be(fe, stream, dpcm)
> +			be->be[i++] = dpcm->be;
> +
> +		*be_list = be;
> +	}
> +
> +	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
> +
> +	return ret;
> +}
> +
> +static void dpcm_free_be_list(struct dpcm_be_list *be_list)
> +{
> +	kfree(be_list);
> +}
> +
>  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  			       int cmd)
>  {
>  	struct snd_soc_pcm_runtime *be;
> -	struct snd_soc_dpcm *dpcm;
> +	struct dpcm_be_list *be_list;
>  	int ret = 0;
> +	int i;
>  
> -	for_each_dpcm_be(fe, stream, dpcm) {
> +	ret = dpcm_create_be_list(fe, stream, &be_list);
> +	if (ret < 0)
> +		return ret;
> +
> +	for(i = 0; i < be_list->num; i++) {
>  		struct snd_pcm_substream *be_substream;
>  
> -		be = dpcm->be;
> +		be = be_list->be[i];
>  		be_substream = snd_soc_dpcm_get_substream(be, stream);
>  
>  		/* is this op for this BE ? */
> @@ -2092,6 +2138,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  	if (ret < 0)
>  		dev_err(fe->dev, "ASoC: %s() failed at %s (%d)\n",
>  			__func__, be->dai_link->name, ret);
> +	dpcm_free_be_list(be_list);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger);
> 
> base-commit: 4ac6d90867a4de2e12117e755dbd76e08d88697f
>