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

[Shuai Xue]

在 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@xxxxxxxxxxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > ---
> >   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@xxxxxxxxx/T/#u

IMHO, I think it also has no side affect.

@Tony, could you help to confirm this?

Thank.
Shuai