[PATCH v3 07/12] ACPI / APEI: Make the nmi_fixmap_idx per-ghes to allow multiple in_nmi() users

[bp@alien8.de (Borislav Petkov)]

On Wed, May 16, 2018 at 03:51:14PM +0100, James Morse wrote:
> The first two overload existing architectural behavior, the third improves all
> this with a third standard option. Its the standard story!

:-)

> I thought this was safe because its just ghes_copy_tofrom_phys()s access to the
> fixmap slots that needs mutual exclusion.
>
> Polled and all the IRQ flavours are kept apart by the spin_lock_irqsave(), and
> the NMIs have their own fixmap entry. (This is fine until there is more than
> once source of NMI)

For example:

ghes_probe()

	switch (generic->notify.type) {

	...

        case ACPI_HEST_NOTIFY_NMI:
		ghes_nmi_add(ghes);
	}

	...

	ghes_proc();
	  ghes_read_estatus();
		 spin_lock_irqsave(&ghes_ioremap_lock_irq, flags);

		 memcpy...

	-> NMI

		ghes_notify_nmi();
		 ghes_read_estatus();
		 ..
		   if (in_nmi) {
			   raw_spin_lock(&ghes_ioremap_lock_nmi);

		...
	<- NMI

ghes->estatus from above, before the NMI fired, has gotten some nice
scribbling over. AFAICT.

Now, I don't know whether this can happen with the ARM facilities but if
they're NMI-like, I don't see why not.

Which means, that this code is not really reentrant and if should be
fixed to be callable from different contexts, then it should use private
buffers and be careful about locking.

Oh, and that

	if (in_nmi)
		lock()
	else
		lock_irqsave()

pattern is really yucky. And it is an explosion waiting to happen.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.