[PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[Naoya Horiguchi]

Hi,

I updated the series based on previous feedbacks/discussions.

Changelog v2 -> v3:
- 1/3 aligned returning code with "goto out" approach,
- 3/3 added one Tested-by tag,
- 3/3 fixed comment on kill_accessing_process().

Changelog v1 -> v2:
- 3/3 fixed on initializing local variables and calculation logic
  of error virtual address,

v1: https://lore.kernel.org/linux-mm/20210412224320.1747638-1-nao.horiguchi@gmail.com/
v2 (only 3/3 is posted): https://lore.kernel.org/linux-mm/20210419023658.GA1962954@u2004/

Thanks,
Naoya Horiguchi

--- quote from cover letter of v1 ---

I wrote this patchset to materialize what I think is the current
allowable solution mentioned by the previous discussion [1].
I simply borrowed Tony's mutex patch and Aili's return code patch,
then I queued another one to find error virtual address in the best
effort manner.  I know that this is not a perfect solution, but
should work for some typical case.

[1]: https://lore.kernel.org/linux-mm/20210331192540.2141052f@alex-virtual-machine/
---
Summary:

Aili Yao (1):
      mm,hwpoison: return -EHWPOISON when page already

Naoya Horiguchi (1):
      mm,hwpoison: add kill_accessing_process() to find error virtual address

Tony Luck (1):
      mm/memory-failure: Use a mutex to avoid memory_failure() races

 arch/x86/kernel/cpu/mce/core.c |  13 ++-
 include/linux/swapops.h        |   5 ++
 mm/memory-failure.c            | 181 +++++++++++++++++++++++++++++++++++++----
 3 files changed, 183 insertions(+), 16 deletions(-)

[PATCH v3 1/3] mm/memory-failure: Use a mutex to avoid memory_failure() races

[Naoya Horiguchi]

From: Tony Luck <tony.luck@intel.com>

There can be races when multiple CPUs consume poison from the same
page. The first into memory_failure() atomically sets the HWPoison
page flag and begins hunting for tasks that map this page. Eventually
it invalidates those mappings and may send a SIGBUS to the affected
tasks.

But while all that work is going on, other CPUs see a "success"
return code from memory_failure() and so they believe the error
has been handled and continue executing.

Fix by wrapping most of the internal parts of memory_failure() in
a mutex.

Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git v5.12-rc8/mm/memory-failure.c v5.12-rc8_patched/mm/memory-failure.c
index 24210c9bd843..4087308e4b32 100644
--- v5.12-rc8/mm/memory-failure.c
+++ v5.12-rc8_patched/mm/memory-failure.c
@@ -1381,6 +1381,8 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 	return rc;
 }
 
+static DEFINE_MUTEX(mf_mutex);
+
 /**
  * memory_failure - Handle memory failure of a page.
  * @pfn: Page Number of the corrupted page
@@ -1404,7 +1406,7 @@ int memory_failure(unsigned long pfn, int flags)
 	struct page *hpage;
 	struct page *orig_head;
 	struct dev_pagemap *pgmap;
-	int res;
+	int res = 0;
 	unsigned long page_flags;
 	bool retry = true;
 
@@ -1424,13 +1426,18 @@ int memory_failure(unsigned long pfn, int flags)
 		return -ENXIO;
 	}
 
+	mutex_lock(&mf_mutex);
+
 try_again:
-	if (PageHuge(p))
-		return memory_failure_hugetlb(pfn, flags);
+	if (PageHuge(p)) {
+		res = memory_failure_hugetlb(pfn, flags);
+		goto unlock_mutex;
+	}
+
 	if (TestSetPageHWPoison(p)) {
 		pr_err("Memory failure: %#lx: already hardware poisoned\n",
 			pfn);
-		return 0;
+		goto unlock_mutex;
 	}
 
 	orig_head = hpage = compound_head(p);
@@ -1463,17 +1470,19 @@ int memory_failure(unsigned long pfn, int flags)
 				res = MF_FAILED;
 			}
 			action_result(pfn, MF_MSG_BUDDY, res);
-			return res == MF_RECOVERED ? 0 : -EBUSY;
+			res = res == MF_RECOVERED ? 0 : -EBUSY;
 		} else {
 			action_result(pfn, MF_MSG_KERNEL_HIGH_ORDER, MF_IGNORED);
-			return -EBUSY;
+			res = -EBUSY;
 		}
+		goto unlock_mutex;
 	}
 
 	if (PageTransHuge(hpage)) {
 		if (try_to_split_thp_page(p, "Memory Failure") < 0) {
 			action_result(pfn, MF_MSG_UNSPLIT_THP, MF_IGNORED);
-			return -EBUSY;
+			res = -EBUSY;
+			goto unlock_mutex;
 		}
 		VM_BUG_ON_PAGE(!page_count(p), p);
 	}
@@ -1497,7 +1506,7 @@ int memory_failure(unsigned long pfn, int flags)
 	if (PageCompound(p) && compound_head(p) != orig_head) {
 		action_result(pfn, MF_MSG_DIFFERENT_COMPOUND, MF_IGNORED);
 		res = -EBUSY;
-		goto out;
+		goto unlock_page;
 	}
 
 	/*
@@ -1517,14 +1526,14 @@ int memory_failure(unsigned long pfn, int flags)
 		num_poisoned_pages_dec();
 		unlock_page(p);
 		put_page(p);
-		return 0;
+		goto unlock_mutex;
 	}
 	if (hwpoison_filter(p)) {
 		if (TestClearPageHWPoison(p))
 			num_poisoned_pages_dec();
 		unlock_page(p);
 		put_page(p);
-		return 0;
+		goto unlock_mutex;
 	}
 
 	if (!PageTransTail(p) && !PageLRU(p))
@@ -1543,7 +1552,7 @@ int memory_failure(unsigned long pfn, int flags)
 	if (!hwpoison_user_mappings(p, pfn, flags, &p)) {
 		action_result(pfn, MF_MSG_UNMAP_FAILED, MF_IGNORED);
 		res = -EBUSY;
-		goto out;
+		goto unlock_page;
 	}
 
 	/*
@@ -1552,13 +1561,15 @@ int memory_failure(unsigned long pfn, int flags)
 	if (PageLRU(p) && !PageSwapCache(p) && p->mapping == NULL) {
 		action_result(pfn, MF_MSG_TRUNCATED_LRU, MF_IGNORED);
 		res = -EBUSY;
-		goto out;
+		goto unlock_page;
 	}
 
 identify_page_state:
 	res = identify_page_state(pfn, p, page_flags);
-out:
+unlock_page:
 	unlock_page(p);
+unlock_mutex:
+	mutex_unlock(&mf_mutex);
 	return res;
 }
 EXPORT_SYMBOL_GPL(memory_failure);
-- 
2.25.1

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

[Naoya Horiguchi]

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

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;
 	}
 
-- 
2.25.1

[PATCH v3 3/3] mm,hwpoison: add kill_accessing_process() to find error virtual address

[Naoya Horiguchi]

From: Naoya Horiguchi <naoya.horiguchi@nec.com>

The previous patch solves the infinite MCE loop issue when multiple
MCE events races.  The remaining issue is to make sure that all threads
processing Action Required MCEs send to the current processes the
SIGBUS with the proper virtual address and the error size.

This patch suggests to do page table walk to find the error virtual
address.  If we find multiple virtual addresses in walking, we now can't
determine which one is correct, so we fall back to sending SIGBUS in
kill_me_maybe() without error info as we do now.  This corner case needs
to be solved in the future.

Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Tested-by: Aili Yao <yaoaili@kingsoft.com>
---
change log v1 -> v2:
- initialize local variables in check_hwpoisoned_entry() and
  hwpoison_pte_range()
- fix and improve logic to calculate error address offset.
---
 arch/x86/kernel/cpu/mce/core.c |  13 ++-
 include/linux/swapops.h        |   5 ++
 mm/memory-failure.c            | 143 ++++++++++++++++++++++++++++++++-
 3 files changed, 158 insertions(+), 3 deletions(-)

diff --git v5.12-rc8/arch/x86/kernel/cpu/mce/core.c v5.12-rc8_patched/arch/x86/kernel/cpu/mce/core.c
index 7962355436da..3ce23445a48c 100644
--- v5.12-rc8/arch/x86/kernel/cpu/mce/core.c
+++ v5.12-rc8_patched/arch/x86/kernel/cpu/mce/core.c
@@ -1257,19 +1257,28 @@ 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;
+	int ret;
 
 	pr_err("Uncorrected hardware memory error in user-access at %llx", p->mce_addr);
 
 	if (!p->mce_ripv)
 		flags |= MF_MUST_KILL;
 
-	if (!memory_failure(p->mce_addr >> PAGE_SHIFT, flags) &&
-	    !(p->mce_kflags & MCE_IN_KERNEL_COPYIN)) {
+	ret = memory_failure(p->mce_addr >> PAGE_SHIFT, flags);
+	if (!ret && !(p->mce_kflags & MCE_IN_KERNEL_COPYIN)) {
 		set_mce_nospec(p->mce_addr >> PAGE_SHIFT, p->mce_whole_page);
 		sync_core();
 		return;
 	}
 
+	/*
+	 * -EHWPOISON from memory_failure() means that it already sent SIGBUS
+	 * to the current process with the proper error info, so no need to
+	 * send it here again.
+	 */
+	if (ret == -EHWPOISON)
+		return;
+
 	if (p->mce_vaddr != (void __user *)-1l) {
 		force_sig_mceerr(BUS_MCEERR_AR, p->mce_vaddr, PAGE_SHIFT);
 	} else {
diff --git v5.12-rc8/include/linux/swapops.h v5.12-rc8_patched/include/linux/swapops.h
index d9b7c9132c2f..98ea67fcf360 100644
--- v5.12-rc8/include/linux/swapops.h
+++ v5.12-rc8_patched/include/linux/swapops.h
@@ -323,6 +323,11 @@ static inline int is_hwpoison_entry(swp_entry_t entry)
 	return swp_type(entry) == SWP_HWPOISON;
 }
 
+static inline unsigned long hwpoison_entry_to_pfn(swp_entry_t entry)
+{
+	return swp_offset(entry);
+}
+
 static inline void num_poisoned_pages_inc(void)
 {
 	atomic_long_inc(&num_poisoned_pages);
diff --git v5.12-rc8/mm/memory-failure.c v5.12-rc8_patched/mm/memory-failure.c
index 39d0ff0339b9..7cc563e1770a 100644
--- v5.12-rc8/mm/memory-failure.c
+++ v5.12-rc8_patched/mm/memory-failure.c
@@ -56,6 +56,7 @@
 #include <linux/kfifo.h>
 #include <linux/ratelimit.h>
 #include <linux/page-isolation.h>
+#include <linux/pagewalk.h>
 #include "internal.h"
 #include "ras/ras_event.h"
 
@@ -554,6 +555,141 @@ static void collect_procs(struct page *page, struct list_head *tokill,
 		collect_procs_file(page, tokill, force_early);
 }
 
+struct hwp_walk {
+	struct to_kill tk;
+	unsigned long pfn;
+	int flags;
+};
+
+static int set_to_kill(struct to_kill *tk, unsigned long addr, short shift)
+{
+	/* Abort pagewalk when finding multiple mappings to the error page. */
+	if (tk->addr)
+		return 1;
+	tk->addr = addr;
+	tk->size_shift = shift;
+	return 0;
+}
+
+static int check_hwpoisoned_entry(pte_t pte, unsigned long addr, short shift,
+				unsigned long poisoned_pfn, struct to_kill *tk)
+{
+	unsigned long pfn = 0;
+
+	if (pte_present(pte)) {
+		pfn = pte_pfn(pte);
+	} else {
+		swp_entry_t swp = pte_to_swp_entry(pte);
+
+		if (is_hwpoison_entry(swp))
+			pfn = hwpoison_entry_to_pfn(swp);
+	}
+
+	if (!pfn || pfn != poisoned_pfn)
+		return 0;
+
+	return set_to_kill(tk, addr, shift);
+}
+
+static int hwpoison_pte_range(pmd_t *pmdp, unsigned long addr,
+			      unsigned long end, struct mm_walk *walk)
+{
+	struct hwp_walk *hwp = (struct hwp_walk *)walk->private;
+	int ret = 0;
+	pte_t *ptep;
+	spinlock_t *ptl;
+
+	ptl = pmd_trans_huge_lock(pmdp, walk->vma);
+	if (ptl) {
+		pmd_t pmd = *pmdp;
+
+		if (pmd_present(pmd)) {
+			unsigned long pfn = pmd_pfn(pmd);
+
+			if (pfn <= hwp->pfn && hwp->pfn < pfn + HPAGE_PMD_NR) {
+				unsigned long hwpoison_vaddr = addr +
+					((hwp->pfn - pfn) << PAGE_SHIFT);
+
+				ret = set_to_kill(&hwp->tk, hwpoison_vaddr,
+						  PAGE_SHIFT);
+			}
+		}
+		spin_unlock(ptl);
+		goto out;
+	}
+
+	if (pmd_trans_unstable(pmdp))
+		goto out;
+
+	ptep = pte_offset_map_lock(walk->vma->vm_mm, pmdp, addr, &ptl);
+	for (; addr != end; ptep++, addr += PAGE_SIZE) {
+		ret = check_hwpoisoned_entry(*ptep, addr, PAGE_SHIFT,
+					     hwp->pfn, &hwp->tk);
+		if (ret == 1)
+			break;
+	}
+	pte_unmap_unlock(ptep - 1, ptl);
+out:
+	cond_resched();
+	return ret;
+}
+
+#ifdef CONFIG_HUGETLB_PAGE
+static int hwpoison_hugetlb_range(pte_t *ptep, unsigned long hmask,
+			    unsigned long addr, unsigned long end,
+			    struct mm_walk *walk)
+{
+	struct hwp_walk *hwp = (struct hwp_walk *)walk->private;
+	pte_t pte = huge_ptep_get(ptep);
+	struct hstate *h = hstate_vma(walk->vma);
+
+	return check_hwpoisoned_entry(pte, addr, huge_page_shift(h),
+				      hwp->pfn, &hwp->tk);
+}
+#else
+#define hwpoison_hugetlb_range	NULL
+#endif
+
+static struct mm_walk_ops hwp_walk_ops = {
+	.pmd_entry = hwpoison_pte_range,
+	.hugetlb_entry = hwpoison_hugetlb_range,
+};
+
+/*
+ * Sends SIGBUS to the current process with the error info.
+ *
+ * This function is intended to handle "Action Required" MCEs on already
+ * hardware poisoned pages. They could happen, for example, when
+ * memory_failure() failed to unmap the error page at the first call, or
+ * when multiple local machine checks happened on different CPUs.
+ *
+ * MCE handler currently has no easy access to the error virtual address,
+ * so this function walks page table to find it. One challenge on this is
+ * to reliably get the proper virual address of the error to report to
+ * applications via SIGBUS. A process could map a page multiple times to
+ * different virtual addresses, then we now have no way to tell which virtual
+ * address was accessed when the Action Required MCE was generated.
+ * So in such a corner case, we now give up and fall back to sending SIGBUS
+ * with no error info.
+ */
+static int kill_accessing_process(struct task_struct *p, unsigned long pfn,
+				  int flags)
+{
+	int ret;
+	struct hwp_walk priv = {
+		.pfn = pfn,
+	};
+	priv.tk.tsk = p;
+
+	mmap_read_lock(p->mm);
+	ret = walk_page_range(p->mm, 0, TASK_SIZE_MAX, &hwp_walk_ops,
+			      (void *)&priv);
+	if (!ret && priv.tk.addr)
+		kill_proc(&priv.tk, pfn, flags);
+	mmap_read_unlock(p->mm);
+	return ret ? -EFAULT : -EHWPOISON;
+}
+
 static const char *action_name[] = {
 	[MF_IGNORED] = "Ignored",
 	[MF_FAILED] = "Failed",
@@ -1228,7 +1364,10 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
 	if (TestSetPageHWPoison(head)) {
 		pr_err("Memory failure: %#lx: already hardware poisoned\n",
 		       pfn);
-		return -EHWPOISON;
+		res = -EHWPOISON;
+		if (flags & MF_ACTION_REQUIRED)
+			res = kill_accessing_process(current, page_to_pfn(head), flags);
+		return res;
 	}
 
 	num_poisoned_pages_inc();
@@ -1438,6 +1577,8 @@ int memory_failure(unsigned long pfn, int flags)
 		pr_err("Memory failure: %#lx: already hardware poisoned\n",
 			pfn);
 		res = -EHWPOISON;
+		if (flags & MF_ACTION_REQUIRED)
+			res = kill_accessing_process(current, pfn, flags);
 		goto unlock_mutex;
 	}
 
-- 
2.25.1

Re: [PATCH v3 1/3] mm/memory-failure: Use a mutex to avoid memory_failure() races

[Borislav Petkov]

On Wed, Apr 21, 2021 at 09:57:26AM +0900, Naoya Horiguchi wrote:
> From: Tony Luck <tony.luck@intel.com>
> 
> There can be races when multiple CPUs consume poison from the same
> page. The first into memory_failure() atomically sets the HWPoison
> page flag and begins hunting for tasks that map this page. Eventually
> it invalidates those mappings and may send a SIGBUS to the affected
> tasks.
> 
> But while all that work is going on, other CPUs see a "success"
> return code from memory_failure() and so they believe the error
> has been handled and continue executing.
> 
> Fix by wrapping most of the internal parts of memory_failure() in
> a mutex.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  mm/memory-failure.c | 37 ++++++++++++++++++++++++-------------
>  1 file changed, 24 insertions(+), 13 deletions(-)

Reviewed-by: Borislav Petkov <bp@suse.de>

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 3/3] mm,hwpoison: add kill_accessing_process() to find error virtual address

[Borislav Petkov]

On Wed, Apr 21, 2021 at 09:57:28AM +0900, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> The previous patch solves the infinite MCE loop issue when multiple

"previous patch" has no meaning when it is in git.

> MCE events races.  The remaining issue is to make sure that all threads

	    "race."

> processing Action Required MCEs send to the current processes the

s/the //

> SIGBUS with the proper virtual address and the error size.
> 
> This patch suggests to do page table walk to find the error virtual

Avoid having "This patch" or "This commit" in the commit message. It is
tautologically useless.

Also, do

$ git grep 'This patch' Documentation/process

for more details.

> address.  If we find multiple virtual addresses in walking, we now can't

Who's "we"?				during the pagetable walk

> determine which one is correct, so we fall back to sending SIGBUS in
> kill_me_maybe() without error info as we do now.  This corner case needs
> to be solved in the future.

Solved how? If you can't map which error comes from which process, you
can't do anything here. You could send SIGBUS to all but you might
injure some innocent bystanders this way.

Just code structuring suggestions below - mm stuff is for someone else
to review properly.

> +static int hwpoison_pte_range(pmd_t *pmdp, unsigned long addr,
> +			      unsigned long end, struct mm_walk *walk)
> +{
> +	struct hwp_walk *hwp = (struct hwp_walk *)walk->private;
> +	int ret = 0;
> +	pte_t *ptep;
> +	spinlock_t *ptl;
> +
> +	ptl = pmd_trans_huge_lock(pmdp, walk->vma);
> +	if (ptl) {

Save yourself an indentation level:

	if (!ptl)
		goto unlock;

> +		pmd_t pmd = *pmdp;
> +
> +		if (pmd_present(pmd)) {

... ditto...

> +			unsigned long pfn = pmd_pfn(pmd);
> +
> +			if (pfn <= hwp->pfn && hwp->pfn < pfn + HPAGE_PMD_NR) {
> +				unsigned long hwpoison_vaddr = addr +
> +					((hwp->pfn - pfn) << PAGE_SHIFT);

... which will allow you to not break those.

> +
> +				ret = set_to_kill(&hwp->tk, hwpoison_vaddr,
> +						  PAGE_SHIFT);
> +			}
> +		}
> +		spin_unlock(ptl);
> +		goto out;
> +	}
> +
> +	if (pmd_trans_unstable(pmdp))
> +		goto out;
> +
> +	ptep = pte_offset_map_lock(walk->vma->vm_mm, pmdp, addr, &ptl);
> +	for (; addr != end; ptep++, addr += PAGE_SIZE) {
> +		ret = check_hwpoisoned_entry(*ptep, addr, PAGE_SHIFT,
> +					     hwp->pfn, &hwp->tk);
> +		if (ret == 1)
> +			break;
> +	}
> +	pte_unmap_unlock(ptep - 1, ptl);
> +out:
> +	cond_resched();
> +	return ret;
> +}


-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

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

[HORIGUCHI NAOYA(堀口 直也)]

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

Re: [PATCH v3 3/3] mm,hwpoison: add kill_accessing_process() to find error virtual address

[HORIGUCHI NAOYA(堀口 直也)]

On Thu, Apr 22, 2021 at 07:02:13PM +0200, Borislav Petkov wrote:
> On Wed, Apr 21, 2021 at 09:57:28AM +0900, Naoya Horiguchi wrote:
> > From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> > 
> > The previous patch solves the infinite MCE loop issue when multiple
> 
> "previous patch" has no meaning when it is in git.
> 
> > MCE events races.  The remaining issue is to make sure that all threads
> 
> 	    "race."
> 
> > processing Action Required MCEs send to the current processes the
> 
> s/the //

I'll fix these grammar errors.

> 
> > SIGBUS with the proper virtual address and the error size.
> > 
> > This patch suggests to do page table walk to find the error virtual
> 
> Avoid having "This patch" or "This commit" in the commit message. It is
> tautologically useless.
> 
> Also, do
> 
> $ git grep 'This patch' Documentation/process
> 
> for more details.

I didn't know the following rule:

    Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
    instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
    to do frotz", as if you are giving orders to the codebase to change
    its behaviour.

I'll follow this in my future post.

> 
> > address.  If we find multiple virtual addresses in walking, we now can't
> 
> Who's "we"?				during the pagetable walk

I wrongly abused rhetorical "we". I'll change this sentence in passive form.

> 
> > determine which one is correct, so we fall back to sending SIGBUS in
> > kill_me_maybe() without error info as we do now.  This corner case needs
> > to be solved in the future.
> 
> Solved how?

I don't know exactly.  MCE subsystem seems to have code extracting linear
address, so I wonder that that could be used as a hint to memory_failure()
to find the proper virtual address.

> If you can't map which error comes from which process, you
> can't do anything here. You could send SIGBUS to all but you might
> injure some innocent bystanders this way.

The situation in question is caused by action required MCE, so
we know which process we should send SIGBUS to. So if we choose
to send SIGBUS to all, no innocent bystanders would be affected.
But when the process have multiple virtual addresses associated
with the error physical address, the process receives multiple
SIGBUSs and all but one have wrong value in si_addr in siginfo_t,
so that's confusing.

> 
> Just code structuring suggestions below - mm stuff is for someone else
> to review properly.

Thank you, I'll update with them.

- Naoya Horiguchi

> 
> > +static int hwpoison_pte_range(pmd_t *pmdp, unsigned long addr,
> > +			      unsigned long end, struct mm_walk *walk)
> > +{
> > +	struct hwp_walk *hwp = (struct hwp_walk *)walk->private;
> > +	int ret = 0;
> > +	pte_t *ptep;
> > +	spinlock_t *ptl;
> > +
> > +	ptl = pmd_trans_huge_lock(pmdp, walk->vma);
> > +	if (ptl) {
> 
> Save yourself an indentation level:
> 
> 	if (!ptl)
> 		goto unlock;
> 
> > +		pmd_t pmd = *pmdp;
> > +
> > +		if (pmd_present(pmd)) {
> 
> ... ditto...
> 
> > +			unsigned long pfn = pmd_pfn(pmd);
> > +
> > +			if (pfn <= hwp->pfn && hwp->pfn < pfn + HPAGE_PMD_NR) {
> > +				unsigned long hwpoison_vaddr = addr +
> > +					((hwp->pfn - pfn) << PAGE_SHIFT);
> 
> ... which will allow you to not break those.
> 
> > +
> > +				ret = set_to_kill(&hwp->tk, hwpoison_vaddr,
> > +						  PAGE_SHIFT);
> > +			}
> > +		}
> > +		spin_unlock(ptl);
> > +		goto out;
> > +	}
> > +
> > +	if (pmd_trans_unstable(pmdp))
> > +		goto out;
> > +
> > +	ptep = pte_offset_map_lock(walk->vma->vm_mm, pmdp, addr, &ptl);
> > +	for (; addr != end; ptep++, addr += PAGE_SIZE) {
> > +		ret = check_hwpoisoned_entry(*ptep, addr, PAGE_SHIFT,
> > +					     hwp->pfn, &hwp->tk);
> > +		if (ret == 1)
> > +			break;
> > +	}
> > +	pte_unmap_unlock(ptep - 1, ptl);
> > +out:
> > +	cond_resched();
> > +	return ret;
> > +}
> 
> 
> -- 
> Regards/Gruss,
>     Boris.
> 
> https://people.kernel.org/tglx/notes-about-netiquette
> 

Re: [PATCH v3 3/3] mm,hwpoison: add kill_accessing_process() to find error virtual address

[Borislav Petkov]

On Fri, Apr 23, 2021 at 02:18:34AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
> I don't know exactly.  MCE subsystem seems to have code extracting linear
> address, so I wonder that that could be used as a hint to memory_failure()
> to find the proper virtual address.

See "Table 15-3. Address Mode in IA32_MCi_MISC[8:6]" in the SDM -
apparently it can report all kinds of address types, depending on the hw
incarnation or MCA bank type or whatnot. Tony knows :)

> The situation in question is caused by action required MCE, so
> we know which process we should send SIGBUS to. So if we choose
> to send SIGBUS to all, no innocent bystanders would be affected.
> But when the process have multiple virtual addresses associated
> with the error physical address, the process receives multiple
> SIGBUSs and all but one have wrong value in si_addr in siginfo_t,
> so that's confusing.

Is that scenario real or hypothetical?

Because I'd expect that if we send it a SIGBUS and we poison that page,
then all the VAs mapping it will have to handle the situation that that
page has been poisoned and pulled from under them.

So from a hw perspective, there won't be any more accesses to the faulty
physical page.

In a perfect world, that is...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v3 3/3] mm,hwpoison: add kill_accessing_process() to find error virtual address

[HORIGUCHI NAOYA(堀口 直也)]

On Fri, Apr 23, 2021 at 01:57:25PM +0200, Borislav Petkov wrote:
> On Fri, Apr 23, 2021 at 02:18:34AM +0000, HORIGUCHI NAOYA(堀口 直也) wrote:
> > I don't know exactly.  MCE subsystem seems to have code extracting linear
> > address, so I wonder that that could be used as a hint to memory_failure()
> > to find the proper virtual address.
> 
> See "Table 15-3. Address Mode in IA32_MCi_MISC[8:6]" in the SDM -
> apparently it can report all kinds of address types, depending on the hw
> incarnation or MCA bank type or whatnot. Tony knows :)

"15.9.3.2 Architecturally Defined SRAR Errors" says that the register
is supposed to have physical address.

    For both the data load and instruction fetch errors, the ADDRV and MISCV
    flags in the IA32_MCi_STATUS register are set to indicate that the offending
    physical address information is available from the IA32_MCi_MISC and the
    IA32_MCi_ADDR registers.

> > The situation in question is caused by action required MCE, so
> > we know which process we should send SIGBUS to. So if we choose
> > to send SIGBUS to all, no innocent bystanders would be affected.
> > But when the process have multiple virtual addresses associated
> > with the error physical address, the process receives multiple
> > SIGBUSs and all but one have wrong value in si_addr in siginfo_t,
> > so that's confusing.
> 
> Is that scenario real or hypothetical?
> 
> Because I'd expect that if we send it a SIGBUS and we poison that page,
> then all the VAs mapping it will have to handle the situation that that
> page has been poisoned and pulled from under them.

IIUC, the above should be done by the first MCE handling. In "already
hwpoisoned" case, the page has already been poisoned and all mapping for it
should be already unmapped, then what we need additionally is to send SIGBUS
to report to the application that it should take some action or abort
immediately.

Thanks,
Naoya Horiguchi

RE: [PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[Luck, Tony]

> I updated the series based on previous feedbacks/discussions.

Tested-by: Tony Luck <tony.luck@intel.com>

-Tony

RE: [PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[Luck, Tony]

>> I updated the series based on previous feedbacks/discussions.
>
> Tested-by: Tony Luck <tony.luck@intel.com>

Maybe this series should get a "Cc: stable" tag too?

I don't have a specific commit to provide for Fixes:, but it deals
with an issue where sometimes processes return from the machine
check handler and hit the same machine check again.

It's hard[1] to hit in v5.8, but somehow gets progressively easier in
newer versions.

-Tony

[1] Initially I thought it didn't happen at all in v5.8. Then wasted a bunch
of time trying to bisect. But my "git bisect good" responses turned out to
be randomly assigned when I just hadn't tried enough times to make the
problem show up.

Re: [PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[HORIGUCHI NAOYA(堀口 直也)]

On Fri, May 07, 2021 at 03:37:07AM +0000, Luck, Tony wrote:
> >> I updated the series based on previous feedbacks/discussions.
> >
> > Tested-by: Tony Luck <tony.luck@intel.com>
> 
> Maybe this series should get a "Cc: stable" tag too?

The second patch (I squashed 2/3 and 3/3 into one in v4) is a little
too large (more than 100 lines of change), which doesn't meet stable rule.
But the first patch looks suitable to me for stable.

> 
> I don't have a specific commit to provide for Fixes:, but it deals
> with an issue where sometimes processes return from the machine
> check handler and hit the same machine check again.

If we consider that this issue is visible after LMCE is supported,
the following commit could be the target of Fixes tag.

    commit 243d657eaf540db882f73497060da5a4f7d86a90
    Author: Ashok Raj <ashok.raj@intel.com>
    Date:   Thu Jun 4 18:55:24 2015 +0200

        x86/mce: Handle Local MCE events

but according to your test result, any other factor might affect when
the issue actually got reproducible.

> 
> It's hard[1] to hit in v5.8, but somehow gets progressively easier in
> newer versions.
> 
> -Tony
> 
> [1] Initially I thought it didn't happen at all in v5.8. Then wasted a bunch
> of time trying to bisect. But my "git bisect good" responses turned out to
> be randomly assigned when I just hadn't tried enough times to make the
> problem show up.

Thanks for time and effort for testing.

- Naoya

Re: [PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[Aili Yao]

On Fri, 7 May 2021 05:24:22 +0000
HORIGUCHI NAOYA(堀口　直也) <naoya.horiguchi@nec.com> wrote:

> On Fri, May 07, 2021 at 03:37:07AM +0000, Luck, Tony wrote:
> > >> I updated the series based on previous feedbacks/discussions.  
> > >
> > > Tested-by: Tony Luck <tony.luck@intel.com>  
> > 
> > Maybe this series should get a "Cc: stable" tag too?  
> 
> The second patch (I squashed 2/3 and 3/3 into one in v4) is a little
> too large (more than 100 lines of change), which doesn't meet stable rule.
> But the first patch looks suitable to me for stable.

Before cc:stable, would you please do one stress test first?
It failed in my server, but I didn't dig into it, maybe the fail is meaningless.
Just a small suggestion.

Thanks!
Aili Yao

RE: [PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[Luck, Tony]

> Before cc:stable, would you please do one stress test first?
> It failed in my server, but I didn't dig into it, maybe the fail is meaningless.
> Just a small suggestion.

Upstream plus these three patches passed an overnight 8-hour stress test
on four machines here (running a test that's been failing with hung kernels
and kernel crashes w/o these patches).

What were the symptoms of your failed server?

-Tony

Re: [PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[Aili Yao]

On Fri, 7 May 2021 18:02:13 +0000
"Luck, Tony" <tony.luck@intel.com> wrote:

> > Before cc:stable, would you please do one stress test first?
> > It failed in my server, but I didn't dig into it, maybe the fail is meaningless.
> > Just a small suggestion.  
> 
> Upstream plus these three patches passed an overnight 8-hour stress test
> on four machines here (running a test that's been failing with hung kernels
> and kernel crashes w/o these patches).
> 
> What were the symptoms of your failed server?
> 

Sorry, I am not sure if the stress test in my server is same with yours.
Usually, I will do one RAS stress test from mce-test/cases/stress before post something.
Maybe this test is not general useful or you have tested it and passed or you may think
the test is not proper though or you tested and you don't think it's one real issue,
then just ignore it please.

Thanks!
Aili Yao

RE: [PATCH v3 0/3] mm,hwpoison: fix sending SIGBUS for Action Required MCE

[Luck, Tony]

> Sorry, I am not sure if the stress test in my server is same with yours.

It's better if you test different things ... that's how we get better coverage.

> Usually, I will do one RAS stress test from mce-test/cases/stress before post something.
> Maybe this test is not general useful or you have tested it and passed or you may think
> the test is not proper though or you tested and you don't think it's one real issue,
> then just ignore it please.

I haven't re-run the mce-test cases for a while. The test that was run on these patches
involves a multi-threaded application running on all logical CPUs. When poison is injected
many of the CPUs can hit the poison close together. So it's a good stress test for the new
series.

-Tony