| Message ID | 20250217063335.22257-5-xueshuai@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/hwpoison: Fix regressions in memory failure handling | expand |
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? Thanks. .
在 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
> > 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? Correct. Re-runing these calls is harmless. -Tony
On 2025/2/20 1:15, Luck, Tony wrote: >>> 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? > > Correct. Re-runing these calls is harmless. Got it. Thanks both. > > -Tony >
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; } /*
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(-)