diff mbox series

[v3] mm/hwpoison: set PageHWPoison after passing prechecks in memory_failure_hugetlb()

Message ID 20220316014534.224447-1-naoya.horiguchi@linux.dev (mailing list archive)
State New
Headers show
Series [v3] mm/hwpoison: set PageHWPoison after passing prechecks in memory_failure_hugetlb() | expand

Commit Message

Naoya Horiguchi March 16, 2022, 1:45 a.m. UTC 
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
(which was a hugetlb when memory_failure() was called, but was removed
or demoted when memory_failure_hugetlb() is called).  This results in
killing wrong processes.  So set PageHWPoison flag after passing prechecks.

The actual user-visible effect might be obscure because even if
memory_failure() works as expected, some random process could be killed.
Even worse, the actual error is left unhandled, so no one prevents later
access to it, which might lead to more serious results like consuming
corrupted data.

This patch depends on Miaohe's patch titled "mm/memory-failure.c: fix
race with changing page compound again".

Reported-by: Mike Kravetz <mike.kravetz@oracle.com>
Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
Cc: <stable@vger.kernel.org>
---
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.
---
 mm/hugetlb.c        |  8 +++++---
 mm/memory-failure.c | 43 +++++++++++++++++++++----------------------
 2 files changed, 26 insertions(+), 25 deletions(-)

Comments

Miaohe Lin March 16, 2022, 7:25 a.m. UTC | #1 
On 2022/3/16 9:45, 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
> (which was a hugetlb when memory_failure() was called, but was removed
> or demoted when memory_failure_hugetlb() is called).  This results in
> killing wrong processes.  So set PageHWPoison flag after passing prechecks.
> 
> The actual user-visible effect might be obscure because even if
> memory_failure() works as expected, some random process could be killed.
> Even worse, the actual error is left unhandled, so no one prevents later
> access to it, which might lead to more serious results like consuming
> corrupted data.
> 
> This patch depends on Miaohe's patch titled "mm/memory-failure.c: fix
> race with changing page compound again".
> 
> Reported-by: Mike Kravetz <mike.kravetz@oracle.com>
> Signed-off-by: Naoya Horiguchi <naoya.horiguchi@nec.com>
> Cc: <stable@vger.kernel.org>

This patch looks good to me. Many thanks!

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

> ---
> 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.
> ---
>  mm/hugetlb.c        |  8 +++++---
>  mm/memory-failure.c | 43 +++++++++++++++++++++----------------------
>  2 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index f294db835f4b..345fed90842e 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -6761,14 +6761,16 @@ bool isolate_huge_page(struct page *page, struct list_head *list)
>  
>  int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
>  {
> +	struct page *head;
>  	int ret = 0;
>  
>  	*hugetlb = false;
>  	spin_lock_irq(&hugetlb_lock);
> -	if (PageHeadHuge(page)) {
> +	head = compound_head(page);
> +	if (PageHeadHuge(head)) {
>  		*hugetlb = true;
> -		if (HPageFreed(page) || HPageMigratable(page))
> -			ret = get_page_unless_zero(page);
> +		if (HPageFreed(head) || HPageMigratable(head))
> +			ret = get_page_unless_zero(head);
>  		else
>  			ret = -EBUSY;
>  	}
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index a9bfd04d2a3c..c66d75b45864 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1193,7 +1193,7 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
>  	int ret = 0;
>  	bool hugetlb = false;
>  
> -	ret = get_hwpoison_huge_page(head, &hugetlb);
> +	ret = get_hwpoison_huge_page(page, &hugetlb);
>  	if (hugetlb)
>  		return ret;
>  
> @@ -1280,11 +1280,10 @@ static int get_any_page(struct page *p, unsigned long flags)
>  
>  static int __get_unpoison_page(struct page *page)
>  {
> -	struct page *head = compound_head(page);
>  	int ret = 0;
>  	bool hugetlb = false;
>  
> -	ret = get_hwpoison_huge_page(head, &hugetlb);
> +	ret = get_hwpoison_huge_page(page, &hugetlb);
>  	if (hugetlb)
>  		return ret;
>  
> @@ -1502,34 +1501,21 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>  	struct page *head = compound_head(p);
>  	int res;
>  	unsigned long page_flags;
> -
> -	if (TestSetPageHWPoison(head)) {
> -		pr_err("Memory failure: %#lx: already hardware poisoned\n",
> -		       pfn);
> -		res = -EHWPOISON;
> -		if (flags & MF_ACTION_REQUIRED)
> -			res = kill_accessing_process(current, page_to_pfn(head), flags);
> -		return res;
> -	}
> -
> -	num_poisoned_pages_inc();
> +	bool put = false;
> +	bool already_hwpoisoned = false;
>  
>  	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);
> +			if (page_handle_poison(p, true, false))
>  				res = MF_RECOVERED;
> -			}
>  			action_result(pfn, MF_MSG_FREE_HUGE, res);
>  			return res == MF_RECOVERED ? 0 : -EBUSY;
>  		} else if (res < 0) {
> @@ -1553,13 +1539,18 @@ 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);
> +		put = true;
>  		res = -EOPNOTSUPP;
>  		goto out;
>  	}
>  
> +	if (TestSetPageHWPoison(head)) {
> +		put = already_hwpoisoned = true;
> +		goto out;
> +	}
> +
> +	num_poisoned_pages_inc();
> +
>  	/*
>  	 * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
>  	 * simply disable it. In order to make it work properly, we need
> @@ -1584,6 +1575,14 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)
>  	return identify_page_state(pfn, p, page_flags);
>  out:
>  	unlock_page(head);
> +	if (put)
> +		put_page(p);
> +	if (already_hwpoisoned) {
> +		pr_err("Memory failure: %#lx: already hardware poisoned\n", pfn);
> +		res = -EHWPOISON;
> +		if (flags & MF_ACTION_REQUIRED)
> +			res = kill_accessing_process(current, page_to_pfn(head), flags);
> +	}
>  	return res;
>  }
>  
>
diff mbox series

Patch

diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index f294db835f4b..345fed90842e 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -6761,14 +6761,16 @@  bool isolate_huge_page(struct page *page, struct list_head *list)
 
 int get_hwpoison_huge_page(struct page *page, bool *hugetlb)
 {
+	struct page *head;
 	int ret = 0;
 
 	*hugetlb = false;
 	spin_lock_irq(&hugetlb_lock);
-	if (PageHeadHuge(page)) {
+	head = compound_head(page);
+	if (PageHeadHuge(head)) {
 		*hugetlb = true;
-		if (HPageFreed(page) || HPageMigratable(page))
-			ret = get_page_unless_zero(page);
+		if (HPageFreed(head) || HPageMigratable(head))
+			ret = get_page_unless_zero(head);
 		else
 			ret = -EBUSY;
 	}
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index a9bfd04d2a3c..c66d75b45864 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1193,7 +1193,7 @@  static int __get_hwpoison_page(struct page *page, unsigned long flags)
 	int ret = 0;
 	bool hugetlb = false;
 
-	ret = get_hwpoison_huge_page(head, &hugetlb);
+	ret = get_hwpoison_huge_page(page, &hugetlb);
 	if (hugetlb)
 		return ret;
 
@@ -1280,11 +1280,10 @@  static int get_any_page(struct page *p, unsigned long flags)
 
 static int __get_unpoison_page(struct page *page)
 {
-	struct page *head = compound_head(page);
 	int ret = 0;
 	bool hugetlb = false;
 
-	ret = get_hwpoison_huge_page(head, &hugetlb);
+	ret = get_hwpoison_huge_page(page, &hugetlb);
 	if (hugetlb)
 		return ret;
 
@@ -1502,34 +1501,21 @@  static int memory_failure_hugetlb(unsigned long pfn, int flags)
 	struct page *head = compound_head(p);
 	int res;
 	unsigned long page_flags;
-
-	if (TestSetPageHWPoison(head)) {
-		pr_err("Memory failure: %#lx: already hardware poisoned\n",
-		       pfn);
-		res = -EHWPOISON;
-		if (flags & MF_ACTION_REQUIRED)
-			res = kill_accessing_process(current, page_to_pfn(head), flags);
-		return res;
-	}
-
-	num_poisoned_pages_inc();
+	bool put = false;
+	bool already_hwpoisoned = false;
 
 	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);
+			if (page_handle_poison(p, true, false))
 				res = MF_RECOVERED;
-			}
 			action_result(pfn, MF_MSG_FREE_HUGE, res);
 			return res == MF_RECOVERED ? 0 : -EBUSY;
 		} else if (res < 0) {
@@ -1553,13 +1539,18 @@  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);
+		put = true;
 		res = -EOPNOTSUPP;
 		goto out;
 	}
 
+	if (TestSetPageHWPoison(head)) {
+		put = already_hwpoisoned = true;
+		goto out;
+	}
+
+	num_poisoned_pages_inc();
+
 	/*
 	 * TODO: hwpoison for pud-sized hugetlb doesn't work right now, so
 	 * simply disable it. In order to make it work properly, we need
@@ -1584,6 +1575,14 @@  static int memory_failure_hugetlb(unsigned long pfn, int flags)
 	return identify_page_state(pfn, p, page_flags);
 out:
 	unlock_page(head);
+	if (put)
+		put_page(p);
+	if (already_hwpoisoned) {
+		pr_err("Memory failure: %#lx: already hardware poisoned\n", pfn);
+		res = -EHWPOISON;
+		if (flags & MF_ACTION_REQUIRED)
+			res = kill_accessing_process(current, page_to_pfn(head), flags);
+	}
 	return res;
 }