On 2023/12/1 01:39, James Morse wrote:
Hi Boris, Shuai,
On 29/11/2023 18:54, Borislav Petkov wrote:
On Sun, Nov 26, 2023 at 08:25:38PM +0800, Shuai Xue wrote:
On Sat, Nov 25, 2023 at 02:44:52PM +0800, Shuai Xue wrote:
- an AR error consumed by current process is deferred to handle in a dedicated kernel thread, but memory_failure() assumes that it runs in the current context
On x86? ARM?
Pease point to the exact code flow.
An AR error consumed by current process is deferred to handle in a dedicated kernel thread on ARM platform. The AR error is handled in bellow flow:
Please don't think of errors as "action required" - that's a user-space signal code. If the page could be fixed by memory-failure(), you may never get a signal. (all this was the fix for always sending an action-required signal)
I assume you mean the CPU accessed a poisoned location and took a synchronous error.
Yes, I mean that CPU accessed a poisoned location and took a synchronous error.
[usr space task einj_mem_uc consumd data poison, CPU 3] STEP 0
[ghes_sdei_critical_callback: current einj_mem_uc, CPU 3] STEP 1 ghes_sdei_critical_callback => __ghes_sdei_callback => ghes_in_nmi_queue_one_entry // peak and read estatus => irq_work_queue(&ghes_proc_irq_work) <=> ghes_proc_in_irq // irq_work [ghes_sdei_critical_callback: return]
[ghes_proc_in_irq: current einj_mem_uc, CPU 3] STEP 2 => ghes_do_proc => ghes_handle_memory_failure => ghes_do_memory_failure => memory_failure_queue // put work task on current CPU => if (kfifo_put(&mf_cpu->fifo, entry)) schedule_work_on(smp_processor_id(), &mf_cpu->work); => task_work_add(current, &estatus_node->task_work, TWA_RESUME); [ghes_proc_in_irq: return]
// kworker preempts einj_mem_uc on CPU 3 due to RESCHED flag STEP 3 [memory_failure_work_func: current kworker, CPU 3] => memory_failure_work_func(&mf_cpu->work) => while kfifo_get(&mf_cpu->fifo, &entry); // until get no work => memory_failure(entry.pfn, entry.flags);
From the comment above that function:
- The function is primarily of use for corruptions that
- happen outside the current execution context (e.g. when
- detected by a background scrubber)
- Must run in process context (e.g. a work queue) with interrupts
- enabled and no spinlocks held.
[ghes_kick_task_work: current einj_mem_uc, other cpu] STEP 4 => memory_failure_queue_kick => cancel_work_sync - waiting memory_failure_work_func finish => memory_failure_work_func(&mf_cpu->work) => kfifo_get(&mf_cpu->fifo, &entry); // no work
[einj_mem_uc resume at the same PC, trigger a page fault STEP 5
STEP 0: A user space task, named einj_mem_uc consume a poison. The firmware notifies hardware error to kernel through is SDEI (ACPI_HEST_NOTIFY_SOFTWARE_DELEGATED).
STEP 1: The swapper running on CPU 3 is interrupted. irq_work_queue() rasie a irq_work to handle hardware errors in IRQ context
STEP2: In IRQ context, ghes_proc_in_irq() queues memory failure work on current CPU in workqueue and add task work to sync with the workqueue.
STEP3: The kworker preempts the current running thread and get CPU 3. Then memory_failure() is processed in kworker.
See above.
STEP4: ghes_kick_task_work() is called as task_work to ensure any queued workqueue has been done before returning to user-space.
STEP5: Upon returning to user-space, the task einj_mem_uc resumes at the current instruction, because the poison page is unmapped by memory_failure() in step 3, so a page fault will be triggered.
memory_failure() assumes that it runs in the current context on both x86 and ARM platform.
for example: memory_failure() in mm/memory-failure.c:
if (flags & MF_ACTION_REQUIRED) { folio = page_folio(p); res = kill_accessing_process(current, folio_pfn(folio), flags); }And?
Do you see the check above it?
if (TestSetPageHWPoison(p)) {
test_and_set_bit() returns true only when the page was poisoned already.
- This function is intended to handle "Action Required" MCEs on already
- hardware poisoned pages. They could happen, for example, when
- memory_failure() failed to unmap the error page at the first call, or
- when multiple local machine checks happened on different CPUs.
And that's kill_accessing_process().
So AFAIU, the kworker running memory_failure() would only mark the page as poison.
The killing happens when memory_failure() runs again and the process touches the page again.
But I'd let James confirm here.
Yes, this is what is expected to happen with the existing code.
The first pass will remove the pages from all processes that have it mapped before this user-space task can restart. Restarting the task will make it access a poisoned page, kicking off the second path which delivers the signal.
The reason for two passes is send_sig_mceerr() likes to clear_siginfo(), so even if you queued action-required before leaving GHES, memory-failure() would stomp on it.
I still don't know what you're fixing here.
The problem is if the user-space process registered for early messages, it gets a signal on the first pass. If it returns from that signal, it will access the poisoned page and get the action-required signal.
How is this making Qemu go wrong?
The problem here is that we need to assume, the first pass memory failure handle and unmap the poisoned page successfully.
- If so, it may work by the second pass action-requried signal because it access an unmapped page. But IMHO, we can improve by just sending one pass signal, so that the Guest will vmexit only once, right?
- If not, there is no second pass signal. The exist code does not handle the error code from memory_failure(), so a exception loop happens resulting a hard lockup panic.
Besides, in production environment, a second access to an already known poison page will introduce more risk of error propagation.
As to how this works for you given Boris' comments above: kill_procs() is also called from hwpoison_user_mappings(), which takes the flags given to memory-failure(). This is where the action-optional signals come from.
Thank you very much for involving to review and comment.
Best Regards, Shuai