jimzah/freebsd-src
1
0
Fork 0
mirror of https://github.com/freebsd/freebsd-src.git synced 2024-11-26 20:12:44 +00:00
Code Issues Actions 2 Packages Projects Releases Wiki Activity

sound: Fix chn_trigger() and vchan_trigger() races

Browse Source 
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
This commit is contained in:
Christos Margiolis 2024-11-26 15:48:18 +01:00
parent 5bd08172b4
commit 5ac39263d8
2 changed files with 35 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

56
sys/dev/sound/pcm/channel.c
View File

@ -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;
}

13
sys/dev/sound/pcm/vchan.c
View File

@ -146,20 +146,19 @@ vchan_trigger(kobj_t obj, void *data, int go)
int ret, otrigger;
info = data;
c = info->channel;
p = c->parentchannel;
CHN_LOCKASSERT(c);
if (!PCMTRIG_COMMON(go) || go == info->trigger)
return (0);
c = info->channel;
p = c->parentchannel;
otrigger = info->trigger;
info->trigger = go;
CHN_LOCKASSERT(c);
CHN_UNLOCK(c);
CHN_LOCK(p);
otrigger = info->trigger;
info->trigger = go;
switch (go) {
case PCMTRIG_START:
if (otrigger != PCMTRIG_START)