diff mbox series

[v3,2/3] mm,hwpoison: return -EHWPOISON when page already

Message ID 20210421005728.1994268-3-nao.horiguchi@gmail.com (mailing list archive)
State New, archived
Headers show
Series mm,hwpoison: fix sending SIGBUS for Action Required MCE | expand

Commit Message

Naoya Horiguchi April 21, 2021, 12:57 a.m. UTC 
From: Aili Yao <yaoaili@kingsoft.com>

When the page is already poisoned, another memory_failure() call in the
same page now returns 0, meaning OK. For nested memory mce handling, this
behavior may lead to one mce looping, Example:

1. When LCME is enabled, and there are two processes A && B running on
different core X && Y separately, which will access one same page, then
the page corrupted when process A access it, a MCE will be rasied to
core X and the error process is just underway.

2. Then B access the page and trigger another MCE to core Y, it will also
do error process, it will see TestSetPageHWPoison be true, and 0 is
returned.

3. The kill_me_maybe will check the return:

    1244 static void kill_me_maybe(struct callback_head *cb)
    1245 {
    ...
    1254         if (!memory_failure(p->mce_addr >> PAGE_SHIFT, flags) &&
    1255             !(p->mce_kflags & MCE_IN_KERNEL_COPYIN)) {
    1256                 set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page);
    1257                 sync_core();
    1258                 return;
    1259         }
    ...
    1267 }

4. The error process for B will end, and may nothing happened if
kill-early is not set, The process B will re-excute instruction and get
into mce again and then loop happens. And also the set_mce_nospec()
here is not proper, may refer to commit fd0e786d9d09 ("x86/mm,
mm/hwpoison: Don't unconditionally unmap kernel 1:1 pages").

For other cases which care the return value of memory_failure() should
check why they want to process a memory error which have already been
processed. This behavior seems reasonable.

Signed-off-by: Aili Yao <yaoaili@kingsoft.com>
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Borislav Petkov April 22, 2021, 5:02 p.m. UTC | #1 
On Wed, Apr 21, 2021 at 09:57:27AM +0900, Naoya Horiguchi wrote:
> From: Aili Yao <yaoaili@kingsoft.com>

> Subject: Re: [PATCH v3 2/3] mm,hwpoison: return -EHWPOISON when page already

		... Return -EHWPOISON to denote that the page has already been poisoned"

> When the page is already poisoned, another memory_failure() call in the
> same page now returns 0, meaning OK. For nested memory mce handling, this
> behavior may lead to one mce looping,

s/mce/MCE/g

> Example:

For example:

> 1. When LCME is enabled, and there are two processes A && B running on
> different core X && Y separately, which will access one same page, then

				  which access the same page...

s/&&/and/g

> the page corrupted when process A access it, a MCE will be rasied to
> core X and the error process is just underway.

... and you lost me here. I don't understand what that is trying to say.
Is that trying to say that when process A encounters the error, the MCE
will be raised on CPU X?

> 2. Then B access the page and trigger another MCE to core Y, it will also
> do error process, it will see TestSetPageHWPoison be true, and 0 is
> returned.

That sentence needs massaging.

> 3. The kill_me_maybe will check the return:
> 
>     1244 static void kill_me_maybe(struct callback_head *cb)
>     1245 {
>     ...
>     1254         if (!memory_failure(p->mce_addr >> PAGE_SHIFT, flags) &&
>     1255             !(p->mce_kflags & MCE_IN_KERNEL_COPYIN)) {
>     1256                 set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page);
>     1257                 sync_core();
>     1258                 return;
>     1259         }
>     ...
>     1267 }

No need for the line numbers.

> 4. The error process for B will end, and may nothing happened if
> kill-early is not set, The process B will re-excute instruction and get
> into mce again and then loop happens. And also the set_mce_nospec()
> here is not proper, may refer to commit fd0e786d9d09 ("x86/mm,
> mm/hwpoison: Don't unconditionally unmap kernel 1:1 pages").

That needs massaging too.

> For other cases which care the return value of memory_failure() should
> check why they want to process a memory error which have already been
> processed. This behavior seems reasonable.

This whole commit message needs sanitizing.

Also, looking at the next patch, you can merge this one into the next
because the next one is acting on -EHWPOISON so it all belongs together
in a single patch.

Thx.
HORIGUCHI NAOYA(堀口 直也) April 23, 2021, 2:17 a.m. UTC | #2 
On Thu, Apr 22, 2021 at 07:02:04PM +0200, Borislav Petkov wrote:
> On Wed, Apr 21, 2021 at 09:57:27AM +0900, Naoya Horiguchi wrote:
> > From: Aili Yao <yaoaili@kingsoft.com>
> 
...
> This whole commit message needs sanitizing.
> 
> Also, looking at the next patch, you can merge this one into the next
> because the next one is acting on -EHWPOISON so it all belongs together
> in a single patch.

OK, I'll fold this into 3/3.

Thanks,
Naoya Horiguchi
diff mbox series

Patch

diff --git v5.12-rc8/mm/memory-failure.c v5.12-rc8_patched/mm/memory-failure.c
index 4087308e4b32..39d0ff0339b9 100644
--- v5.12-rc8/mm/memory-failure.c
+++ v5.12-rc8_patched/mm/memory-failure.c
@@ -1228,7 +1228,7 @@  static int memory_failure_hugetlb(unsigned long pfn, int flags)
 	if (TestSetPageHWPoison(head)) {
 		pr_err("Memory failure: %#lx: already hardware poisoned\n",
 		       pfn);
-		return 0;
+		return -EHWPOISON;
 	}
 
 	num_poisoned_pages_inc();
@@ -1437,6 +1437,7 @@  int memory_failure(unsigned long pfn, int flags)
 	if (TestSetPageHWPoison(p)) {
 		pr_err("Memory failure: %#lx: already hardware poisoned\n",
 			pfn);
+		res = -EHWPOISON;
 		goto unlock_mutex;
 	}