Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 15 Jul 2021 12:33:21 +0200
Message-ID: <s5ho8b3x3wu.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>
Cc:     perex@perex.cz, tiwai@suse.com, alsa-devel@alsa-project.org,
        "linux-kernel@vger.kernel.org >> linux-kernel" 
        <linux-kernel@vger.kernel.org>
Subject: Re: [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load()
In-Reply-To: <7b0fcdaf-cd4f-4728-2eae-48c151a92e10@gmail.com>
References: <7b0fcdaf-cd4f-4728-2eae-48c151a92e10@gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 15 Jul 2021 12:20:36 +0200,
Jia-Ju Bai wrote:
> 
> Hello,
> 
> I find there is a possible ABBA deadlock in the SB16 driver in Linux 5.10:
> 
> In snd_sb_csp_stop():
> 876:     spin_lock_irqsave(&p->chip->mixer_lock, flags);
> 882:     spin_lock(&p->chip->reg_lock);
> 
> In snd_sb_csp_load():
> 614:     spin_lock_irqsave(&p->chip->reg_lock, flags);
> 653:     spin_lock(&p->chip->mixer_lock);
> 
> When snd_sb_csp_stop() and snd_sb_csp_load() are concurrently
> executed, the deadlock can occur.
> 
> I check the code and find a possible case of such concurrent execution:
> 
> #CPU1:
> snd_sb16_playback_close
>   snd_sb16_csp_playback_close (csp->ops.csp_stop(csp))
>     snd_sb_csp_stop
> 
> #CPU2:
> snd_sb_csp_ioctl
>   snd_sb_csp_riff_load
>     snd_sb_csp_load_user
>       snd_sb_csp_load
> 
> I am not quite sure whether this possible deadlock is real and how to
> fix it if it is real.
> Any feedback would be appreciated, thanks

The impact must be quite low, as both functions run in different
state (running or stopped), so those are basically exclusive.
And, above all, there is no VM supporting this chip, hence it's only
for the real hardware and it's about very old ISA boards that maybe
only less than handful people in the world can run now.

About the fix: just split the locks in snb_sb_csp_stop() (also
snd_sb_csp_start()) like below should suffice.


thanks,

Takashi

--- a/sound/isa/sb/sb16_csp.c
+++ b/sound/isa/sb/sb16_csp.c
@@ -816,6 +816,7 @@ static int snd_sb_csp_start(struct snd_sb_csp * p, int sample_width, int channel
 	mixR = snd_sbmixer_read(p->chip, SB_DSP4_PCM_DEV + 1);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL & 0x7);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR & 0x7);
+	spin_unlock_irqrestore(&p->chip->mixer_lock, flags);
 
 	spin_lock(&p->chip->reg_lock);
 	set_mode_register(p->chip, 0xc0);	/* c0 = STOP */
@@ -855,6 +856,7 @@ static int snd_sb_csp_start(struct snd_sb_csp * p, int sample_width, int channel
 	spin_unlock(&p->chip->reg_lock);
 
 	/* restore PCM volume */
+	spin_lock_irqsave(&p->chip->mixer_lock, flags);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR);
 	spin_unlock_irqrestore(&p->chip->mixer_lock, flags);
@@ -880,6 +882,7 @@ static int snd_sb_csp_stop(struct snd_sb_csp * p)
 	mixR = snd_sbmixer_read(p->chip, SB_DSP4_PCM_DEV + 1);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL & 0x7);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR & 0x7);
+	spin_unlock_irqrestore(&p->chip->mixer_lock, flags);
 
 	spin_lock(&p->chip->reg_lock);
 	if (p->running & SNDRV_SB_CSP_ST_QSOUND) {
@@ -894,6 +897,7 @@ static int snd_sb_csp_stop(struct snd_sb_csp * p)
 	spin_unlock(&p->chip->reg_lock);
 
 	/* restore PCM volume */
+	spin_lock_irqsave(&p->chip->mixer_lock, flags);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV, mixL);
 	snd_sbmixer_write(p->chip, SB_DSP4_PCM_DEV + 1, mixR);
 	spin_unlock_irqrestore(&p->chip->mixer_lock, flags);