On Sun, 16 May 2021 13:23:21 +0200, Sergey Senozhatsky wrote:
On (21/05/16 11:49), Takashi Iwai wrote:
Subject: [PATCH] ALSA: intel8x0: Don't update period unless prepared
The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever the hardware sets the corresponding status bit for each stream. This works fine for most cases as long as the hardware behaves properly. But when the hardware gives a wrong bit set, this leads to a NULL dereference Oops, and reportedly, this seems what happened on a VM.
VM, yes. I didn't see NULL derefs, my VMs crash because of div by zero in `% size`.
Ah, right, I'll fix the description.
For fixing the crash, this patch adds a internal flag indicating that the stream is ready to be updated, and check it (as well as the flag being in suspended) to ignore such spurious update.
I reproduced the "spurious IRQ" case, and the patch handled it correctly (VM did not crash).
Cc: stable@vger.kernel.org Reported-by: Sergey Senozhatsky senozhatsky@chromium.org Signed-off-by: Takashi Iwai tiwai@suse.de
I'll keep running test, but seems that it works as intended
Tested-by: Sergey Senozhatsky senozhatsky@chromium.org
OK, below is the revised patch I'm going to apply.
Thanks!
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH v2] ALSA: intel8x0: Don't update period unless prepared
The interrupt handler of intel8x0 calls snd_intel8x0_update() whenever the hardware sets the corresponding status bit for each stream. This works fine for most cases as long as the hardware behaves properly. But when the hardware gives a wrong bit set, this leads to a zero- division Oops, and reportedly, this seems what happened on a VM.
For fixing the crash, this patch adds a internal flag indicating that the stream is ready to be updated, and check it (as well as the flag being in suspended) to ignore such spurious update.
Cc: stable@vger.kernel.org Reported-and-tested-by: Sergey Senozhatsky senozhatsky@chromium.org Signed-off-by: Takashi Iwai tiwai@suse.de --- v1->v2: fixed description, updated tested-by tag
sound/pci/intel8x0.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c index 35903d1a1cbd..5b124c4ad572 100644 --- a/sound/pci/intel8x0.c +++ b/sound/pci/intel8x0.c @@ -331,6 +331,7 @@ struct ichdev { unsigned int ali_slot; /* ALI DMA slot */ struct ac97_pcm *pcm; int pcm_open_flag; + unsigned int prepared:1; unsigned int suspended: 1; };
@@ -691,6 +692,9 @@ static inline void snd_intel8x0_update(struct intel8x0 *chip, struct ichdev *ich int status, civ, i, step; int ack = 0;
+ if (!ichdev->prepared || ichdev->suspended) + return; + spin_lock_irqsave(&chip->reg_lock, flags); status = igetbyte(chip, port + ichdev->roff_sr); civ = igetbyte(chip, port + ICH_REG_OFF_CIV); @@ -881,6 +885,7 @@ static int snd_intel8x0_hw_params(struct snd_pcm_substream *substream, if (ichdev->pcm_open_flag) { snd_ac97_pcm_close(ichdev->pcm); ichdev->pcm_open_flag = 0; + ichdev->prepared = 0; } err = snd_ac97_pcm_open(ichdev->pcm, params_rate(hw_params), params_channels(hw_params), @@ -902,6 +907,7 @@ static int snd_intel8x0_hw_free(struct snd_pcm_substream *substream) if (ichdev->pcm_open_flag) { snd_ac97_pcm_close(ichdev->pcm); ichdev->pcm_open_flag = 0; + ichdev->prepared = 0; } return 0; } @@ -976,6 +982,7 @@ static int snd_intel8x0_pcm_prepare(struct snd_pcm_substream *substream) ichdev->pos_shift = (runtime->sample_bits > 16) ? 2 : 1; } snd_intel8x0_setup_periods(chip, ichdev); + ichdev->prepared = 1; return 0; }