On Tue, 30 Apr 2019 10:32:47 +0200,
Liwei Song wrote:
>
>
>
> On 04/30/2019 03:31 PM, Takashi Iwai wrote:
> > On Tue, 30 Apr 2019 08:10:53 +0200,
> > Song liwei wrote:
> >>
> >> From: Liwei Song <liwei.song@xxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxx>
> >
> > 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?
>
> This is the information from /proc/interrupt
> 134: 0 102 0 0 IR-PCI-MSI 514048-edge snd_hda_intel:card0
Hm, then it's interesting...
> > 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?
>
> Yes, It works.
>
> Considering a previous patch like the one you provide will import some issue,
> so I choose check the invalid value to low the risk, but just as you mentioned,
> It is not a good solution.
>
> commit 542cedec53c9e8b73f3f05bf8468823598c50489
> Author: Yu Zhao <yuzhao@xxxxxxxxxx>
> Date: Tue Sep 11 15:12:46 2018 -0600
>
> Revert "ASoC: Intel: Skylake: Acquire irq after RIRB allocation"
>
> This reverts commit 12eeeb4f4733bbc4481d01df35933fc15beb8b19.
>
> The patch doesn't fix accessing memory with null pointer in
> skl_interrupt().
>
> There are two problems: 1) skl_init_chip() is called twice, before
> and after dma buffer is allocate. The first call sets bus->chip_init
> which prevents the second from initializing bus->corb.buf and
> rirb.buf from bus->rb.area. 2) snd_hdac_bus_init_chip() enables
> interrupt before snd_hdac_bus_init_cmd_io() initializing dma buffers.
> There is a small window which skl_interrupt() can be called if irq
> has been acquired. If so, it crashes when using null dma buffer
> pointers.
Actually this followed by another fix b61749a89f82,
sound: enable interrupt after dma buffer initialization
and this moved the IRQ enablement after snd_hdac_bus_init_cmd_io().
So I wonder how the irq gets triggered in your case.
If it were a shared irq, it's understandable. But for MSI, it should
have been the isolated source.
In anyway, for the latest tree, the change I suggested would cover
better although it's more radical as you pointed.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
