Re: [PATCH v2] mm: hwpoison: coredump: support recovery from dump_user_range()

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2023/4/20 10:03, Jane Chu wrote:
> 
> On 4/19/2023 5:03 AM, Kefeng Wang wrote:
>>
>>
>> On 2023/4/19 15:25, HORIGUCHI NAOYA(堀口 直也) wrote:
>>> On Tue, Apr 18, 2023 at 05:45:06PM +0800, Kefeng Wang wrote:
>>>>
>>>>
...
>>>>>> @@ -371,6 +372,14 @@ size_t _copy_mc_to_iter(const void *addr, 
>>>>>> size_t bytes, struct iov_iter *i)
>>>>>>    EXPORT_SYMBOL_GPL(_copy_mc_to_iter);
>>>>>>    #endif /* CONFIG_ARCH_HAS_COPY_MC */
>>>>>> +static void *memcpy_from_iter(struct iov_iter *i, void *to, const 
>>>>>> void *from,
>>>>>> +                 size_t size)
>>>>>> +{
>>>>>> +    if (iov_iter_is_copy_mc(i))
>>>>>> +        return (void *)copy_mc_to_kernel(to, from, size);
>>>>>
>>>>> Is it helpful to call memory_failure_queue() if copy_mc_to_kernel() 
>>>>> fails
>>>>> due to a memory error?
>>>>
>>>> For dump_user_range(), the task is dying, if copy incomplete size, the
>>>> coredump will fail and task will exit, also memory_failure will
>>>> be called by kill_me_maybe(),
>>>>
>>>>   CPU: 0 PID: 1418 Comm: test Tainted: G   M               6.3.0-rc5 
>>>> #29
>>>>   Call Trace:
>>>>    <TASK>
>>>>    dump_stack_lvl+0x37/0x50
>>>>    memory_failure+0x51/0x970
>>>>    kill_me_maybe+0x5b/0xc0
>>>>    task_work_run+0x5a/0x90
>>>>    exit_to_user_mode_prepare+0x194/0x1a0
>>>>    irqentry_exit_to_user_mode+0x9/0x30
>>>>    noist_exc_machine_check+0x40/0x80
>>>>    asm_exc_machine_check+0x33/0x40
>>>
>>> Is this call trace printed out when copy_mc_to_kernel() failed by 
>>> finding
>>> a memory error (or in some testcase using error injection)?
>>
>> I add dump_stack() into memory_failure() to check whether the poisoned
>> memory is called or not, and the call trace shows it do call
>> memory_failure()， but I get confused when do the test.
>>
>>> In my understanding, an MCE should not be triggered when MC-safe copy 
>>> tries
>>> to access to a memory error.  So I feel that we might be talking about
>>> different scenarios.
>>>
>>> When I questioned previously, I thought about the following scenario:
>>>
>>>    - a process terminates abnormally for any reason like segmentation 
>>> fault,
>>>    - then, kernel tries to create a coredump,
>>>    - during this, the copying routine accesses to corrupted page to 
>>> read.
>>>
>> Yes, we tested like your described,
>>
>> 1) inject memory error into a process
>> 2) send a SIGABT/SIGBUS to process to trigger the coredump
>>
>> Without patch, the system panic, and with patch only process exits.
>>
>>> In this case the corrupted page should not be handled by 
>>> memory_failure()
>>> yet (because otherwise properly handled hwpoisoned page should be 
>>> ignored
>>> by coredump process).  The coredump process would exit with failure with
>>> your patch, but then, the corrupted page is still left unhandled and can
>>> be reused, so any other thread can easily access to it again.
>>
>> As shown above, the corrupted page will be handled by 
>> memory_failure(), but what I'm wondering,
>> 1) memory_failure() is not always called
>> 2) look at the above call trace, it looks like from asynchronous
>>     interrupt, not from synchronous exception, right?
>>
>>>
>>> You can find a few other places (like __wp_page_copy_user and 
>>> ksm_might_need_to_copy)
>>> to call memory_failure_queue() to cope with such unhandled error pages.
>>> So does memcpy_from_iter() do the same?
>>
>> I add some debug print in do_machine_check() on x86:
>>
>> 1) COW,
>>    m.kflags: MCE_IN_KERNEL_RECOV
>>    fixup_type: EX_TYPE_DEFAULT_MCE_SAFE
>>
>>    CPU: 11 PID: 2038 Comm: einj_mem_uc
>>    Call Trace:
>>     <#MC>
>>     dump_stack_lvl+0x37/0x50
>>     do_machine_check+0x7ad/0x840
>>     exc_machine_check+0x5a/0x90
>>     asm_exc_machine_check+0x1e/0x40
>>    RIP: 0010:copy_mc_fragile+0x35/0x62
>>
>>    if (m.kflags & MCE_IN_KERNEL_RECOV) {
>>            if (!fixup_exception(regs, X86_TRAP_MC, 0, 0))
>>                    mce_panic("Failed kernel mode recovery", &m, msg);
>>    }
>>
>>    if (m.kflags & MCE_IN_KERNEL_COPYIN)
>>            queue_task_work(&m, msg, kill_me_never);
>>
>> There is no memory_failure() called when
>> EX_TYPE_DEFAULT_MCE_SAFE, also EX_TYPE_FAULT_MCE_SAFE too,
>> so we manually add a memory_failure_queue() to handle with
>> the poisoned page.
>>
>> 2） Coredump,  nothing print about m.kflags and fixup_type,
>> with above check, add a memory_failure_queue() or memory_failure() seems
>> to be needed for memcpy_from_iter(), but it is totally different from
>> the COW scenario
>>
>>
>> Another question, other copy_mc_to_kernel() callers, eg,
>> nvdimm/dm-writecache/dax, there are not call memory_failure_queue(),
>> should they need a memory_failure_queue(), if so, why not add it into
>> do_machine_check() ?
> 

What I mean is that EX_TYPE_DEFAULT_MCE_SAFE/EX_TYPE_FAULT_MCE_SAFE
is designed to identify fixups which allow in kernel #MC recovery,
that is, the caller of copy_mc_to_kernel() must know the source
is a user address, so we could add a MCE_IN_KERNEL_COPYIN fro
the MCE_SAFE type.

diff --git a/arch/x86/kernel/cpu/mce/severity.c 
b/arch/x86/kernel/cpu/mce/severity.c
index c4477162c07d..63e94484c5d6 100644
--- a/arch/x86/kernel/cpu/mce/severity.c
+++ b/arch/x86/kernel/cpu/mce/severity.c
@@ -293,12 +293,11 @@ static noinstr int error_context(struct mce *m, 
struct pt_regs *regs)
         case EX_TYPE_COPY:
                 if (!copy_user)
                         return IN_KERNEL;
-               m->kflags |= MCE_IN_KERNEL_COPYIN;
                 fallthrough;

         case EX_TYPE_FAULT_MCE_SAFE:
         case EX_TYPE_DEFAULT_MCE_SAFE:
-               m->kflags |= MCE_IN_KERNEL_RECOV;
+               m->kflags |= MCE_IN_KERNEL_RECOV | MCE_IN_KERNEL_COPYIN;
                 return IN_KERNEL_RECOV;

         default:

then we could drop memory_failure_queue(pfn, flags) from cow/ksm copy, 
or every Machine Check safe memory copy will need a memory_failure_xx() 
call.

+Thomas，who add the two types, could you share some comments about 
this,thanks.

> In the dax case, if the source address is poisoned, and we do follow up 
> with memory_failure_queue(pfn, flags), what should the value of the 
> 'flags' be ?


I think flags = 0 is enough to for all copy_mc_xxx to isolate the 
poisoned page.

Thanks.