Re: [PATCH RESEND v2] x86/mce: Set PG_hwpoison page flag to avoid the capture kernel panic

[Zhiquan Li]

On 2023/10/10 16:28, Borislav Petkov wrote:
> This commit message should explain the full scenario, like you did in
> your other reply.
> 

Thanks for your review, Boris!

I'll improve the commit message in V3 as you said.  Just adding the full
scenario part, the paragraphs to introduce the considerations for the 3
solutions and how to validate the patch are unnecessary, right?

> Also explain how the poison flag is consumed by the kdump kernel and put
> that in the comment below.
> 

Aha, this is the neat thing about the patch.  The main task of kdump
kernel is providing a "window" - /proc/vmcore, for the dump program to
access old memory.  A dump program running in userspace determines the
"policy".  Which pages need to be dumped is determined by the
configuration of dump program, it reads out the pages that the
sustaining engineer is interested in and excludes the rest.  The de
facto dump program (makedumpfile) already supports to identify those
poisoned pages and exclude them a decade ago:

https://github.com/makedumpfile/makedumpfile/commit/030800d88d4baca5f60ade0acc1846a65608883c

That's why I said the solution 1 is remaking the wheels, scanning MCE
banks, checking the poison flag, and excluding error pages are
duplicated actions.  To set the HWPosion flag in the production kernel
before panics is the only missing step to make everything work.


>> [Tony: Changed TestSetPageHWPoison() to SetPageHWPoison()]
>>
>> Co-developed-by: Youquan Song <youquan.song@xxxxxxxxx>
>> Signed-off-by: Youquan Song <youquan.song@xxxxxxxxx>
>> Signed-off-by: Zhiquan Li <zhiquan1.li@xxxxxxxxx>
>> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
> What does Tony's SOB mean here?
> 
> If I read it correctly, it is him sending this patch now. But you're
> sending it so you folks need to read up on SOB chains.
> 

When we were developing the patch internally, Tony contributed a lot of
precious ideas and guidance, not only the code change he mentioned in
commit message.

The previous V2 sent by Tony missed the merge window of v6.5, so I
re-based it onto the latest v6.6 rc, re-validated and re-send the patch.
 And I will follow up the feedback from community.


>> Reviewed-by: Naoya Horiguchi <naoya.horiguchi@xxxxxxx>
>>
>> ---
>> V2 RESEND notes:
>> - No changes on this, just rebasing as v6.6-rc1 is out.
>> - Added the tag from Naoya.
>>   Link: https://lore.kernel.org/all/20230719211625.298785-1-tony.luck@xxxxxxxxx/#t
>>
>> Changes since V1:
>> - Revised the commit message as per Naoya's suggestion.
>> - Replaced "TODO" comment in code with comments based on mailing list
>>   discussion on the lack of value in covering other page types.
>>   Link: https://lore.kernel.org/all/20230127015030.30074-1-tony.luck@xxxxxxxxx/
>> ---
>>  arch/x86/kernel/cpu/mce/core.c | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
>> index 6f35f724cc14..2725698268f3 100644
>> --- a/arch/x86/kernel/cpu/mce/core.c
>> +++ b/arch/x86/kernel/cpu/mce/core.c
>> @@ -156,6 +156,22 @@ void mce_unregister_decode_chain(struct notifier_block *nb)
>>  }
>>  EXPORT_SYMBOL_GPL(mce_unregister_decode_chain);
>>  
>> +/*
>> + * Kdump can exclude the HWPosion page to avoid touch the error page again,
>> + * the prerequisite is the PG_hwpoison page flag is set. However, for some
>> + * MCE fatal error cases, there are no opportunity to queue a task
>> + * for calling memory_failure(), as a result, the capture kernel panics.
>> + * This function marks the page as HWPoison before kernel panic() for MCE.
>> + */
>> +static void mce_set_page_hwpoison_now(unsigned long pfn)
>> +{
>> +	struct page *p;
>> +
>> +	p = pfn_to_online_page(pfn);
>> +	if (p)
>> +		SetPageHWPoison(p);
>> +}
> there's no need for that function - just put everything...
> 
>> +
>>  static void __print_mce(struct mce *m)
>>  {
>>  	pr_emerg(HW_ERR "CPU %d: Machine Check%s: %Lx Bank %d: %016Lx\n",
>> @@ -286,6 +302,8 @@ static noinstr void mce_panic(const char *msg, struct mce *final, char *exp)
>>  	if (!fake_panic) {
>>  		if (panic_timeout == 0)
>>  			panic_timeout = mca_cfg.panic_timeout;
>> +		if (final && (final->status & MCI_STATUS_ADDRV))
>> +			mce_set_page_hwpoison_now(final->addr >> PAGE_SHIFT);
> ... here, along with the comment.
> 

Good idea. I'll send V3 as you said.


Best Regards,
Zhiquan