| Message ID | 1773638882.61613536382796.JavaMail.epsvc@epcpadp4 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger() | expand |
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 > > >
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);
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(). 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(-)