Re: [PATCH v2 4/5] mm/hwpoison: Fix incorrect "not recovered" report for recovered clean pages

[Shuai Xue <xueshuai@linux.alibaba.com>]

在 2025/2/19 14:34, Miaohe Lin 写道:
> On 2025/2/17 14:33, Shuai Xue wrote:
>> When an uncorrected memory error is consumed there is a race between
>> the CMCI from the memory controller reporting an uncorrected error
>> with a UCNA signature, and the core reporting and SRAR signature
>> machine check when the data is about to be consumed.
>>
>> If the CMCI wins that race, the page is marked poisoned when
>> uc_decode_notifier() calls memory_failure(). For dirty pages,
>> memory_failure() invokes try_to_unmap() with the TTU_HWPOISON flag,
>> converting the PTE to a hwpoison entry. As a result,
>> kill_accessing_process():
>>
>> - call walk_page_range() and return 1 regardless of whether
>>    try_to_unmap() succeeds or fails,
>> - call kill_proc() to make sure a SIGBUS is sent
>> - return -EHWPOISON to indicate that SIGBUS is already sent to the
>>    process and kill_me_maybe() doesn't have to send it again.
>>
>> However, for clean pages, the TTU_HWPOISON flag is cleared, leaving the
>> PTE unchanged and not converted to a hwpoison entry. Conversely, for
>> clean pages where PTE entries are not marked as hwpoison,
>> kill_accessing_process() returns -EFAULT, causing kill_me_maybe() to
>> send a SIGBUS.
>>
>> Console log looks like this:
>>
>>      Memory failure: 0x827ca68: corrupted page was clean: dropped without side effects
>>      Memory failure: 0x827ca68: recovery action for clean LRU page: Recovered
>>      Memory failure: 0x827ca68: already hardware poisoned
>>      mce: Memory error not recovered
>>
>> To fix it, return 0 for "corrupted page was clean", preventing an
>> unnecessary SIGBUS.
>>
>> Fixes: 046545a661af ("mm/hwpoison: fix error page recovered but reported "not recovered"")
>> Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
>> Cc: stable@vger.kernel.org
>> ---
>>   mm/memory-failure.c | 11 ++++++++---
>>   1 file changed, 8 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 995a15eb67e2..b037952565be 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -881,12 +881,17 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn,
>>   	mmap_read_lock(p->mm);
>>   	ret = walk_page_range(p->mm, 0, TASK_SIZE, &hwpoison_walk_ops,
>>   			      (void *)&priv);
>> +	/*
>> +	 * ret = 1 when CMCI wins, regardless of whether try_to_unmap()
>> +	 * succeeds or fails, then kill the process with SIGBUS.
>> +	 * ret = 0 when poison page is a clean page and it's dropped, no
>> +	 * SIGBUS is needed.
>> +	 */
>>   	if (ret == 1 && priv.tk.addr)
>>   		kill_proc(&priv.tk, pfn, flags);
>> -	else
>> -		ret = 0;
>>   	mmap_read_unlock(p->mm);
>> -	return ret > 0 ? -EHWPOISON : -EFAULT;
>> +
>> +	return ret > 0 ? -EHWPOISON : 0;
> 
> The caller kill_me_maybe will do set_mce_nospec + sync_core again.
> 
> static void kill_me_maybe(struct callback_head *cb)
> {
> 	struct task_struct *p = container_of(cb, struct task_struct, mce_kill_me);
> 	int flags = MF_ACTION_REQUIRED;
> 	...
> 	ret = memory_failure(pfn, flags);
> 	if (!ret) {
> 		set_mce_nospec(pfn);
> 		sync_core();
> 		return;
> 	}
> 
> Is this expected?
> 

the second set_mce_nospec do nothing and have no side affect.

sync_core() is introduced by Tony [1]:

Also moved sync_core(). The comments for this function say that it should
only be called when instructions have been changed/re-mapped. Recovery for
an instruction fetch may change the physical address. But that doesn't happen
until the scheduled work runs (which could be on another CPU).

[1]https://lore.kernel.org/all/20200824221237.5397-1-tony.luck@intel.com/T/#u

IMHO, I think it also has no side affect.

@Tony, could you help to confirm this?

Thank.
Shuai