* [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load()
@ 2021-07-15 10:20 ` Jia-Ju Bai
0 siblings, 0 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2021-07-15 10:20 UTC (permalink / raw)
To: perex, tiwai, alsa-devel,
linux-kernel@vger.kernel.org >> linux-kernel
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
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 6+ messages in thread
* [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load()
@ 2021-07-15 10:20 ` Jia-Ju Bai
0 siblings, 0 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2021-07-15 10:20 UTC (permalink / raw)
To: perex, tiwai, alsa-devel,
linux-kernel@vger.kernel.org >> linux-kernel
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
Best wishes,
Jia-Ju Bai
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load()
2021-07-15 10:20 ` Jia-Ju Bai
@ 2021-07-15 10:33 ` Takashi Iwai
-1 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2021-07-15 10:33 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: perex, tiwai, alsa-devel,
linux-kernel@vger.kernel.org >> linux-kernel
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load()
@ 2021-07-15 10:33 ` Takashi Iwai
0 siblings, 0 replies; 6+ messages in thread
From: Takashi Iwai @ 2021-07-15 10:33 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: linux-kernel@vger.kernel.org >> linux-kernel, alsa-devel, tiwai
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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load()
2021-07-15 10:33 ` Takashi Iwai
@ 2021-07-19 2:22 ` Jia-Ju Bai
-1 siblings, 0 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2021-07-19 2:22 UTC (permalink / raw)
To: Takashi Iwai
Cc: perex, tiwai, alsa-devel,
linux-kernel@vger.kernel.org >> linux-kernel
On 2021/7/15 18:33, Takashi Iwai wrote:
> 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 for the feedback and explanation :)
The patch looks good to me.
>
> --- 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);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load()
@ 2021-07-19 2:22 ` Jia-Ju Bai
0 siblings, 0 replies; 6+ messages in thread
From: Jia-Ju Bai @ 2021-07-19 2:22 UTC (permalink / raw)
To: Takashi Iwai
Cc: linux-kernel@vger.kernel.org >> linux-kernel, alsa-devel, tiwai
On 2021/7/15 18:33, Takashi Iwai wrote:
> 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 for the feedback and explanation :)
The patch looks good to me.
>
> --- 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);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-19 2:23 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15 10:20 [BUG] ALSA: sb16: possible ABBA deadlock in snd_sb_csp_stop() and snd_sb_csp_load() Jia-Ju Bai
2021-07-15 10:20 ` Jia-Ju Bai
2021-07-15 10:33 ` Takashi Iwai
2021-07-15 10:33 ` Takashi Iwai
2021-07-19 2:22 ` Jia-Ju Bai
2021-07-19 2:22 ` Jia-Ju Bai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.