[PATCH] powerpc/mce: Fix access error in mce handler

[Michael Ellerman]

Ganesh Goudar <ganeshgr at linux.ibm.com> writes:
> We queue an irq work for deferred processing of mce event
> in realmode mce handler, where translation is disabled.
> Queuing of the work may result in accessing memory outside
> RMO region, such access needs the translation to be enabled
> for an LPAR running with hash mmu else the kernel crashes.
>
> So enable the translation before queuing the work.
>
> Without this change following trace is seen on injecting machine
> check error in an LPAR running with hash mmu.

What type of error are you injecting?

> Oops: Kernel access of bad area, sig: 11 [#1]
> LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries
> CPU: 5 PID: 1883 Comm: insmod Tainted: G        OE     5.14.0-mce+ #137
> NIP:  c000000000735d60 LR: c000000000318640 CTR: 0000000000000000
> REGS: c00000001ebff9a0 TRAP: 0300   Tainted: G       OE      (5.14.0-mce+)
> MSR:  8000000000001003 <SF,ME,RI,LE>  CR: 28008228  XER: 00000001
> CFAR: c00000000031863c DAR: c00000027fa8fe08 DSISR: 40000000 IRQMASK: 0
> GPR00: c0000000003186d0 c00000001ebffc40 c000000001b0df00 c0000000016337e8
> GPR04: c0000000016337e8 c00000027fa8fe08 0000000000000023 c0000000016337f0
> GPR08: 0000000000000023 c0000000012ffe08 0000000000000000 c008000001460240
> GPR12: 0000000000000000 c00000001ec9a900 c00000002ac4bd00 0000000000000000
> GPR16: 00000000000005a0 c0080000006b0000 c0080000006b05a0 c000000000ff3068
> GPR20: c00000002ac4bbc0 0000000000000001 c00000002ac4bbc0 c008000001490298
> GPR24: c008000001490108 c000000001636198 c008000001470090 c008000001470058
> GPR28: 0000000000000510 c008000001000000 c008000008000019 0000000000000019
> NIP [c000000000735d60] llist_add_batch+0x0/0x40
> LR [c000000000318640] __irq_work_queue_local+0x70/0xc0
> Call Trace:
> [c00000001ebffc40] [c00000001ebffc0c] 0xc00000001ebffc0c (unreliable)
> [c00000001ebffc60] [c0000000003186d0] irq_work_queue+0x40/0x70
> [c00000001ebffc80] [c00000000004425c] machine_check_queue_event+0xbc/0xd0
> [c00000001ebffcf0] [c00000000000838c] machine_check_early_common+0x16c/0x1f4
>
> Fixes: 74c3354bc1d89 ("powerpc/pseries/mce: restore msr before returning from handler")

Please explain in more detail why that commit caused this breakage.

> diff --git a/arch/powerpc/kernel/mce.c b/arch/powerpc/kernel/mce.c
> index 47a683cd00d2..9d1e39d42e3e 100644
> --- a/arch/powerpc/kernel/mce.c
> +++ b/arch/powerpc/kernel/mce.c
> @@ -249,6 +249,7 @@ void machine_check_queue_event(void)
>  {
>  	int index;
>  	struct machine_check_event evt;
> +	unsigned long msr;
>  
>  	if (!get_mce_event(&evt, MCE_EVENT_RELEASE))
>  		return;
> @@ -262,8 +263,19 @@ void machine_check_queue_event(void)
>  	memcpy(&local_paca->mce_info->mce_event_queue[index],
>  	       &evt, sizeof(evt));
>  
> -	/* Queue irq work to process this event later. */
> -	irq_work_queue(&mce_event_process_work);
> +	/* Queue irq work to process this event later. Before
> +	 * queuing the work enable translation for non radix LPAR,
> +	 * as irq_work_queue may try to access memory outside RMO
> +	 * region.
> +	 */
> +	if (!radix_enabled() && firmware_has_feature(FW_FEATURE_LPAR)) {
> +		msr = mfmsr();
> +		mtmsr(msr | MSR_IR | MSR_DR);
> +		irq_work_queue(&mce_event_process_work);
> +		mtmsr(msr);
> +	} else {
> +		irq_work_queue(&mce_event_process_work);
> +	}
>  }

We already went to virtual mode and queued (different) irq work in
arch/powerpc/platforms/pseries/ras.c:mce_handle_error()

We also called save_mce_event() which also might have queued irq work,
via machine_check_ue_event().

So it really feels like something about the design is wrong if we have
to go to virtual mode again and queue more irq work here.

I guess we can probably merge this as a backportable fix, doing anything
else would be a bigger change.

Looking at ras.c there's the comment:

	 * Enable translation as we will be accessing per-cpu variables
	 * in save_mce_event() which may fall outside RMO region, also

But AFAICS it's only irq_work_queue() that touches anything percpu?

So maybe we should just not be using irq_work_queue(). It's a pretty
thin wrapper around set_dec(1), perhaps we just need to hand-roll some
real-mode friendly way of doing that.

cheers