[PATCH v8 0/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[PATCH v8 0/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[Naoya Horiguchi]

Hi,

I updated v7 (https://lore.kernel.org/linux-mm/20220407112929.1344748-1-naoya.horiguchi@linux.dev/)
based on the comment from Miaohe Lin.  Now I have a few additional patches.
Any comments and suggestions are welcomed.

Thanks,
Naoya Horiguchi
---
Summary:

Naoya Horiguchi (3):
      mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()
      mm/hwpoison: put page in already hwpoisoned case with MF_COUNT_INCREASED
      Revert "mm/memory-failure.c: fix race with changing page compound again"

 include/linux/hugetlb.h |   6 ++
 include/linux/mm.h      |   9 ++-
 include/ras/ras_event.h |   1 -
 mm/hugetlb.c            |  10 ++++
 mm/memory-failure.c     | 152 ++++++++++++++++++++++++++++++++----------------
 5 files changed, 126 insertions(+), 52 deletions(-)

[PATCH v8 1/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[Naoya Horiguchi]

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

There is a race condition between memory_failure_hugetlb() and hugetlb
free/demotion, which causes setting PageHWPoison flag on the wrong page.
The one simple result is that wrong processes can be killed, but another
(more serious) one is that the actual error is left unhandled, so no one
prevents later access to it, and that might lead to more serious results
like consuming corrupted data.

Think about the below race window:

  CPU 1                                   CPU 2
  memory_failure_hugetlb
  struct page *head = compound_head(p);
                                          hugetlb page might be freed to
                                          buddy, or even changed to another
                                          compound page.

  get_hwpoison_page -- page is not what we want now...

The current code first does prechecks roughly and then reconfirms
after taking refcount, but it's found that it makes code overly
complicated, so move the prechecks in a single hugetlb_lock range.

A newly introduced function, try_memory_failure_hugetlb(), always
takes hugetlb_lock (even for non-hugetlb pages).  That can be
improved, but memory_failure() is rare in principle, so should
not be a big problem.

Fixes: 761ad8d7c7b5 ("mm: hwpoison: introduce memory_failure_hugetlb()")
Reported-by: Mike Kravetz <mike.kravetz@oracle.com>
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Cc: stable@vger.kernel.org
---
ChangeLog v7 -> v8
- move hwpoison_filter() within page locking.

ChangeLog v6 -> v7:
- Move lock_page() to try_memory_failure_hugetlb()
  (based on bug report from Dan Carpenter)
- Add Fixes: tag and CC to stable.

ChangeLog v5 -> v6:
- Moved racy precheck operations into hugetlb_lock (based on
  Mike's comment).
- rebased onto v5.18-rc1.
- dropped CC to stable.

ChangeLog v4 -> v5:
- call TestSetPageHWPoison() when page_handle_poison() fails.
- call TestSetPageHWPoison() for unhandlable cases (MF_MSG_UNKNOWN and
  MF_MSG_DIFFERENT_PAGE_SIZE).
- Set PageHWPoison on the head page only when the error page is surely
  a hugepage, otherwise set the flag on the raw page.
- rebased onto v5.17-rc8-mmotm-2022-03-16-17-42

ChangeLog v3 -> v4:
- squash with "mm/memory-failure.c: fix race with changing page
  compound again".
- update patch subject and description based on it.

ChangeLog v2 -> v3:
- rename the patch because page lock is not the primary factor to
  solve the reported issue.
- updated description in the same manner.
- call page_handle_poison() instead of __page_handle_poison() for
  free hugepage case.
- reorder put_page and unlock_page (thanks to Miaohe Lin)

ChangeLog v1 -> v2:
- pass subpage to get_hwpoison_huge_page() instead of head page.
- call compound_head() in hugetlb_lock to avoid race with hugetlb
  demotion/free.
---
 include/linux/hugetlb.h |   6 ++
 include/linux/mm.h      |   8 +++
 mm/hugetlb.c            |  10 +++
 mm/memory-failure.c     | 145 ++++++++++++++++++++++++++++------------
 4 files changed, 127 insertions(+), 42 deletions(-)

diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 53c1b6082a4c..ac2a1d758a80 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -169,6 +169,7 @@ long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
 						long freed);
 bool isolate_huge_page(struct page *page, struct list_head *list);
 int get_hwpoison_huge_page(struct page *page, bool *hugetlb);
+int get_huge_page_for_hwpoison(unsigned long pfn, int flags);
 void putback_active_hugepage(struct page *page);
 void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason);
 void free_huge_page(struct page *page);
@@ -378,6 +379,11 @@ static inline int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
 	return 0;
 }
 
+static inline int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
+{
+	return 0;
+}
+
 static inline void putback_active_hugepage(struct page *page)
 {
 }
diff --git a/include/linux/mm.h b/include/linux/mm.h
index e34edb775334..9f44254af8ce 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3197,6 +3197,14 @@ extern int sysctl_memory_failure_recovery;
 extern void shake_page(struct page *p);
 extern atomic_long_t num_poisoned_pages __read_mostly;
 extern int soft_offline_page(unsigned long pfn, int flags);
+#ifdef CONFIG_MEMORY_FAILURE
+extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags);
+#else
+static inline int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
+{
+	return 0;
+}
+#endif
 
 #ifndef arch_memory_failure
 static inline int arch_memory_failure(unsigned long pfn, int flags)
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f8ca7cca3c1a..3fc721789743 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6785,6 +6785,16 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
 	return ret;
 }
 
+int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
+{
+	int ret;
+
+	spin_lock_irq(&hugetlb_lock);
+	ret = __get_huge_page_for_hwpoison(pfn, flags);
+	spin_unlock_irq(&hugetlb_lock);
+	return ret;
+}
+
 void putback_active_hugepage(struct page *page)
 {
 	spin_lock_irq(&hugetlb_lock);
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index dcb6bb9cf731..2020944398c9 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1498,50 +1498,113 @@ static int try_to_split_thp_page(struct page *page, const char *msg)
 	return 0;
 }
 
-static int memory_failure_hugetlb(unsigned long pfn, int flags)
+/*
+ * Called from hugetlb code with hugetlb_lock held.
+ *
+ * Return values:
+ *   0             - free hugepage
+ *   1             - in-use hugepage
+ *   2             - not a hugepage
+ *   -EBUSY        - the hugepage is busy (try to retry)
+ *   -EHWPOISON    - the hugepage is already hwpoisoned
+ */
+int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
+{
+	struct page *page = pfn_to_page(pfn);
+	struct page *head = compound_head(page);
+	int ret = 2;	/* fallback to normal page handling */
+	bool count_increased = false;
+
+	if (!PageHeadHuge(head))
+		goto out;
+
+	if (flags & MF_COUNT_INCREASED) {
+		ret = 1;
+		count_increased = true;
+	} else if (HPageFreed(head) || HPageMigratable(head)) {
+		ret = get_page_unless_zero(head);
+		if (ret)
+			count_increased = true;
+	} else {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	if (TestSetPageHWPoison(head)) {
+		ret = -EHWPOISON;
+		goto out;
+	}
+
+	return ret;
+out:
+	if (count_increased)
+		put_page(head);
+	return ret;
+}
+
+#ifdef CONFIG_HUGETLB_PAGE
+/*
+ * Taking refcount of hugetlb pages needs extra care about race conditions
+ * with basic operations like hugepage allocation/free/demotion.
+ * So some of prechecks for hwpoison (pinning, and testing/setting
+ * PageHWPoison) should be done in single hugetlb_lock range.
+ */
+static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
 {
-	struct page *p = pfn_to_page(pfn);
-	struct page *head = compound_head(p);
 	int res;
+	struct page *p = pfn_to_page(pfn);
+	struct page *head;
 	unsigned long page_flags;
+	bool retry = true;
 
-	if (TestSetPageHWPoison(head)) {
-		pr_err("Memory failure: %#lx: already hardware poisoned\n",
-		       pfn);
-		res = -EHWPOISON;
-		if (flags & MF_ACTION_REQUIRED)
+	*hugetlb = 1;
+retry:
+	res = get_huge_page_for_hwpoison(pfn, flags);
+	if (res == 2) { /* fallback to normal page handling */
+		*hugetlb = 0;
+		return 0;
+	} else if (res == -EHWPOISON) {
+		pr_err("Memory failure: %#lx: already hardware poisoned\n", pfn);
+		if (flags & MF_ACTION_REQUIRED) {
+			head = compound_head(p);
 			res = kill_accessing_process(current, page_to_pfn(head), flags);
+		}
 		return res;
+	} else if (res == -EBUSY) {
+		if (retry) {
+			retry = false;
+			goto retry;
+		}
+		action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED);
+		return res;
+	}
+
+	head = compound_head(p);
+	lock_page(head);
+
+	if (hwpoison_filter(p)) {
+		ClearPageHWPoison(head);
+		res = -EOPNOTSUPP;
+		goto out;
 	}
 
 	num_poisoned_pages_inc();
 
-	if (!(flags & MF_COUNT_INCREASED)) {
-		res = get_hwpoison_page(p, flags);
-		if (!res) {
-			lock_page(head);
-			if (hwpoison_filter(p)) {
-				if (TestClearPageHWPoison(head))
-					num_poisoned_pages_dec();
-				unlock_page(head);
-				return -EOPNOTSUPP;
-			}
-			unlock_page(head);
-			res = MF_FAILED;
-			if (__page_handle_poison(p)) {
-				page_ref_inc(p);
-				res = MF_RECOVERED;
-			}
-			action_result(pfn, MF_MSG_FREE_HUGE, res);
-			return res == MF_RECOVERED ? 0 : -EBUSY;
-		} else if (res < 0) {
-			action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED);
-			return -EBUSY;
+	/*
+	 * Handling free hugepage.  The possible race with hugepage allocation
+	 * or demotion can be prevented by PageHWPoison flag.
+	 */
+	if (res == 0) {
+		unlock_page(head);
+		res = MF_FAILED;
+		if (__page_handle_poison(p)) {
+			page_ref_inc(p);
+			res = MF_RECOVERED;
 		}
+		action_result(pfn, MF_MSG_FREE_HUGE, res);
+		return res == MF_RECOVERED ? 0 : -EBUSY;
 	}
 
-	lock_page(head);
-
 	/*
 	 * The page could have changed compound pages due to race window.
 	 * If this happens just bail out.
@@ -1554,14 +1617,6 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
 
 	page_flags = head->flags;
 
-	if (hwpoison_filter(p)) {
-		if (TestClearPageHWPoison(head))
-			num_poisoned_pages_dec();
-		put_page(p);
-		res = -EOPNOTSUPP;
-		goto out;
-	}
-
 	/*
 	 * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
 	 * simply disable it. In order to make it work properly, we need
@@ -1588,6 +1643,12 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
 	unlock_page(head);
 	return res;
 }
+#else
+static inline int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
+{
+	return 0;
+}
+#endif
 
 static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 		struct dev_pagemap *pgmap)
@@ -1712,6 +1773,7 @@ int memory_failure(unsigned long pfn, int flags)
 	int res = 0;
 	unsigned long page_flags;
 	bool retry = true;
+	int hugetlb = 0;
 
 	if (!sysctl_memory_failure_recovery)
 		panic("Memory failure on page %lx", pfn);
@@ -1739,10 +1801,9 @@ int memory_failure(unsigned long pfn, int flags)
 	}
 
 try_again:
-	if (PageHuge(p)) {
-		res = memory_failure_hugetlb(pfn, flags);
+	res = try_memory_failure_hugetlb(pfn, flags, &hugetlb);
+	if (hugetlb)
 		goto unlock_mutex;
-	}
 
 	if (TestSetPageHWPoison(p)) {
 		pr_err("Memory failure: %#lx: already hardware poisoned\n",
-- 
2.25.1

Re: [PATCH v8 1/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[Miaohe Lin]

On 2022/4/8 21:53, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> There is a race condition between memory_failure_hugetlb() and hugetlb
> free/demotion, which causes setting PageHWPoison flag on the wrong page.
> The one simple result is that wrong processes can be killed, but another
> (more serious) one is that the actual error is left unhandled, so no one
> prevents later access to it, and that might lead to more serious results
> like consuming corrupted data.
> 
> Think about the below race window:
> 
>   CPU 1                                   CPU 2
>   memory_failure_hugetlb
>   struct page *head = compound_head(p);
>                                           hugetlb page might be freed to
>                                           buddy, or even changed to another
>                                           compound page.
> 
>   get_hwpoison_page -- page is not what we want now...
> 
> The current code first does prechecks roughly and then reconfirms
> after taking refcount, but it's found that it makes code overly
> complicated, so move the prechecks in a single hugetlb_lock range.
> 
> A newly introduced function, try_memory_failure_hugetlb(), always
> takes hugetlb_lock (even for non-hugetlb pages).  That can be
> improved, but memory_failure() is rare in principle, so should
> not be a big problem.
> 
> Fixes: 761ad8d7c7b5 ("mm: hwpoison: introduce memory_failure_hugetlb()")
> Reported-by: Mike Kravetz <mike.kravetz@oracle.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

This patch looks good to me now. Many thanks for your hard work! Looking forward to
see your non-huge page version patch. ;)

Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

> Cc: stable@vger.kernel.org
> ---
> ChangeLog v7 -> v8
> - move hwpoison_filter() within page locking.
> 
> ChangeLog v6 -> v7:
> - Move lock_page() to try_memory_failure_hugetlb()
>   (based on bug report from Dan Carpenter)
> - Add Fixes: tag and CC to stable.
> 
> ChangeLog v5 -> v6:
> - Moved racy precheck operations into hugetlb_lock (based on
>   Mike's comment).
> - rebased onto v5.18-rc1.
> - dropped CC to stable.
> 
> ChangeLog v4 -> v5:
> - call TestSetPageHWPoison() when page_handle_poison() fails.
> - call TestSetPageHWPoison() for unhandlable cases (MF_MSG_UNKNOWN and
>   MF_MSG_DIFFERENT_PAGE_SIZE).
> - Set PageHWPoison on the head page only when the error page is surely
>   a hugepage, otherwise set the flag on the raw page.
> - rebased onto v5.17-rc8-mmotm-2022-03-16-17-42
> 
> ChangeLog v3 -> v4:
> - squash with "mm/memory-failure.c: fix race with changing page
>   compound again".
> - update patch subject and description based on it.
> 
> ChangeLog v2 -> v3:
> - rename the patch because page lock is not the primary factor to
>   solve the reported issue.
> - updated description in the same manner.
> - call page_handle_poison() instead of __page_handle_poison() for
>   free hugepage case.
> - reorder put_page and unlock_page (thanks to Miaohe Lin)
> 
> ChangeLog v1 -> v2:
> - pass subpage to get_hwpoison_huge_page() instead of head page.
> - call compound_head() in hugetlb_lock to avoid race with hugetlb
>   demotion/free.
> ---
>  include/linux/hugetlb.h |   6 ++
>  include/linux/mm.h      |   8 +++
>  mm/hugetlb.c            |  10 +++
>  mm/memory-failure.c     | 145 ++++++++++++++++++++++++++++------------
>  4 files changed, 127 insertions(+), 42 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 53c1b6082a4c..ac2a1d758a80 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -169,6 +169,7 @@ long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
>  						long freed);
>  bool isolate_huge_page(struct page *page, struct list_head *list);
>  int get_hwpoison_huge_page(struct page *page, bool *hugetlb);
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags);
>  void putback_active_hugepage(struct page *page);
>  void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason);
>  void free_huge_page(struct page *page);
> @@ -378,6 +379,11 @@ static inline int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
>  	return 0;
>  }
>  
> +static inline int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	return 0;
> +}
> +
>  static inline void putback_active_hugepage(struct page *page)
>  {
>  }
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e34edb775334..9f44254af8ce 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3197,6 +3197,14 @@ extern int sysctl_memory_failure_recovery;
>  extern void shake_page(struct page *p);
>  extern atomic_long_t num_poisoned_pages __read_mostly;
>  extern int soft_offline_page(unsigned long pfn, int flags);
> +#ifdef CONFIG_MEMORY_FAILURE
> +extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags);
> +#else
> +static inline int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	return 0;
> +}
> +#endif
>  
>  #ifndef arch_memory_failure
>  static inline int arch_memory_failure(unsigned long pfn, int flags)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f8ca7cca3c1a..3fc721789743 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6785,6 +6785,16 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
>  	return ret;
>  }
>  
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	int ret;
> +
> +	spin_lock_irq(&hugetlb_lock);
> +	ret = __get_huge_page_for_hwpoison(pfn, flags);
> +	spin_unlock_irq(&hugetlb_lock);
> +	return ret;
> +}
> +
>  void putback_active_hugepage(struct page *page)
>  {
>  	spin_lock_irq(&hugetlb_lock);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index dcb6bb9cf731..2020944398c9 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1498,50 +1498,113 @@ static int try_to_split_thp_page(struct page *page, const char *msg)
>  	return 0;
>  }
>  
> -static int memory_failure_hugetlb(unsigned long pfn, int flags)
> +/*
> + * Called from hugetlb code with hugetlb_lock held.
> + *
> + * Return values:
> + *   0             - free hugepage
> + *   1             - in-use hugepage
> + *   2             - not a hugepage
> + *   -EBUSY        - the hugepage is busy (try to retry)
> + *   -EHWPOISON    - the hugepage is already hwpoisoned
> + */
> +int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	struct page *page = pfn_to_page(pfn);
> +	struct page *head = compound_head(page);
> +	int ret = 2;	/* fallback to normal page handling */
> +	bool count_increased = false;
> +
> +	if (!PageHeadHuge(head))
> +		goto out;
> +
> +	if (flags & MF_COUNT_INCREASED) {
> +		ret = 1;
> +		count_increased = true;
> +	} else if (HPageFreed(head) || HPageMigratable(head)) {
> +		ret = get_page_unless_zero(head);
> +		if (ret)
> +			count_increased = true;
> +	} else {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	if (TestSetPageHWPoison(head)) {
> +		ret = -EHWPOISON;
> +		goto out;
> +	}
> +
> +	return ret;
> +out:
> +	if (count_increased)
> +		put_page(head);
> +	return ret;
> +}
> +
> +#ifdef CONFIG_HUGETLB_PAGE
> +/*
> + * Taking refcount of hugetlb pages needs extra care about race conditions
> + * with basic operations like hugepage allocation/free/demotion.
> + * So some of prechecks for hwpoison (pinning, and testing/setting
> + * PageHWPoison) should be done in single hugetlb_lock range.
> + */
> +static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
>  {
> -	struct page *p = pfn_to_page(pfn);
> -	struct page *head = compound_head(p);
>  	int res;
> +	struct page *p = pfn_to_page(pfn);
> +	struct page *head;
>  	unsigned long page_flags;
> +	bool retry = true;
>  
> -	if (TestSetPageHWPoison(head)) {
> -		pr_err("Memory failure: %#lx: already hardware poisoned\n",
> -		       pfn);
> -		res = -EHWPOISON;
> -		if (flags & MF_ACTION_REQUIRED)
> +	*hugetlb = 1;
> +retry:
> +	res = get_huge_page_for_hwpoison(pfn, flags);
> +	if (res == 2) { /* fallback to normal page handling */
> +		*hugetlb = 0;
> +		return 0;
> +	} else if (res == -EHWPOISON) {
> +		pr_err("Memory failure: %#lx: already hardware poisoned\n", pfn);
> +		if (flags & MF_ACTION_REQUIRED) {
> +			head = compound_head(p);
>  			res = kill_accessing_process(current, page_to_pfn(head), flags);
> +		}
>  		return res;
> +	} else if (res == -EBUSY) {
> +		if (retry) {
> +			retry = false;
> +			goto retry;
> +		}
> +		action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED);
> +		return res;
> +	}
> +
> +	head = compound_head(p);
> +	lock_page(head);
> +
> +	if (hwpoison_filter(p)) {
> +		ClearPageHWPoison(head);
> +		res = -EOPNOTSUPP;
> +		goto out;
>  	}
>  
>  	num_poisoned_pages_inc();
>  
> -	if (!(flags & MF_COUNT_INCREASED)) {
> -		res = get_hwpoison_page(p, flags);
> -		if (!res) {
> -			lock_page(head);
> -			if (hwpoison_filter(p)) {
> -				if (TestClearPageHWPoison(head))
> -					num_poisoned_pages_dec();
> -				unlock_page(head);
> -				return -EOPNOTSUPP;
> -			}
> -			unlock_page(head);
> -			res = MF_FAILED;
> -			if (__page_handle_poison(p)) {
> -				page_ref_inc(p);
> -				res = MF_RECOVERED;
> -			}
> -			action_result(pfn, MF_MSG_FREE_HUGE, res);
> -			return res == MF_RECOVERED ? 0 : -EBUSY;
> -		} else if (res < 0) {
> -			action_result(pfn, MF_MSG_UNKNOWN, MF_IGNORED);
> -			return -EBUSY;
> +	/*
> +	 * Handling free hugepage.  The possible race with hugepage allocation
> +	 * or demotion can be prevented by PageHWPoison flag.
> +	 */
> +	if (res == 0) {
> +		unlock_page(head);
> +		res = MF_FAILED;
> +		if (__page_handle_poison(p)) {
> +			page_ref_inc(p);
> +			res = MF_RECOVERED;
>  		}
> +		action_result(pfn, MF_MSG_FREE_HUGE, res);
> +		return res == MF_RECOVERED ? 0 : -EBUSY;
>  	}
>  
> -	lock_page(head);
> -
>  	/*
>  	 * The page could have changed compound pages due to race window.
>  	 * If this happens just bail out.
> @@ -1554,14 +1617,6 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>  
>  	page_flags = head->flags;
>  
> -	if (hwpoison_filter(p)) {
> -		if (TestClearPageHWPoison(head))
> -			num_poisoned_pages_dec();
> -		put_page(p);
> -		res = -EOPNOTSUPP;
> -		goto out;
> -	}
> -
>  	/*
>  	 * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
>  	 * simply disable it. In order to make it work properly, we need
> @@ -1588,6 +1643,12 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>  	unlock_page(head);
>  	return res;
>  }
> +#else
> +static inline int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb)
> +{
> +	return 0;
> +}
> +#endif
>  
>  static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>  		struct dev_pagemap *pgmap)
> @@ -1712,6 +1773,7 @@ int memory_failure(unsigned long pfn, int flags)
>  	int res = 0;
>  	unsigned long page_flags;
>  	bool retry = true;
> +	int hugetlb = 0;
>  
>  	if (!sysctl_memory_failure_recovery)
>  		panic("Memory failure on page %lx", pfn);
> @@ -1739,10 +1801,9 @@ int memory_failure(unsigned long pfn, int flags)
>  	}
>  
>  try_again:
> -	if (PageHuge(p)) {
> -		res = memory_failure_hugetlb(pfn, flags);
> +	res = try_memory_failure_hugetlb(pfn, flags, &hugetlb);
> +	if (hugetlb)
>  		goto unlock_mutex;
> -	}
>  
>  	if (TestSetPageHWPoison(p)) {
>  		pr_err("Memory failure: %#lx: already hardware poisoned\n",
> 

Re: [PATCH v8 1/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[Mike Kravetz]

On 4/8/22 06:53, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> There is a race condition between memory_failure_hugetlb() and hugetlb
> free/demotion, which causes setting PageHWPoison flag on the wrong page.
> The one simple result is that wrong processes can be killed, but another
> (more serious) one is that the actual error is left unhandled, so no one
> prevents later access to it, and that might lead to more serious results
> like consuming corrupted data.
> 
> Think about the below race window:
> 
>   CPU 1                                   CPU 2
>   memory_failure_hugetlb
>   struct page *head = compound_head(p);
>                                           hugetlb page might be freed to
>                                           buddy, or even changed to another
>                                           compound page.
> 
>   get_hwpoison_page -- page is not what we want now...
> 
> The current code first does prechecks roughly and then reconfirms
> after taking refcount, but it's found that it makes code overly
> complicated, so move the prechecks in a single hugetlb_lock range.
> 
> A newly introduced function, try_memory_failure_hugetlb(), always
> takes hugetlb_lock (even for non-hugetlb pages).  That can be
> improved, but memory_failure() is rare in principle, so should
> not be a big problem.

Thanks!

I think this approach is the safest and makes the code easier to understand.

> 
> Fixes: 761ad8d7c7b5 ("mm: hwpoison: introduce memory_failure_hugetlb()")
> Reported-by: Mike Kravetz <mike.kravetz@oracle.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> Cc: stable@vger.kernel.org
> ---
...
> ---
>  include/linux/hugetlb.h |   6 ++
>  include/linux/mm.h      |   8 +++
>  mm/hugetlb.c            |  10 +++
>  mm/memory-failure.c     | 145 ++++++++++++++++++++++++++++------------
>  4 files changed, 127 insertions(+), 42 deletions(-)
> 
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 53c1b6082a4c..ac2a1d758a80 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -169,6 +169,7 @@ long hugetlb_unreserve_pages(struct inode *inode, long start, long end,
>  						long freed);
>  bool isolate_huge_page(struct page *page, struct list_head *list);
>  int get_hwpoison_huge_page(struct page *page, bool *hugetlb);
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags);
>  void putback_active_hugepage(struct page *page);
>  void move_hugetlb_state(struct page *oldpage, struct page *newpage, int reason);
>  void free_huge_page(struct page *page);
> @@ -378,6 +379,11 @@ static inline int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
>  	return 0;
>  }
>  
> +static inline int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	return 0;
> +}
> +
>  static inline void putback_active_hugepage(struct page *page)
>  {
>  }
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index e34edb775334..9f44254af8ce 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3197,6 +3197,14 @@ extern int sysctl_memory_failure_recovery;
>  extern void shake_page(struct page *p);
>  extern atomic_long_t num_poisoned_pages __read_mostly;
>  extern int soft_offline_page(unsigned long pfn, int flags);
> +#ifdef CONFIG_MEMORY_FAILURE
> +extern int __get_huge_page_for_hwpoison(unsigned long pfn, int flags);
> +#else
> +static inline int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	return 0;
> +}
> +#endif
>  
>  #ifndef arch_memory_failure
>  static inline int arch_memory_failure(unsigned long pfn, int flags)
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f8ca7cca3c1a..3fc721789743 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6785,6 +6785,16 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
>  	return ret;
>  }
>  
> +int get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	int ret;
> +
> +	spin_lock_irq(&hugetlb_lock);
> +	ret = __get_huge_page_for_hwpoison(pfn, flags);
> +	spin_unlock_irq(&hugetlb_lock);
> +	return ret;
> +}
> +
>  void putback_active_hugepage(struct page *page)
>  {
>  	spin_lock_irq(&hugetlb_lock);
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index dcb6bb9cf731..2020944398c9 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1498,50 +1498,113 @@ static int try_to_split_thp_page(struct page *page, const char *msg)
>  	return 0;
>  }
>  
> -static int memory_failure_hugetlb(unsigned long pfn, int flags)
> +/*
> + * Called from hugetlb code with hugetlb_lock held.
> + *
> + * Return values:
> + *   0             - free hugepage
> + *   1             - in-use hugepage
> + *   2             - not a hugepage
> + *   -EBUSY        - the hugepage is busy (try to retry)
> + *   -EHWPOISON    - the hugepage is already hwpoisoned
> + */
> +int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
> +{
> +	struct page *page = pfn_to_page(pfn);
> +	struct page *head = compound_head(page);
> +	int ret = 2;	/* fallback to normal page handling */
> +	bool count_increased = false;
> +
> +	if (!PageHeadHuge(head))
> +		goto out;
> +
> +	if (flags & MF_COUNT_INCREASED) {
> +		ret = 1;
> +		count_increased = true;
> +	} else if (HPageFreed(head) || HPageMigratable(head)) {
> +		ret = get_page_unless_zero(head);
> +		if (ret)
> +			count_increased = true;

The above code works as designed, but may be a bit confusing.  If HPageFreed()
we KNOW ref count is zero, so no need to even call get_page_unless_zero() as
it will always return false in this case.  It might be more clear if written
as separate else if statements such as:

	} else if (HPageFreed(head)) {
		ret = 0;
	} else if (HPageMigratable(head)) {
		ret = get_page_unless_zero(head);
		if (ret)
			count_increased = true;

Not insisting this be changed.  Just easier to understand IMO.

Again, thanks for your work on this!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH v8 1/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[Miaohe Lin]

On 2022/4/15 1:56, Mike Kravetz wrote:
> On 4/8/22 06:53, Naoya Horiguchi wrote:
>> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
>>
>> There is a race condition between memory_failure_hugetlb() and hugetlb
>> free/demotion, which causes setting PageHWPoison flag on the wrong page.
>> The one simple result is that wrong processes can be killed, but another
>> (more serious) one is that the actual error is left unhandled, so no one
>> prevents later access to it, and that might lead to more serious results
>> like consuming corrupted data.
>>
>> Think about the below race window:
>>
>>   CPU 1                                   CPU 2
>>   memory_failure_hugetlb
>>   struct page *head = compound_head(p);
>>                                           hugetlb page might be freed to
>>                                           buddy, or even changed to another
>>                                           compound page.
>>
>>   get_hwpoison_page -- page is not what we want now...
>>
>> The current code first does prechecks roughly and then reconfirms
>> after taking refcount, but it's found that it makes code overly
>> complicated, so move the prechecks in a single hugetlb_lock range.
>>
>> A newly introduced function, try_memory_failure_hugetlb(), always
>> takes hugetlb_lock (even for non-hugetlb pages).  That can be
>> improved, but memory_failure() is rare in principle, so should
>> not be a big problem.
...
> 
> The above code works as designed, but may be a bit confusing.  If HPageFreed()
> we KNOW ref count is zero, so no need to even call get_page_unless_zero() as
> it will always return false in this case.  It might be more clear if written
> as separate else if statements such as:
> 
> 	} else if (HPageFreed(head)) {
> 		ret = 0;
> 	} else if (HPageMigratable(head)) {
> 		ret = get_page_unless_zero(head);
> 		if (ret)
> 			count_increased = true;
> 

This code here is consistent with the logic in get_hwpoison_huge_page. If change is required,
they might need to be changed together.

BTW: They look a bit confusing for me at first but I get used to it later. ;)

Thanks!

> Not insisting this be changed.  Just easier to understand IMO.
> 
> Again, thanks for your work on this!
> 
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> 

Re: [PATCH v8 1/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[HORIGUCHI NAOYA(堀口 直也)]

On Fri, Apr 15, 2022 at 09:55:14AM +0800, Miaohe Lin wrote:
> On 2022/4/15 1:56, Mike Kravetz wrote:
> > On 4/8/22 06:53, Naoya Horiguchi wrote:
> >> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> >>
> >> There is a race condition between memory_failure_hugetlb() and hugetlb
> >> free/demotion, which causes setting PageHWPoison flag on the wrong page.
> >> The one simple result is that wrong processes can be killed, but another
> >> (more serious) one is that the actual error is left unhandled, so no one
> >> prevents later access to it, and that might lead to more serious results
> >> like consuming corrupted data.
> >>
> >> Think about the below race window:
> >>
> >>   CPU 1                                   CPU 2
> >>   memory_failure_hugetlb
> >>   struct page *head = compound_head(p);
> >>                                           hugetlb page might be freed to
> >>                                           buddy, or even changed to another
> >>                                           compound page.
> >>
> >>   get_hwpoison_page -- page is not what we want now...
> >>
> >> The current code first does prechecks roughly and then reconfirms
> >> after taking refcount, but it's found that it makes code overly
> >> complicated, so move the prechecks in a single hugetlb_lock range.
> >>
> >> A newly introduced function, try_memory_failure_hugetlb(), always
> >> takes hugetlb_lock (even for non-hugetlb pages).  That can be
> >> improved, but memory_failure() is rare in principle, so should
> >> not be a big problem.
> ...
> > 
> > The above code works as designed, but may be a bit confusing.  If HPageFreed()
> > we KNOW ref count is zero, so no need to even call get_page_unless_zero() as
> > it will always return false in this case.  It might be more clear if written
> > as separate else if statements such as:
> > 
> > 	} else if (HPageFreed(head)) {
> > 		ret = 0;
> > 	} else if (HPageMigratable(head)) {
> > 		ret = get_page_unless_zero(head);
> > 		if (ret)
> > 			count_increased = true;
> > 
> 
> This code here is consistent with the logic in get_hwpoison_huge_page. If change is required,
> they might need to be changed together.
> 
> BTW: They look a bit confusing for me at first but I get used to it later. ;)

Thank you for comments, Mike, Miaohe.
Patch 1/3 is to stable, so I'll submit a separate cleanup patch for these changes.

> 
> Thanks!
> 
> > Not insisting this be changed.  Just easier to understand IMO.
> > 
> > Again, thanks for your work on this!
> > 
> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

Thank you!

- Naoya Horiguchi

[PATCH 4/3] mm, hugetlb, hwpoison: separate branch for free and in-use hugepage

[Naoya Horiguchi]

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

We know that HPageFreed pages should have page refcount 0, so
get_page_unless_zero() always fails and returns 0.  So explicitly separate
the branch based on page state for minor optimization and better readability.

Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/hugetlb.c        | 4 +++-
 mm/memory-failure.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index e38cbfdf3e61..3638f166e554 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6786,7 +6786,9 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
 	spin_lock_irq(&hugetlb_lock);
 	if (PageHeadHuge(page)) {
 		*hugetlb = true;
-		if (HPageFreed(page) || HPageMigratable(page))
+		if (HPageFreed(page))
+			ret = 0;
+		else if (HPageMigratable(page))
 			ret = get_page_unless_zero(page);
 		else
 			ret = -EBUSY;
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 5e3ad640f5bb..661079a37f29 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1517,7 +1517,9 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
 	if (flags & MF_COUNT_INCREASED) {
 		ret = 1;
 		count_increased = true;
-	} else if (HPageFreed(head) || HPageMigratable(head)) {
+	} else if (HPageFreed(head)) {
+		ret = 0;
+	} else if (HPageMigratable(head)) {
 		ret = get_page_unless_zero(head);
 		if (ret)
 			count_increased = true;
-- 
2.23.0

Re: [PATCH 4/3] mm, hugetlb, hwpoison: separate branch for free and in-use hugepage

[Mike Kravetz]

On 4/14/22 21:18, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> We know that HPageFreed pages should have page refcount 0, so
> get_page_unless_zero() always fails and returns 0.  So explicitly separate
> the branch based on page state for minor optimization and better readability.
> 
> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
> Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  mm/hugetlb.c        | 4 +++-
>  mm/memory-failure.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)

Thank you!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index e38cbfdf3e61..3638f166e554 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6786,7 +6786,9 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
>  	spin_lock_irq(&hugetlb_lock);
>  	if (PageHeadHuge(page)) {
>  		*hugetlb = true;
> -		if (HPageFreed(page) || HPageMigratable(page))
> +		if (HPageFreed(page))
> +			ret = 0;
> +		else if (HPageMigratable(page))
>  			ret = get_page_unless_zero(page);
>  		else
>  			ret = -EBUSY;
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 5e3ad640f5bb..661079a37f29 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1517,7 +1517,9 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
>  	if (flags & MF_COUNT_INCREASED) {
>  		ret = 1;
>  		count_increased = true;
> -	} else if (HPageFreed(head) || HPageMigratable(head)) {
> +	} else if (HPageFreed(head)) {
> +		ret = 0;
> +	} else if (HPageMigratable(head)) {
>  		ret = get_page_unless_zero(head);
>  		if (ret)
>  			count_increased = true;

Re: [PATCH 4/3] mm, hugetlb, hwpoison: separate branch for free and in-use hugepage

[Miaohe Lin]

On 2022/4/15 12:18, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> We know that HPageFreed pages should have page refcount 0, so
> get_page_unless_zero() always fails and returns 0.  So explicitly separate
> the branch based on page state for minor optimization and better readability.
> 
> Suggested-by: Mike Kravetz <mike.kravetz@oracle.com>
> Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  mm/hugetlb.c        | 4 +++-
>  mm/memory-failure.c | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 

Thanks!

Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index e38cbfdf3e61..3638f166e554 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6786,7 +6786,9 @@ int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
>  	spin_lock_irq(&hugetlb_lock);
>  	if (PageHeadHuge(page)) {
>  		*hugetlb = true;
> -		if (HPageFreed(page) || HPageMigratable(page))
> +		if (HPageFreed(page))
> +			ret = 0;
> +		else if (HPageMigratable(page))
>  			ret = get_page_unless_zero(page);
>  		else
>  			ret = -EBUSY;
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 5e3ad640f5bb..661079a37f29 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1517,7 +1517,9 @@ int __get_huge_page_for_hwpoison(unsigned long pfn, int flags)
>  	if (flags & MF_COUNT_INCREASED) {
>  		ret = 1;
>  		count_increased = true;
> -	} else if (HPageFreed(head) || HPageMigratable(head)) {
> +	} else if (HPageFreed(head)) {
> +		ret = 0;
> +	} else if (HPageMigratable(head)) {
>  		ret = get_page_unless_zero(head);
>  		if (ret)
>  			count_increased = true;
> 

[PATCH v8 2/3] mm/hwpoison: put page in already hwpoisoned case with MF_COUNT_INCREASED

[Naoya Horiguchi]

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

In already hwpoisoned case, memory_failure() is supposed to return with
releasing the page refcount taken for error handling. But currently the
refcount is not released when called with MF_COUNT_INCREASED, which
makes page refcount inconsistent.  This should be rare and non-critical,
but it might be inconvenient in testing (unpoison doesn't work).

Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 mm/memory-failure.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 2020944398c9..b2e32cdc3823 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1811,6 +1811,8 @@ int memory_failure(unsigned long pfn, int flags)
 		res = -EHWPOISON;
 		if (flags & MF_ACTION_REQUIRED)
 			res = kill_accessing_process(current, pfn, flags);
+		if (flags & MF_COUNT_INCREASED)
+			put_page(p);
 		goto unlock_mutex;
 	}
 
-- 
2.25.1

Re: [PATCH v8 2/3] mm/hwpoison: put page in already hwpoisoned case with MF_COUNT_INCREASED

[Miaohe Lin]

On 2022/4/8 21:53, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> In already hwpoisoned case, memory_failure() is supposed to return with
> releasing the page refcount taken for error handling. But currently the
> refcount is not released when called with MF_COUNT_INCREASED, which
> makes page refcount inconsistent.  This should be rare and non-critical,
> but it might be inconvenient in testing (unpoison doesn't work).

IMHO, this issue will lead to memoryleak as page isn't freed even owner process
is killed. So we might need a Fixes tag?

Anyway, this patch looks good to me. Thanks!
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

> 
> Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  mm/memory-failure.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 2020944398c9..b2e32cdc3823 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1811,6 +1811,8 @@ int memory_failure(unsigned long pfn, int flags)
>  		res = -EHWPOISON;
>  		if (flags & MF_ACTION_REQUIRED)
>  			res = kill_accessing_process(current, pfn, flags);
> +		if (flags & MF_COUNT_INCREASED)
> +			put_page(p);
>  		goto unlock_mutex;
>  	}
>  
> 

Re: [PATCH v8 2/3] mm/hwpoison: put page in already hwpoisoned case with MF_COUNT_INCREASED

[Mike Kravetz]

On 4/8/22 06:53, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> In already hwpoisoned case, memory_failure() is supposed to return with
> releasing the page refcount taken for error handling. But currently the
> refcount is not released when called with MF_COUNT_INCREASED, which
> makes page refcount inconsistent.  This should be rare and non-critical,
> but it might be inconvenient in testing (unpoison doesn't work).
> 
> Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  mm/memory-failure.c | 2 ++
>  1 file changed, 2 insertions(+)

Thanks!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

-- 
Mike Kravetz

> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 2020944398c9..b2e32cdc3823 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1811,6 +1811,8 @@ int memory_failure(unsigned long pfn, int flags)
>  		res = -EHWPOISON;
>  		if (flags & MF_ACTION_REQUIRED)
>  			res = kill_accessing_process(current, pfn, flags);
> +		if (flags & MF_COUNT_INCREASED)
> +			put_page(p);
>  		goto unlock_mutex;
>  	}
>  

[PATCH v8 3/3] Revert "mm/memory-failure.c: fix race with changing page compound again"

[Naoya Horiguchi]

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

Reverts commit 888af2701db7 ("mm/memory-failure.c: fix race with
changing page compound again") because now we fetch the page refcount
under hugetlb_lock in try_memory_failure_hugetlb() so that the race
check is no longer necessary.

Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
---
 include/linux/mm.h      |  1 -
 include/ras/ras_event.h |  1 -
 mm/memory-failure.c     | 11 -----------
 3 files changed, 13 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 9f44254af8ce..d446e834a3e5 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -3251,7 +3251,6 @@ enum mf_action_page_type {
 	MF_MSG_BUDDY,
 	MF_MSG_DAX,
 	MF_MSG_UNSPLIT_THP,
-	MF_MSG_DIFFERENT_PAGE_SIZE,
 	MF_MSG_UNKNOWN,
 };
 
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 1e694fd239b9..d0337a41141c 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -374,7 +374,6 @@ TRACE_EVENT(aer_event,
 	EM ( MF_MSG_BUDDY, "free buddy page" )				\
 	EM ( MF_MSG_DAX, "dax page" )					\
 	EM ( MF_MSG_UNSPLIT_THP, "unsplit thp" )			\
-	EM ( MF_MSG_DIFFERENT_PAGE_SIZE, "different page size" )	\
 	EMe ( MF_MSG_UNKNOWN, "unknown page" )
 
 /*
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index b2e32cdc3823..e2674532678b 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -733,7 +733,6 @@ static const char * const action_page_types[] = {
 	[MF_MSG_BUDDY]			= "free buddy page",
 	[MF_MSG_DAX]			= "dax page",
 	[MF_MSG_UNSPLIT_THP]		= "unsplit thp",
-	[MF_MSG_DIFFERENT_PAGE_SIZE]	= "different page size",
 	[MF_MSG_UNKNOWN]		= "unknown page",
 };
 
@@ -1605,16 +1604,6 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
 		return res == MF_RECOVERED ? 0 : -EBUSY;
 	}
 
-	/*
-	 * The page could have changed compound pages due to race window.
-	 * If this happens just bail out.
-	 */
-	if (!PageHuge(p) || compound_head(p) != head) {
-		action_result(pfn, MF_MSG_DIFFERENT_PAGE_SIZE, MF_IGNORED);
-		res = -EBUSY;
-		goto out;
-	}
-
 	page_flags = head->flags;
 
 	/*
-- 
2.25.1

Re: [PATCH v8 3/3] Revert "mm/memory-failure.c: fix race with changing page compound again"

[Miaohe Lin]

On 2022/4/8 21:53, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> Reverts commit 888af2701db7 ("mm/memory-failure.c: fix race with
> changing page compound again") because now we fetch the page refcount
> under hugetlb_lock in try_memory_failure_hugetlb() so that the race
> check is no longer necessary.
> 

Thanks a lot.
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

> Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  include/linux/mm.h      |  1 -
>  include/ras/ras_event.h |  1 -
>  mm/memory-failure.c     | 11 -----------
>  3 files changed, 13 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 9f44254af8ce..d446e834a3e5 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3251,7 +3251,6 @@ enum mf_action_page_type {
>  	MF_MSG_BUDDY,
>  	MF_MSG_DAX,
>  	MF_MSG_UNSPLIT_THP,
> -	MF_MSG_DIFFERENT_PAGE_SIZE,
>  	MF_MSG_UNKNOWN,
>  };
>  
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 1e694fd239b9..d0337a41141c 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -374,7 +374,6 @@ TRACE_EVENT(aer_event,
>  	EM ( MF_MSG_BUDDY, "free buddy page" )				\
>  	EM ( MF_MSG_DAX, "dax page" )					\
>  	EM ( MF_MSG_UNSPLIT_THP, "unsplit thp" )			\
> -	EM ( MF_MSG_DIFFERENT_PAGE_SIZE, "different page size" )	\
>  	EMe ( MF_MSG_UNKNOWN, "unknown page" )
>  
>  /*
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index b2e32cdc3823..e2674532678b 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -733,7 +733,6 @@ static const char * const action_page_types[] = {
>  	[MF_MSG_BUDDY]			= "free buddy page",
>  	[MF_MSG_DAX]			= "dax page",
>  	[MF_MSG_UNSPLIT_THP]		= "unsplit thp",
> -	[MF_MSG_DIFFERENT_PAGE_SIZE]	= "different page size",
>  	[MF_MSG_UNKNOWN]		= "unknown page",
>  };
>  
> @@ -1605,16 +1604,6 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
>  		return res == MF_RECOVERED ? 0 : -EBUSY;
>  	}
>  
> -	/*
> -	 * The page could have changed compound pages due to race window.
> -	 * If this happens just bail out.
> -	 */
> -	if (!PageHuge(p) || compound_head(p) != head) {
> -		action_result(pfn, MF_MSG_DIFFERENT_PAGE_SIZE, MF_IGNORED);
> -		res = -EBUSY;
> -		goto out;
> -	}
> -
>  	page_flags = head->flags;
>  
>  	/*
> 

Re: [PATCH v8 3/3] Revert "mm/memory-failure.c: fix race with changing page compound again"

[Mike Kravetz]

On 4/8/22 06:53, Naoya Horiguchi wrote:
> From: Naoya Horiguchi <naoya.horiguchi@nec.com>
> 
> Reverts commit 888af2701db7 ("mm/memory-failure.c: fix race with
> changing page compound again") because now we fetch the page refcount
> under hugetlb_lock in try_memory_failure_hugetlb() so that the race
> check is no longer necessary.
> 
> Suggested-by: Miaohe Lin <linmiaohe@huawei.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> ---
>  include/linux/mm.h      |  1 -
>  include/ras/ras_event.h |  1 -
>  mm/memory-failure.c     | 11 -----------
>  3 files changed, 13 deletions(-)

Thanks!

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
-- 
Mike Kravetz

Re: [PATCH v8 0/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[Andrew Morton]

On Fri,  8 Apr 2022 22:53:20 +0900 Naoya Horiguchi <naoya.horiguchi@linux.dev> wrote:

> I updated v7 (https://lore.kernel.org/linux-mm/20220407112929.1344748-1-naoya.horiguchi@linux.dev/)
> based on the comment from Miaohe Lin.  Now I have a few additional patches.
> Any comments and suggestions are welcomed.

As [1/3] is cc:stable I queued that for 5.18-rcX and the other two
patches I queued for 5.19.

This plan means that [1/3] will be merged into mainline without the
other two patches yet present, which is a combination which you may not
have tested.  Please confirm that this will be OK?

Re: [PATCH v8 0/3] mm/hwpoison: fix race between hugetlb free/demotion and memory_failure_hugetlb()

[Naoya Horiguchi]

On Fri, Apr 08, 2022 at 04:47:55PM -0700, Andrew Morton wrote:
> On Fri,  8 Apr 2022 22:53:20 +0900 Naoya Horiguchi <naoya.horiguchi@linux.dev> wrote:
> 
> > I updated v7 (https://lore.kernel.org/linux-mm/20220407112929.1344748-1-naoya.horiguchi@linux.dev/)
> > based on the comment from Miaohe Lin.  Now I have a few additional patches.
> > Any comments and suggestions are welcomed.
> 
> As [1/3] is cc:stable I queued that for 5.18-rcX and the other two
> patches I queued for 5.19.
> 
> This plan means that [1/3] will be merged into mainline without the
> other two patches yet present, which is a combination which you may not
> have tested.  Please confirm that this will be OK?

Yes, the plan is fine to me.
I've tested the kernel with all 3 patched, but 2/3 and 3/3 are simple and
the effect should be predictable.  I'll test only with 1/3 next week.

Thanks,
Naoya Horiguchi