The branch main has been updated by christos:
URL:
https://cgit.FreeBSD.org/src/commit/?id=5ac39263d825d7b2f8a89614a63fee90ffc77c07
commit 5ac39263d825d7b2f8a89614a63fee90ffc77c07
Author: Christos Margiolis <chris...@freebsd.org>
AuthorDate: 2024-11-26 14:48:18 +0000
Commit: Christos Margiolis <chris...@freebsd.org>
CommitDate: 2024-11-26 14:48:18 +0000
sound: Fix chn_trigger() and vchan_trigger() races
Consider the following scenario:
1. CHN currently has its trigger set to PCMTRIG_STOP.
2. Thread A locks CHN, calls CHANNEL_TRIGGER(PCMTRIG_START), sets the
trigger to PCMTRIG_START and unlocks.
3. Thread B picks up the lock, calls CHANNEL_TRIGGER(PCMTRIG_ABORT) and
returns a non-zero value, so it returns from chn_trigger() as well.
4. Thread A picks up the lock and adds CHN to the list, which is
_wrong_, because the last call to CHANNEL_TRIGGER() was with
PCMTRIG_ABORT, meaning the channel is stopped, yet we are adding it
to the list and marking it as started.
Another problematic scenario:
1. Thread A locks CHN, sets the trigger to PCMTRIG_ABORT, and unlocks
CHN. It then locks PCM and _removes_ CHN from the list.
2. In the meantime, since thread A unlocked CHN, thread B has locked it,
set the trigger to PCMTRIG_START, unlocked it, and is now blocking on
PCM held by thread A.
3. At the same time, thread C locks CHN, sets the trigger back to
PCMTRIG_ABORT, unlocks CHN, and is also blocking on PCM. However,
once thread A unlocks PCM, because thread C is higher-priority than
thread B, it picks up the PCM lock instead of thread B, and because
CHN is already removed from the list, and thread B hasn't added it
back yet, we take a page fault in CHN_REMOVE() by trying to remove a
non-existent element.
To fix the former scenario, set the channel trigger before the call to
CHANNEL_TRIGGER() (could also come after, doesn't really matter) and
check if anything changed one we lock CHN back.
To fix the latter scenario, use the SAFE variants of CHN_INSERT_HEAD()
and CHN_REMOVE(). A similar scenario can occur in vchan_trigger(), so do
the trigger setting after we've locked the parent channel.
Sponsored by: The FreeBSD Foundation
MFC after: 2 days
Reviewed by: dev_submerge.ch
Differential Revision: https://reviews.freebsd.org/D47461
---
sys/dev/sound/pcm/channel.c | 56 +++++++++++++++++++++++----------------------
sys/dev/sound/pcm/vchan.c | 11 ++++-----
2 files changed, 34 insertions(+), 33 deletions(-)
diff --git a/sys/dev/sound/pcm/channel.c b/sys/dev/sound/pcm/channel.c
index eb70e910f51d..59cdb8cd07f5 100644
--- a/sys/dev/sound/pcm/channel.c
+++ b/sys/dev/sound/pcm/channel.c
@@ -2318,44 +2318,46 @@ chn_trigger(struct pcm_channel *c, int go)
if (go == c->trigger)
return (0);
+ if (snd_verbose > 3) {
+ device_printf(c->dev, "%s() %s: calling go=0x%08x , "
+ "prev=0x%08x\n", __func__, c->name, go, c->trigger);
+ }
+
+ c->trigger = go;
ret = CHANNEL_TRIGGER(c->methods, c->devinfo, go);
if (ret != 0)
return (ret);
+ CHN_UNLOCK(c);
+ PCM_LOCK(d);
+ CHN_LOCK(c);
+
+ /*
+ * Do nothing if another thread set a different trigger while we had
+ * dropped the mutex.
+ */
+ if (go != c->trigger) {
+ PCM_UNLOCK(d);
+ return (0);
+ }
+
+ /*
+ * Use the SAFE variants to prevent inserting/removing an already
+ * existing/missing element.
+ */
switch (go) {
case PCMTRIG_START:
- if (snd_verbose > 3)
- device_printf(c->dev,
- "%s() %s: calling go=0x%08x , "
- "prev=0x%08x\n", __func__, c->name, go,
- c->trigger);
- if (c->trigger != PCMTRIG_START) {
- c->trigger = go;
- CHN_UNLOCK(c);
- PCM_LOCK(d);
- CHN_INSERT_HEAD(d, c, channels.pcm.busy);
- PCM_UNLOCK(d);
- CHN_LOCK(c);
- chn_syncstate(c);
- }
+ CHN_INSERT_HEAD_SAFE(d, c, channels.pcm.busy);
+ PCM_UNLOCK(d);
+ chn_syncstate(c);
break;
case PCMTRIG_STOP:
case PCMTRIG_ABORT:
- if (snd_verbose > 3)
- device_printf(c->dev,
- "%s() %s: calling go=0x%08x , "
- "prev=0x%08x\n", __func__, c->name, go,
- c->trigger);
- if (c->trigger == PCMTRIG_START) {
- c->trigger = go;
- CHN_UNLOCK(c);
- PCM_LOCK(d);
- CHN_REMOVE(d, c, channels.pcm.busy);
- PCM_UNLOCK(d);
- CHN_LOCK(c);
- }
+ CHN_REMOVE_SAFE(d, c, channels.pcm.busy);
+ PCM_UNLOCK(d);
break;
default:
+ PCM_UNLOCK(d);
break;
}
diff --git a/sys/dev/sound/pcm/vchan.c b/sys/dev/sound/pcm/vchan.c
index ed13a3c55961..4165d0712b94 100644
--- a/sys/dev/sound/pcm/vchan.c
+++ b/sys/dev/sound/pcm/vchan.c
@@ -146,20 +146,19 @@ vchan_trigger(kobj_t obj, void *data, int go)
int ret, otrigger;
info = data;
-
- if (!PCMTRIG_COMMON(go) || go == info->trigger)
- return (0);
-
c = info->channel;
p = c->parentchannel;
- otrigger = info->trigger;
- info->trigger = go;
CHN_LOCKASSERT(c);
+ if (!PCMTRIG_COMMON(go) || go == info->trigger)
+ return (0);
CHN_UNLOCK(c);
CHN_LOCK(p);
+ otrigger = info->trigger;
+ info->trigger = go;
+
switch (go) {
case PCMTRIG_START:
if (otrigger != PCMTRIG_START)
