Re: [PATCH v4 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 17 Feb 2021 05:31:49 +0100,
Gyeongtaek Lee wrote:
> 
> If stop by underrun and DPCM BE disconnection is run simultaneously,
> data abort can be occurred by the sequence below.
> 
> CPU0					CPU1
> 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 should be acquired during
> iteration of dpcm list in dpcm_be_dai_trigger().

I don't think we can apply spin lock there blindly.  There is
non-atomic PCM that must not take a spin lock there, too.


thanks,

Takashi

> 
> Signed-off-by: Gyeongtaek Lee <gt82.lee@xxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
>  sound/soc/soc-pcm.c | 62 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index ee51dc7fd893..718f6b3a309a 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2074,12 +2074,17 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
>  	return ret;
>  }
>  
> +static int dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
> +		struct snd_soc_pcm_runtime *be, int stream);
> +
>  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  			       int cmd)
>  {
>  	struct snd_soc_dpcm *dpcm;
> +	unsigned long flags;
>  	int ret = 0;
>  
> +	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
>  	for_each_dpcm_be(fe, stream, dpcm) {
>  
>  		struct snd_soc_pcm_runtime *be = dpcm->be;
> @@ -2102,7 +2107,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  
>  			ret = soc_pcm_trigger(be_substream, cmd);
>  			if (ret)
> -				return ret;
> +				break;
>  
>  			be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
>  			break;
> @@ -2112,7 +2117,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  
>  			ret = soc_pcm_trigger(be_substream, cmd);
>  			if (ret)
> -				return ret;
> +				break;
>  
>  			be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
>  			break;
> @@ -2122,7 +2127,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  
>  			ret = soc_pcm_trigger(be_substream, cmd);
>  			if (ret)
> -				return ret;
> +				break;
>  
>  			be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
>  			break;
> @@ -2131,12 +2136,12 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  			    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
>  				continue;
>  
> -			if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
> +			if (!dpcm_can_be_free_stop(fe, be, stream))
>  				continue;
>  
>  			ret = soc_pcm_trigger(be_substream, cmd);
>  			if (ret)
> -				return ret;
> +				break;
>  
>  			be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP;
>  			break;
> @@ -2144,12 +2149,12 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  			if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
>  				continue;
>  
> -			if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
> +			if (!dpcm_can_be_free_stop(fe, be, stream))
>  				continue;
>  
>  			ret = soc_pcm_trigger(be_substream, cmd);
>  			if (ret)
> -				return ret;
> +				break;
>  
>  			be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND;
>  			break;
> @@ -2157,17 +2162,20 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>  			if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
>  				continue;
>  
> -			if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
> +			if (!dpcm_can_be_free_stop(fe, be, stream))
>  				continue;
>  
>  			ret = soc_pcm_trigger(be_substream, cmd);
>  			if (ret)
> -				return ret;
> +				break;
>  
>  			be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
>  			break;
>  		}
> +		if (ret)
> +			break;
>  	}
> +	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
>  
>  	return ret;
>  }
> @@ -2905,10 +2913,9 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
>  	struct snd_soc_dpcm *dpcm;
>  	int state;
>  	int ret = 1;
> -	unsigned long flags;
>  	int i;
>  
> -	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> +	lockdep_assert_held(&fe->card->dpcm_lock);
>  	for_each_dpcm_fe(be, stream, dpcm) {
>  
>  		if (dpcm->fe == fe)
> @@ -2922,17 +2929,12 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
>  			}
>  		}
>  	}
> -	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
>  
>  	/* it's safe to do this BE DAI */
>  	return ret;
>  }
>  
> -/*
> - * We can only hw_free, stop, pause or suspend a BE DAI if any of it's FE
> - * are not running, paused or suspended for the specified stream direction.
> - */
> -int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
> +static int dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
>  		struct snd_soc_pcm_runtime *be, int stream)
>  {
>  	const enum snd_soc_dpcm_state state[] = {
> @@ -2943,6 +2945,23 @@ int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
>  
>  	return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
>  }
> +
> +/*
> + * We can only hw_free, stop, pause or suspend a BE DAI if any of it's FE
> + * are not running, paused or suspended for the specified stream direction.
> + */
> +int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
> +		struct snd_soc_pcm_runtime *be, int stream)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> +	ret =  dpcm_can_be_free_stop(fe, be, stream);
> +	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_free_stop);
>  
>  /*
> @@ -2952,6 +2971,9 @@ EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_free_stop);
>  int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
>  		struct snd_soc_pcm_runtime *be, int stream)
>  {
> +	unsigned long flags;
> +	int ret;
> +
>  	const enum snd_soc_dpcm_state state[] = {
>  		SND_SOC_DPCM_STATE_START,
>  		SND_SOC_DPCM_STATE_PAUSED,
> @@ -2959,6 +2981,10 @@ int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
>  		SND_SOC_DPCM_STATE_PREPARE,
>  	};
>  
> -	return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
> +	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> +	ret = snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
> +	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_params);
> -- 
> 2.21.0
> 
> 
> 



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux