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@samsung.com Cc: stable@vger.kernel.org
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