Re: [RFC PATCH v2 0/5] ASoC: soc-pcm: fix trigger race conditions with shared BE

[Takashi Iwai <tiwai@suse.de>]

On Thu, 07 Oct 2021 20:13:22 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >> Using snd_pcm_stream_lock_irqsave(be_substream, flags); will prevent
> >> multiple triggers indeed, but the state management is handled by
> >> dpcm_lock, so I think we have to use dpcm_lock/mutex in all BE transitions.
> >>
> >> if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) &&
> >>     (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) &&
> >>  			    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
> >>
> >> if ((be->dpcm[stream].state != SND_SOC_DPCM_STATE_PREPARE) &&
> >>     (be->dpcm[stream].state != SND_SOC_DPCM_STATE_STOP) &&
> >>  			    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
> > 
> > The stream lock can be put around those appropriate places, I suppose?
> 
> I doubled checked the code a bit more, and all functions using
> be->dpcm[stream].state and be->dpcm[stream].users are protected by the
> card->mutex.
> 
> The exceptions are dpcm_be_dai_trigger() and dpcm_show_state() so we
> probably don't need to worry too much about these fields.
> 
> I am more nervous about that the dpcm_lock was supposed to protect. It
> was added in "ASoC: dpcm: prevent snd_soc_dpcm use after free" to solve
> a race condition, according to the commit log between
> 
> void dpcm_be_disconnect(
>     	...
>     	list_del(&dpcm->list_be);
>     	list_del(&dpcm->list_fe);
>     	kfree(dpcm);
>     	...
> 
> and
>     	for_each_dpcm_fe()
>     	for_each_dpcm_be*()
> 
> That would suggest that every one of those loops should be protected,
> but that's not the case at all. In some cases the spinlock is taken
> *inside* of the loops.
> 
> I think this is what explains the problem reported by Gyeongtaek Lee in
> 
> https://lore.kernel.org/alsa-devel/002f01d7b4f5$c030f4a0$4092dde0$@samsung.com/
> 
> the for_each_dpcm_be() loop in dpcm_be_dai_trigger() is NOT protected.
> 
> But if we add a spin-lock in there, the atomicity remains a problem.
> 
> I think the only solution is to follow the example of the PCM case,
> where the type of lock depends on the FE types, with the assumption that
> there are no mixed atomic/non-atomic FE configurations.

Yes, and I guess we can simply replace those all card->dpcm_lock with
FE's stream lock.  It'll solve the atomicity problem, too, and the FE
stream lock can be applied outside the loop of dpcm_be_disconnect()
gracefully.

And, this should solve the race with dpcm_be_dai_trigger() as well,
because it's called from dpcm_fe_dai_trigger() that is called already
inside the FE's stream lock held by PCM core.  A PoC is something like
below.  (I replaced the superfluous *_irqsave with *_irq there)

The scenario above (using the FE stream lock) is one missing piece,
though: the compress API seems using the DPCM, and this might not
work.  It needs more verification.


BTW, looking through the current code, I wonder how the
snd_pcm_stream_lock_irq() call in dpcm_set_fe_update_state() doesn't
deadlock when nonatomic=true.  The function may be called from
dpcm_fe_dai_prepare(), which is a PCM prepare callback for FE.  And, a
PCM prepare callback is called always inside the stream mutex, which
is *the* stream lock in the case of nonatomic mode.
Maybe I might overlook something obvious...


thanks,

Takashi

---
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 8e6dd8a257c5..5872a8864f3b 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -893,8 +893,6 @@ struct snd_soc_card {
 	struct mutex pcm_mutex;
 	enum snd_soc_pcm_subclass pcm_subclass;
 
-	spinlock_t dpcm_lock;
-
 	int (*probe)(struct snd_soc_card *card);
 	int (*late_probe)(struct snd_soc_card *card);
 	int (*remove)(struct snd_soc_card *card);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index c830e96afba2..51ef9917ca98 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2339,7 +2339,6 @@ int snd_soc_register_card(struct snd_soc_card *card)
 	mutex_init(&card->mutex);
 	mutex_init(&card->dapm_mutex);
 	mutex_init(&card->pcm_mutex);
-	spin_lock_init(&card->dpcm_lock);
 
 	return snd_soc_bind_card(card);
 }
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 48f71bb81a2f..c171e5f431b9 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -38,6 +38,15 @@ static inline const char *soc_codec_dai_name(struct snd_soc_pcm_runtime *rtd)
 	return (rtd)->num_codecs == 1 ? asoc_rtd_to_codec(rtd, 0)->name : "multicodec";
 }
 
+#define dpcm_fe_stream_lock_irq(fe, stream) \
+	snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(fe, stream))
+#define dpcm_fe_stream_unlock_irq(fe, stream) \
+	snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(fe, stream))
+#define dpcm_fe_stream_lock_irqsave(fe, stream, flags)			\
+	snd_pcm_stream_lock_irq(snd_soc_dpcm_get_substream(fe, stream), flags)
+#define dpcm_fe_stream_unlock_irqrestore(fe, stream, flags)		\
+	snd_pcm_stream_unlock_irq(snd_soc_dpcm_get_substream(fe, stream), flags)
+
 #ifdef CONFIG_DEBUG_FS
 static const char *dpcm_state_string(enum snd_soc_dpcm_state state)
 {
@@ -73,7 +82,6 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
 	struct snd_pcm_hw_params *params = &fe->dpcm[stream].hw_params;
 	struct snd_soc_dpcm *dpcm;
 	ssize_t offset = 0;
-	unsigned long flags;
 
 	/* FE state */
 	offset += scnprintf(buf + offset, size - offset,
@@ -101,7 +109,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
 		goto out;
 	}
 
-	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_lock_irq(fe, stream);
 	for_each_dpcm_be(fe, stream, dpcm) {
 		struct snd_soc_pcm_runtime *be = dpcm->be;
 		params = &dpcm->hw_params;
@@ -122,7 +130,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
 					   params_channels(params),
 					   params_rate(params));
 	}
-	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_unlock_irq(fe, stream);
 out:
 	return offset;
 }
@@ -1129,7 +1137,6 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
 		struct snd_soc_pcm_runtime *be, int stream)
 {
 	struct snd_soc_dpcm *dpcm;
-	unsigned long flags;
 
 	/* only add new dpcms */
 	for_each_dpcm_be(fe, stream, dpcm) {
@@ -1141,14 +1148,14 @@ static int dpcm_be_connect(struct snd_soc_pcm_runtime *fe,
 	if (!dpcm)
 		return -ENOMEM;
 
+	dpcm_fe_stream_lock_irq(fe, stream);
 	dpcm->be = be;
 	dpcm->fe = fe;
 	be->dpcm[stream].runtime = fe->dpcm[stream].runtime;
 	dpcm->state = SND_SOC_DPCM_LINK_STATE_NEW;
-	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
 	list_add(&dpcm->list_be, &fe->dpcm[stream].be_clients);
 	list_add(&dpcm->list_fe, &be->dpcm[stream].fe_clients);
-	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_unlock_irq(fe, stream);
 
 	dev_dbg(fe->dev, "connected new DPCM %s path %s %s %s\n",
 			stream ? "capture" : "playback",  fe->dai_link->name,
@@ -1191,8 +1198,8 @@ static void dpcm_be_reparent(struct snd_soc_pcm_runtime *fe,
 void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 {
 	struct snd_soc_dpcm *dpcm, *d;
-	unsigned long flags;
 
+	dpcm_fe_stream_lock_irq(fe, stream);
 	for_each_dpcm_be_safe(fe, stream, dpcm, d) {
 		dev_dbg(fe->dev, "ASoC: BE %s disconnect check for %s\n",
 				stream ? "capture" : "playback",
@@ -1210,12 +1217,11 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
 
 		dpcm_remove_debugfs_state(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);
 	}
+	dpcm_fe_stream_unlock_irq(fe, stream);
 }
 
 /* get BE for DAI widget and stream */
@@ -1429,12 +1435,11 @@ int dpcm_process_paths(struct snd_soc_pcm_runtime *fe,
 void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream)
 {
 	struct snd_soc_dpcm *dpcm;
-	unsigned long flags;
 
-	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_lock_irq(fe, stream);
 	for_each_dpcm_be(fe, stream, dpcm)
 		dpcm_set_be_update_state(dpcm->be, stream, SND_SOC_DPCM_UPDATE_NO);
-	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_unlock_irq(fe, stream);
 }
 
 void dpcm_be_dai_stop(struct snd_soc_pcm_runtime *fe, int stream,
@@ -2352,7 +2357,6 @@ static int dpcm_run_update_startup(struct snd_soc_pcm_runtime *fe, int stream)
 	struct snd_soc_dpcm *dpcm;
 	enum snd_soc_dpcm_trigger trigger = fe->dai_link->trigger[stream];
 	int ret = 0;
-	unsigned long flags;
 
 	dev_dbg(fe->dev, "ASoC: runtime %s open on FE %s\n",
 			stream ? "capture" : "playback", fe->dai_link->name);
@@ -2421,7 +2425,7 @@ static int dpcm_run_update_startup(struct snd_soc_pcm_runtime *fe, int stream)
 	dpcm_be_dai_shutdown(fe, stream);
 disconnect:
 	/* disconnect any pending BEs */
-	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_lock_irq(fe, stream);
 	for_each_dpcm_be(fe, stream, dpcm) {
 		struct snd_soc_pcm_runtime *be = dpcm->be;
 
@@ -2433,7 +2437,7 @@ static int dpcm_run_update_startup(struct snd_soc_pcm_runtime *fe, int stream)
 			be->dpcm[stream].state == SND_SOC_DPCM_STATE_NEW)
 				dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
 	}
-	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_unlock_irq(fe, stream);
 
 	if (ret < 0)
 		dev_err(fe->dev, "ASoC: %s() failed (%d)\n", __func__, ret);
@@ -2843,10 +2847,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);
+	dpcm_fe_stream_lock_irq(fe, stream);
 	for_each_dpcm_fe(be, stream, dpcm) {
 
 		if (dpcm->fe == fe)
@@ -2860,7 +2863,7 @@ static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
 			}
 		}
 	}
-	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+	dpcm_fe_stream_unlock_irq(fe, stream);
 
 	/* it's safe to do this BE DAI */
 	return ret;