mm/hwpoison: Fix error page recovered but reported "not recovered"
Subsystems (2)
PATCH 1/1 mm/hwpoison: Fix error page recovered but reported "not recovered"
From: Youquan Song <[email protected]>
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() and the machine check processing code finds the page already poisoned. It calls kill_accessing_process() to make sure a SIGBUS is sent. But returns the wrong error code.
Console log looks like this:
[34775.674296] mce: Uncorrected hardware memory error in user-access at 3710b3400 [34775.675413] Memory failure: 0x3710b3: recovery action for dirty LRU page: Recovered [34775.690310] Memory failure: 0x3710b3: already hardware poisoned [34775.696247] Memory failure: 0x3710b3: Sending SIGBUS to einj_mem_uc:361438 due to hardware memory corruption [34775.706072] mce: Memory error not recovered
Fix kill_accessing_process() to return -EHWPOISON to avoid the noise message "Memory error not recovered" and skip duplicate SIGBUS.
[Tony: Reworded some parts of commit message]
---
This code is very subtle ... the fix makes the "not recovered" message go away ... but I'm not more than 75% confident that this is the right fix. Please check carefully :-)
mm/memory-failure.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
mm/memory-failure.c +2 -1
@@ -707,7 +707,8 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn,if (ret ==1 && priv.tk.addr)kill_proc (&priv.tk, pfn, flags);mmap_read_unlock (p->mm); -return ret ? -EFAULT : -EHWPOISON ; + +return (ret <0 ) ? -EFAULT : -EHWPOISON ; }static const char *action_name[] = {
Signature · 1 line
2.31.1
Quoted 43 lines · Tony Luck
From: Youquan Song <[email protected]>
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()callsmemory_failure()and the machine check processing code finds the page already poisoned. It callskill_accessing_process()to make sure a SIGBUS is sent. But returns the wrong error code.Console log looks like this:
[34775.674296] mce: Uncorrected hardware memory error in user-access at 3710b3400 [34775.675413] Memory failure: 0x3710b3: recovery action for dirty LRU page: Recovered [34775.690310] Memory failure: 0x3710b3: already hardware poisoned [34775.696247] Memory failure: 0x3710b3: Sending SIGBUS to einj_mem_uc:361438 due to hardware memory corruption [34775.706072] mce: Memory error not recoveredFix
kill_accessing_process()to return-EHWPOISONto avoid the noise message "Memory error not recovered" and skip duplicate SIGBUS.[Tony: Reworded some parts of commit message]
Fixes a3f5d80ea401 ("mm,hwpoison: send SIGBUS with error virutal address")Signed-off-by Youquan Song <[email protected]>Signed-off-by Tony Luck <[email protected]>---
This code is very subtle ... the fix makes the "not recovered" message go away ... but I'm not more than 75% confident that this is the right fix. Please check carefully :-)
mm/memory-failure.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)mm/memory-failure.c +0
-0
mm/memory-failure.c static int kill_accessing_process(struct task_struct *p, unsigned long pfn, · line 710
Quoted 4 lines
mm/memory-failure.c +0
-1@@ -707,7 +707,8 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn,if (ret ==1 && priv.tk.addr)kill_proc (&priv.tk, pfn, flags);mmap_read_unlock (p->mm); -return ret ? -EFAULT : -EHWPOISON ;
Thank you for reporting, the original code was wrong and the trinary operation returns opposite code. -EHWPOISON here is to notify kill_me_maybe() that it does not have to send SIGBUS in its own because kill_accessing_process() already sent SIGBUS with proper virtual address info.
mm/memory-failure.c static int kill_accessing_process(struct task_struct *p, unsigned long pfn, · line 711
Quoted 2 lines
mm/memory-failure.c +2
-0+ +return (ret <0 ) ? -EFAULT : -EHWPOISON ;
It seems to me that this returns -EHWPOISON whether any hwpoison entry is found or not, so this fix can cause another issue.
We want to return -EHWPOISON only when kill_accessing_process() sent SIGBUS, so I think that the following diff should be what we want.
Could you check this fix works for you?
Thanks,
Naoya Horiguchi
---
mm/memory-failure.c +3 -1
@@ -707,8 +707,10 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn, (void *)&priv);if (ret ==1 && priv.tk.addr)kill_proc (&priv.tk, pfn, flags); +else + ret =0 ;mmap_read_unlock (p->mm); -return ret ? -EFAULT : -EHWPOISON ; +return ret >0 ? -EHWPOISON : -EFAULT ; }static const char *action_name[] = {
Quoted 9 lines · Naoya Horiguchi
Quoted 1 line · Tony Luck
From: Youquan Song <[email protected]>
mm/memory-failure.c +0
-0@@ -707,8 +707,10 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn, (void *)&priv);
mm/memory-failure.c static int kill_accessing_process(struct task_struct *p, unsigned long pfn, · line 716
Quoted 10 lines
mm/memory-failure.c +3
-1if (ret ==1 && priv.tk.addr)kill_proc (&priv.tk, pfn, flags); +else + ret =0 ;mmap_read_unlock (p->mm); -return ret ? -EFAULT : -EHWPOISON ; +return ret >0 ? -EHWPOISON : -EFAULT ; }static const char *action_name[] = {
Yes. This fixes the problem (and your explanation helped me understand this code better).
Fell free to take any words you need from the original patch comment and switch to:
Thanks for looking (and fixing!)
-Tony
Quoted 19 lines · Tony
Quoted 18 lines · Naoya Horiguchi
Quoted 1 line · Tony Luck
From: Youquan Song <[email protected]>
mm/memory-failure.c +3
-1@@ -707,8 +707,10 @@ static int kill_accessing_process(struct task_struct *p, unsigned long pfn, (void *)&priv);if (ret ==1 && priv.tk.addr)kill_proc (&priv.tk, pfn, flags); +else + ret =0 ;mmap_read_unlock (p->mm); -return ret ? -EFAULT : -EHWPOISON ; +return ret >0 ? -EHWPOISON : -EFAULT ; }
Quoted 5 lines
mm/memory-failure.c static int kill_accessing_process(struct task_struct *p, unsigned long pfn, · line 716
Quoted 1 line
mm/memory-failure.c +0
-0static const char *action_name[] = {Yes. This fixes the problem (and your explanation helped me understand this code better).
Thank you for confirming. I just sent v2.
Quoted 6 lines
Fell free to take any words you need from the original patch comment and switch to:
Reported-by Youquan Song <[email protected]>Thanks for looking (and fixing!)
Your welcome :)
- Naoya