git: 5ac39263d825 - main - sound: Fix chn_trigger() and vchan_trigger() races

The branch main has been updated by christos:

URL: https://cgit.FreeBSD.org/src/commit/?id=5ac39263d825d7b2f8a89614a63fee90ffc77c07

commit 5ac39263d825d7b2f8a89614a63fee90ffc77c07
Author:     Christos Margiolis <christos@FreeBSD.org>
AuthorDate: 2024-11-26 14:48:18 +0000
Commit:     Christos Margiolis <christos@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)