Date: Sun, 28 Apr 2024 19:49:01 GMT From: Christos Margiolis <christos@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: 03614fcba25b - main - sound: Fix panic caused by sleeping-channel destruction during asynchronous detach Message-ID: <202404281949.43SJn1Nq063237@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202404281949.43SJn1Nq063237>