On 2023/4/6 PM8:39, Xiaofei Tan wrote:
> Hi Shuai,
>
> Thanks for your this effort, and it's great.
> Some comments below.
>
> 在 2023/3/17 15:24, 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:
>>
>> - task work for synchronous errors.
>> - and workqueue for asynchronous errors.
>>
>> Then for 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@xxxxxxxxxxxxxxxxx>
>> ---
>> drivers/acpi/apei/ghes.c | 114 ++++++++++++++++++++++-----------------
>> include/acpi/ghes.h | 3 --
>> mm/memory-failure.c | 13 -----
>> 3 files changed, 64 insertions(+), 66 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index cccd96596efe..1901ee3498c4 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -452,45 +452,79 @@ 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");
>> + force_sig(SIGBUS);
>> }
>> -static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>> +static void 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;
>> + return;
>> pfn = PHYS_PFN(physical_addr);
>> if (!pfn_valid(pfn) && !arch_is_platform_page(physical_addr)) {
>> pr_warn_ratelimited(FW_WARN GHES_PFX
>> "Invalid address in generic error data: %#llx\n",
>> physical_addr);
>> - return false;
>> + return;
>
> For synchronous errors, we need send SIGBUS to the current task if not recovered,
> as the behavior of this patch in the function memory_failure_cb().
> Such abnormal branches should also be taken as not recovered.
You are right. Thank you for pointing this out. I overlooked the abnormal
branches. To sum up, there are three cases:
- 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.
As you commented, abnormal branches case should also be send SIGBUS, I will
handle it in ghes_proc_in_irq() if no work (task work or workqueue work) is
queued.
Best Regards,
Shuai
>
>
>> + }
>> +
>> + if (flags == MF_ACTION_REQUIRED && current->mm) {
>> + twcb = kmalloc(sizeof(*twcb), GFP_ATOMIC);
>> + if (!twcb)
>> + return;
>
> It's the same here.
>
>
>> +
>> + twcb->pfn = pfn;
>> + twcb->flags = flags;
>> + init_task_work(&twcb->twork, memory_failure_cb);
>> + task_work_add(current, &twcb->twork, TWA_RESUME);
>> + return;
>> }
>> memory_failure_queue(pfn, flags);
>> - return true;
>> }
>> -static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>> +static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>> int sev, bool sync)
>> {
>> int flags = -1;
>> @@ -498,7 +532,7 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>> struct cper_sec_mem_err *mem_err = acpi_hest_get_payload(gdata);
>> if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
>> - return false;
>> + return;
>
> and here.
>
>
>> /* iff following two events can be handled properly by now */
>> if (sec_sev == GHES_SEV_CORRECTED &&
>> @@ -508,16 +542,15 @@ static bool ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata,
>> flags = sync ? MF_ACTION_REQUIRED : 0;
>> if (flags != -1)
>> - return ghes_do_memory_failure(mem_err->physical_addr, flags);
>> + ghes_do_memory_failure(mem_err->physical_addr, flags);
>> - return false;
>> + return;
>> }
>> -static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
>> +static void 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);
>> - bool queued = false;
>> int sec_sev, i;
>> char *p;
>> int flags = sync ? MF_ACTION_REQUIRED : 0;
>> @@ -526,7 +559,7 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
>> sec_sev = ghes_severity(gdata->error_severity);
>> if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
>> - return false;
>> + return;
>
> and here.
>
>
>> p = (char *)(err + 1);
>> for (i = 0; i < err->err_info_num; i++) {
>> @@ -542,7 +575,7 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
>> * and don't filter out 'corrected' error here.
>> */
>> if (is_cache && has_pa) {
>> - queued = ghes_do_memory_failure(err_info->physical_fault_addr, flags);
>> + ghes_do_memory_failure(err_info->physical_fault_addr, flags);
>> p += err_info->length;
>> continue;
>> }
>> @@ -555,8 +588,6 @@ static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata,
>> error_type);
>> p += err_info->length;
>> }
>
> and here, for the case that memory failure is not done, as PA is invalid.
>
>
>> -
>> - return queued;
>> }
>> /*
>> @@ -654,7 +685,7 @@ static void ghes_defer_non_standard_event(struct acpi_hest_generic_data *gdata,
>> schedule_work(&entry->work);
>> }
>> -static bool ghes_do_proc(struct ghes *ghes,
>> +static void ghes_do_proc(struct ghes *ghes,
>> const struct acpi_hest_generic_status *estatus)
>> {
>> int sev, sec_sev;
>> @@ -662,7 +693,6 @@ static bool ghes_do_proc(struct ghes *ghes,
>> guid_t *sec_type;
>> 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);
>> @@ -681,13 +711,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, sync);
>> + 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, sync);
>> + ghes_handle_arm_hw_error(gdata, sev, sync);
>> } else {
>> void *err = acpi_hest_get_payload(gdata);
>> @@ -697,8 +727,6 @@ static bool ghes_do_proc(struct ghes *ghes,
>> gdata->error_data_length);
>> }
>> }
>> -
>> - return queued;
>> }
>> static void __ghes_print_estatus(const char *pfx,
>> @@ -1000,9 +1028,7 @@ 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;
>> u32 len, node_len;
>> - int ret;
>> llnode = llist_del_all(&ghes_estatus_llist);
>> /*
>> @@ -1017,25 +1043,14 @@ static void ghes_proc_in_irq(struct irq_work *irq_work)
>> estatus = GHES_ESTATUS_FROM_NODE(estatus_node);
>> len = cper_estatus_len(estatus);
>> node_len = GHES_ESTATUS_NODE_LEN(len);
>> - task_work_pending = ghes_do_proc(estatus_node->ghes, estatus);
>> + ghes_do_proc(estatus_node->ghes, estatus);
>> 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 +1111,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;
