From nobody Tue Nov 26 14:48:55 2024 X-Original-To: dev-commits-src-main@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 4XyQSR1dS1z5fYZT; Tue, 26 Nov 2024 14:48:55 +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 4XyQSR1B2mz4WdB; Tue, 26 Nov 2024 14:48:55 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1732632535; 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=R6WAnTGHdEaX6N2kYErAo+hQQwZzmMn5T6sFzBsHW6U=; b=spBVQryfArBUKutqORTESFPP7oAd0ew8ctoP2QgfLCtol2qvNV6JXq5w7jK/rEkYfDzo9T kDgr0b+LLfJnWzvgVfZn/Gl+/NIdY62W1Nq0G1/Us6b0dCcuZZK3v9V6grMg41+CfCd1z5 A3M1RYZ1vs/PMiTWioksGz12H2mzR9SDqozUXRpa4eyyb+KP7yalISLfne0/qvtQvuoWAV GY9DvPU7XwhB9COlQIEyu/1GvFgTc4hyk5hxtWiShnf7xxlAg3hqbeeQH1HsQPJGimcf2z RTs2+1QPQNbNV58WvYnN8dws3Lwlh8cQMvFtb0rOzu3dKU6F4qbrC8GwPwf8XQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1732632535; 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=R6WAnTGHdEaX6N2kYErAo+hQQwZzmMn5T6sFzBsHW6U=; b=C28B4KANac27tzLK/NannK4EWcriH32iutwfSDW4dedkxpT28ZnrCL81iWuOwCVVK2mNL3 8mSPA2FU5bITbqD63feo7IrKmVupULsP3AIIWKBuOBL+w6CbtAH2ukKOG5Df/32tJHHPHh g+6UiAH0/xPXPEBY8m7yR6hhNt361/BiA6Ad9wAhQpDVV54u4LbI/yJagsufZZ20bXEPEx 4GplyagSq4l2rpLs8o7gW/e38RD0SlHodWYngxexv1ZzQ3TgRuxTbfwIo4N7Jr4RIy4ttD wTbWOxCc48DkaKkakgd0ORp6C5hEwfumZ9/qKxBBXKS/9fVUIbjxjmdSvxACQw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1732632535; a=rsa-sha256; cv=none; b=krH7WAkBDW+TYYYJ4+fCFiHoY/n0P0POMzrdCh5u7Bt0QrGVP6XPiuDUshbQsudcNlvtye r8ILvX5K0oKfjwmRfAIrrb3Lnphs3Q4cCTHTiPVeXcapXBIGlht9SY9Cq00JrZVaHOf7Mh QSt1wYwI+11pjIEUir1SSdSciTxANBWDrpw9JbXTSopz6GeAg+sNlAlgRE/By8XBL4k7le jg4hluy735iYunxDfhYbRoaSEA7X/FXop45q8+zwh+oIN3gglvFnvQ9IgmErpbJAKaoOgA NHKbg3iTWYN/bjuWgWX3lgK9IugQX5DkUeZuQjO5yehQ86vYAnP51BN2UPQJQg== 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 4XyQSR0nCyzSy9; Tue, 26 Nov 2024 14:48:55 +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 4AQEmtPc084081; Tue, 26 Nov 2024 14:48:55 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 4AQEmt7m084078; Tue, 26 Nov 2024 14:48:55 GMT (envelope-from git) Date: Tue, 26 Nov 2024 14:48:55 GMT Message-Id: <202411261448.4AQEmt7m084078@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Christos Margiolis Subject: git: 5ac39263d825 - main - sound: Fix chn_trigger() and vchan_trigger() races List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@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/main X-Git-Reftype: branch X-Git-Commit: 5ac39263d825d7b2f8a89614a63fee90ffc77c07 Auto-Submitted: auto-generated The branch main has been updated by christos: URL: https://cgit.FreeBSD.org/src/commit/?id=5ac39263d825d7b2f8a89614a63fee90ffc77c07 commit 5ac39263d825d7b2f8a89614a63fee90ffc77c07 Author: Christos Margiolis AuthorDate: 2024-11-26 14:48:18 +0000 Commit: Christos Margiolis 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)