ACPI: APEI: set memory failure flags as MF_ACTION_REQUIRED on action required events

There are two major types of uncorrected error (UC) :

- Action Required: The error is detected and the processor already consumes the
  memory. OS requires to take action (for example, offline failure page/kill
  failure thread) to recover this uncorrectable error.

- Action Optional: The error is detected out of processor execution context.
  Some data in the memory are corrupted. But the data have not been consumed.
  OS is optional to take action to recover this uncorrectable error.

For X86 platforms, we can easily distinguish between these two types
based on the MCA Bank. While for arm64 platform, the memory failure
flags for all UCs which severity are GHES_SEV_RECOVERABLE are set as 0,
a.k.a, Action Optional now.

If UC is detected by a background scrubber, it is obviously an Action
Optional error.  For other errors, we should conservatively regard them
as Action Required.

cper_sec_mem_err::error_type identifies the type of error that occurred
if CPER_MEM_VALID_ERROR_TYPE is set. So, set memory failure flags as 0
for Scrub Uncorrected Error (type 14). Otherwise, set memory failure
flags as MF_ACTION_REQUIRED.

Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
---
 drivers/acpi/apei/ghes.c | 10 ++++++++--
 include/linux/cper.h     |  3 +++
 2 files changed, 11 insertions(+), 2 deletions(-)

On Thu, Oct 27, 2022 at 6:25 AM Shuai Xue <xueshuai@linux.alibaba.com> wrote:
>
> There are two major types of uncorrected error (UC) :
>
> - Action Required: The error is detected and the processor already consumes the
>   memory. OS requires to take action (for example, offline failure page/kill
>   failure thread) to recover this uncorrectable error.
>
> - Action Optional: The error is detected out of processor execution context.
>   Some data in the memory are corrupted. But the data have not been consumed.
>   OS is optional to take action to recover this uncorrectable error.
>
> For X86 platforms, we can easily distinguish between these two types
> based on the MCA Bank. While for arm64 platform, the memory failure
> flags for all UCs which severity are GHES_SEV_RECOVERABLE are set as 0,
> a.k.a, Action Optional now.
>
> If UC is detected by a background scrubber, it is obviously an Action
> Optional error.  For other errors, we should conservatively regard them
> as Action Required.
>
> cper_sec_mem_err::error_type identifies the type of error that occurred
> if CPER_MEM_VALID_ERROR_TYPE is set. So, set memory failure flags as 0
> for Scrub Uncorrected Error (type 14). Otherwise, set memory failure
> flags as MF_ACTION_REQUIRED.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>

I need input from the APEI reviewers on this.

Thanks!

> ---
>  drivers/acpi/apei/ghes.c | 10 ++++++++--
>  include/linux/cper.h     |  3 +++
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 80ad530583c9..6c03059cbfc6 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -474,8 +474,14 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>         if (sec_sev == GHES_SEV_CORRECTED &&
>             (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
>                 flags = MF_SOFT_OFFLINE;
> -       if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
> -               flags = 0;
> +       if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE) {
> +               if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
> +                       flags = mem_err->error_type == CPER_MEM_SCRUB_UC ?
> +                                       0 :
> +                                       MF_ACTION_REQUIRED;
> +               else
> +                       flags = MF_ACTION_REQUIRED;
> +       }
>
>         if (flags != -1)
>                 return ghes_do_memory_failure(mem_err->physical_addr, flags);
> diff --git a/include/linux/cper.h b/include/linux/cper.h
> index eacb7dd7b3af..b77ab7636614 100644
> --- a/include/linux/cper.h
> +++ b/include/linux/cper.h
> @@ -235,6 +235,9 @@ enum {
>  #define CPER_MEM_VALID_BANK_ADDRESS            0x100000
>  #define CPER_MEM_VALID_CHIP_ID                 0x200000
>
> +#define CPER_MEM_SCRUB_CE                      13
> +#define CPER_MEM_SCRUB_UC                      14
> +
>  #define CPER_MEM_EXT_ROW_MASK                  0x3
>  #define CPER_MEM_EXT_ROW_SHIFT                 16
>
> --
> 2.20.1.9.gb50a0d7
>

>> cper_sec_mem_err::error_type identifies the type of error that occurred
>> if CPER_MEM_VALID_ERROR_TYPE is set. So, set memory failure flags as 0
>> for Scrub Uncorrected Error (type 14). Otherwise, set memory failure
>> flags as MF_ACTION_REQUIRED.

On x86 the "action required" cases are signaled by a synchronous machine check
that is delivered before the instruction that is attempting to consume the uncorrected
data retires. I.e., it is guaranteed that the uncorrected error has not been propagated
because it is not visible in any architectural state.

APEI signaled errors don't fall into that category on x86 ... the uncorrected data
could have been consumed and propagated long before the signaling used for
APEI can alert the OS.

Does ARM deliver APEI signals synchronously?

If not, then this patch might deliver a false sense of security to applications
about the state of uncorrected data in the system.

-Tony

在 2022/10/29 AM1:08, Rafael J. Wysocki 写道:
> On Thu, Oct 27, 2022 at 6:25 AM Shuai Xue <xueshuai@linux.alibaba.com> wrote:
>>
>> There are two major types of uncorrected error (UC) :
>>
>> - Action Required: The error is detected and the processor already consumes the
>>   memory. OS requires to take action (for example, offline failure page/kill
>>   failure thread) to recover this uncorrectable error.
>>
>> - Action Optional: The error is detected out of processor execution context.
>>   Some data in the memory are corrupted. But the data have not been consumed.
>>   OS is optional to take action to recover this uncorrectable error.
>>
>> For X86 platforms, we can easily distinguish between these two types
>> based on the MCA Bank. While for arm64 platform, the memory failure
>> flags for all UCs which severity are GHES_SEV_RECOVERABLE are set as 0,
>> a.k.a, Action Optional now.
>>
>> If UC is detected by a background scrubber, it is obviously an Action
>> Optional error.  For other errors, we should conservatively regard them
>> as Action Required.
>>
>> cper_sec_mem_err::error_type identifies the type of error that occurred
>> if CPER_MEM_VALID_ERROR_TYPE is set. So, set memory failure flags as 0
>> for Scrub Uncorrected Error (type 14). Otherwise, set memory failure
>> flags as MF_ACTION_REQUIRED.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> 
> I need input from the APEI reviewers on this.
> 
> Thanks!

Hi, Rafael,

Sorry, I missed this email. Thank you for you quick reply. Let's discuss with
reviewers.

Thank you.

Cheers,
Shuai


> 
>> ---
>>  drivers/acpi/apei/ghes.c | 10 ++++++++--
>>  include/linux/cper.h     |  3 +++
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 80ad530583c9..6c03059cbfc6 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -474,8 +474,14 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>>         if (sec_sev == GHES_SEV_CORRECTED &&
>>             (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
>>                 flags = MF_SOFT_OFFLINE;
>> -       if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
>> -               flags = 0;
>> +       if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE) {
>> +               if (mem_err->validation_bits & CPER_MEM_VALID_ERROR_TYPE)
>> +                       flags = mem_err->error_type == CPER_MEM_SCRUB_UC ?
>> +                                       0 :
>> +                                       MF_ACTION_REQUIRED;
>> +               else
>> +                       flags = MF_ACTION_REQUIRED;
>> +       }
>>
>>         if (flags != -1)
>>                 return ghes_do_memory_failure(mem_err->physical_addr, flags);
>> diff --git a/include/linux/cper.h b/include/linux/cper.h
>> index eacb7dd7b3af..b77ab7636614 100644
>> --- a/include/linux/cper.h
>> +++ b/include/linux/cper.h
>> @@ -235,6 +235,9 @@ enum {
>>  #define CPER_MEM_VALID_BANK_ADDRESS            0x100000
>>  #define CPER_MEM_VALID_CHIP_ID                 0x200000
>>
>> +#define CPER_MEM_SCRUB_CE                      13
>> +#define CPER_MEM_SCRUB_UC                      14
>> +
>>  #define CPER_MEM_EXT_ROW_MASK                  0x3
>>  #define CPER_MEM_EXT_ROW_SHIFT                 16
>>
>> --
>> 2.20.1.9.gb50a0d7
>>

在 2022/10/29 AM1:25, Luck, Tony 写道:
>>> cper_sec_mem_err::error_type identifies the type of error that occurred
>>> if CPER_MEM_VALID_ERROR_TYPE is set. So, set memory failure flags as 0
>>> for Scrub Uncorrected Error (type 14). Otherwise, set memory failure
>>> flags as MF_ACTION_REQUIRED.
> 
> On x86 the "action required" cases are signaled by a synchronous machine check
> that is delivered before the instruction that is attempting to consume the uncorrected
> data retires. I.e., it is guaranteed that the uncorrected error has not been propagated
> because it is not visible in any architectural state.

On arm, if a 2-bit (uncorrectable) error is detected, and the memory access has been
architecturally executed, that error is considered “consumed”. The CPU will take a
synchronous error exception, signaled as synchronous external abort (SEA), which is
analogously to MCE.

> 
> APEI signaled errors don't fall into that category on x86 ... the uncorrected data
> could have been consumed and propagated long before the signaling used for
> APEI can alert the OS.
> 
> Does ARM deliver APEI signals synchronously?
> 
> If not, then this patch might deliver a false sense of security to applications
> about the state of uncorrected data in the system.
> 

Well, it does not always. There are many APEI notification, such as SCI, GSIV, GPIO,
SDEI, SEA, etc. Not all APEI notifications are synchronously and it depends on
hardware signal. As far as I know, if a UE is detected and consumed, synchronous external
abort is signaled to firmware and firmware then performs a first-level triage and
synchronously notify OS by SDEI or SEA notification. On the other hand, if CE is
detected, a asynchronous interrupt will be signaled and firmware could notify OS
by GPIO or GSIV.

Best Regards,
Shuai

在 2022/11/2 PM7:53, Shuai Xue 写道:
> 
> 
> 在 2022/10/29 AM1:25, Luck, Tony 写道:
>>>> cper_sec_mem_err::error_type identifies the type of error that occurred
>>>> if CPER_MEM_VALID_ERROR_TYPE is set. So, set memory failure flags as 0
>>>> for Scrub Uncorrected Error (type 14). Otherwise, set memory failure
>>>> flags as MF_ACTION_REQUIRED.
>>
>> On x86 the "action required" cases are signaled by a synchronous machine check
>> that is delivered before the instruction that is attempting to consume the uncorrected
>> data retires. I.e., it is guaranteed that the uncorrected error has not been propagated
>> because it is not visible in any architectural state.
> 
> On arm, if a 2-bit (uncorrectable) error is detected, and the memory access has been
> architecturally executed, that error is considered “consumed”. The CPU will take a
> synchronous error exception, signaled as synchronous external abort (SEA), which is
> analogously to MCE.
> 
>>
>> APEI signaled errors don't fall into that category on x86 ... the uncorrected data
>> could have been consumed and propagated long before the signaling used for
>> APEI can alert the OS.
>>
>> Does ARM deliver APEI signals synchronously?
>>
>> If not, then this patch might deliver a false sense of security to applications
>> about the state of uncorrected data in the system.
>>
> 
> Well, it does not always. There are many APEI notification, such as SCI, GSIV, GPIO,
> SDEI, SEA, etc. Not all APEI notifications are synchronously and it depends on
> hardware signal. As far as I know, if a UE is detected and consumed, synchronous external
> abort is signaled to firmware and firmware then performs a first-level triage and
> synchronously notify OS by SDEI or SEA notification. On the other hand, if CE is
> detected, a asynchronous interrupt will be signaled and firmware could notify OS
> by GPIO or GSIV.
> 
> Best Regards,
> Shuai
> 
> 


Hi, Tony,

Prefetch data with UE error triggers async interrupt on both X86 and Arm64 platform
(CMCI in X86 and SPI in arm64). It does not belongs to scrub UEs. I have to admit that
cper_sec_mem_err::error_type is not an appropriate basis to distinguish
"action required" cases.



acpi_hest_generic_data::flags (UEFI spec section N.2.2) could be used to indicate
Action Optional (Scrub/Prefetch).

	Bit 5 – Latent error: If set this flag indicates that action has been
	taken to ensure error containment (such a poisoning data), but
	the error has not been fully corrected and the data has not been
	consumed. System software may choose to take further
	corrective action before the data is consumed.

Our hardware team has submitted a proposal to UEFI community to add a new bit:

	Bit 8 – sync flag; if set this flag indicates that
	this event record is synchronous(e.g. cpu
	core consumes poison data, then cause
	instruction/data abort); if not set, this event
	record is asynchronous.

With bit 8, we will know it is "Action Required".


I will send a new patch set to rework GHES error handling after the proposal is accept.


Thank you.

Best Regards
Shuai

Hi Shuai Xue，

On 2023/4/11 18:48, Shuai Xue wrote:
> There are two major types of uncorrected recoverable (UCR) errors :
> 
> - Action Required (AR): The error is detected and the processor already
>    consumes the memory. OS requires to take action (for example, offline
>    failure page/kill failure thread) to recover this uncorrectable error.
> 
> - Action Optional (AO): The error is detected out of processor execution
>    context. Some data in the memory are corrupted. But the data have not
>    been consumed. OS is optional to take action to recover this
>    uncorrectable error.
> 
> The essential difference between AR and AO errors is that AR is a
> synchronous event, while AO is an asynchronous event. The hardware will
> signal a synchronous exception (Machine Check Exception on X86 and
> Synchronous External Abort on Arm64) when an error is detected and the
> memory access has been architecturally executed.
> 
> When APEI firmware first is enabled, a platform may describe one error
> source for the handling of synchronous errors (e.g. MCE or SEA notification
> ), or for handling asynchronous errors (e.g. SCI or External Interrupt
> notification). In other words, we can distinguish synchronous errors by
> APEI notification. For AR errors, kernel will kill current process
> accessing the poisoned page by sending SIGBUS with BUS_MCEERR_AR. In
> addition, for AO errors, kernel will notify the process who owns the
> poisoned page by sending SIGBUS with BUS_MCEERR_AO in early kill mode.
> However, the GHES driver always sets mf_flags to 0 so that all UCR errors
> are handled as AO errors in memory failure.
> 
> To this end, set memory failure flags as MF_ACTION_REQUIRED on synchronous
> events.

As your mentioned in cover-letter, we met same issue, and hope it could 
be fixed ASAP, this patch looks good to me,

Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>


> 
> Fixes: ba61ca4aab47 ("ACPI, APEI, GHES: Add hardware memory error recovery support")'
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>   drivers/acpi/apei/ghes.c | 29 +++++++++++++++++++++++------
>   1 file changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 34ad071a64e9..c479b85899f5 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -101,6 +101,20 @@ static inline bool is_hest_type_generic_v2(struct ghes *ghes)
>   	return ghes->generic->header.type == ACPI_HEST_TYPE_GENERIC_ERROR_V2;
>   }
>   
> +/*
> + * A platform may describe one error source for the handling of synchronous
> + * errors (e.g. MCE or SEA), or for handling asynchronous errors (e.g. SCI
> + * or External Interrupt). On x86, the HEST notifications are always
> + * asynchronous, so only SEA on ARM is delivered as a synchronous
> + * notification.
> + */
> +static inline bool is_hest_sync_notify(struct ghes *ghes)
> +{
> +	u8 notify_type = ghes->generic->notify.type;
> +
> +	return notify_type == ACPI_HEST_NOTIFY_SEA;
> +}
> +
>   /*
>    * This driver isn't really modular, however for the time being,
>    * continuing to use module_param is the easiest way to remain
> @@ -477,7 +491,7 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>   }
>   
>   static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
> -				       int sev)
> +				       int sev, bool sync)
>   {
>   	int flags = -1;
>   	int sec_sev = ghes_severity(gdata->error_severity);
> @@ -491,7 +505,7 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>   	    (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
>   		flags = MF_SOFT_OFFLINE;
>   	if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
> -		flags = 0;
> +		flags = sync ? MF_ACTION_REQUIRED : 0;
>   
>   	if (flags != -1)
>   		return ghes_do_memory_failure(mem_err->physical_addr, flags);
> @@ -499,9 +513,11 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>   	return false;
>   }
>   
> -static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
> +				       int sev, bool sync)
>   {
>   	struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
> +	int flags = sync ? MF_ACTION_REQUIRED : 0;
>   	bool queued = false;
>   	int sec_sev, i;
>   	char *p;
> @@ -526,7 +542,7 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int s
>   		 * and don't filter out 'corrected' error here.
>   		 */
>   		if (is_cache && has_pa) {
> -			queued = ghes_do_memory_failure(err_info->physical_fault_addr, 0);
> +			queued = ghes_do_memory_failure(err_info->physical_fault_addr, flags);
>   			p += err_info->length;
>   			continue;
>   		}
> @@ -647,6 +663,7 @@ static bool ghes_do_proc(struct ghes *ghes,
>   	const guid_t *fru_id = &guid_null;
>   	char *fru_text = "";
>   	bool queued = false;
> +	bool sync = is_hest_sync_notify(ghes);
>   
>   	sev = ghes_severity(estatus->error_severity);
>   	apei_estatus_for_each_section(estatus, gdata) {
> @@ -664,13 +681,13 @@ static bool ghes_do_proc(struct ghes *ghes,
>   			atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);
>   
>   			arch_apei_report_mem_error(sev, mem_err);
> -			queued = ghes_handle_memory_failure(gdata, sev);
> +			queued = ghes_handle_memory_failure(gdata, sev, sync);
>   		}
>   		else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
>   			ghes_handle_aer(gdata);
>   		}
>   		else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
> -			queued = ghes_handle_arm_hw_error(gdata, sev);
> +			queued = ghes_handle_arm_hw_error(gdata, sev, sync);
>   		} else {
>   			void *err = acpi_hest_get_payload(gdata);
>

On 2023/4/11 18:48, Shuai Xue wrote:
> Hardware errors could be signaled by synchronous interrupt, e.g.  when an
> error is detected by a background scrubber, or signaled by synchronous
> exception, e.g. when an uncorrected error is consumed. Both synchronous and
> asynchronous error are queued and handled by a dedicated kthread in
> workqueue.
> 
> commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for
> synchronous errors") keep track of whether memory_failure() work was
> queued, and make task_work pending to flush out the workqueue so that the
> work for synchronous error is processed before returning to user-space.
> The trick ensures that the corrupted page is unmapped and poisoned. And
> after returning to user-space, the task starts at current instruction which
> triggering a page fault in which kernel will send SIGBUS to current process
> due to VM_FAULT_HWPOISON.
> 
> However, the memory failure recovery for hwpoison-aware mechanisms does not
> work as expected. For example, hwpoison-aware user-space processes like
> QEMU register their customized SIGBUS handler and enable early kill mode by
> seting PF_MCE_EARLY at initialization. Then the kernel will directy notify
> the process by sending a SIGBUS signal in memory failure with wrong
> si_code: the actual user-space process accessing the corrupt memory
> location, but its memory failure work is handled in a kthread context, so
> it will send SIGBUS with BUS_MCEERR_AO si_code to the actual user-space
> process instead of BUS_MCEERR_AR in kill_proc().
> 
> To this end, separate synchronous and asynchronous error handling into
> different paths like X86 platform does:
> 
> - valid synchronous errors: queue a task_work to synchronously send SIGBUS
>    before ret_to_user.
> - valid asynchronous errors: queue a work into workqueue to asynchronously
>    handle memory failure.
> - abnormal branches such as invalid PA, unexpected severity, no memory
>    failure config support, invalid GUID section, OOM, etc.
> 
> Then for valid synchronous errors, the current context in memory failure is
> exactly belongs to the task consuming poison data and it will send SIBBUS
> with proper si_code.
> 
> Fixes: 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous errors")
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>   drivers/acpi/apei/ghes.c | 91 +++++++++++++++++++++++++++-------------
>   include/acpi/ghes.h      |  3 --
>   mm/memory-failure.c      | 13 ------
>   3 files changed, 61 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index c479b85899f5..4b70955e25f9 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -452,28 +452,51 @@ static void ghes_clear_estatus(struct ghes *ghes,
>   }
>   
>   /*
> - * Called as task_work before returning to user-space.
> - * Ensure any queued work has been done before we return to the context that
> - * triggered the notification.
> + * struct sync_task_work - for synchronous RAS event
> + *
> + * @twork:                callback_head for task work
> + * @pfn:                  page frame number of corrupted page
> + * @flags:                fine tune action taken
> + *
> + * Structure to pass task work to be handled before
> + * ret_to_user via task_work_add().
>    */
> -static void ghes_kick_task_work(struct callback_head *head)
> +struct sync_task_work {
> +	struct callback_head twork;
> +	u64 pfn;
> +	int flags;
> +};
> +
> +static void memory_failure_cb(struct callback_head *twork)
>   {
> -	struct acpi_hest_generic_status *estatus;
> -	struct ghes_estatus_node *estatus_node;
> -	u32 node_len;
> +	int ret;
> +	struct sync_task_work *twcb =
> +		container_of(twork, struct sync_task_work, twork);
>   
> -	estatus_node = container_of(head, struct ghes_estatus_node, task_work);
> -	if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> -		memory_failure_queue_kick(estatus_node->task_work_cpu);
> +	ret = memory_failure(twcb->pfn, twcb->flags);
> +	kfree(twcb);
>   
> -	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
> -	node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus));
> -	gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
> +	if (!ret)
> +		return;
> +
> +	/*
> +	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
> +	 * to the current process with the proper error info,

This should be part of the comments of function memory_failure(),

> +	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
> +	 *
and this part is already there
> +	 * In both cases, no further processing is required.
> +	 */
so, after that, I think we could drop this comment, also the same 
comment in x86's kill_me_maybe().

> +	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
> +		return;
> +
> +	pr_err("Memory error not recovered");
> +	force_sig(SIGBUS);
>   }
>   
>   static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>   {
>   	unsigned long pfn;
> +	struct sync_task_work *twcb;
>   
>   	if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>   		return false;
> @@ -486,6 +509,18 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>   		return false;
>   	}
>   
> +	if (flags == MF_ACTION_REQUIRED && current->mm) {
> +		twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC);
> +		if (!twcb)
> +			return false;
> +
> +		twcb->pfn = pfn;
> +		twcb->flags = flags;
> +		init_task_work(&twcb->twork, memory_failure_cb);
> +		task_work_add(current, &twcb->twork, TWA_RESUME);
> +		return true;
> +	}
> +
>   	memory_failure_queue(pfn, flags);
>   	return true;
>   }
> @@ -1000,9 +1035,8 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>   	struct ghes_estatus_node *estatus_node;
>   	struct acpi_hest_generic *generic;
>   	struct acpi_hest_generic_status *estatus;
> -	bool task_work_pending;
> +	bool queued, sync;
>   	u32 len, node_len;
> -	int ret;
>   
>   	llnode = llist_del_all(&ghes_estatus_llist);
>   	/*
> @@ -1015,27 +1049,25 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>   		estatus_node = llist_entry(llnode, struct ghes_estatus_node,
>   					   llnode);
>   		estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
> +		sync = is_hest_sync_notify(estatus_node->ghes);
>   		len = cper_estatus_len(estatus);
>   		node_len = GHES_ESTATUS_NODE_LEN(len);
> -		task_work_pending = ghes_do_proc(estatus_node->ghes, estatus);
> +
> +		queued = ghes_do_proc(estatus_node->ghes, estatus) > +		/*
> +		 * If no memory failure work is queued for abnormal synchronous
> +		 * errors, do a force kill.
> +		 */
> +		if (sync && !queued)
> +			force_sig(SIGBUS);

It's better to move this part into function ghes_do_proc(), because 
there is already an is_hest_sync_notify(), and no need return value,
so make ghes_do_proc() a void function, Apart from this,

Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>

> +
>   		if (!ghes_estatus_cached(estatus)) {
>   			generic = estatus_node->generic;
>   			if (ghes_print_estatus(NULL, generic, estatus))
>   				ghes_estatus_cache_add(generic, estatus);
>   		}
> -
> -		if (task_work_pending && current->mm) {
> -			estatus_node->task_work.func = ghes_kick_task_work;
> -			estatus_node->task_work_cpu = smp_processor_id();
> -			ret = task_work_add(current, &estatus_node->task_work,
> -					    TWA_RESUME);
> -			if (ret)
> -				estatus_node->task_work.func = NULL;
> -		}
> -
> -		if (!estatus_node->task_work.func)
> -			gen_pool_free(ghes_estatus_pool,
> -				      (unsigned long)estatus_node, node_len);
> +		gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node,
> +			      node_len);
>   
>   		llnode = next;
>   	}
> @@ -1096,7 +1128,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
>   
>   	estatus_node->ghes = ghes;
>   	estatus_node->generic = ghes->generic;
> -	estatus_node->task_work.func = NULL;
>   	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>   
>   	if (__ghes_read_estatus(estatus, buf_paddr, fixmap_idx, len)) {
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 3c8bba9f1114..e5e0c308d27f 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -35,9 +35,6 @@ struct ghes_estatus_node {
>   	struct llist_node llnode;
>   	struct acpi_hest_generic *generic;
>   	struct ghes *ghes;
> -
> -	int task_work_cpu;
> -	struct callback_head task_work;
>   };
>   
>   struct ghes_estatus_cache {
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index fae9baf3be16..6ea8c325acb3 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2355,19 +2355,6 @@ static void memory_failure_work_func(struct work_struct *work)
>   	}
>   }
>   
> -/*
> - * Process memory_failure work queued on the specified CPU.
> - * Used to avoid return-to-userspace racing with the memory_failure workqueue.
> - */
> -void memory_failure_queue_kick(int cpu)
> -{
> -	struct memory_failure_cpu *mf_cpu;
> -
> -	mf_cpu = &per_cpu(memory_failure_cpu, cpu);
> -	cancel_work_sync(&mf_cpu->work);
> -	memory_failure_work_func(&mf_cpu->work);
> -}
> -
>   static int __init memory_failure_init(void)
>   {
>   	struct memory_failure_cpu *mf_cpu;

On 2023/4/11 PM10:17, Kefeng Wang wrote:
> Hi Shuai Xue，
> 
> On 2023/4/11 18:48, Shuai Xue wrote:
>> There are two major types of uncorrected recoverable (UCR) errors :
>>
>> - Action Required (AR): The error is detected and the processor already
>>    consumes the memory. OS requires to take action (for example, offline
>>    failure page/kill failure thread) to recover this uncorrectable error.
>>
>> - Action Optional (AO): The error is detected out of processor execution
>>    context. Some data in the memory are corrupted. But the data have not
>>    been consumed. OS is optional to take action to recover this
>>    uncorrectable error.
>>
>> The essential difference between AR and AO errors is that AR is a
>> synchronous event, while AO is an asynchronous event. The hardware will
>> signal a synchronous exception (Machine Check Exception on X86 and
>> Synchronous External Abort on Arm64) when an error is detected and the
>> memory access has been architecturally executed.
>>
>> When APEI firmware first is enabled, a platform may describe one error
>> source for the handling of synchronous errors (e.g. MCE or SEA notification
>> ), or for handling asynchronous errors (e.g. SCI or External Interrupt
>> notification). In other words, we can distinguish synchronous errors by
>> APEI notification. For AR errors, kernel will kill current process
>> accessing the poisoned page by sending SIGBUS with BUS_MCEERR_AR. In
>> addition, for AO errors, kernel will notify the process who owns the
>> poisoned page by sending SIGBUS with BUS_MCEERR_AO in early kill mode.
>> However, the GHES driver always sets mf_flags to 0 so that all UCR errors
>> are handled as AO errors in memory failure.
>>
>> To this end, set memory failure flags as MF_ACTION_REQUIRED on synchronous
>> events.
> 
> As your mentioned in cover-letter, we met same issue, and hope it could be fixed ASAP, this patch looks good to me,
> 
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>

Thank you.

Cheers,
Shuai

> 
>>
>> Fixes: ba61ca4aab47 ("ACPI, APEI, GHES: Add hardware memory error recovery support")'
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   drivers/acpi/apei/ghes.c | 29 +++++++++++++++++++++++------
>>   1 file changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 34ad071a64e9..c479b85899f5 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -101,6 +101,20 @@ static inline bool is_hest_type_generic_v2(struct ghes *ghes)
>>       return ghes->generic->header.type == ACPI_HEST_TYPE_GENERIC_ERROR_V2;
>>   }
>>   +/*
>> + * A platform may describe one error source for the handling of synchronous
>> + * errors (e.g. MCE or SEA), or for handling asynchronous errors (e.g. SCI
>> + * or External Interrupt). On x86, the HEST notifications are always
>> + * asynchronous, so only SEA on ARM is delivered as a synchronous
>> + * notification.
>> + */
>> +static inline bool is_hest_sync_notify(struct ghes *ghes)
>> +{
>> +    u8 notify_type = ghes->generic->notify.type;
>> +
>> +    return notify_type == ACPI_HEST_NOTIFY_SEA;
>> +}
>> +
>>   /*
>>    * This driver isn't really modular, however for the time being,
>>    * continuing to use module_param is the easiest way to remain
>> @@ -477,7 +491,7 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>   }
>>     static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>> -                       int sev)
>> +                       int sev, bool sync)
>>   {
>>       int flags = -1;
>>       int sec_sev = ghes_severity(gdata->error_severity);
>> @@ -491,7 +505,7 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>>           (gdata->flags & CPER_SEC_ERROR_THRESHOLD_EXCEEDED))
>>           flags = MF_SOFT_OFFLINE;
>>       if (sev == GHES_SEV_RECOVERABLE && sec_sev == GHES_SEV_RECOVERABLE)
>> -        flags = 0;
>> +        flags = sync ? MF_ACTION_REQUIRED : 0;
>>         if (flags != -1)
>>           return ghes_do_memory_failure(mem_err->physical_addr, flags);
>> @@ -499,9 +513,11 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>>       return false;
>>   }
>>   -static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
>> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
>> +                       int sev, bool sync)
>>   {
>>       struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
>> +    int flags = sync ? MF_ACTION_REQUIRED : 0;
>>       bool queued = false;
>>       int sec_sev, i;
>>       char *p;
>> @@ -526,7 +542,7 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int s
>>            * and don't filter out 'corrected' error here.
>>            */
>>           if (is_cache && has_pa) {
>> -            queued = ghes_do_memory_failure(err_info->physical_fault_addr, 0);
>> +            queued = ghes_do_memory_failure(err_info->physical_fault_addr, flags);
>>               p += err_info->length;
>>               continue;
>>           }
>> @@ -647,6 +663,7 @@ static bool ghes_do_proc(struct ghes *ghes,
>>       const guid_t *fru_id = &guid_null;
>>       char *fru_text = "";
>>       bool queued = false;
>> +    bool sync = is_hest_sync_notify(ghes);
>>         sev = ghes_severity(estatus->error_severity);
>>       apei_estatus_for_each_section(estatus, gdata) {
>> @@ -664,13 +681,13 @@ static bool ghes_do_proc(struct ghes *ghes,
>>               atomic_notifier_call_chain(&ghes_report_chain, sev, mem_err);
>>                 arch_apei_report_mem_error(sev, mem_err);
>> -            queued = ghes_handle_memory_failure(gdata, sev);
>> +            queued = ghes_handle_memory_failure(gdata, sev, sync);
>>           }
>>           else if (guid_equal(sec_type, &CPER_SEC_PCIE)) {
>>               ghes_handle_aer(gdata);
>>           }
>>           else if (guid_equal(sec_type, &CPER_SEC_PROC_ARM)) {
>> -            queued = ghes_handle_arm_hw_error(gdata, sev);
>> +            queued = ghes_handle_arm_hw_error(gdata, sev, sync);
>>           } else {
>>               void *err = acpi_hest_get_payload(gdata);
>>

On 2023/4/11 PM10:28, Kefeng Wang wrote:
> 
> 
> On 2023/4/11 18:48, Shuai Xue wrote:
>> Hardware errors could be signaled by synchronous interrupt, e.g.  when an
>> error is detected by a background scrubber, or signaled by synchronous
>> exception, e.g. when an uncorrected error is consumed. Both synchronous and
>> asynchronous error are queued and handled by a dedicated kthread in
>> workqueue.
>>
>> commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for
>> synchronous errors") keep track of whether memory_failure() work was
>> queued, and make task_work pending to flush out the workqueue so that the
>> work for synchronous error is processed before returning to user-space.
>> The trick ensures that the corrupted page is unmapped and poisoned. And
>> after returning to user-space, the task starts at current instruction which
>> triggering a page fault in which kernel will send SIGBUS to current process
>> due to VM_FAULT_HWPOISON.
>>
>> However, the memory failure recovery for hwpoison-aware mechanisms does not
>> work as expected. For example, hwpoison-aware user-space processes like
>> QEMU register their customized SIGBUS handler and enable early kill mode by
>> seting PF_MCE_EARLY at initialization. Then the kernel will directy notify
>> the process by sending a SIGBUS signal in memory failure with wrong
>> si_code: the actual user-space process accessing the corrupt memory
>> location, but its memory failure work is handled in a kthread context, so
>> it will send SIGBUS with BUS_MCEERR_AO si_code to the actual user-space
>> process instead of BUS_MCEERR_AR in kill_proc().
>>
>> To this end, separate synchronous and asynchronous error handling into
>> different paths like X86 platform does:
>>
>> - valid synchronous errors: queue a task_work to synchronously send SIGBUS
>>    before ret_to_user.
>> - valid asynchronous errors: queue a work into workqueue to asynchronously
>>    handle memory failure.
>> - abnormal branches such as invalid PA, unexpected severity, no memory
>>    failure config support, invalid GUID section, OOM, etc.
>>
>> Then for valid synchronous errors, the current context in memory failure is
>> exactly belongs to the task consuming poison data and it will send SIBBUS
>> with proper si_code.
>>
>> Fixes: 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous errors")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   drivers/acpi/apei/ghes.c | 91 +++++++++++++++++++++++++++-------------
>>   include/acpi/ghes.h      |  3 --
>>   mm/memory-failure.c      | 13 ------
>>   3 files changed, 61 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index c479b85899f5..4b70955e25f9 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -452,28 +452,51 @@ static void ghes_clear_estatus(struct ghes *ghes,
>>   }
>>     /*
>> - * Called as task_work before returning to user-space.
>> - * Ensure any queued work has been done before we return to the context that
>> - * triggered the notification.
>> + * struct sync_task_work - for synchronous RAS event
>> + *
>> + * @twork:                callback_head for task work
>> + * @pfn:                  page frame number of corrupted page
>> + * @flags:                fine tune action taken
>> + *
>> + * Structure to pass task work to be handled before
>> + * ret_to_user via task_work_add().
>>    */
>> -static void ghes_kick_task_work(struct callback_head *head)
>> +struct sync_task_work {
>> +    struct callback_head twork;
>> +    u64 pfn;
>> +    int flags;
>> +};
>> +
>> +static void memory_failure_cb(struct callback_head *twork)
>>   {
>> -    struct acpi_hest_generic_status *estatus;
>> -    struct ghes_estatus_node *estatus_node;
>> -    u32 node_len;
>> +    int ret;
>> +    struct sync_task_work *twcb =
>> +        container_of(twork, struct sync_task_work, twork);
>>   -    estatus_node = container_of(head, struct ghes_estatus_node, task_work);
>> -    if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>> -        memory_failure_queue_kick(estatus_node->task_work_cpu);
>> +    ret = memory_failure(twcb->pfn, twcb->flags);
>> +    kfree(twcb);
>>   -    estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>> -    node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus));
>> -    gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
>> +    if (!ret)
>> +        return;
>> +
>> +    /*
>> +     * -EHWPOISON from memory_failure() means that it already sent SIGBUS
>> +     * to the current process with the proper error info,
> 
> This should be part of the comments of function memory_failure(),
> 
>> +     * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
>> +     *
> and this part is already there
>> +     * In both cases, no further processing is required.
>> +     */
> so, after that, I think we could drop this comment, also the same comment in x86's kill_me_maybe().

Ok, I will add comments on return value of memory_failure() and drop both this
comment and that in kill_me_maybe() out.

> 
>> +    if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
>> +        return;
>> +
>> +    pr_err("Memory error not recovered");
>> +    force_sig(SIGBUS);
>>   }
>>     static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>   {
>>       unsigned long pfn;
>> +    struct sync_task_work *twcb;
>>         if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>>           return false;
>> @@ -486,6 +509,18 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>           return false;
>>       }
>>   +    if (flags == MF_ACTION_REQUIRED && current->mm) {
>> +        twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC);
>> +        if (!twcb)
>> +            return false;
>> +
>> +        twcb->pfn = pfn;
>> +        twcb->flags = flags;
>> +        init_task_work(&twcb->twork, memory_failure_cb);
>> +        task_work_add(current, &twcb->twork, TWA_RESUME);
>> +        return true;
>> +    }
>> +
>>       memory_failure_queue(pfn, flags);
>>       return true;
>>   }
>> @@ -1000,9 +1035,8 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>>       struct ghes_estatus_node *estatus_node;
>>       struct acpi_hest_generic *generic;
>>       struct acpi_hest_generic_status *estatus;
>> -    bool task_work_pending;
>> +    bool queued, sync;
>>       u32 len, node_len;
>> -    int ret;
>>         llnode = llist_del_all(&ghes_estatus_llist);
>>       /*
>> @@ -1015,27 +1049,25 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>>           estatus_node = llist_entry(llnode, struct ghes_estatus_node,
>>                          llnode);
>>           estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>> +        sync = is_hest_sync_notify(estatus_node->ghes);
>>           len = cper_estatus_len(estatus);
>>           node_len = GHES_ESTATUS_NODE_LEN(len);
>> -        task_work_pending = ghes_do_proc(estatus_node->ghes, estatus);
>> +
>> +        queued = ghes_do_proc(estatus_node->ghes, estatus) > +        /*
>> +         * If no memory failure work is queued for abnormal synchronous
>> +         * errors, do a force kill.
>> +         */
>> +        if (sync && !queued)
>> +            force_sig(SIGBUS);
> 
> It's better to move this part into function ghes_do_proc(), because there is already an is_hest_sync_notify(), and no need return value,
> so make ghes_do_proc() a void function, Apart from this,

Good idea. I will do this and send a new version.

> 
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>

Thank you.

Cheers,
Shuai

> 
>> +
>>           if (!ghes_estatus_cached(estatus)) {
>>               generic = estatus_node->generic;
>>               if (ghes_print_estatus(NULL, generic, estatus))
>>                   ghes_estatus_cache_add(generic, estatus);
>>           }
>> -
>> -        if (task_work_pending && current->mm) {
>> -            estatus_node->task_work.func = ghes_kick_task_work;
>> -            estatus_node->task_work_cpu = smp_processor_id();
>> -            ret = task_work_add(current, &estatus_node->task_work,
>> -                        TWA_RESUME);
>> -            if (ret)
>> -                estatus_node->task_work.func = NULL;
>> -        }
>> -
>> -        if (!estatus_node->task_work.func)
>> -            gen_pool_free(ghes_estatus_pool,
>> -                      (unsigned long)estatus_node, node_len);
>> +        gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node,
>> +                  node_len);
>>             llnode = next;
>>       }
>> @@ -1096,7 +1128,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
>>         estatus_node->ghes = ghes;
>>       estatus_node->generic = ghes->generic;
>> -    estatus_node->task_work.func = NULL;
>>       estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>>         if (__ghes_read_estatus(estatus, buf_paddr, fixmap_idx, len)) {
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index 3c8bba9f1114..e5e0c308d27f 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -35,9 +35,6 @@ struct ghes_estatus_node {
>>       struct llist_node llnode;
>>       struct acpi_hest_generic *generic;
>>       struct ghes *ghes;
>> -
>> -    int task_work_cpu;
>> -    struct callback_head task_work;
>>   };
>>     struct ghes_estatus_cache {
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index fae9baf3be16..6ea8c325acb3 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2355,19 +2355,6 @@ static void memory_failure_work_func(struct work_struct *work)
>>       }
>>   }
>>   -/*
>> - * Process memory_failure work queued on the specified CPU.
>> - * Used to avoid return-to-userspace racing with the memory_failure workqueue.
>> - */
>> -void memory_failure_queue_kick(int cpu)
>> -{
>> -    struct memory_failure_cpu *mf_cpu;
>> -
>> -    mf_cpu = &per_cpu(memory_failure_cpu, cpu);
>> -    cancel_work_sync(&mf_cpu->work);
>> -    memory_failure_work_func(&mf_cpu->work);
>> -}
>> -
>>   static int __init memory_failure_init(void)
>>   {
>>       struct memory_failure_cpu *mf_cpu;

在 2023/4/11 18:48, Shuai Xue 写道:
> Hardware errors could be signaled by synchronous interrupt, e.g.  when an
> error is detected by a background scrubber, or signaled by synchronous
> exception, e.g. when an uncorrected error is consumed. Both synchronous and
> asynchronous error are queued and handled by a dedicated kthread in
> workqueue.
>
> commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for
> synchronous errors") keep track of whether memory_failure() work was
> queued, and make task_work pending to flush out the workqueue so that the
> work for synchronous error is processed before returning to user-space.
> The trick ensures that the corrupted page is unmapped and poisoned. And
> after returning to user-space, the task starts at current instruction which
> triggering a page fault in which kernel will send SIGBUS to current process
> due to VM_FAULT_HWPOISON.
>
> However, the memory failure recovery for hwpoison-aware mechanisms does not
> work as expected. For example, hwpoison-aware user-space processes like
> QEMU register their customized SIGBUS handler and enable early kill mode by
> seting PF_MCE_EARLY at initialization. Then the kernel will directy notify
> the process by sending a SIGBUS signal in memory failure with wrong
> si_code: the actual user-space process accessing the corrupt memory
> location, but its memory failure work is handled in a kthread context, so
> it will send SIGBUS with BUS_MCEERR_AO si_code to the actual user-space
> process instead of BUS_MCEERR_AR in kill_proc().
>
> To this end, separate synchronous and asynchronous error handling into
> different paths like X86 platform does:
>
> - valid synchronous errors: queue a task_work to synchronously send SIGBUS
>    before ret_to_user.
> - valid asynchronous errors: queue a work into workqueue to asynchronously
>    handle memory failure.
> - abnormal branches such as invalid PA, unexpected severity, no memory
>    failure config support, invalid GUID section, OOM, etc.
>
> Then for valid synchronous errors, the current context in memory failure is
> exactly belongs to the task consuming poison data and it will send SIBBUS
> with proper si_code.
>
> Fixes: 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous errors")
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> ---
>   drivers/acpi/apei/ghes.c | 91 +++++++++++++++++++++++++++-------------
>   include/acpi/ghes.h      |  3 --
>   mm/memory-failure.c      | 13 ------
>   3 files changed, 61 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index c479b85899f5..4b70955e25f9 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -452,28 +452,51 @@ static void ghes_clear_estatus(struct ghes *ghes,
>   }
>   
>   /*
> - * Called as task_work before returning to user-space.
> - * Ensure any queued work has been done before we return to the context that
> - * triggered the notification.
> + * struct sync_task_work - for synchronous RAS event
> + *
> + * @twork:                callback_head for task work
> + * @pfn:                  page frame number of corrupted page
> + * @flags:                fine tune action taken
> + *
> + * Structure to pass task work to be handled before
> + * ret_to_user via task_work_add().
>    */
> -static void ghes_kick_task_work(struct callback_head *head)
> +struct sync_task_work {
> +	struct callback_head twork;
> +	u64 pfn;
> +	int flags;
> +};
> +
> +static void memory_failure_cb(struct callback_head *twork)
>   {
> -	struct acpi_hest_generic_status *estatus;
> -	struct ghes_estatus_node *estatus_node;
> -	u32 node_len;
> +	int ret;
> +	struct sync_task_work *twcb =
> +		container_of(twork, struct sync_task_work, twork);
>   
> -	estatus_node = container_of(head, struct ghes_estatus_node, task_work);
> -	if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> -		memory_failure_queue_kick(estatus_node->task_work_cpu);
> +	ret = memory_failure(twcb->pfn, twcb->flags);
> +	kfree(twcb);
>   
> -	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
> -	node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus));
> -	gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
> +	if (!ret)
> +		return;
> +
> +	/*
> +	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
> +	 * to the current process with the proper error info,
> +	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
> +	 *
> +	 * In both cases, no further processing is required.
> +	 */
> +	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
> +		return;
> +
> +	pr_err("Memory error not recovered");

The print could add the following SIGBUS signal sending.
Such as "Sending SIGBUS to current task due to memory error not recovered"

> +	force_sig(SIGBUS);
>   }
>   
>   static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>   {
>   	unsigned long pfn;
> +	struct sync_task_work *twcb;
>   
>   	if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>   		return false;
> @@ -486,6 +509,18 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>   		return false;
>   	}
>   
> +	if (flags == MF_ACTION_REQUIRED && current->mm) {
> +		twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC);
> +		if (!twcb)
> +			return false;
> +
> +		twcb->pfn = pfn;
> +		twcb->flags = flags;
> +		init_task_work(&twcb->twork, memory_failure_cb);
> +		task_work_add(current, &twcb->twork, TWA_RESUME);
> +		return true;
> +	}
> +
>   	memory_failure_queue(pfn, flags);
>   	return true;
>   }
> @@ -1000,9 +1035,8 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>   	struct ghes_estatus_node *estatus_node;
>   	struct acpi_hest_generic *generic;
>   	struct acpi_hest_generic_status *estatus;
> -	bool task_work_pending;
> +	bool queued, sync;
>   	u32 len, node_len;
> -	int ret;
>   
>   	llnode = llist_del_all(&ghes_estatus_llist);
>   	/*
> @@ -1015,27 +1049,25 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>   		estatus_node = llist_entry(llnode, struct ghes_estatus_node,
>   					   llnode);
>   		estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
> +		sync = is_hest_sync_notify(estatus_node->ghes);
>   		len = cper_estatus_len(estatus);
>   		node_len = GHES_ESTATUS_NODE_LEN(len);
> -		task_work_pending = ghes_do_proc(estatus_node->ghes, estatus);
> +
> +		queued = ghes_do_proc(estatus_node->ghes, estatus);
> +		/*
> +		 * If no memory failure work is queued for abnormal synchronous
> +		 * errors, do a force kill.
> +		 */
> +		if (sync && !queued)
> +			force_sig(SIGBUS);

Could also add one similar print here as above
Apart from this,
Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>

> +
>   		if (!ghes_estatus_cached(estatus)) {
>   			generic = estatus_node->generic;
>   			if (ghes_print_estatus(NULL, generic, estatus))
>   				ghes_estatus_cache_add(generic, estatus);
>   		}
> -
> -		if (task_work_pending && current->mm) {
> -			estatus_node->task_work.func = ghes_kick_task_work;
> -			estatus_node->task_work_cpu = smp_processor_id();
> -			ret = task_work_add(current, &estatus_node->task_work,
> -					    TWA_RESUME);
> -			if (ret)
> -				estatus_node->task_work.func = NULL;
> -		}
> -
> -		if (!estatus_node->task_work.func)
> -			gen_pool_free(ghes_estatus_pool,
> -				      (unsigned long)estatus_node, node_len);
> +		gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node,
> +			      node_len);
>   
>   		llnode = next;
>   	}
> @@ -1096,7 +1128,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
>   
>   	estatus_node->ghes = ghes;
>   	estatus_node->generic = ghes->generic;
> -	estatus_node->task_work.func = NULL;
>   	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>   
>   	if (__ghes_read_estatus(estatus, buf_paddr, fixmap_idx, len)) {
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 3c8bba9f1114..e5e0c308d27f 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -35,9 +35,6 @@ struct ghes_estatus_node {
>   	struct llist_node llnode;
>   	struct acpi_hest_generic *generic;
>   	struct ghes *ghes;
> -
> -	int task_work_cpu;
> -	struct callback_head task_work;
>   };
>   
>   struct ghes_estatus_cache {
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index fae9baf3be16..6ea8c325acb3 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2355,19 +2355,6 @@ static void memory_failure_work_func(struct work_struct *work)
>   	}
>   }
>   
> -/*
> - * Process memory_failure work queued on the specified CPU.
> - * Used to avoid return-to-userspace racing with the memory_failure workqueue.
> - */
> -void memory_failure_queue_kick(int cpu)
> -{
> -	struct memory_failure_cpu *mf_cpu;
> -
> -	mf_cpu = &per_cpu(memory_failure_cpu, cpu);
> -	cancel_work_sync(&mf_cpu->work);
> -	memory_failure_work_func(&mf_cpu->work);
> -}
> -
>   static int __init memory_failure_init(void)
>   {
>   	struct memory_failure_cpu *mf_cpu;

On 2023/4/12 PM12:05, Xiaofei Tan wrote:
> 
> 在 2023/4/11 18:48, Shuai Xue 写道:
>> Hardware errors could be signaled by synchronous interrupt, e.g.  when an
>> error is detected by a background scrubber, or signaled by synchronous
>> exception, e.g. when an uncorrected error is consumed. Both synchronous and
>> asynchronous error are queued and handled by a dedicated kthread in
>> workqueue.
>>
>> commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for
>> synchronous errors") keep track of whether memory_failure() work was
>> queued, and make task_work pending to flush out the workqueue so that the
>> work for synchronous error is processed before returning to user-space.
>> The trick ensures that the corrupted page is unmapped and poisoned. And
>> after returning to user-space, the task starts at current instruction which
>> triggering a page fault in which kernel will send SIGBUS to current process
>> due to VM_FAULT_HWPOISON.
>>
>> However, the memory failure recovery for hwpoison-aware mechanisms does not
>> work as expected. For example, hwpoison-aware user-space processes like
>> QEMU register their customized SIGBUS handler and enable early kill mode by
>> seting PF_MCE_EARLY at initialization. Then the kernel will directy notify
>> the process by sending a SIGBUS signal in memory failure with wrong
>> si_code: the actual user-space process accessing the corrupt memory
>> location, but its memory failure work is handled in a kthread context, so
>> it will send SIGBUS with BUS_MCEERR_AO si_code to the actual user-space
>> process instead of BUS_MCEERR_AR in kill_proc().
>>
>> To this end, separate synchronous and asynchronous error handling into
>> different paths like X86 platform does:
>>
>> - valid synchronous errors: queue a task_work to synchronously send SIGBUS
>>    before ret_to_user.
>> - valid asynchronous errors: queue a work into workqueue to asynchronously
>>    handle memory failure.
>> - abnormal branches such as invalid PA, unexpected severity, no memory
>>    failure config support, invalid GUID section, OOM, etc.
>>
>> Then for valid synchronous errors, the current context in memory failure is
>> exactly belongs to the task consuming poison data and it will send SIBBUS
>> with proper si_code.
>>
>> Fixes: 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous errors")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
>> ---
>>   drivers/acpi/apei/ghes.c | 91 +++++++++++++++++++++++++++-------------
>>   include/acpi/ghes.h      |  3 --
>>   mm/memory-failure.c      | 13 ------
>>   3 files changed, 61 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index c479b85899f5..4b70955e25f9 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -452,28 +452,51 @@ static void ghes_clear_estatus(struct ghes *ghes,
>>   }
>>     /*
>> - * Called as task_work before returning to user-space.
>> - * Ensure any queued work has been done before we return to the context that
>> - * triggered the notification.
>> + * struct sync_task_work - for synchronous RAS event
>> + *
>> + * @twork:                callback_head for task work
>> + * @pfn:                  page frame number of corrupted page
>> + * @flags:                fine tune action taken
>> + *
>> + * Structure to pass task work to be handled before
>> + * ret_to_user via task_work_add().
>>    */
>> -static void ghes_kick_task_work(struct callback_head *head)
>> +struct sync_task_work {
>> +    struct callback_head twork;
>> +    u64 pfn;
>> +    int flags;
>> +};
>> +
>> +static void memory_failure_cb(struct callback_head *twork)
>>   {
>> -    struct acpi_hest_generic_status *estatus;
>> -    struct ghes_estatus_node *estatus_node;
>> -    u32 node_len;
>> +    int ret;
>> +    struct sync_task_work *twcb =
>> +        container_of(twork, struct sync_task_work, twork);
>>   -    estatus_node = container_of(head, struct ghes_estatus_node, task_work);
>> -    if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>> -        memory_failure_queue_kick(estatus_node->task_work_cpu);
>> +    ret = memory_failure(twcb->pfn, twcb->flags);
>> +    kfree(twcb);
>>   -    estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>> -    node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus));
>> -    gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
>> +    if (!ret)
>> +        return;
>> +
>> +    /*
>> +     * -EHWPOISON from memory_failure() means that it already sent SIGBUS
>> +     * to the current process with the proper error info,
>> +     * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
>> +     *
>> +     * In both cases, no further processing is required.
>> +     */
>> +    if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
>> +        return;
>> +
>> +    pr_err("Memory error not recovered");
> 
> The print could add the following SIGBUS signal sending.
> Such as "Sending SIGBUS to current task due to memory error not recovered"
> 
>> +    force_sig(SIGBUS);
>>   }
>>     static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>   {
>>       unsigned long pfn;
>> +    struct sync_task_work *twcb;
>>         if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
>>           return false;
>> @@ -486,6 +509,18 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>>           return false;
>>       }
>>   +    if (flags == MF_ACTION_REQUIRED && current->mm) {
>> +        twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC);
>> +        if (!twcb)
>> +            return false;
>> +
>> +        twcb->pfn = pfn;
>> +        twcb->flags = flags;
>> +        init_task_work(&twcb->twork, memory_failure_cb);
>> +        task_work_add(current, &twcb->twork, TWA_RESUME);
>> +        return true;
>> +    }
>> +
>>       memory_failure_queue(pfn, flags);
>>       return true;
>>   }
>> @@ -1000,9 +1035,8 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>>       struct ghes_estatus_node *estatus_node;
>>       struct acpi_hest_generic *generic;
>>       struct acpi_hest_generic_status *estatus;
>> -    bool task_work_pending;
>> +    bool queued, sync;
>>       u32 len, node_len;
>> -    int ret;
>>         llnode = llist_del_all(&ghes_estatus_llist);
>>       /*
>> @@ -1015,27 +1049,25 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>>           estatus_node = llist_entry(llnode, struct ghes_estatus_node,
>>                          llnode);
>>           estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>> +        sync = is_hest_sync_notify(estatus_node->ghes);
>>           len = cper_estatus_len(estatus);
>>           node_len = GHES_ESTATUS_NODE_LEN(len);
>> -        task_work_pending = ghes_do_proc(estatus_node->ghes, estatus);
>> +
>> +        queued = ghes_do_proc(estatus_node->ghes, estatus);
>> +        /*
>> +         * If no memory failure work is queued for abnormal synchronous
>> +         * errors, do a force kill.
>> +         */
>> +        if (sync && !queued)
>> +            force_sig(SIGBUS);
> 
> Could also add one similar print here as above
> Apart from this,
> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>

Thanks :)

Sorry, I missed your replies, because Thunderbird marks an email as Junk,
just move it to the Junk folder.

I'd like to add above warning message and pick up your reviewed-by tag.

Cheers,
Shuai



> 
>> +
>>           if (!ghes_estatus_cached(estatus)) {
>>               generic = estatus_node->generic;
>>               if (ghes_print_estatus(NULL, generic, estatus))
>>                   ghes_estatus_cache_add(generic, estatus);
>>           }
>> -
>> -        if (task_work_pending && current->mm) {
>> -            estatus_node->task_work.func = ghes_kick_task_work;
>> -            estatus_node->task_work_cpu = smp_processor_id();
>> -            ret = task_work_add(current, &estatus_node->task_work,
>> -                        TWA_RESUME);
>> -            if (ret)
>> -                estatus_node->task_work.func = NULL;
>> -        }
>> -
>> -        if (!estatus_node->task_work.func)
>> -            gen_pool_free(ghes_estatus_pool,
>> -                      (unsigned long)estatus_node, node_len);
>> +        gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node,
>> +                  node_len);
>>             llnode = next;
>>       }
>> @@ -1096,7 +1128,6 @@ static int ghes_in_nmi_queue_one_entry(struct ghes *ghes,
>>         estatus_node->ghes = ghes;
>>       estatus_node->generic = ghes->generic;
>> -    estatus_node->task_work.func = NULL;
>>       estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>>         if (__ghes_read_estatus(estatus, buf_paddr, fixmap_idx, len)) {
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index 3c8bba9f1114..e5e0c308d27f 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -35,9 +35,6 @@ struct ghes_estatus_node {
>>       struct llist_node llnode;
>>       struct acpi_hest_generic *generic;
>>       struct ghes *ghes;
>> -
>> -    int task_work_cpu;
>> -    struct callback_head task_work;
>>   };
>>     struct ghes_estatus_cache {
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index fae9baf3be16..6ea8c325acb3 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2355,19 +2355,6 @@ static void memory_failure_work_func(struct work_struct *work)
>>       }
>>   }
>>   -/*
>> - * Process memory_failure work queued on the specified CPU.
>> - * Used to avoid return-to-userspace racing with the memory_failure workqueue.
>> - */
>> -void memory_failure_queue_kick(int cpu)
>> -{
>> -    struct memory_failure_cpu *mf_cpu;
>> -
>> -    mf_cpu = &per_cpu(memory_failure_cpu, cpu);
>> -    cancel_work_sync(&mf_cpu->work);
>> -    memory_failure_work_func(&mf_cpu->work);
>> -}
>> -
>>   static int __init memory_failure_init(void)
>>   {
>>       struct memory_failure_cpu *mf_cpu;

On Mon, Dec 18, 2023 at 02:45:19PM +0800, Shuai Xue wrote:
> Synchronous error was detected as a result of user-space process accessing
> a 2-bit uncorrected error. The CPU will take a synchronous error exception
> such as Synchronous External Abort (SEA) on Arm64. The kernel will queue a
> memory_failure() work which poisons the related page, unmaps the page, and
> then sends a SIGBUS to the process, so that a system wide panic can be
> avoided.
> 
> However, no memory_failure() work will be queued when abnormal synchronous
> errors occur. These errors can include situations such as invalid PA,
> unexpected severity, no memory failure config support, invalid GUID
> section, etc. In such case, the user-space process will trigger SEA again.
> This loop can potentially exceed the platform firmware threshold or even
> trigger a kernel hard lockup, leading to a system reboot.
> 
> Fix it by performing a force kill if no memory_failure() work is queued for synchronous errors.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>  drivers/acpi/apei/ghes.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

On Mon, Dec 18, 2023 at 02:45:20PM +0800, Shuai Xue wrote:
> Part of return value comments for memory_failure() were originally
> documented at the call site. Move those comments to the function
> declaration to improve code readability and to provide developers with
> immediate access to function usage and return information.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 9 +--------
>  mm/memory-failure.c            | 9 ++++++---
>  2 files changed, 7 insertions(+), 11 deletions(-)
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

On Mon, Dec 18, 2023 at 02:45:21PM +0800, Shuai Xue wrote:
> Hardware errors could be signaled by asynchronous interrupt, e.g. when an
> error is detected by a background scrubber, or signaled by synchronous
> exception, e.g. when a CPU tries to access a poisoned cache line. Both
> synchronous and asynchronous error are queued as a memory_failure() work
> and handled by a dedicated kthread in workqueue.
> 
> However, the memory failure recovery sends SIBUS with wrong BUS_MCEERR_AO
> si_code for synchronous errors in early kill mode, even MF_ACTION_REQUIRED
> is set. The main problem is that the memory failure work is handled in
> kthread context but not the user-space process which is accessing the
> corrupt memory location, so it will send SIGBUS with BUS_MCEERR_AO si_code
> to the user-space process instead of BUS_MCEERR_AR in kill_proc().
> 
> To this end, queue memory_failure() as a task_work so that the current
> context in memory_failure() is exactly belongs to the process consuming
> poison data and it will send SIBBUS with proper si_code.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  drivers/acpi/apei/ghes.c | 77 +++++++++++++++++++++++-----------------
>  include/acpi/ghes.h      |  3 --
>  mm/memory-failure.c      | 13 -------
>  3 files changed, 44 insertions(+), 49 deletions(-)
> 


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

On Mon Sep 2, 2024 at 6:00 AM EEST, Shuai Xue wrote:
> Part of return value comments for memory_failure() were originally
> documented at the call site. Move those comments to the function
> declaration to improve code readability and to provide developers with
> immediate access to function usage and return information.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>  arch/x86/kernel/cpu/mce/core.c | 7 -------
>  mm/memory-failure.c            | 9 ++++++---
>  2 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index b85ec7a4ec9e..66693b6dd1cd 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1361,13 +1361,6 @@ static void kill_me_maybe(struct callback_head *cb)
>  		return;
>  	}
>  
> -	/*
> -	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
> -	 * to the current process with the proper error info,
> -	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
> -	 *
> -	 * In both cases, no further processing is required.
> -	 */
>  	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
>  		return;
>  
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7066fc84f351..df26e2ff5e06 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2199,9 +2199,12 @@ static void kill_procs_now(struct page *p, unsigned long pfn, int flags,
>   * Must run in process context (e.g. a work queue) with interrupts
>   * enabled and no spinlocks held.
>   *
> - * Return: 0 for successfully handled the memory error,
> - *         -EOPNOTSUPP for hwpoison_filter() filtered the error event,
> - *         < 0(except -EOPNOTSUPP) on failure.
> + * Return values:

s/Return values/Return:/ 

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

> + *   0             - success,
> + *   -EOPNOTSUPP   - hwpoison_filter() filtered the error event,
> + *   -EHWPOISON    - the page was already poisoned, potentially
> + *                   kill process,
> + *   other negative values - failure.
>   */
>  int memory_failure(unsigned long pfn, int flags)
>  {

BR, Jarkko

在 2024/9/4 00:10, Jarkko Sakkinen 写道:
> On Mon Sep 2, 2024 at 6:00 AM EEST, Shuai Xue wrote:
>> Part of return value comments for memory_failure() were originally
>> documented at the call site. Move those comments to the function
>> declaration to improve code readability and to provide developers with
>> immediate access to function usage and return information.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> ---
>>   arch/x86/kernel/cpu/mce/core.c | 7 -------
>>   mm/memory-failure.c            | 9 ++++++---
>>   2 files changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
>> index b85ec7a4ec9e..66693b6dd1cd 100644
>> --- a/arch/x86/kernel/cpu/mce/core.c
>> +++ b/arch/x86/kernel/cpu/mce/core.c
>> @@ -1361,13 +1361,6 @@ static void kill_me_maybe(struct callback_head *cb)
>>   		return;
>>   	}
>>   
>> -	/*
>> -	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
>> -	 * to the current process with the proper error info,
>> -	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
>> -	 *
>> -	 * In both cases, no further processing is required.
>> -	 */
>>   	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
>>   		return;
>>   
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 7066fc84f351..df26e2ff5e06 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2199,9 +2199,12 @@ static void kill_procs_now(struct page *p, unsigned long pfn, int flags,
>>    * Must run in process context (e.g. a work queue) with interrupts
>>    * enabled and no spinlocks held.
>>    *
>> - * Return: 0 for successfully handled the memory error,
>> - *         -EOPNOTSUPP for hwpoison_filter() filtered the error event,
>> - *         < 0(except -EOPNOTSUPP) on failure.
>> + * Return values:
> 
> s/Return values/Return:/
> 
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> 

Hi, Jarkko,

Thank you for the reminder. Will fix it in next version.

Best Regards,
Shuai

Hi, all,

Gentle ping.

Best Regards,
Shuai

在 2024/9/2 11:00, Shuai Xue 写道:
> ## Changes Log
> 
> changes since v11:
> - rebase to Linux 6.11-rc6
> - fix grammer and typo in commit log (per Borislav)
> - remove `sync_` perfix of `sync_task_work`  (per Borislav)
> - comments flags and description of `task_work`  (per Borislav)
> 
> changes since v10:
> - rebase to v6.8-rc2
> 
> changes since v9:
> - split patch 2 to address exactly one issue in one patch (per Borislav)
> - rewrite commit log according to template (per Borislav)
> - pickup reviewed-by tag of patch 1 from James Morse
> - alloc and free twcb through gen_pool_{alloc, free) (Per James)
> - rewrite cover letter
> 
> changes since v8:
> - remove the bug fix tag of patch 2 (per Jarkko Sakkinen)
> - remove the declaration of memory_failure_queue_kick (per Naoya Horiguchi)
> - rewrite the return value comments of memory_failure (per Naoya Horiguchi)
> 
> changes since v7:
> - rebase to Linux v6.6-rc2 (no code changed)
> - rewritten the cover letter to explain the motivation of this patchset
> 
> changes since v6:
> - add more explicty error message suggested by Xiaofei
> - pick up reviewed-by tag from Xiaofei
> - pick up internal reviewed-by tag from Baolin
> 
> changes since v5 by addressing comments from Kefeng:
> - document return value of memory_failure()
> - drop redundant comments in call site of memory_failure()
> - make ghes_do_proc void and handle abnormal case within it
> - pick up reviewed-by tag from Kefeng Wang
> 
> changes since v4 by addressing comments from Xiaofei:
> - do a force kill only for abnormal sync errors
> 
> changes since v3 by addressing comments from Xiaofei:
> - do a force kill for abnormal memory failure error such as invalid PA,
> unexpected severity, OOM, etc
> - pcik up tested-by tag from Ma Wupeng
> 
> changes since v2 by addressing comments from Naoya:
> - rename mce_task_work to sync_task_work
> - drop ACPI_HEST_NOTIFY_MCE case in is_hest_sync_notify()
> - add steps to reproduce this problem in cover letter
> 
> changes since v1:
> - synchronous events by notify type
> - Link: https://lore.kernel.org/lkml/20221206153354.92394-3-xueshuai@linux.alibaba.com/
> 
> ## Cover Letter
> 
> There are two major types of uncorrected recoverable (UCR) errors :
> 
> - Synchronous error: The error is detected and raised at the point of the
>    consumption in the execution flow, e.g. when a CPU tries to access
>    a poisoned cache line. The CPU will take a synchronous error exception
>    such as Synchronous External Abort (SEA) on Arm64 and Machine Check
>    Exception (MCE) on X86. OS requires to take action (for example, offline
>    failure page/kill failure thread) to recover this uncorrectable error.
> 
> - Asynchronous error: The error is detected out of processor execution
>    context, e.g. when an error is detected by a background scrubber. Some data
>    in the memory are corrupted. But the data have not been consumed. OS is
>    optional to take action to recover this uncorrectable error.
> 
> Currently, both synchronous and asynchronous error use
> memory_failure_queue() to schedule memory_failure() exectute in kworker
> context. As a result, when a user-space process is accessing a poisoned
> data, a data abort is taken and the memory_failure() is executed in the
> kworker context:
> 
>    - will send wrong si_code by SIGBUS signal in early_kill mode, and
>    - can not kill the user-space in some cases resulting a synchronous
>      error infinite loop
> 
> Issue 1: send wrong si_code in early_kill mode
> 
> Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
> could be used to determine whether a synchronous exception occurs on
> ARM64 platform.  When a synchronous exception is detected, the kernel is
> expected to terminate the current process which has accessed poisoned
> page. This is done by sending a SIGBUS signal with an error code
> BUS_MCEERR_AR, indicating an action-required machine check error on
> read.
> 
> However, when kill_proc() is called to terminate the processes who have
> the poisoned page mapped, it sends the incorrect SIGBUS error code
> BUS_MCEERR_AO because the context in which it operates is not the one
> where the error was triggered.
> 
> To reproduce this problem:
> 
>    # STEP1: enable early kill mode
>    #sysctl -w vm.memory_failure_early_kill=1
>    vm.memory_failure_early_kill = 1
> 
>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>    #einj_mem_uc single
>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>    injecting ...
>    triggering ...
>    signal 7 code 5 addr 0xffffb0d75000
>    page not present
>    Test passed
> 
> The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
> error and it is not fact.
> 
> To fix it, queue memory_failure() as a task_work so that it runs in
> the context of the process that is actually consuming the poisoned data.
> 
> After this patch set:
> 
>    # STEP1: enable early kill mode
>    #sysctl -w vm.memory_failure_early_kill=1
>    vm.memory_failure_early_kill = 1
> 
>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>    #einj_mem_uc single
>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>    injecting ...
>    triggering ...
>    signal 7 code 4 addr 0xffffb0d75000
>    page not present
>    Test passed
> 
> The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
> error as we expected.
> 
> Issue 2: a synchronous error infinite loop due to memory_failure() failed
> 
> If a user-space process, e.g. devmem, a poisoned page which has been set
> HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
> current processs with error info. Because the memory_failure() is
> executed in the kworker contex, it will just do nothing but return
> EFAULT. So, devmem will access the posioned page and trigger an
> excepction again, resulting in a synchronous error infinite loop. Such
> loop may cause platform firmware to exceed some threshold and reboot
> when Linux could have recovered from this error.
> 
> To reproduce this problem:
> 
>    # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
>    #einj_mem_uc single
>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>    injecting ...
>    triggering ...
>    signal 7 code 4 addr 0xffffb0d75000
>    page not present
>    Test passed
> 
>    # STEP 2: access the same page and it will trigger a synchronous error infinite loop
>    devmem 0x4092d55b400
> 
> To fix it, if memory_failure() failed, perform a force kill to current process.
> 
> Issue 3: a synchronous error infinite loop due to no memory_failure() queued
> 
> No memory_failure() work is queued unless all bellow preconditions check passed:
> 
> - `if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))` in ghes_handle_memory_failure()
> - `if (flags == -1)` in ghes_handle_memory_failure()
> - `if (!IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))` in ghes_do_memory_failure()
> - `if (!pfn_valid(pfn) && !arch_is_platform_page(physical_addr)) ` in ghes_do_memory_failure()
> 
> If the preconditions are not passed, the user-space process will trigger SEA again.
> This loop can potentially exceed the platform firmware threshold or even
> trigger a kernel hard lockup, leading to a system reboot.
> 
> To fix it, if no memory_failure() queued, perform a force kill to current process.
> 
> And the the memory errors triggered in kernel-mode[5], also relies on this
> patchset to kill the failure thread.
> 
> Lv Ying and XiuQi from Huawei also proposed to address similar problem[2][4].
> Acknowledge to discussion with them.
> 
> [1] Add ARMv8 RAS virtualization support in QEMU https://patchew.org/QEMU/20200512030609.19593-1-gengdongjiu@huawei.com/
> [2] https://lore.kernel.org/lkml/20221205115111.131568-3-lvying6@huawei.com/
> [3] https://lkml.kernel.org/r/20220914064935.7851-1-xueshuai@linux.alibaba.com
> [4] https://lore.kernel.org/lkml/20221209095407.383211-1-lvying6@huawei.com/
> [5] https://patchwork.kernel.org/project/linux-arm-kernel/cover/20240528085915.1955987-1-tongtiangen@huawei.com/
> 
> Shuai Xue (3):
>    ACPI: APEI: send SIGBUS to current task if synchronous memory error
>      not recovered
>    mm: memory-failure: move return value documentation to function
>      declaration
>    ACPI: APEI: handle synchronous exceptions in task work
> 
>   arch/x86/kernel/cpu/mce/core.c |  7 ---
>   drivers/acpi/apei/ghes.c       | 86 +++++++++++++++++++++-------------
>   include/acpi/ghes.h            |  3 --
>   include/linux/mm.h             |  1 -
>   mm/memory-failure.c            | 22 +++------
>   5 files changed, 60 insertions(+), 59 deletions(-)
>

On Fri Sep 20, 2024 at 7:30 AM EEST, Shuai Xue wrote:
> Synchronous error was detected as a result of user-space process accessing
> a 2-bit uncorrected error. The CPU will take a synchronous error exception
> such as Synchronous External Abort (SEA) on Arm64. The kernel will queue a
> memory_failure() work which poisons the related page, unmaps the page, and
> then sends a SIGBUS to the process, so that a system wide panic can be
> avoided.
>
> However, no memory_failure() work will be queued when abnormal synchronous
> errors occur. These errors can include situations such as invalid PA,
> unexpected severity, no memory failure config support, invalid GUID
> section, etc. In such case, the user-space process will trigger SEA again.
> This loop can potentially exceed the platform firmware threshold or even
> trigger a kernel hard lockup, leading to a system reboot.
>
> Fix it by performing a force kill if no memory_failure() work is queued
> for synchronous errors.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> ---
>  drivers/acpi/apei/ghes.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 623cc0cb4a65..93eb11482832 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -801,6 +801,16 @@ static bool ghes_do_proc(struct ghes *ghes,
>  		}
>  	}
>  
> +	/*
> +	 * If no memory failure work is queued for abnormal synchronous
> +	 * errors, do a force kill.
> +	 */
> +	if (sync && !queued) {
> +		pr_err("%s:%d: hardware memory corruption (SIGBUS)\n",
> +			current->comm, task_pid_nr(current));
> +		force_sig(SIGBUS);
> +	}
> +
>  	return queued;
>  }
>  

Looks good to me!

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

On Fri Sep 20, 2024 at 7:30 AM EEST, Shuai Xue wrote:
> The memory uncorrected error could be signaled by asynchronous interrupt
> (specifically, SPI in arm64 platform), e.g. when an error is detected by
> a background scrubber, or signaled by synchronous exception
> (specifically, data abort excepction in arm64 platform), e.g. when a CPU
> tries to access a poisoned cache line. Currently, both synchronous and
> asynchronous error use memory_failure_queue() to schedule
> memory_failure() exectute in kworker context.
>
> As a result, when a user-space process is accessing a poisoned data, a
> data abort is taken and the memory_failure() is executed in the kworker
> context:
>
>   - will send wrong si_code by SIGBUS signal in early_kill mode, and
>   - can not kill the user-space in some cases resulting a synchronous
>     error infinite loop
>
> Issue 1: send wrong si_code in early_kill mode
>
> Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
> could be used to determine whether a synchronous exception occurs on
> ARM64 platform.  When a synchronous exception is detected, the kernel is
> expected to terminate the current process which has accessed poisoned
> page. This is done by sending a SIGBUS signal with an error code
> BUS_MCEERR_AR, indicating an action-required machine check error on
> read.
>
> However, when kill_proc() is called to terminate the processes who have
> the poisoned page mapped, it sends the incorrect SIGBUS error code
> BUS_MCEERR_AO because the context in which it operates is not the one
> where the error was triggered.
>
> To reproduce this problem:
>
>   #sysctl -w vm.memory_failure_early_kill=1
>   vm.memory_failure_early_kill = 1
>
>   # STEP2: inject an UCE error and consume it to trigger a synchronous error
>   #einj_mem_uc single
>   0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>   injecting ...
>   triggering ...
>   signal 7 code 5 addr 0xffffb0d75000
>   page not present
>   Test passed
>
> The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
> error and it is not fact.
>
> After this patch:
>
>   # STEP1: enable early kill mode
>   #sysctl -w vm.memory_failure_early_kill=1
>   vm.memory_failure_early_kill = 1
>   # STEP2: inject an UCE error and consume it to trigger a synchronous error
>   #einj_mem_uc single
>   0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>   injecting ...
>   triggering ...
>   signal 7 code 4 addr 0xffffb0d75000
>   page not present
>   Test passed
>
> The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
> error as we expected.
>
> Issue 2: a synchronous error infinite loop
>
> If a user-space process, e.g. devmem, a poisoned page which has been set
> HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
> current processs with error info. Because the memory_failure() is
> executed in the kworker contex, it will just do nothing but return
> EFAULT. So, devmem will access the posioned page and trigger an
> excepction again, resulting in a synchronous error infinite loop. Such
> loop may cause platform firmware to exceed some threshold and reboot
> when Linux could have recovered from this error.
>
> To reproduce this problem:
>
>   # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
>   #einj_mem_uc single
>   0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>   injecting ...
>   triggering ...
>   signal 7 code 4 addr 0xffffb0d75000
>   page not present
>   Test passed
>
>   # STEP 2: access the same page and it will trigger a synchronous error infinite loop
>   devmem 0x4092d55b400
>
> To fix above two issues, queue memory_failure() as a task_work so that it runs in
> the context of the process that is actually consuming the poisoned data.
>
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>  drivers/acpi/apei/ghes.c | 78 +++++++++++++++++++++++-----------------
>  include/acpi/ghes.h      |  3 --
>  include/linux/mm.h       |  1 -
>  mm/memory-failure.c      | 13 -------
>  4 files changed, 45 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 93eb11482832..60d8044f14d1 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -467,28 +467,42 @@ static void ghes_clear_estatus(struct ghes *ghes,
>  }
>  
>  /*
> - * Called as task_work before returning to user-space.
> - * Ensure any queued work has been done before we return to the context that
> - * triggered the notification.
> + * struct task_work - for synchronous RAS event
> + *
> + * @twork:                callback_head for task work
> + * @pfn:                  page frame number of corrupted page
> + * @flags:                work control flags
> + *
> + * Structure to pass task work to be handled before
> + * returning to user-space via task_work_add().
>   */
> -static void ghes_kick_task_work(struct callback_head *head)
> +struct task_work {
> +	struct callback_head twork;
> +	u64 pfn;
> +	int flags;
> +};

I'd rename this as ghes_task_work. It is too generic name IMHO, easily
confused with task_work.h definitions.

BR, Jarkko

在 2024/9/20 19:44, Jarkko Sakkinen 写道:
> On Fri Sep 20, 2024 at 7:30 AM EEST, Shuai Xue wrote:
>> The memory uncorrected error could be signaled by asynchronous interrupt
>> (specifically, SPI in arm64 platform), e.g. when an error is detected by
>> a background scrubber, or signaled by synchronous exception
>> (specifically, data abort excepction in arm64 platform), e.g. when a CPU
>> tries to access a poisoned cache line. Currently, both synchronous and
>> asynchronous error use memory_failure_queue() to schedule
>> memory_failure() exectute in kworker context.
>>
>> As a result, when a user-space process is accessing a poisoned data, a
>> data abort is taken and the memory_failure() is executed in the kworker
>> context:
>>
>>    - will send wrong si_code by SIGBUS signal in early_kill mode, and
>>    - can not kill the user-space in some cases resulting a synchronous
>>      error infinite loop
>>
>> Issue 1: send wrong si_code in early_kill mode
>>
>> Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
>> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
>> could be used to determine whether a synchronous exception occurs on
>> ARM64 platform.  When a synchronous exception is detected, the kernel is
>> expected to terminate the current process which has accessed poisoned
>> page. This is done by sending a SIGBUS signal with an error code
>> BUS_MCEERR_AR, indicating an action-required machine check error on
>> read.
>>
>> However, when kill_proc() is called to terminate the processes who have
>> the poisoned page mapped, it sends the incorrect SIGBUS error code
>> BUS_MCEERR_AO because the context in which it operates is not the one
>> where the error was triggered.
>>
>> To reproduce this problem:
>>
>>    #sysctl -w vm.memory_failure_early_kill=1
>>    vm.memory_failure_early_kill = 1
>>
>>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>>    #einj_mem_uc single
>>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>    injecting ...
>>    triggering ...
>>    signal 7 code 5 addr 0xffffb0d75000
>>    page not present
>>    Test passed
>>
>> The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
>> error and it is not fact.
>>
>> After this patch:
>>
>>    # STEP1: enable early kill mode
>>    #sysctl -w vm.memory_failure_early_kill=1
>>    vm.memory_failure_early_kill = 1
>>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>>    #einj_mem_uc single
>>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>    injecting ...
>>    triggering ...
>>    signal 7 code 4 addr 0xffffb0d75000
>>    page not present
>>    Test passed
>>
>> The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
>> error as we expected.
>>
>> Issue 2: a synchronous error infinite loop
>>
>> If a user-space process, e.g. devmem, a poisoned page which has been set
>> HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
>> current processs with error info. Because the memory_failure() is
>> executed in the kworker contex, it will just do nothing but return
>> EFAULT. So, devmem will access the posioned page and trigger an
>> excepction again, resulting in a synchronous error infinite loop. Such
>> loop may cause platform firmware to exceed some threshold and reboot
>> when Linux could have recovered from this error.
>>
>> To reproduce this problem:
>>
>>    # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
>>    #einj_mem_uc single
>>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>    injecting ...
>>    triggering ...
>>    signal 7 code 4 addr 0xffffb0d75000
>>    page not present
>>    Test passed
>>
>>    # STEP 2: access the same page and it will trigger a synchronous error infinite loop
>>    devmem 0x4092d55b400
>>
>> To fix above two issues, queue memory_failure() as a task_work so that it runs in
>> the context of the process that is actually consuming the poisoned data.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>> ---
>>   drivers/acpi/apei/ghes.c | 78 +++++++++++++++++++++++-----------------
>>   include/acpi/ghes.h      |  3 --
>>   include/linux/mm.h       |  1 -
>>   mm/memory-failure.c      | 13 -------
>>   4 files changed, 45 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 93eb11482832..60d8044f14d1 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -467,28 +467,42 @@ static void ghes_clear_estatus(struct ghes *ghes,
>>   }
>>   
>>   /*
>> - * Called as task_work before returning to user-space.
>> - * Ensure any queued work has been done before we return to the context that
>> - * triggered the notification.
>> + * struct task_work - for synchronous RAS event
>> + *
>> + * @twork:                callback_head for task work
>> + * @pfn:                  page frame number of corrupted page
>> + * @flags:                work control flags
>> + *
>> + * Structure to pass task work to be handled before
>> + * returning to user-space via task_work_add().
>>    */
>> -static void ghes_kick_task_work(struct callback_head *head)
>> +struct task_work {
>> +	struct callback_head twork;
>> +	u64 pfn;
>> +	int flags;
>> +};
> 
> I'd rename this as ghes_task_work. It is too generic name IMHO, easily
> confused with task_work.h definitions.
> 
> BR, Jarkko

Agreed, I will rename it in next version.

Thank you.

Best Regards,
Shuai

On Mon, 14 Oct 2024 16:42:38 +0800
Shuai Xue <xueshuai@linux.alibaba.com> wrote:

> Synchronous error was detected as a result of user-space process accessing
> a 2-bit uncorrected error. The CPU will take a synchronous error exception
> such as Synchronous External Abort (SEA) on Arm64. The kernel will queue a
> memory_failure() work which poisons the related page, unmaps the page, and
> then sends a SIGBUS to the process, so that a system wide panic can be
> avoided.
> 
> However, no memory_failure() work will be queued when abnormal synchronous
> errors occur. These errors can include situations such as invalid PA,
> unexpected severity, no memory failure config support, invalid GUID
> section, etc. In such case, the user-space process will trigger SEA again.
> This loop can potentially exceed the platform firmware threshold or even
> trigger a kernel hard lockup, leading to a system reboot.
> 
> Fix it by performing a force kill if no memory_failure() work is queued
> for synchronous errors.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

The subtle cases in here are the various other forms of delayed handling
buried in some of the record handling that don't set queued.
I've been through them all and have convinced myself that either 
hey should never be synchronous or that there is no attempt to
recover in kernel today (non memory things such as CXL protocol
collapse, which might I guess be detected synchronously on a read
- though I'd expect poison and a memory error first) so the correct
thing to do is what you have here.

Fiddly code though with a lot of paths, so more eyes welcome!

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

+CC linux-cxl for info.
 
> ---
>  drivers/acpi/apei/ghes.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index ada93cfde9ba..f2ee28c44d7a 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -801,6 +801,16 @@ static bool ghes_do_proc(struct ghes *ghes,
>  		}
>  	}
>  
> +	/*
> +	 * If no memory failure work is queued for abnormal synchronous
> +	 * errors, do a force kill.
> +	 */
> +	if (sync && !queued) {
> +		pr_err("%s:%d: hardware memory corruption (SIGBUS)\n",
> +			current->comm, task_pid_nr(current));
> +		force_sig(SIGBUS);
> +	}
> +
>  	return queued;
>  }
>

On Mon, 14 Oct 2024 16:42:39 +0800
Shuai Xue <xueshuai@linux.alibaba.com> wrote:

> Part of return value comments for memory_failure() were originally
> documented at the call site. Move those comments to the function
> declaration to improve code readability and to provide developers with
> immediate access to function usage and return information.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
Seems sensible.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  arch/x86/kernel/cpu/mce/core.c | 7 -------
>  mm/memory-failure.c            | 9 ++++++---
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> index 2a938f429c4d..c90d8fcd246a 100644
> --- a/arch/x86/kernel/cpu/mce/core.c
> +++ b/arch/x86/kernel/cpu/mce/core.c
> @@ -1373,13 +1373,6 @@ static void kill_me_maybe(struct callback_head *cb)
>  		return;
>  	}
>  
> -	/*
> -	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
> -	 * to the current process with the proper error info,
> -	 * -EOPNOTSUPP means hwpoison_filter() filtered the error event,
> -	 *
> -	 * In both cases, no further processing is required.
> -	 */
>  	if (ret == -EHWPOISON || ret == -EOPNOTSUPP)
>  		return;
>  
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 96ce31e5a203..1c5098f32d48 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2209,9 +2209,12 @@ static void kill_procs_now(struct page *p, unsigned long pfn, int flags,
>   * Must run in process context (e.g. a work queue) with interrupts
>   * enabled and no spinlocks held.
>   *
> - * Return: 0 for successfully handled the memory error,
> - *         -EOPNOTSUPP for hwpoison_filter() filtered the error event,
> - *         < 0(except -EOPNOTSUPP) on failure.
> + * Return:
> + *   0             - success,
> + *   -EOPNOTSUPP   - hwpoison_filter() filtered the error event,
> + *   -EHWPOISON    - the page was already poisoned, potentially
> + *                   kill process,
> + *   other negative values - failure.
>   */
>  int memory_failure(unsigned long pfn, int flags)
>  {

On Mon, 14 Oct 2024 16:42:40 +0800
Shuai Xue <xueshuai@linux.alibaba.com> wrote:

> The memory uncorrected error could be signaled by asynchronous interrupt
> (specifically, SPI in arm64 platform), e.g. when an error is detected by
> a background scrubber, or signaled by synchronous exception
> (specifically, data abort excepction in arm64 platform), e.g. when a CPU

exception

> tries to access a poisoned cache line. Currently, both synchronous and
> asynchronous error use memory_failure_queue() to schedule
> memory_failure() exectute in kworker context.
memory_failure() to execute in kworker context.

(spell check in general)
> 
> As a result, when a user-space process is accessing a poisoned data, a
> data abort is taken and the memory_failure() is executed in the kworker
> context:
context, memory_failure():
//No subject of the following two bullets otherwise.
> 
>   - will send wrong si_code by SIGBUS signal in early_kill mode, and
>   - can not kill the user-space in some cases resulting a synchronous
>     error infinite loop
> 
> Issue 1: send wrong si_code in early_kill mode
> 
> Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
> could be used to determine whether a synchronous exception occurs on
> ARM64 platform.  When a synchronous exception is detected, the kernel is
> expected to terminate the current process which has accessed poisoned
> page. This is done by sending a SIGBUS signal with an error code
> BUS_MCEERR_AR, indicating an action-required machine check error on
> read.
> 
> However, when kill_proc() is called to terminate the processes who have
> the poisoned page mapped, it sends the incorrect SIGBUS error code
> BUS_MCEERR_AO because the context in which it operates is not the one
> where the error was triggered.
> 
> To reproduce this problem:
> 
>   #sysctl -w vm.memory_failure_early_kill=1
>   vm.memory_failure_early_kill = 1
> 
>   # STEP2: inject an UCE error and consume it to trigger a synchronous error
>   #einj_mem_uc single
>   0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>   injecting ...
>   triggering ...
>   signal 7 code 5 addr 0xffffb0d75000
>   page not present
>   Test passed
> 
> The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
> error and it is not fact.
> 
> After this patch:
> 
>   # STEP1: enable early kill mode
>   #sysctl -w vm.memory_failure_early_kill=1
>   vm.memory_failure_early_kill = 1
>   # STEP2: inject an UCE error and consume it to trigger a synchronous error
>   #einj_mem_uc single
>   0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>   injecting ...
>   triggering ...
>   signal 7 code 4 addr 0xffffb0d75000
>   page not present
>   Test passed
> 
> The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
> error as we expected.
> 
> Issue 2: a synchronous error infinite loop
> 
> If a user-space process, e.g. devmem, a poisoned page which has been set

devmem accesses a poisoned page for which the HWPoison flag is set

> HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
> current processs with error info. Because the memory_failure() is
> executed in the kworker contex, it will just do nothing but return

context

> EFAULT. So, devmem will access the posioned page and trigger an
> excepction again, resulting in a synchronous error infinite loop. Such

exception

> loop may cause platform firmware to exceed some threshold and reboot
> when Linux could have recovered from this error.
> 
> To reproduce this problem:
> 
>   # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
>   #einj_mem_uc single
>   0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>   injecting ...
>   triggering ...
>   signal 7 code 4 addr 0xffffb0d75000
>   page not present
>   Test passed
> 
>   # STEP 2: access the same page and it will trigger a synchronous error infinite loop
>   devmem 0x4092d55b400
> 
> To fix above two issues, queue memory_failure() as a task_work so that it runs in
> the context of the process that is actually consuming the poisoned data.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
One other trivial comment inline.

Whilst this also looks fine to me, there are others who (hopefully)
understand these paths better than me.  With that said.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/acpi/apei/ghes.c | 78 +++++++++++++++++++++++-----------------
>  include/acpi/ghes.h      |  3 --
>  include/linux/mm.h       |  1 -
>  mm/memory-failure.c      | 13 -------
>  4 files changed, 45 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index f2ee28c44d7a..95e9520eb803 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -467,28 +467,42 @@ static void ghes_clear_estatus(struct ghes *ghes,
>  }
>  
>  /*
> - * Called as task_work before returning to user-space.
> - * Ensure any queued work has been done before we return to the context that
> - * triggered the notification.
> + * struct ghes_task_work - for synchronous RAS event
> + *
> + * @twork:                callback_head for task work
> + * @pfn:                  page frame number of corrupted page
> + * @flags:                work control flags
> + *
> + * Structure to pass task work to be handled before
> + * returning to user-space via task_work_add().
>   */
> -static void ghes_kick_task_work(struct callback_head *head)
> +struct ghes_task_work {
> +	struct callback_head twork;
> +	u64 pfn;
> +	int flags;
> +};
> +
> +static void memory_failure_cb(struct callback_head *twork)
>  {
> -	struct acpi_hest_generic_status *estatus;
> -	struct ghes_estatus_node *estatus_node;
> -	u32 node_len;
> +	struct ghes_task_work *twcb = container_of(twork, struct ghes_task_work, twork);
> +	unsigned long pfn = twcb->pfn;

This local variable is not used consistently.  I'd just
drop it in favor of always accessing via twcb->pfn

> +	int ret;
>  
> -	estatus_node = container_of(head, struct ghes_estatus_node, task_work);
> -	if (IS_ENABLED(CONFIG_ACPI_APEI_MEMORY_FAILURE))
> -		memory_failure_queue_kick(estatus_node->task_work_cpu);
> +	ret = memory_failure(twcb->pfn, twcb->flags);

> +	gen_pool_free(ghes_estatus_pool, (unsigned long)twcb, sizeof(*twcb));
>  
> -	estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
> -	node_len = GHES_ESTATUS_NODE_LEN(cper_estatus_len(estatus));
> -	gen_pool_free(ghes_estatus_pool, (unsigned long)estatus_node, node_len);
> +	if (!ret || ret == -EHWPOISON || ret == -EOPNOTSUPP)
> +		return;
> +
> +	pr_err("%#lx: Sending SIGBUS to %s:%d due to hardware memory corruption\n",
> +			pfn, current->comm, task_pid_nr(current));
> +	force_sig(SIGBUS);
>  }

在 2024/10/17 17:39, Jonathan Cameron 写道:
> On Mon, 14 Oct 2024 16:42:38 +0800
> Shuai Xue <xueshuai@linux.alibaba.com> wrote:
> 
>> Synchronous error was detected as a result of user-space process accessing
>> a 2-bit uncorrected error. The CPU will take a synchronous error exception
>> such as Synchronous External Abort (SEA) on Arm64. The kernel will queue a
>> memory_failure() work which poisons the related page, unmaps the page, and
>> then sends a SIGBUS to the process, so that a system wide panic can be
>> avoided.
>>
>> However, no memory_failure() work will be queued when abnormal synchronous
>> errors occur. These errors can include situations such as invalid PA,
>> unexpected severity, no memory failure config support, invalid GUID
>> section, etc. In such case, the user-space process will trigger SEA again.
>> This loop can potentially exceed the platform firmware threshold or even
>> trigger a kernel hard lockup, leading to a system reboot.
>>
>> Fix it by performing a force kill if no memory_failure() work is queued
>> for synchronous errors.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
> The subtle cases in here are the various other forms of delayed handling
> buried in some of the record handling that don't set queued.
> I've been through them all and have convinced myself that either
> hey should never be synchronous or that there is no attempt to
> recover in kernel today (non memory things such as CXL protocol
> collapse, which might I guess be detected synchronously on a read
> - though I'd expect poison and a memory error first) so the correct
> thing to do is what you have here.
> 
> Fiddly code though with a lot of paths, so more eyes welcome!
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> +CC linux-cxl for info.

Thanks :)

Best Regards,
Shuai

在 2024/10/17 17:41, Jonathan Cameron 写道:
> On Mon, 14 Oct 2024 16:42:39 +0800
> Shuai Xue <xueshuai@linux.alibaba.com> wrote:
> 
>> Part of return value comments for memory_failure() were originally
>> documented at the call site. Move those comments to the function
>> declaration to improve code readability and to provide developers with
>> immediate access to function usage and return information.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> Seems sensible.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Thanks.

Best Regards,
Shuai

在 2024/10/17 17:56, Jonathan Cameron 写道:
> On Mon, 14 Oct 2024 16:42:40 +0800
> Shuai Xue <xueshuai@linux.alibaba.com> wrote:
> 
>> The memory uncorrected error could be signaled by asynchronous interrupt
>> (specifically, SPI in arm64 platform), e.g. when an error is detected by
>> a background scrubber, or signaled by synchronous exception
>> (specifically, data abort excepction in arm64 platform), e.g. when a CPU
> 
> exception
> 
>> tries to access a poisoned cache line. Currently, both synchronous and
>> asynchronous error use memory_failure_queue() to schedule
>> memory_failure() exectute in kworker context.
> memory_failure() to execute in kworker context.
> 
> (spell check in general)

Sorry for typos. Will fix it in next version. Thanks.
>>
>> As a result, when a user-space process is accessing a poisoned data, a
>> data abort is taken and the memory_failure() is executed in the kworker
>> context:
> context, memory_failure():
> //No subject of the following two bullets otherwise.

I see, will fix it.
>>
>>    - will send wrong si_code by SIGBUS signal in early_kill mode, and
>>    - can not kill the user-space in some cases resulting a synchronous
>>      error infinite loop
>>
>> Issue 1: send wrong si_code in early_kill mode
>>
>> Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
>> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
>> could be used to determine whether a synchronous exception occurs on
>> ARM64 platform.  When a synchronous exception is detected, the kernel is
>> expected to terminate the current process which has accessed poisoned
>> page. This is done by sending a SIGBUS signal with an error code
>> BUS_MCEERR_AR, indicating an action-required machine check error on
>> read.
>>
>> However, when kill_proc() is called to terminate the processes who have
>> the poisoned page mapped, it sends the incorrect SIGBUS error code
>> BUS_MCEERR_AO because the context in which it operates is not the one
>> where the error was triggered.
>>
>> To reproduce this problem:
>>
>>    #sysctl -w vm.memory_failure_early_kill=1
>>    vm.memory_failure_early_kill = 1
>>
>>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>>    #einj_mem_uc single
>>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>    injecting ...
>>    triggering ...
>>    signal 7 code 5 addr 0xffffb0d75000
>>    page not present
>>    Test passed
>>
>> The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
>> error and it is not fact.
>>
>> After this patch:
>>
>>    # STEP1: enable early kill mode
>>    #sysctl -w vm.memory_failure_early_kill=1
>>    vm.memory_failure_early_kill = 1
>>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>>    #einj_mem_uc single
>>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>    injecting ...
>>    triggering ...
>>    signal 7 code 4 addr 0xffffb0d75000
>>    page not present
>>    Test passed
>>
>> The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
>> error as we expected.
>>
>> Issue 2: a synchronous error infinite loop
>>
>> If a user-space process, e.g. devmem, a poisoned page which has been set
> 
> devmem accesses a poisoned page for which the HWPoison flag is set
> 
>> HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
>> current processs with error info. Because the memory_failure() is
>> executed in the kworker contex, it will just do nothing but return
> 
> context
> 
>> EFAULT. So, devmem will access the posioned page and trigger an
>> excepction again, resulting in a synchronous error infinite loop. Such
> 
> exception

I see, will fix it.

> 
>> loop may cause platform firmware to exceed some threshold and reboot
>> when Linux could have recovered from this error.
>>
>> To reproduce this problem:
>>
>>    # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
>>    #einj_mem_uc single
>>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>    injecting ...
>>    triggering ...
>>    signal 7 code 4 addr 0xffffb0d75000
>>    page not present
>>    Test passed
>>
>>    # STEP 2: access the same page and it will trigger a synchronous error infinite loop
>>    devmem 0x4092d55b400
>>
>> To fix above two issues, queue memory_failure() as a task_work so that it runs in
>> the context of the process that is actually consuming the poisoned data.
>>
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> One other trivial comment inline.
> 
> Whilst this also looks fine to me, there are others who (hopefully)
> understand these paths better than me.  With that said.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thank you for valuable comments.

> 
>> ---
>>   drivers/acpi/apei/ghes.c | 78 +++++++++++++++++++++++-----------------
>>   include/acpi/ghes.h      |  3 --
>>   include/linux/mm.h       |  1 -
>>   mm/memory-failure.c      | 13 -------
>>   4 files changed, 45 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index f2ee28c44d7a..95e9520eb803 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -467,28 +467,42 @@ static void ghes_clear_estatus(struct ghes *ghes,
>>   }
>>   
>>   /*
>> - * Called as task_work before returning to user-space.
>> - * Ensure any queued work has been done before we return to the context that
>> - * triggered the notification.
>> + * struct ghes_task_work - for synchronous RAS event
>> + *
>> + * @twork:                callback_head for task work
>> + * @pfn:                  page frame number of corrupted page
>> + * @flags:                work control flags
>> + *
>> + * Structure to pass task work to be handled before
>> + * returning to user-space via task_work_add().
>>    */
>> -static void ghes_kick_task_work(struct callback_head *head)
>> +struct ghes_task_work {
>> +	struct callback_head twork;
>> +	u64 pfn;
>> +	int flags;
>> +};
>> +
>> +static void memory_failure_cb(struct callback_head *twork)
>>   {
>> -	struct acpi_hest_generic_status *estatus;
>> -	struct ghes_estatus_node *estatus_node;
>> -	u32 node_len;
>> +	struct ghes_task_work *twcb = container_of(twork, struct ghes_task_work, twork);
>> +	unsigned long pfn = twcb->pfn;
> 
> This local variable is not used consistently.  I'd just
> drop it in favor of always accessing via twcb->pfn

Agreed, will remove local pfn.


Best Regards,
Shuai

Hi, Jarkko,


在 2024/10/14 16:42, Shuai Xue 写道:
> The memory uncorrected error could be signaled by asynchronous interrupt
> (specifically, SPI in arm64 platform), e.g. when an error is detected by
> a background scrubber, or signaled by synchronous exception
> (specifically, data abort excepction in arm64 platform), e.g. when a CPU
> tries to access a poisoned cache line. Currently, both synchronous and
> asynchronous error use memory_failure_queue() to schedule
> memory_failure() exectute in kworker context.
> 
> As a result, when a user-space process is accessing a poisoned data, a
> data abort is taken and the memory_failure() is executed in the kworker
> context:
> 
>    - will send wrong si_code by SIGBUS signal in early_kill mode, and
>    - can not kill the user-space in some cases resulting a synchronous
>      error infinite loop
> 
> Issue 1: send wrong si_code in early_kill mode
> 
> Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
> could be used to determine whether a synchronous exception occurs on
> ARM64 platform.  When a synchronous exception is detected, the kernel is
> expected to terminate the current process which has accessed poisoned
> page. This is done by sending a SIGBUS signal with an error code
> BUS_MCEERR_AR, indicating an action-required machine check error on
> read.
> 
> However, when kill_proc() is called to terminate the processes who have
> the poisoned page mapped, it sends the incorrect SIGBUS error code
> BUS_MCEERR_AO because the context in which it operates is not the one
> where the error was triggered.
> 
> To reproduce this problem:
> 
>    #sysctl -w vm.memory_failure_early_kill=1
>    vm.memory_failure_early_kill = 1
> 
>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>    #einj_mem_uc single
>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>    injecting ...
>    triggering ...
>    signal 7 code 5 addr 0xffffb0d75000
>    page not present
>    Test passed
> 
> The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
> error and it is not fact.
> 
> After this patch:
> 
>    # STEP1: enable early kill mode
>    #sysctl -w vm.memory_failure_early_kill=1
>    vm.memory_failure_early_kill = 1
>    # STEP2: inject an UCE error and consume it to trigger a synchronous error
>    #einj_mem_uc single
>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>    injecting ...
>    triggering ...
>    signal 7 code 4 addr 0xffffb0d75000
>    page not present
>    Test passed
> 
> The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
> error as we expected.
> 
> Issue 2: a synchronous error infinite loop
> 
> If a user-space process, e.g. devmem, a poisoned page which has been set
> HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
> current processs with error info. Because the memory_failure() is
> executed in the kworker contex, it will just do nothing but return
> EFAULT. So, devmem will access the posioned page and trigger an
> excepction again, resulting in a synchronous error infinite loop. Such
> loop may cause platform firmware to exceed some threshold and reboot
> when Linux could have recovered from this error.
> 
> To reproduce this problem:
> 
>    # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
>    #einj_mem_uc single
>    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>    injecting ...
>    triggering ...
>    signal 7 code 4 addr 0xffffb0d75000
>    page not present
>    Test passed
> 
>    # STEP 2: access the same page and it will trigger a synchronous error infinite loop
>    devmem 0x4092d55b400
> 
> To fix above two issues, queue memory_failure() as a task_work so that it runs in
> the context of the process that is actually consuming the poisoned data.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
>   drivers/acpi/apei/ghes.c | 78 +++++++++++++++++++++++-----------------
>   include/acpi/ghes.h      |  3 --
>   include/linux/mm.h       |  1 -
>   mm/memory-failure.c      | 13 -------
>   4 files changed, 45 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index f2ee28c44d7a..95e9520eb803 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -467,28 +467,42 @@ static void ghes_clear_estatus(struct ghes *ghes,
>   }
>   
>   /*
> - * Called as task_work before returning to user-space.
> - * Ensure any queued work has been done before we return to the context that
> - * triggered the notification.
> + * struct ghes_task_work - for synchronous RAS event
> + *
> + * @twork:                callback_head for task work
> + * @pfn:                  page frame number of corrupted page
> + * @flags:                work control flags
> + *
> + * Structure to pass task work to be handled before
> + * returning to user-space via task_work_add().
>    */


Do you have any futer comments about this patch? Any comments are
welcomed. If not, are you happy to explictly give the reveiwed-by tag?

Best Regard,
Shuai

On Tue Oct 22, 2024 at 4:11 AM EEST, Shuai Xue wrote:
> Hi, Jarkko,
>
>
> 在 2024/10/14 16:42, Shuai Xue 写道:
> > The memory uncorrected error could be signaled by asynchronous interrupt
> > (specifically, SPI in arm64 platform), e.g. when an error is detected by
> > a background scrubber, or signaled by synchronous exception
> > (specifically, data abort excepction in arm64 platform), e.g. when a CPU
> > tries to access a poisoned cache line. Currently, both synchronous and
> > asynchronous error use memory_failure_queue() to schedule
> > memory_failure() exectute in kworker context.
> > 
> > As a result, when a user-space process is accessing a poisoned data, a
> > data abort is taken and the memory_failure() is executed in the kworker
> > context:
> > 
> >    - will send wrong si_code by SIGBUS signal in early_kill mode, and
> >    - can not kill the user-space in some cases resulting a synchronous
> >      error infinite loop
> > 
> > Issue 1: send wrong si_code in early_kill mode
> > 
> > Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
> > MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
> > could be used to determine whether a synchronous exception occurs on
> > ARM64 platform.  When a synchronous exception is detected, the kernel is
> > expected to terminate the current process which has accessed poisoned
> > page. This is done by sending a SIGBUS signal with an error code
> > BUS_MCEERR_AR, indicating an action-required machine check error on
> > read.
> > 
> > However, when kill_proc() is called to terminate the processes who have
> > the poisoned page mapped, it sends the incorrect SIGBUS error code
> > BUS_MCEERR_AO because the context in which it operates is not the one
> > where the error was triggered.
> > 
> > To reproduce this problem:
> > 
> >    #sysctl -w vm.memory_failure_early_kill=1
> >    vm.memory_failure_early_kill = 1
> > 
> >    # STEP2: inject an UCE error and consume it to trigger a synchronous error
> >    #einj_mem_uc single
> >    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
> >    injecting ...
> >    triggering ...
> >    signal 7 code 5 addr 0xffffb0d75000
> >    page not present
> >    Test passed
> > 
> > The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
> > error and it is not fact.
> > 
> > After this patch:
> > 
> >    # STEP1: enable early kill mode
> >    #sysctl -w vm.memory_failure_early_kill=1
> >    vm.memory_failure_early_kill = 1
> >    # STEP2: inject an UCE error and consume it to trigger a synchronous error
> >    #einj_mem_uc single
> >    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
> >    injecting ...
> >    triggering ...
> >    signal 7 code 4 addr 0xffffb0d75000
> >    page not present
> >    Test passed
> > 
> > The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
> > error as we expected.
> > 
> > Issue 2: a synchronous error infinite loop
> > 
> > If a user-space process, e.g. devmem, a poisoned page which has been set
> > HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
> > current processs with error info. Because the memory_failure() is
> > executed in the kworker contex, it will just do nothing but return
> > EFAULT. So, devmem will access the posioned page and trigger an
> > excepction again, resulting in a synchronous error infinite loop. Such
> > loop may cause platform firmware to exceed some threshold and reboot
> > when Linux could have recovered from this error.
> > 
> > To reproduce this problem:
> > 
> >    # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
> >    #einj_mem_uc single
> >    0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
> >    injecting ...
> >    triggering ...
> >    signal 7 code 4 addr 0xffffb0d75000
> >    page not present
> >    Test passed
> > 
> >    # STEP 2: access the same page and it will trigger a synchronous error infinite loop
> >    devmem 0x4092d55b400
> > 
> > To fix above two issues, queue memory_failure() as a task_work so that it runs in
> > the context of the process that is actually consuming the poisoned data.
> > 
> > Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> > Tested-by: Ma Wupeng <mawupeng1@huawei.com>
> > Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> > Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
> > Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> > ---
> >   drivers/acpi/apei/ghes.c | 78 +++++++++++++++++++++++-----------------
> >   include/acpi/ghes.h      |  3 --
> >   include/linux/mm.h       |  1 -
> >   mm/memory-failure.c      | 13 -------
> >   4 files changed, 45 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> > index f2ee28c44d7a..95e9520eb803 100644
> > --- a/drivers/acpi/apei/ghes.c
> > +++ b/drivers/acpi/apei/ghes.c
> > @@ -467,28 +467,42 @@ static void ghes_clear_estatus(struct ghes *ghes,
> >   }
> >   
> >   /*
> > - * Called as task_work before returning to user-space.
> > - * Ensure any queued work has been done before we return to the context that
> > - * triggered the notification.
> > + * struct ghes_task_work - for synchronous RAS event
> > + *
> > + * @twork:                callback_head for task work
> > + * @pfn:                  page frame number of corrupted page
> > + * @flags:                work control flags
> > + *
> > + * Structure to pass task work to be handled before
> > + * returning to user-space via task_work_add().
> >    */
>
>
> Do you have any futer comments about this patch? Any comments are
> welcomed. If not, are you happy to explictly give the reveiwed-by tag?

Sorry I've been busy switching to a new job.

I read this now through and both commit messages and the code changes
look sane to me so I guess I don't have any problem with that:

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

>
> Best Regard,
> Shuai

BR, Jarkko

在 2024/10/25 22:40, Jarkko Sakkinen 写道:
> On Tue Oct 22, 2024 at 4:11 AM EEST, Shuai Xue wrote:
>> Hi, Jarkko,
>>
>>
>> 在 2024/10/14 16:42, Shuai Xue 写道:
>>> The memory uncorrected error could be signaled by asynchronous interrupt
>>> (specifically, SPI in arm64 platform), e.g. when an error is detected by
>>> a background scrubber, or signaled by synchronous exception
>>> (specifically, data abort excepction in arm64 platform), e.g. when a CPU
>>> tries to access a poisoned cache line. Currently, both synchronous and
>>> asynchronous error use memory_failure_queue() to schedule
>>> memory_failure() exectute in kworker context.
>>>
>>> As a result, when a user-space process is accessing a poisoned data, a
>>> data abort is taken and the memory_failure() is executed in the kworker
>>> context:
>>>
>>>     - will send wrong si_code by SIGBUS signal in early_kill mode, and
>>>     - can not kill the user-space in some cases resulting a synchronous
>>>       error infinite loop
>>>
>>> Issue 1: send wrong si_code in early_kill mode
>>>
>>> Since commit a70297d22132 ("ACPI: APEI: set memory failure flags as
>>> MF_ACTION_REQUIRED on synchronous events")', the flag MF_ACTION_REQUIRED
>>> could be used to determine whether a synchronous exception occurs on
>>> ARM64 platform.  When a synchronous exception is detected, the kernel is
>>> expected to terminate the current process which has accessed poisoned
>>> page. This is done by sending a SIGBUS signal with an error code
>>> BUS_MCEERR_AR, indicating an action-required machine check error on
>>> read.
>>>
>>> However, when kill_proc() is called to terminate the processes who have
>>> the poisoned page mapped, it sends the incorrect SIGBUS error code
>>> BUS_MCEERR_AO because the context in which it operates is not the one
>>> where the error was triggered.
>>>
>>> To reproduce this problem:
>>>
>>>     #sysctl -w vm.memory_failure_early_kill=1
>>>     vm.memory_failure_early_kill = 1
>>>
>>>     # STEP2: inject an UCE error and consume it to trigger a synchronous error
>>>     #einj_mem_uc single
>>>     0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>>     injecting ...
>>>     triggering ...
>>>     signal 7 code 5 addr 0xffffb0d75000
>>>     page not present
>>>     Test passed
>>>
>>> The si_code (code 5) from einj_mem_uc indicates that it is BUS_MCEERR_AO
>>> error and it is not fact.
>>>
>>> After this patch:
>>>
>>>     # STEP1: enable early kill mode
>>>     #sysctl -w vm.memory_failure_early_kill=1
>>>     vm.memory_failure_early_kill = 1
>>>     # STEP2: inject an UCE error and consume it to trigger a synchronous error
>>>     #einj_mem_uc single
>>>     0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>>     injecting ...
>>>     triggering ...
>>>     signal 7 code 4 addr 0xffffb0d75000
>>>     page not present
>>>     Test passed
>>>
>>> The si_code (code 4) from einj_mem_uc indicates that it is BUS_MCEERR_AR
>>> error as we expected.
>>>
>>> Issue 2: a synchronous error infinite loop
>>>
>>> If a user-space process, e.g. devmem, a poisoned page which has been set
>>> HWPosion flag, kill_accessing_process() is called to send SIGBUS to the
>>> current processs with error info. Because the memory_failure() is
>>> executed in the kworker contex, it will just do nothing but return
>>> EFAULT. So, devmem will access the posioned page and trigger an
>>> excepction again, resulting in a synchronous error infinite loop. Such
>>> loop may cause platform firmware to exceed some threshold and reboot
>>> when Linux could have recovered from this error.
>>>
>>> To reproduce this problem:
>>>
>>>     # STEP 1: inject an UCE error, and kernel will set HWPosion flag for related page
>>>     #einj_mem_uc single
>>>     0: single   vaddr = 0xffffb0d75400 paddr = 4092d55b400
>>>     injecting ...
>>>     triggering ...
>>>     signal 7 code 4 addr 0xffffb0d75000
>>>     page not present
>>>     Test passed
>>>
>>>     # STEP 2: access the same page and it will trigger a synchronous error infinite loop
>>>     devmem 0x4092d55b400
>>>
>>> To fix above two issues, queue memory_failure() as a task_work so that it runs in
>>> the context of the process that is actually consuming the poisoned data.
>>>
>>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>>> Tested-by: Ma Wupeng <mawupeng1@huawei.com>
>>> Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>> Reviewed-by: Xiaofei Tan <tanxiaofei@huawei.com>
>>> Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>> ---
>>>    drivers/acpi/apei/ghes.c | 78 +++++++++++++++++++++++-----------------
>>>    include/acpi/ghes.h      |  3 --
>>>    include/linux/mm.h       |  1 -
>>>    mm/memory-failure.c      | 13 -------
>>>    4 files changed, 45 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index f2ee28c44d7a..95e9520eb803 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>> @@ -467,28 +467,42 @@ static void ghes_clear_estatus(struct ghes *ghes,
>>>    }
>>>    
>>>    /*
>>> - * Called as task_work before returning to user-space.
>>> - * Ensure any queued work has been done before we return to the context that
>>> - * triggered the notification.
>>> + * struct ghes_task_work - for synchronous RAS event
>>> + *
>>> + * @twork:                callback_head for task work
>>> + * @pfn:                  page frame number of corrupted page
>>> + * @flags:                work control flags
>>> + *
>>> + * Structure to pass task work to be handled before
>>> + * returning to user-space via task_work_add().
>>>     */
>>
>>
>> Do you have any futer comments about this patch? Any comments are
>> welcomed. If not, are you happy to explictly give the reveiwed-by tag?
> 
> Sorry I've been busy switching to a new job.
> 
> I read this now through and both commit messages and the code changes
> look sane to me so I guess I don't have any problem with that:
> 
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> 
>>
>> Best Regard,
>> Shuai
> 
> BR, Jarkko


Thank you.

BR. Shuai

On Mon, Oct 28, 2024 at 04:11:40PM +0800, Shuai Xue wrote:
> Synchronous error was detected as a result of user-space process accessing
> a 2-bit uncorrected error. The CPU will take a synchronous error exception
> such as Synchronous External Abort (SEA) on Arm64. The kernel will queue a
> memory_failure() work which poisons the related page, unmaps the page, and
> then sends a SIGBUS to the process, so that a system wide panic can be
> avoided.
> 
> However, no memory_failure() work will be queued when abnormal synchronous
> errors occur. These errors can include situations such as invalid PA,
> unexpected severity, no memory failure config support, invalid GUID
> section, etc. In such case, the user-space process will trigger SEA again.
> This loop can potentially exceed the platform firmware threshold or even
> trigger a kernel hard lockup, leading to a system reboot.
> 
> Fix it by performing a force kill if no memory_failure() work is queued
> for synchronous errors.
> 
> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/acpi/apei/ghes.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index ada93cfde9ba..f2ee28c44d7a 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -801,6 +801,16 @@ static bool ghes_do_proc(struct ghes *ghes,
>  		}
>  	}
>  
> +	/*
> +	 * If no memory failure work is queued for abnormal synchronous
> +	 * errors, do a force kill.
> +	 */
> +	if (sync && !queued) {
> +		pr_err("%s:%d: hardware memory corruption (SIGBUS)\n",
> +			current->comm, task_pid_nr(current));

I think it would help to include the GHES_PFX to indicate where this
message is coming from. The pr_fmt() macro could also be introduced
instead.

Also, you may want to include the HW_ERR prefix. Not all kernel messages
related to hardware errors have this prefix today. But maybe that should
be changed so there is more consistent messaging.

Thanks,
Yazen

在 2024/10/30 22:08, Yazen Ghannam 写道:
> On Wed, Oct 30, 2024 at 09:54:00AM +0800, Shuai Xue wrote:
>>
>>
>> 在 2024/10/30 04:48, Yazen Ghannam 写道:
>>> On Mon, Oct 28, 2024 at 04:11:40PM +0800, Shuai Xue wrote:
>>>> Synchronous error was detected as a result of user-space process accessing
>>>> a 2-bit uncorrected error. The CPU will take a synchronous error exception
>>>> such as Synchronous External Abort (SEA) on Arm64. The kernel will queue a
>>>> memory_failure() work which poisons the related page, unmaps the page, and
>>>> then sends a SIGBUS to the process, so that a system wide panic can be
>>>> avoided.
>>>>
>>>> However, no memory_failure() work will be queued when abnormal synchronous
>>>> errors occur. These errors can include situations such as invalid PA,
>>>> unexpected severity, no memory failure config support, invalid GUID
>>>> section, etc. In such case, the user-space process will trigger SEA again.
>>>> This loop can potentially exceed the platform firmware threshold or even
>>>> trigger a kernel hard lockup, leading to a system reboot.
>>>>
>>>> Fix it by performing a force kill if no memory_failure() work is queued
>>>> for synchronous errors.
>>>>
>>>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>>> ---
>>>>    drivers/acpi/apei/ghes.c | 10 ++++++++++
>>>>    1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>>> index ada93cfde9ba..f2ee28c44d7a 100644
>>>> --- a/drivers/acpi/apei/ghes.c
>>>> +++ b/drivers/acpi/apei/ghes.c
>>>> @@ -801,6 +801,16 @@ static bool ghes_do_proc(struct ghes *ghes,
>>>>    		}
>>>>    	}
>>>> +	/*
>>>> +	 * If no memory failure work is queued for abnormal synchronous
>>>> +	 * errors, do a force kill.
>>>> +	 */
>>>> +	if (sync && !queued) {
>>>> +		pr_err("%s:%d: hardware memory corruption (SIGBUS)\n",
>>>> +			current->comm, task_pid_nr(current));
>>>
>>> I think it would help to include the GHES_PFX to indicate where this
>>> message is coming from. The pr_fmt() macro could also be introduced
>>> instead.
>>
>> Yes, GHES_PFX is a effective prefix and will be consistent to other message
>> in GHES driver. Will add it in next version.
>>
>> What do you mean about pr_fmt()?
> 
> This can be used to set a prefix for an entire section of code. The
> pr_*() macros will pick it up without needing to include a prefix for
> each call.
> 
> This is described in "Documentation/core-api/printk-basics.rst".

Got you point, a pr_fmt is much more convenient, but it is beyond this 
patch. I would like to send a patch to add pr_fmt and then replace each 
call after this patch merged.

> 
>>
>>>
>>> Also, you may want to include the HW_ERR prefix. Not all kernel messages
>>> related to hardware errors have this prefix today. But maybe that should
>>> be changed so there is more consistent messaging.
>>>
>>
>> Do we really need a HW_ERR prefix? The other case which use HW_ERR prefix
>> are for hardware registers. The messages which send SIGBUS does
>> not include HW_ERR, e.g. in kill_proc(), kill_procs().
>>
>>      pr_err("%#lx: Sending SIGBUS to %s:%d due to hardware memory
>> corruption\n",...
>>      pr_err("%#lx: forcibly killing %s:%d because of failure to unmap
>> corrupted page\n",...
>>
>>
> 
> Correct, HW_ERR isn't used there. My interpretation is that it can be
> used whenever an event is due to a hardware error (real or simulated).
> This is a very clear message to a user.
> 
> It may be redundant in some cases (like here where the message already
> says "hardware memory corruption"). But I think it would be go to use it
> anyway for consistency.
> 
> I think other relevant places in the kernel should also be updated. But
> that is beyond this patch, and I don't expect it to be done here and
> now.

Ok, I will add the HW_ERR in next version, if no one else objects.
> 
> Thanks,
> Yazen

Thank you.
Best Regards,
Shuai