[PATCH] ALSA: sb: fix potential deadlock on &chip->mixer_lock

[PATCH] ALSA: sb: fix potential deadlock on &chip->mixer_lock

[Chengfeng Ye]

As &chip->mixer_lock is also acquired by the irq snd_sb8dsp_interrupt()
which executes under hard-irq context, code executing under process
context should disable irq before acquiring the lock, otherwise
deadlock could happen if the process context hold the lock then
preempt by the interruption.

As the ALSA Driver document described, PCM prepare callbacks are not
executed with irq disabled by default, thus the acquiring of
&chip->mixer_lock should be irq disabled.

Possible deadlock scenario:
snd_sb8_playback_prepare
    -> spin_lock(&chip->mixer_lock);
        <irq interrupt>
        -> snd_sb8dsp_interrupt()
        -> snd_sb8_capture_trigger()
        -> spin_lock(&chip->mixer_lock); (deadlock here)

This flaw was found using an experimental static analysis tool we are
developing for irq-related deadlock.

The tentative patch fix the potential deadlock by spin_lock_irqsave().

Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
---
 sound/isa/sb/sb8_main.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/sound/isa/sb/sb8_main.c b/sound/isa/sb/sb8_main.c
index 2ed176a5a574..81af8dcddcd2 100644
--- a/sound/isa/sb/sb8_main.c
+++ b/sound/isa/sb/sb8_main.c
@@ -148,10 +148,12 @@ static int snd_sb8_playback_prepare(struct snd_pcm_substream *substream)
 		snd_sbdsp_command(chip, format);
 	else if (stereo) {
 		/* set playback stereo mode */
-		spin_lock(&chip->mixer_lock);
+		unsigned long flags;
+
+		spin_lock_irqsave(&chip->mixer_lock, flags);
 		mixreg = snd_sbmixer_read(chip, SB_DSP_STEREO_SW);
 		snd_sbmixer_write(chip, SB_DSP_STEREO_SW, mixreg | 0x02);
-		spin_unlock(&chip->mixer_lock);
+		spin_unlock_irqrestore(&chip->mixer_lock, flags);
 
 		/* Soundblaster hardware programming reference guide, 3-23 */
 		snd_sbdsp_command(chip, SB_DSP_DMA8_EXIT);
@@ -164,12 +166,13 @@ static int snd_sb8_playback_prepare(struct snd_pcm_substream *substream)
 	}
 	snd_sbdsp_command(chip, SB_DSP_SAMPLE_RATE);
 	if (stereo) {
+		unsigned long flags;
 		snd_sbdsp_command(chip, 256 - runtime->rate_den / 2);
-		spin_lock(&chip->mixer_lock);
+		spin_lock_irqsave(&chip->mixer_lock, flags);
 		/* save output filter status and turn it off */
 		mixreg = snd_sbmixer_read(chip, SB_DSP_PLAYBACK_FILT);
 		snd_sbmixer_write(chip, SB_DSP_PLAYBACK_FILT, mixreg | 0x20);
-		spin_unlock(&chip->mixer_lock);
+		spin_unlock_irqrestore(&chip->mixer_lock, flags);
 		/* just use force_mode16 for temporary storate... */
 		chip->force_mode16 = mixreg;
 	} else {
@@ -289,12 +292,13 @@ static int snd_sb8_capture_prepare(struct snd_pcm_substream *substream)
 		snd_sbdsp_command(chip, SB_DSP_STEREO_8BIT);
 	snd_sbdsp_command(chip, SB_DSP_SAMPLE_RATE);
 	if (stereo) {
+		unsigned long flags;
 		snd_sbdsp_command(chip, 256 - runtime->rate_den / 2);
-		spin_lock(&chip->mixer_lock);
+		spin_lock_irqsave(&chip->mixer_lock, flags);
 		/* save input filter status and turn it off */
 		mixreg = snd_sbmixer_read(chip, SB_DSP_CAPTURE_FILT);
 		snd_sbmixer_write(chip, SB_DSP_CAPTURE_FILT, mixreg | 0x20);
-		spin_unlock(&chip->mixer_lock);
+		spin_lock_irqsave(&chip->mixer_lock, flags);
 		/* just use force_mode16 for temporary storate... */
 		chip->force_mode16 = mixreg;
 	} else {
-- 
2.17.1

Re: [PATCH] ALSA: sb: fix potential deadlock on &chip->mixer_lock

[Takashi Iwai]

On Tue, 27 Jun 2023 11:56:16 +0200,
Chengfeng Ye wrote:
> 
> As &chip->mixer_lock is also acquired by the irq snd_sb8dsp_interrupt()
> which executes under hard-irq context, code executing under process
> context should disable irq before acquiring the lock, otherwise
> deadlock could happen if the process context hold the lock then
> preempt by the interruption.
> 
> As the ALSA Driver document described, PCM prepare callbacks are not
> executed with irq disabled by default, thus the acquiring of
> &chip->mixer_lock should be irq disabled.
> 
> Possible deadlock scenario:
> snd_sb8_playback_prepare
>     -> spin_lock(&chip->mixer_lock);
>         <irq interrupt>
>         -> snd_sb8dsp_interrupt()
>         -> snd_sb8_capture_trigger()
>         -> spin_lock(&chip->mixer_lock); (deadlock here)
> 
> This flaw was found using an experimental static analysis tool we are
> developing for irq-related deadlock.
> 
> The tentative patch fix the potential deadlock by spin_lock_irqsave().
> 
> Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>

I believe it's a false-positive.  There is already a call
	spin_lock_irqsave(&chip->reg_lock, flags);
beforehand.


thanks,

Takashi

Re: [PATCH] ALSA: sb: fix potential deadlock on &chip->mixer_lock

[叶澄锋]

Oh yes, sorry for not considering that one.

Best regards
Chengfeng

Takashi Iwai <tiwai@suse.de> 于2023年6月27日周二 18:01写道：
>
> On Tue, 27 Jun 2023 11:56:16 +0200,
> Chengfeng Ye wrote:
> >
> > As &chip->mixer_lock is also acquired by the irq snd_sb8dsp_interrupt()
> > which executes under hard-irq context, code executing under process
> > context should disable irq before acquiring the lock, otherwise
> > deadlock could happen if the process context hold the lock then
> > preempt by the interruption.
> >
> > As the ALSA Driver document described, PCM prepare callbacks are not
> > executed with irq disabled by default, thus the acquiring of
> > &chip->mixer_lock should be irq disabled.
> >
> > Possible deadlock scenario:
> > snd_sb8_playback_prepare
> >     -> spin_lock(&chip->mixer_lock);
> >         <irq interrupt>
> >         -> snd_sb8dsp_interrupt()
> >         -> snd_sb8_capture_trigger()
> >         -> spin_lock(&chip->mixer_lock); (deadlock here)
> >
> > This flaw was found using an experimental static analysis tool we are
> > developing for irq-related deadlock.
> >
> > The tentative patch fix the potential deadlock by spin_lock_irqsave().
> >
> > Signed-off-by: Chengfeng Ye <dg573847474@gmail.com>
>
> I believe it's a false-positive.  There is already a call
>         spin_lock_irqsave(&chip->reg_lock, flags);
> beforehand.
>
>
> thanks,
>
> Takashi