* [PATCH v4 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()
[not found] <CGME20210217043150epcas2p3d7776bc10dc822875cc23b3e721658b6@epcas2p3.samsung.com>
@ 2021-02-17 4:31 ` Gyeongtaek Lee
2021-02-17 7:29 ` Takashi Iwai
0 siblings, 1 reply; 2+ messages in thread
From: Gyeongtaek Lee @ 2021-02-17 4:31 UTC (permalink / raw)
To: 'Kuninori Morimoto', broonie, cpgs
Cc: alsa-devel, khw0178.kim, 'Takashi Iwai',
'Pierre-Louis Bossart',
lgirdwood, kimty, donggyun.ko, hmseo, cpgs, s47.kang,
pilsun.jang, tkjung
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(-)
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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v4 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()
2021-02-17 4:31 ` [PATCH v4 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger() Gyeongtaek Lee
@ 2021-02-17 7:29 ` Takashi Iwai
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2021-02-17 7:29 UTC (permalink / raw)
To: Gyeongtaek Lee
Cc: alsa-devel, khw0178.kim, 'Kuninori Morimoto',
kimty, lgirdwood, 'Pierre-Louis Bossart',
broonie, hmseo, cpgs, donggyun.ko, s47.kang, pilsun.jang, tkjung
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
>
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-02-17 7:30 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20210217043150epcas2p3d7776bc10dc822875cc23b3e721658b6@epcas2p3.samsung.com>
2021-02-17 4:31 ` [PATCH v4 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger() Gyeongtaek Lee
2021-02-17 7:29 ` Takashi Iwai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).