Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 30 Apr 2019 09:31:32 +0200
Message-ID: <s5himuwhru3.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     "Song liwei" <liwei.song@windriver.com>
Cc:     <alsa-devel@alsa-project.org>, "Yu Zhao" <yuzhao@google.com>,
        "Mark Brown" <broonie@kernel.org>,
        "Keyon Jie" <yang.jie@linux.intel.com>,
        "Jaroslav Kysela" <perex@perex.cz>,
        "linux-kernel" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ALSA: hda: check RIRB to avoid use NULL pointer
In-Reply-To: <1556604653-47363-1-git-send-email-liwei.song@windriver.com>
References: <1556604653-47363-1-git-send-email-liwei.song@windriver.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=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 30 Apr 2019 08:10:53 +0200,
Song liwei wrote:
> 
> From: Liwei Song <liwei.song@windriver.com>
> 
> Fix the following BUG:
> 
> BUG: unable to handle kernel NULL pointer dereference at 000000000000000c
> Workqueue: events azx_probe_work [snd_hda_intel]
> RIP: 0010:snd_hdac_bus_update_rirb+0x80/0x160 [snd_hda_core]
> Call Trace:
>  <IRQ>
>  azx_interrupt+0x78/0x140 [snd_hda_codec]
>  __handle_irq_event_percpu+0x49/0x300
>  handle_irq_event_percpu+0x23/0x60
>  handle_irq_event+0x3c/0x60
>  handle_edge_irq+0xdb/0x180
>  handle_irq+0x23/0x30
>  do_IRQ+0x6a/0x140
>  common_interrupt+0xf/0xf
> 
> The Call Trace happened when run kdump on a NFS rootfs system.
> Exist the following calling sequence when boot the second kernel:
> 
> azx_first_init()
>    --> azx_acquire_irq()
>                       <-- interrupt come in, azx_interrupt() was called
>    --> hda_intel_init_chip()
>       --> azx_init_chip()
>          --> snd_hdac_bus_init_chip()
>               --> snd_hdac_bus_init_cmd_io();
>                     --> init rirb.buf and corb.buf
> 
> Interrupt happened after azx_acquire_irq() while RIRB still didn't got
> initialized, then NULL pointer will be used when process the interrupt.
> 
> Check the value of RIRB to ensure it is not NULL, to aviod some special
> case may hang the system.
> 
> Fixes: 14752412721c ("ALSA: hda - Add the controller helper codes to hda-core module")
> Signed-off-by: Liwei Song <liwei.song@windriver.com>

Oh, that's indeed a race there.

But I guess the check introduced by the patch is still error-prone.
Basically the interrupt handling should be moved after the chip
initialization.  I suppose that your platform uses the shared
interrupt, not the MSI?

In anyway, alternative (and likely more certain) fix would be to move
the azx_acquir_irq() call like the patch below (note: totally
untested).  Could you check whether it works?


thanks,

Takashi


--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -1788,9 +1788,6 @@ static int azx_first_init(struct azx *chip)
 			chip->msi = 0;
 	}
 
-	if (azx_acquire_irq(chip, 0) < 0)
-		return -EBUSY;
-
 	pci_set_master(pci);
 	synchronize_irq(bus->irq);
 
@@ -1904,6 +1901,9 @@ static int azx_first_init(struct azx *chip)
 		return -ENODEV;
 	}
 
+	if (azx_acquire_irq(chip, 0) < 0)
+		return -EBUSY;
+
 	strcpy(card->driver, "HDA-Intel");
 	strlcpy(card->shortname, driver_short_names[chip->driver_type],
 		sizeof(card->shortname));