On 2024/3/8 18:18, Borislav Petkov wrote:
> On Sun, Feb 04, 2024 at 04:01:44PM +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. 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 should
>> terminate the current process which accessing the poisoned page. This is
>
> "which has accessed poison data"
Thank you. Will fix the grammer.
>
>> done by sending a SIGBUS signal with an error code BUS_MCEERR_AR,
>> indicating an action-required machine check error on read.
>>
>> However, the memory failure recovery is incorrectly sending a SIGBUS
>> with wrong error code BUS_MCEERR_AO for synchronous errors in early kill
>> mode, even MF_ACTION_REQUIRED is set. The main problem is that
>
> "even if"
Thank you. Will fix the grammer.
>
>> synchronous errors are queued as a memory_failure() work, and are
>> executed within a kernel thread context, not the user-space process that
>> encountered the corrupted memory on ARM64 platform. As a result, when
>> kill_proc() is called to terminate the process, it sends the incorrect
>> SIGBUS error code because the context in which it operates is not the
>> one where the error was triggered.
>>
>> To this end, queue memory_failure() as a task_work so that it runs in
>> the context of the process that is actually consuming the poisoned data,
>> and it will send SIBBUS with si_code BUS_MCEERR_AR.
>
> SIGBUS
Sorry, will fix the typo.
>
>>
>> Signed-off-by: Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>
>> Tested-by: Ma Wupeng <mawupeng1@xxxxxxxxxx>
>> Reviewed-by: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
>> Reviewed-by: Xiaofei Tan <tanxiaofei@xxxxxxxxxx>
>> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>> ---
>> drivers/acpi/apei/ghes.c | 77 +++++++++++++++++++++++-----------------
>> include/acpi/ghes.h | 3 --
>> mm/memory-failure.c | 13 -------
>> 3 files changed, 44 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 0892550732d4..e5086d795bee 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -465,28 +465,41 @@ 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
>
> What's so special about it being a "sync_"?
>
> task_work is just fine and something else could use it too.
You are right, the `sync_task_work` is only use for synchronous RAS event right, but
it could be also use for other purpose in the future. The purpose can be specified
through flags.
I will remove the `sync_` prefix.
>
>> + *
>> + * @twork: callback_head for task work
>> + * @pfn: page frame number of corrupted page
>> + * @flags: fine tune action taken
>
> s/fine tune action taken/work control flags/
>
Will fix it.
>> + *
>> + * Structure to pass task work to be handled before
>> + * ret_to_user via task_work_add().
>
> What is "ret_to_user"?
>
> If this is an ARM thing, then make sure you explain stuff properly and
> detailed. This driver is used by multiple architectures.
It is not ARM specific thing. I mean it is used by task_work before returning to user-space.
+ * 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 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);
>
> Ugly linebreak - no need for it.
Will fix it.
>
>> - 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("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;
>> @@ -499,6 +512,18 @@ static bool ghes_do_memory_failure(u64 physical_addr, int flags)
>> return false;
>> }
>>
>> + if (flags == MF_ACTION_REQUIRED && current->mm) {
>> + twcb = (void *)gen_pool_alloc(ghes_estatus_pool, sizeof(*twcb));
>> + 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;
>> }
>> @@ -746,7 +771,7 @@ int cxl_cper_unregister_callback(cxl_cper_callback callback)
>> }
>> EXPORT_SYMBOL_NS_GPL(cxl_cper_unregister_callback, CXL);
>>
>> -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;
>> @@ -814,8 +839,6 @@ static bool ghes_do_proc(struct ghes *ghes,
>> pr_err("Sending SIGBUS to current task due to memory error not recovered");
>> force_sig(SIGBUS);
>> }
>> -
>> - return queued;
>> }
>>
>> static void __ghes_print_estatus(const char *pfx,
>> @@ -1117,9 +1140,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);
>> /*
>> @@ -1134,25 +1155,16 @@ 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);
>
> I have no clue why this is being removed.
Before this patch, a memory_failure() work is queued into workqueue for
both the asynchronous interrupt and synchronous exception. So
memory_failure() will be executed asynchronously. For NMIlike
notifications, commit 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure()
queue for synchronous errors") keeps track of whether memory_failure() work
was queued, and makes task_work pending to flush out the queue. It ensures
any queued work has been done before we return to the context that
triggered the notification.
In this patch:
- a memory_failure() work is queued into workqueue for asynchronous interrupt
- a memory_failure() task_work is queued by task_work_add for synchronous exception
The memory_failure() task_work will be handled before returning to user
space, so we does not need to queue a flushing task_work any anymore.
>
> Why doesn't a synchronous exception on ARM call into ghes_proc_in_irq()?
/*
* SEA can interrupt SError, mask it and describe this as an NMI so
* that APEI defers the handling.
*/
local_daif_restore(DAIF_ERRCTX);
nmi_enter();
=> ghes_notify_sea
=> ghes_in_nmi_spool_from_list
=> ghes_in_nmi_queue_one_entry // also called in __ghes_sdei_callback
=> irq_work_queue(&ghes_proc_irq_work);
nmi_exit();
>
> That SDEI thing certainly does.
>
> Well looka here:
>
> 7f17b4a121d0 ("ACPI: APEI: Kick the memory_failure() queue for synchronous errors")
>
> that thing does exactly what you're trying to "fix". So why doesn't that
> work for you?
>
Commit a70297d22132 (ACPI: APEI: set memory failure flags as MF_ACTION_REQUIRED on synchronous events)
set MF_ACTION_REQUIRED for synchronous events.
/*
* Send all the processes who have the page mapped a signal.
* ``action optional'' if they are not immediately affected by the error
* ``action required'' if error happened in current execution context
*/
static int kill_proc(struct to_kill *tk, unsigned long pfn, int flags)
{
...
if ((flags & MF_ACTION_REQUIRED) && (t == current))
ret = force_sig_mceerr(BUS_MCEERR_AR,
(void __user *)tk->addr, addr_lsb);
else
ret = send_sig_mceerr(BUS_MCEERR_AO, (void __user *)tk->addr,
addr_lsb, t);
...
}
Because the memory_failure() running in a kthread context, the false branch in kill_proc()
will send SIGBUS with BUS_MCEERR_AO. But we except it as a BUS_MCEERR_AR.
Thank you for valuable comments :)
Best Regards,
Shuai
