* [PATCH] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()
[not found] <CGME20201127014343epcas2p10bf524de048e0a659aa2486080375a75@epcas2p1.samsung.com>
@ 2020-11-27 1:43 ` Gyeongtaek Lee
0 siblings, 0 replies; 4+ messages in thread
From: Gyeongtaek Lee @ 2020-11-27 1:43 UTC (permalink / raw)
To: alsa-devel
Cc: khw0178.kim, kimty, jaewons.kim, donggyun.ko, hmseo,
seungbin.lee, 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.
/* In core X, running dpcm_be_dai_trigger() */
for_each_dpcm_be(fe, stream, dpcm) {
/* In core Y, running dpcm_be_disconnect() */
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);
/* In core X, running dpcm_be_dai_trigger() */
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_do_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 dcab9527ba3d..7c5d950a8628 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2073,6 +2073,9 @@ 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);
+
static int dpcm_do_trigger(struct snd_soc_dpcm *dpcm,
struct snd_pcm_substream *substream, int cmd)
{
@@ -2092,8 +2095,10 @@ 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;
@@ -2113,7 +2118,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
ret = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2123,7 +2128,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
ret = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2133,7 +2138,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
ret = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2142,12 +2147,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 = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP;
break;
@@ -2155,12 +2160,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 = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND;
break;
@@ -2168,17 +2173,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 = dpcm_do_trigger(dpcm, 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;
}
@@ -2916,10 +2924,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)
@@ -2933,17 +2940,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[] = {
@@ -2954,6 +2956,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);
/*
@@ -2963,6 +2982,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,
@@ -2970,6 +2992,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);
base-commit: fa02fcd94b0c8dff6cc65714510cf25ad194b90d
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [PATCH] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()
2020-12-02 22:33 ` Kuninori Morimoto
@ 2020-12-03 1:06 ` Gyeongtaek Lee
0 siblings, 0 replies; 4+ messages in thread
From: Gyeongtaek Lee @ 2020-12-03 1:06 UTC (permalink / raw)
To: 'Kuninori Morimoto', cpgs
Cc: alsa-devel, khw0178.kim, 'Takashi Iwai',
lgirdwood, 'Pierre-Louis Bossart',
kimty, donggyun.ko, hmseo, cpgs, s47.kang, pilsun.jang, tkjung
On 03 Dec 2020 07:33:03 +0900, Kuninori Morimoto wrote:
>Hi Gyeongtaek
>
>Thank you for your patch
>
>> If stop by underrun and DPCM BE disconnection is run simultaneously,
>> data abort can be occurred by the sequence below.
>>
>> /* In core X, running dpcm_be_dai_trigger() */
>> for_each_dpcm_be(fe, stream, dpcm) {
>> /* In core Y, running dpcm_be_disconnect() */
>> 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);
>> /* In core X, running dpcm_be_dai_trigger() */
>> struct snd_soc_pcm_runtime *be = dpcm->be; <== Accessing freed memory
>
>It is easy to read/understand if this code has alignment.
>
>> To prevent this situation, dpcm_lock should be acquired during
>> iteration of dpcm list in dpcm_do_trigger().
>>
>> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
>> Cc: stable@vger.kernel.org
>> ---
>
>Is this bugfix patch for some recent commit ?
>If so, having Fixes: tag is helpful.
>
>Thank you for your help !!
Thank you for your review.
I'll resend the patch v2 after fixing the patch as
your review comment.
Thanks again.
Gyeongtaek Lee
>
>Best regards
>---
>Kuninori Morimoto
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()
2020-12-02 7:26 ` Gyeongtaek Lee
@ 2020-12-02 22:33 ` Kuninori Morimoto
2020-12-03 1:06 ` Gyeongtaek Lee
0 siblings, 1 reply; 4+ messages in thread
From: Kuninori Morimoto @ 2020-12-02 22:33 UTC (permalink / raw)
To: Gyeongtaek Lee
Cc: alsa-devel, khw0178.kim, 'Takashi Iwai',
'Pierre-Louis Bossart',
lgirdwood, kimty, donggyun.ko, hmseo, cpgs, s47.kang,
pilsun.jang, tkjung
Hi Gyeongtaek
Thank you for your patch
> If stop by underrun and DPCM BE disconnection is run simultaneously,
> data abort can be occurred by the sequence below.
>
> /* In core X, running dpcm_be_dai_trigger() */
> for_each_dpcm_be(fe, stream, dpcm) {
> /* In core Y, running dpcm_be_disconnect() */
> 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);
> /* In core X, running dpcm_be_dai_trigger() */
> struct snd_soc_pcm_runtime *be = dpcm->be; <== Accessing freed memory
It is easy to read/understand if this code has alignment.
> To prevent this situation, dpcm_lock should be acquired during
> iteration of dpcm list in dpcm_do_trigger().
>
> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
> Cc: stable@vger.kernel.org
> ---
Is this bugfix patch for some recent commit ?
If so, having Fixes: tag is helpful.
Thank you for your help !!
Best regards
---
Kuninori Morimoto
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()
[not found] <CGME20201202072607epcas2p43171cd23ab1752db8d71b2ed5f581aa8@epcas2p4.samsung.com>
@ 2020-12-02 7:26 ` Gyeongtaek Lee
2020-12-02 22:33 ` Kuninori Morimoto
0 siblings, 1 reply; 4+ messages in thread
From: Gyeongtaek Lee @ 2020-12-02 7:26 UTC (permalink / raw)
To: kuninori.morimoto.gx, cpgs
Cc: alsa-devel, khw0178.kim, 'Takashi Iwai',
'Pierre-Louis Bossart',
lgirdwood, kimty, donggyun.ko, hmseo, 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.
/* In core X, running dpcm_be_dai_trigger() */
for_each_dpcm_be(fe, stream, dpcm) {
/* In core Y, running dpcm_be_disconnect() */
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);
/* In core X, running dpcm_be_dai_trigger() */
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_do_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 dcab9527ba3d..7c5d950a8628 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2073,6 +2073,9 @@ 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);
+
static int dpcm_do_trigger(struct snd_soc_dpcm *dpcm,
struct snd_pcm_substream *substream, int cmd)
{
@@ -2092,8 +2095,10 @@ 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;
@@ -2113,7 +2118,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
ret = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2123,7 +2128,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
ret = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2133,7 +2138,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
ret = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
break;
@@ -2142,12 +2147,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 = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP;
break;
@@ -2155,12 +2160,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 = dpcm_do_trigger(dpcm, be_substream, cmd);
if (ret)
- return ret;
+ break;
be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND;
break;
@@ -2168,17 +2173,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 = dpcm_do_trigger(dpcm, 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;
}
@@ -2916,10 +2924,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)
@@ -2933,17 +2940,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[] = {
@@ -2954,6 +2956,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);
/*
@@ -2963,6 +2982,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,
@@ -2970,6 +2992,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);
base-commit: fa02fcd94b0c8dff6cc65714510cf25ad194b90d
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-12-03 2:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20201127014343epcas2p10bf524de048e0a659aa2486080375a75@epcas2p1.samsung.com>
2020-11-27 1:43 ` [PATCH] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger() Gyeongtaek Lee
[not found] <CGME20201202072607epcas2p43171cd23ab1752db8d71b2ed5f581aa8@epcas2p4.samsung.com>
2020-12-02 7:26 ` Gyeongtaek Lee
2020-12-02 22:33 ` Kuninori Morimoto
2020-12-03 1:06 ` Gyeongtaek Lee
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.