[PATCH] mm/hwpoison: Fix error page recovered but reported "not recovered"

[PATCH] mm/hwpoison: Fix error page recovered but reported "not recovered"

[Tony Luck]

From: Youquan Song <youquan.song@intel.com>

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]

Fixes: a3f5d80ea401 ("mm,hwpoison: send SIGBUS with error virutal address")
Signed-off-by: Youquan Song <youquan.song@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
---

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(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 3a274468f193..a67f558b08ea 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -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[] = {
-- 
2.31.1

Re: [PATCH] mm/hwpoison: Fix error page recovered but reported "not recovered"

[Naoya Horiguchi]

On Fri, Jan 07, 2022 at 11:44:50AM -0800, Tony Luck wrote:
> From: Youquan Song <youquan.song@intel.com>
> 
> 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]
> 
> Fixes: a3f5d80ea401 ("mm,hwpoison: send SIGBUS with error virutal address")
> Signed-off-by: Youquan Song <youquan.song@intel.com>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 
> 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(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 3a274468f193..a67f558b08ea 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -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.

> +
> +	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
---
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 14ae5c18e776..4c9bd1d37301 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -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[] = {

Re: [PATCH] mm/hwpoison: Fix error page recovered but reported "not recovered"

[Luck, Tony]

On Wed, Jan 12, 2022 at 09:11:45PM +0900, Naoya Horiguchi wrote:
> On Fri, Jan 07, 2022 at 11:44:50AM -0800, Tony Luck wrote:
> > From: Youquan Song <youquan.song@intel.com>

> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 14ae5c18e776..4c9bd1d37301 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -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[] = {

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:

Reported-by: Youquan Song <youquan.song@intel.com>

Thanks for looking (and fixing!)

-Tony

Re: [PATCH] mm/hwpoison: Fix error page recovered but reported "not recovered"

[HORIGUCHI NAOYA(堀口 直也)]

On Wed, Jan 12, 2022 at 06:00:51PM -0800, Luck, Tony wrote:
> On Wed, Jan 12, 2022 at 09:11:45PM +0900, Naoya Horiguchi wrote:
> > On Fri, Jan 07, 2022 at 11:44:50AM -0800, Tony Luck wrote:
> > > From: Youquan Song <youquan.song@intel.com>
> 
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index 14ae5c18e776..4c9bd1d37301 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -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[] = {
> 
> Yes. This fixes the problem (and your explanation helped
> me understand this code better).
> 

Thank you for confirming.  I just sent v2.

> Fell free to take any words you need from the original patch
> comment and switch to:
> 
> Reported-by: Youquan Song <youquan.song@intel.com>
> 
> Thanks for looking (and fixing!)

Your welcome :)

- Naoya