From nobody Fri Nov 29 12:28:22 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Y0CBt3CxSz5f662; Fri, 29 Nov 2024 12:28:22 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R10" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Y0CBt2JBhz47pG; Fri, 29 Nov 2024 12:28:22 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1732883302; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=bj84zgBm1+AYca66KfqrZxII9BEVP1+bgxTTnCO3mFk=; b=hRgt8Ymc/plf5s9i8uN7Oew59c06T9VwQMZlUl3Kx8x0t0mvEDM1Quyx6Mq7C18PTOA+6w endY2k/i57rnu+4/duc8KDpEn5hAs2t03AnENgCjPKSEL0tO8IBxTfOSnWXyHrexlR6q+b HfG0+44BLlvEPgFw+5IEBBWSin6OzAE1EDP79k/dlGcl5zzup1Hh6EfCvl9ftrIZS+W7Fm coE6ctkoovkyllQrOzz0Nvs7AC9KsIMO0d+duYOxfJtmIw+LyVQ3zyYb939fMlyU/zqiTj B37iVcF4xKEUElBojdBQYjwls0UA9WdYXTG0fIQ4vPz79+H84wwkx2DS6MKdKA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1732883302; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=bj84zgBm1+AYca66KfqrZxII9BEVP1+bgxTTnCO3mFk=; b=l00iR7sdWGlkzxjQlbilg0p4/R6Got0Ggnn+CiSWm0ZqenVMISjamumBtJUL2R0U0Ub9QK wO37UJJsx09ErBewJCDaJ24ZZxI7GDT66a7KkYV5VFXx5GtBIXij624FzkLp889+R6r5/3 G1sAcevTs8K1gAvrQw81eXAAhdSAfD0OJ8n24dRXPyUllH9xwUtNs+hBh6sby+B085RhwU hoW0dlzTf7nzKWhPWR9dXyONXrjF2yPQwcCRSBeuwf3JuwPBIrCh/ICrYYcX6Eb2E6cLfv eN+Cg6O9/q16+/yoy7oCZu23isVz5mB8yXz+Y+c9/fQNyhGNUUoS5oTzEzFO3w== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1732883302; a=rsa-sha256; cv=none; b=suoxhd4Cqc83nzMTbwECkw1VnE8oEfZGWMRD9HxF6kKXodYHruvlqmP01DYcOmCMJFcQA6 6SMTMA1jKcXwaKLZY7wJDkGOuSKlBRAJuJinabjVHo9MjlvB+r4tDepfKIyPN1YAsy+F2W SlmDGfBn+zCLHwItiI4dbUWj2WN8SDIi8ARbPwTplaTwLvbfbyZkcgxlnMdJmkFYNUkaay pstHFNpFgLgPtXvTqvjCh2l8spIY8csEOWfKD0/eRAW3woRPhRKeUcIdUdFxan3eFR8LTe rIXPOkpfezgKmnDpc+FiGpvVGfgDdQ7m30Y8V4q1A7rSH+pXWTBGzXg8P9xkjQ== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4Y0CBt1wSDzNfd; Fri, 29 Nov 2024 12:28:22 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 4ATCSMsT070296; Fri, 29 Nov 2024 12:28:22 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4ATCSMcH070293; Fri, 29 Nov 2024 12:28:22 GMT (envelope-from git) Date: Fri, 29 Nov 2024 12:28:22 GMT Message-Id: <202411291228.4ATCSMcH070293@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Christos Margiolis Subject: git: 950f0c86d320 - stable/14 - sound: Fix chn_trigger() and vchan_trigger() races List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: christos X-Git-Repository: src X-Git-Refname: refs/heads/stable/14 X-Git-Reftype: branch X-Git-Commit: 950f0c86d3200b8ec609b17f04c62cab9bc76dd8 Auto-Submitted: auto-generated The branch stable/14 has been updated by christos: URL: https://cgit.FreeBSD.org/src/commit/?id=950f0c86d3200b8ec609b17f04c62cab9bc76dd8 commit 950f0c86d3200b8ec609b17f04c62cab9bc76dd8 Author: Christos Margiolis AuthorDate: 2024-11-26 14:48:18 +0000 Commit: Christos Margiolis CommitDate: 2024-11-29 12:28:06 +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 (cherry picked from commit 5ac39263d825d7b2f8a89614a63fee90ffc77c07) --- 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)