Re: [PATCH v9 0/2] ACPI: APEI: handle synchronous errors in task work with proper si_code

[James Morse <james.morse@arm.com>]

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.


>> -----------------------------------------------------------------------------
>> [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?


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.


Thanks,

James