git: 03614fcba25b - main - sound: Fix panic caused by sleeping-channel destruction during asynchronous detach

The branch main has been updated by christos:

URL: https://cgit.FreeBSD.org/src/commit/?id=03614fcba25b9de99e69819bc4690f66a3d24438

commit 03614fcba25b9de99e69819bc4690f66a3d24438
Author:     Christos Margiolis <christos@FreeBSD.org>
AuthorDate: 2024-04-28 19:40:40 +0000
Commit:     Christos Margiolis <christos@FreeBSD.org>
CommitDate: 2024-04-28 19:48:24 +0000

    sound: Fix panic caused by sleeping-channel destruction during asynchronous detach
    
    Currently we are force-destroying all channels unconditionally in
    pcm_killchan(). However, since asynchronous audio device detach is
    possible as of 44e128fe9d92, if we do not check whether the channel is
    sleeping or not and forcefully kill it, we will get a panic from
    cv_timedwait_sig() (called from chn_sleep()), because it will try to use
    a freed lock/cv.
    
    Modify pcm_killchan() (renamed to pcm_killchans() since that's a more
    appropriate name now) to loop through the channel list and destroy only
    the channels that are awake, otherwise wake up the sleeping thread and
    try again. This loop is repeated until all channels are awakened and
    destroyed.
    
    To reduce code duplication, implement chn_shutdown() which wakes up the
    channel and sets CHN_F_DEAD, and use it in pcm_unregister() and
    pcm_killchans().
    
    Reported by:    KASAN
    Fixes:          44e128fe9d92 ("sound: Implement asynchronous device detach")
    Sponsored by:   The FreeBSD Foundation
    MFC after:      1 day
    Reviewed by:    markj
    Differential Revision:  https://reviews.freebsd.org/D44923
---
 sys/dev/sound/pcm/channel.c |  9 +++++++
 sys/dev/sound/pcm/channel.h |  1 +
 sys/dev/sound/pcm/sound.c   | 64 ++++++++++++++++++++++++++++++---------------
 3 files changed, 53 insertions(+), 21 deletions(-)

diff --git a/sys/dev/sound/pcm/channel.c b/sys/dev/sound/pcm/channel.c
index b4872fdb8037..cf9239839aca 100644
--- a/sys/dev/sound/pcm/channel.c
+++ b/sys/dev/sound/pcm/channel.c
@@ -1301,6 +1301,15 @@ chn_kill(struct pcm_channel *c)
 	return (0);
 }
 
+void
+chn_shutdown(struct pcm_channel *c)
+{
+	CHN_LOCKASSERT(c);
+
+	chn_wakeup(c);
+	c->flags |= CHN_F_DEAD;
+}
+
 int
 chn_setvolume_multi(struct pcm_channel *c, int vc, int left, int right,
     int center)
diff --git a/sys/dev/sound/pcm/channel.h b/sys/dev/sound/pcm/channel.h
index 21007454584e..c8d33c583188 100644
--- a/sys/dev/sound/pcm/channel.h
+++ b/sys/dev/sound/pcm/channel.h
@@ -264,6 +264,7 @@ int chn_poll(struct pcm_channel *c, int ev, struct thread *td);
 
 int chn_init(struct pcm_channel *c, void *devinfo, int dir, int direction);
 int chn_kill(struct pcm_channel *c);
+void chn_shutdown(struct pcm_channel *c);
 int chn_reset(struct pcm_channel *c, u_int32_t fmt, u_int32_t spd);
 int chn_setvolume_multi(struct pcm_channel *c, int vc, int left, int right,
     int center);
diff --git a/sys/dev/sound/pcm/sound.c b/sys/dev/sound/pcm/sound.c
index faf4bd1c7dce..0a88d732def7 100644
--- a/sys/dev/sound/pcm/sound.c
+++ b/sys/dev/sound/pcm/sound.c
@@ -674,23 +674,50 @@ pcm_addchan(device_t dev, int dir, kobj_class_t cls, void *devinfo)
 	return (err);
 }
 
-static int
-pcm_killchan(device_t dev)
+static void
+pcm_killchans(struct snddev_info *d)
 {
-	struct snddev_info *d = device_get_softc(dev);
 	struct pcm_channel *ch;
 	int error;
+	bool found;
 
 	PCM_BUSYASSERT(d);
+	do {
+		found = false;
+		CHN_FOREACH(ch, d, channels.pcm) {
+			CHN_LOCK(ch);
+			/*
+			 * Make sure no channel has went to sleep in the
+			 * meantime.
+			 */
+			chn_shutdown(ch);
+			/*
+			 * We have to give a thread sleeping in chn_sleep() a
+			 * chance to observe that the channel is dead.
+			 */
+			if ((ch->flags & CHN_F_SLEEPING) == 0) {
+				found = true;
+				CHN_UNLOCK(ch);
+				break;
+			}
+			CHN_UNLOCK(ch);
+		}
 
-	ch = CHN_FIRST(d, channels.pcm);
+		/*
+		 * All channels are still sleeping. Sleep for a bit and try
+		 * again to see if any of them is awake now.
+		 */
+		if (!found) {
+			pause_sbt("pcmkillchans", SBT_1MS * 5, 0, 0);
+			continue;
+		}
 
-	PCM_LOCK(d);
-	error = pcm_chn_remove(d, ch);
-	PCM_UNLOCK(d);
-	if (error)
-		return (error);
-	return (pcm_chn_destroy(ch));
+		PCM_LOCK(d);
+		error = pcm_chn_remove(d, ch);
+		PCM_UNLOCK(d);
+		if (error == 0)
+			pcm_chn_destroy(ch);
+	} while (!CHN_EMPTY(d, channels.pcm));
 }
 
 static int
@@ -1000,15 +1027,11 @@ pcm_unregister(device_t dev)
 
 	CHN_FOREACH(ch, d, channels.pcm) {
 		CHN_LOCK(ch);
-		if (ch->flags & CHN_F_SLEEPING) {
-			/*
-			 * We are detaching, so do not wait for the timeout in
-			 * chn_read()/chn_write(). Wake up the thread and kill
-			 * the channel immediately.
-			 */
-			CHN_BROADCAST(&ch->intr_cv);
-			ch->flags |= CHN_F_DEAD;
-		}
+		/*
+		 * Do not wait for the timeout in chn_read()/chn_write(). Wake
+		 * up the sleeping thread and kill the channel.
+		 */
+		chn_shutdown(ch);
 		chn_abort(ch);
 		CHN_UNLOCK(ch);
 	}
@@ -1033,8 +1056,7 @@ pcm_unregister(device_t dev)
 	dsp_destroy_dev(dev);
 	(void)mixer_uninit(dev);
 
-	while (!CHN_EMPTY(d, channels.pcm))
-		pcm_killchan(dev);
+	pcm_killchans(d);
 
 	PCM_LOCK(d);
 	PCM_RELEASE(d);