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

From: Ganesh <ganeshgr@linux.ibm.com>
To: Daniel Axtens <dja@axtens.net>,
	linuxppc-dev@lists.ozlabs.org, mpe@ellerman.id.au
Cc: mahesh@linux.ibm.com, npiggin@gmail.com
Subject: Re: [PATCH v2] powerpc/mce: Fix access error in mce handler
Date: Fri, 17 Sep 2021 14:10:03 +0530	[thread overview]
Message-ID: <d0a9d52f-b427-757e-0fab-e63ef7c035b1@linux.ibm.com> (raw)
In-Reply-To: <87lf3v903y.fsf@linkitivity.dja.id.au>

[-- Attachment #1: Type: text/plain, Size: 2655 bytes --]

On 9/17/21 12:09 PM, Daniel Axtens wrote:

> Hi Ganesh,
>
>> 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.
>>
>> After enabling translation in mce_handle_error() we used to
>> leave it enabled to avoid crashing here, but now with the
>> commit 74c3354bc1d89 ("powerpc/pseries/mce: restore msr before
>> returning from handler") we are restoring the MSR to disable
>> translation.
>>
>> Hence to fix this enable the translation before queuing the work.
> [snip]
>
>> Fixes: 74c3354bc1d89 ("powerpc/pseries/mce: restore msr before returning from handler")
> That patch changes arch/powerpc/powerpc/platforms/pseries/ras.c just
> below this comment:
>
>      /*
>       * Enable translation as we will be accessing per-cpu variables
>       * in save_mce_event() which may fall outside RMO region, also
>       * leave it enabled because subsequently we will be queuing work
>       * to workqueues where again per-cpu variables accessed, besides
>       * fwnmi_release_errinfo() crashes when called in realmode on
>       * pseries.
>       * Note: All the realmode handling like flushing SLB entries for
>       *       SLB multihit is done by now.
>       */
>
> That suggests per-cpu variables need protection. In your patch, you
> enable translations just around irq_work_queue:

The comment is bit old, most of it doesn't make any sense now, yes per-cpu
variables cannot be accessed in realmode, but with commit 923b3cf00b3f
("powerpc/mce: Remove per cpu variables from MCE handlers") we moved all of
them to paca.

>> +	/* 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);
>> +	}
> However, just before that in the function, there are a few things that
> access per-cpu variables via the local_paca, e.g.:
>
> memcpy(&local_paca->mce_info->mce_event_queue[index],
>         &evt, sizeof(evt));
>
> Do we need to widen the window where translations are enabled in order
> to protect accesses to local_paca?

paca will be within Real Mode Area, so it can be accessed with translate off.

[-- Attachment #2: Type: text/html, Size: 3620 bytes --]