[PATCH 0/8] A few fixup and cleanup patches for memory-failure

[PATCH 0/8] A few fixup and cleanup patches for memory-failure

[Miaohe Lin]

Hi everyone,
This series contains a few fixup patches to fix potential permanently
locked hpage, fix race window when trying to get hugetlb folio and so
on. Also there is minor cleanup for comments and codestyle. More details
can be found in the respective changelogs.
Thanks!

Miaohe Lin (8):
  mm: memory-failure: fix potential permanently locked hpage
  mm: memory-failure: ensure moving HWPoison flag to the raw error pages
  mm: memory-failure: Don't account hwpoison_filter() filtered pages
  mm: memory-failure: use local variable huge to check hugetlb page
  mm: memory-failure: remove unneeded header files
  mm: memory-failure: minor cleanup for comments and codestyle
  mm: memory-failure: fetch compound head after extra page refcnt is
    held
  mm: memory-failure: fix race window when trying to get hugetlb folio

 mm/memory-failure.c | 49 +++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 19 deletions(-)

-- 
2.33.0

[PATCH 1/8] mm: memory-failure: fix potential permanently locked hpage

[Miaohe Lin]

If hpage isn't Hugetlb page, MF_DELAYED is returned without unlock hpage
leading to hpage permanently locked. But this shouldn't trigger in the
real world because this PageHuge() check is just for potential problems.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index a6221a4bc5ea..d21ee27ad412 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1187,8 +1187,10 @@ static int me_huge_page(struct page_state *ps, struct page *p)
 	struct address_space *mapping;
 	bool extra_pins = false;
 
-	if (!PageHuge(hpage))
+	if (!PageHuge(hpage)) {
+		unlock_page(hpage);
 		return MF_DELAYED;
+	}
 
 	mapping = page_mapping(hpage);
 	if (mapping) {
-- 
2.33.0

Re: [PATCH 1/8] mm: memory-failure: fix potential permanently locked hpage

[Markus Elfring]

> If hpage isn't Hugetlb page, MF_DELAYED is returned without unlock hpage
> leading to hpage permanently locked. But this shouldn't trigger in the
> real world because this PageHuge() check is just for potential problems.

Please choose an imperative change suggestion.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94


How do you think about to avoid empty descriptions in recipient lists?

Regards,
Markus

Re: [PATCH 1/8] mm: memory-failure: fix potential permanently locked hpage

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:37PM +0800, Miaohe Lin wrote:
> If hpage isn't Hugetlb page, MF_DELAYED is returned without unlock hpage
> leading to hpage permanently locked. But this shouldn't trigger in the
> real world because this PageHuge() check is just for potential problems.

Right, so this if-block is dead code, how about simply removing it?

Thanks,
Naoya Horiguchi

> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory-failure.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index a6221a4bc5ea..d21ee27ad412 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1187,8 +1187,10 @@ static int me_huge_page(struct page_state *ps, struct page *p)
>  	struct address_space *mapping;
>  	bool extra_pins = false;
>  
> -	if (!PageHuge(hpage))
> +	if (!PageHuge(hpage)) {
> +		unlock_page(hpage);
>  		return MF_DELAYED;
> +	}
>  
>  	mapping = page_mapping(hpage);
>  	if (mapping) {
> -- 
> 2.33.0
> 
> 
> 

Re: [PATCH 1/8] mm: memory-failure: fix potential permanently locked hpage

[Miaohe Lin]

On 2023/7/10 15:56, Naoya Horiguchi wrote:
> On Sat, Jul 08, 2023 at 04:57:37PM +0800, Miaohe Lin wrote:
>> If hpage isn't Hugetlb page, MF_DELAYED is returned without unlock hpage
>> leading to hpage permanently locked. But this shouldn't trigger in the
>> real world because this PageHuge() check is just for potential problems.
> 
> Right, so this if-block is dead code, how about simply removing it?

From the current code of view, I think this if-block is actually dead code and won't catch
anything too. So removing it makes sense to me. Will do it in v2.

Thanks Naoya.

[PATCH 2/8] mm: memory-failure: ensure moving HWPoison flag to the raw error pages

[Miaohe Lin]

If hugetlb_vmemmap_optimized is enabled, folio_clear_hugetlb_hwpoison()
called from try_memory_failure_hugetlb() won't transfer HWPoison flag to
subpages while folio's HWPoison flag is cleared. So when trying to free
this hugetlb page into buddy, folio_clear_hugetlb_hwpoison() is not called
to move HWPoison flag from head page to the raw error pages even if now
hugetlb_vmemmap_optimized is cleared. This will results in HWPoisoned page
being used again and raw_hwp_page leak.

Fixes: ac5fcde0a96a ("mm, hwpoison: make unpoison aware of raw error info in hwpoisoned hugepage")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index d21ee27ad412..c155122e3c66 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1913,6 +1913,8 @@ void folio_clear_hugetlb_hwpoison(struct folio *folio)
 {
 	if (folio_test_hugetlb_raw_hwp_unreliable(folio))
 		return;
+	if (folio_test_hugetlb_vmemmap_optimized(folio))
+		return;
 	folio_clear_hwpoison(folio);
 	folio_free_raw_hwp(folio, true);
 }
-- 
2.33.0

Re: [PATCH 2/8] mm: memory-failure: ensure moving HWPoison flag to the raw error pages

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:38PM +0800, Miaohe Lin wrote:
> If hugetlb_vmemmap_optimized is enabled, folio_clear_hugetlb_hwpoison()
> called from try_memory_failure_hugetlb() won't transfer HWPoison flag to
> subpages while folio's HWPoison flag is cleared. So when trying to free
> this hugetlb page into buddy, folio_clear_hugetlb_hwpoison() is not called
> to move HWPoison flag from head page to the raw error pages even if now
> hugetlb_vmemmap_optimized is cleared. This will results in HWPoisoned page
> being used again and raw_hwp_page leak.
> 
> Fixes: ac5fcde0a96a ("mm, hwpoison: make unpoison aware of raw error info in hwpoisoned hugepage")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

[PATCH 3/8] mm: memory-failure: Don't account hwpoison_filter() filtered pages

[Miaohe Lin]

mf_generic_kill_procs() will return -EOPNOTSUPP when hwpoison_filter()
filtered dax page. In that case, action_result() isn't expected to be
called to update mf_stats. This will results in inaccurate but benign
memory failure handling statistics.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index c155122e3c66..905758af70f3 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2101,7 +2101,8 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 out:
 	/* drop pgmap ref acquired in caller */
 	put_dev_pagemap(pgmap);
-	action_result(pfn, MF_MSG_DAX, rc ? MF_FAILED : MF_RECOVERED);
+	if (rc != -EOPNOTSUPP)
+		action_result(pfn, MF_MSG_DAX, rc ? MF_FAILED : MF_RECOVERED);
 	return rc;
 }
 
-- 
2.33.0

Re: [PATCH 3/8] mm: memory-failure: Don't account hwpoison_filter() filtered pages

[Markus Elfring]

> mf_generic_kill_procs() will return -EOPNOTSUPP when hwpoison_filter()
> filtered dax page. In that case, action_result() isn't expected to be
> called to update mf_stats. This will results in inaccurate but benign
> memory failure handling statistics.

Please choose another imperative change suggestion (also without a typo).

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94

Regards,
Markus

Re: [PATCH 3/8] mm: memory-failure: Don't account hwpoison_filter() filtered pages

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:39PM +0800, Miaohe Lin wrote:
> mf_generic_kill_procs() will return -EOPNOTSUPP when hwpoison_filter()
> filtered dax page. In that case, action_result() isn't expected to be
> called to update mf_stats. This will results in inaccurate but benign
> memory failure handling statistics.

Nice catch.

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

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

[PATCH 4/8] mm: memory-failure: use local variable huge to check hugetlb page

[Miaohe Lin]

Use local variable huge to check whether page is hugetlb page to avoid
calling PageHuge() multiple times to save cpu cycles. PageHuge() will
be stable while extra page refcnt is held.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 905758af70f3..88e48a4801ee 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2621,7 +2621,7 @@ static int soft_offline_in_use_page(struct page *page)
 	}
 
 	lock_page(page);
-	if (!PageHuge(page))
+	if (!huge)
 		wait_on_page_writeback(page);
 	if (PageHWPoison(page)) {
 		unlock_page(page);
@@ -2630,7 +2630,7 @@ static int soft_offline_in_use_page(struct page *page)
 		return 0;
 	}
 
-	if (!PageHuge(page) && PageLRU(page) && !PageSwapCache(page))
+	if (!huge && PageLRU(page) && !PageSwapCache(page))
 		/*
 		 * Try to invalidate first. This should work for
 		 * non dirty unmapped page cache pages.
-- 
2.33.0

Re: [PATCH 4/8] mm: memory-failure: use local variable huge to check hugetlb page

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:40PM +0800, Miaohe Lin wrote:
> Use local variable huge to check whether page is hugetlb page to avoid
> calling PageHuge() multiple times to save cpu cycles. PageHuge() will
> be stable while extra page refcnt is held.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

[PATCH 5/8] mm: memory-failure: remove unneeded header files

[Miaohe Lin]

Remove some unneeded header files. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 88e48a4801ee..601936f8d30b 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -39,7 +39,6 @@
 #include <linux/kernel.h>
 #include <linux/mm.h>
 #include <linux/page-flags.h>
-#include <linux/kernel-page-flags.h>
 #include <linux/sched/signal.h>
 #include <linux/sched/task.h>
 #include <linux/dax.h>
@@ -50,7 +49,6 @@
 #include <linux/swap.h>
 #include <linux/backing-dev.h>
 #include <linux/migrate.h>
-#include <linux/suspend.h>
 #include <linux/slab.h>
 #include <linux/swapops.h>
 #include <linux/hugetlb.h>
@@ -59,7 +57,6 @@
 #include <linux/memremap.h>
 #include <linux/kfifo.h>
 #include <linux/ratelimit.h>
-#include <linux/page-isolation.h>
 #include <linux/pagewalk.h>
 #include <linux/shmem_fs.h>
 #include <linux/sysctl.h>
-- 
2.33.0

Re: [PATCH 5/8] mm: memory-failure: remove unneeded header files

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:41PM +0800, Miaohe Lin wrote:
> Remove some unneeded header files. No functional change intended.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

[PATCH 6/8] mm: memory-failure: minor cleanup for comments and codestyle

[Miaohe Lin]

Fix some wrong function names and grammar error in comments. Also remove
unneeded space after for_each_process. No functional change intended.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 601936f8d30b..0f93175ed862 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -608,7 +608,7 @@ static void collect_procs_anon(struct page *page, struct list_head *to_kill,
 
 	pgoff = page_to_pgoff(page);
 	read_lock(&tasklist_lock);
-	for_each_process (tsk) {
+	for_each_process(tsk) {
 		struct anon_vma_chain *vmac;
 		struct task_struct *t = task_early_kill(tsk, force_early);
 
@@ -652,7 +652,7 @@ static void collect_procs_file(struct page *page, struct list_head *to_kill,
 			/*
 			 * Send early kill signal to tasks where a vma covers
 			 * the page but the corrupted page is not necessarily
-			 * mapped it in its pte.
+			 * mapped in its pte.
 			 * Assume applications who requested early kill want
 			 * to be informed of all such data corruptions.
 			 */
@@ -2120,7 +2120,7 @@ static DEFINE_MUTEX(mf_mutex);
  * detected by a background scrubber)
  *
  * Must run in process context (e.g. a work queue) with interrupts
- * enabled and no spinlocks hold.
+ * enabled and no spinlocks held.
  *
  * Return: 0 for successfully handled the memory error,
  *         -EOPNOTSUPP for hwpoison_filter() filtered the error event,
@@ -2225,7 +2225,7 @@ int memory_failure(unsigned long pfn, int flags)
 		 * otherwise it may race with THP split.
 		 * And the flag can't be set in get_hwpoison_page() since
 		 * it is called by soft offline too and it is just called
-		 * for !MF_COUNT_INCREASE.  So here seems to be the best
+		 * for !MF_COUNT_INCREASED.  So here seems to be the best
 		 * place.
 		 *
 		 * Don't need care about the above error handling paths for
@@ -2582,10 +2582,10 @@ static bool isolate_page(struct page *page, struct list_head *pagelist)
 
 	/*
 	 * If we succeed to isolate the page, we grabbed another refcount on
-	 * the page, so we can safely drop the one we got from get_any_pages().
+	 * the page, so we can safely drop the one we got from get_any_page().
 	 * If we failed to isolate the page, it means that we cannot go further
 	 * and we will return an error, so drop the reference we got from
-	 * get_any_pages() as well.
+	 * get_any_page() as well.
 	 */
 	put_page(page);
 	return isolated;
-- 
2.33.0

Re: [PATCH 6/8] mm: memory-failure: minor cleanup for comments and codestyle

[Markus Elfring]

> Fix some wrong function names and grammar error in comments. Also remove
> unneeded space after for_each_process. …

Does such a change combination really fit to the known requirement
“Solve only one problem per patch.”?

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n81

Regards,
Markus

Re: [PATCH 6/8] mm: memory-failure: minor cleanup for comments and codestyle

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:42PM +0800, Miaohe Lin wrote:
> Fix some wrong function names and grammar error in comments. Also remove
> unneeded space after for_each_process. No functional change intended.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

[PATCH 7/8] mm: memory-failure: fetch compound head after extra page refcnt is held

[Miaohe Lin]

Page might become thp, huge page or being splited after compound head
is fetched but before page refcnt is bumped. So hpage might be a tail
page leading to VM_BUG_ON_PAGE(PageTail(page)) in PageTransHuge().

Fixes: 415c64c1453a ("mm/memory-failure: split thp earlier in memory error handling")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 0f93175ed862..76d88d27cdbe 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2179,8 +2179,6 @@ int memory_failure(unsigned long pfn, int flags)
 		goto unlock_mutex;
 	}
 
-	hpage = compound_head(p);
-
 	/*
 	 * We need/can do nothing about count=0 pages.
 	 * 1) it's a free page, and therefore in safe hand:
@@ -2219,6 +2217,7 @@ int memory_failure(unsigned long pfn, int flags)
 		}
 	}
 
+	hpage = compound_head(p);
 	if (PageTransHuge(hpage)) {
 		/*
 		 * The flag must be set after the refcount is bumped
-- 
2.33.0

Re: [PATCH 7/8] mm: memory-failure: fetch compound head after extra page refcnt is held

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:43PM +0800, Miaohe Lin wrote:
> Page might become thp, huge page or being splited after compound head
> is fetched but before page refcnt is bumped. So hpage might be a tail
> page leading to VM_BUG_ON_PAGE(PageTail(page)) in PageTransHuge().
> 
> Fixes: 415c64c1453a ("mm/memory-failure: split thp earlier in memory error handling")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

[PATCH 8/8] mm: memory-failure: fix race window when trying to get hugetlb folio

[Miaohe Lin]

page_folio() is fetched before calling get_hwpoison_hugetlb_folio()
without hugetlb_lock being held. So hugetlb page could be demoted
before get_hwpoison_hugetlb_folio() holding hugetlb_lock but after
page_folio() is fetched. So get_hwpoison_hugetlb_folio() will hold
unexpected extra refcnt of hugetlb folio while leaving demoted page
un-refcnted.

Fixes: 25182f05ffed ("mm,hwpoison: fix race with hugetlb page allocation")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 76d88d27cdbe..066bf57f2d22 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1388,8 +1388,14 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
 	bool hugetlb = false;
 
 	ret = get_hwpoison_hugetlb_folio(folio, &hugetlb, false);
-	if (hugetlb)
-		return ret;
+	if (hugetlb) {
+		if (folio == page_folio(page))
+			return ret;
+		if (ret > 0) {
+			folio_put(folio);
+			folio = page_folio(page);
+		}
+	}
 
 	/*
 	 * This check prevents from calling folio_try_get() for any
@@ -1478,8 +1484,12 @@ static int __get_unpoison_page(struct page *page)
 	bool hugetlb = false;
 
 	ret = get_hwpoison_hugetlb_folio(folio, &hugetlb, true);
-	if (hugetlb)
-		return ret;
+	if (hugetlb) {
+		if (folio == page_folio(page))
+			return ret;
+		if (ret > 0)
+			folio_put(folio);
+	}
 
 	/*
 	 * PageHWPoisonTakenOff pages are not only marked as PG_hwpoison,
-- 
2.33.0

Re: [PATCH 8/8] mm: memory-failure: fix race window when trying to get hugetlb folio

[Naoya Horiguchi]

On Sat, Jul 08, 2023 at 04:57:44PM +0800, Miaohe Lin wrote:
> page_folio() is fetched before calling get_hwpoison_hugetlb_folio()
> without hugetlb_lock being held. So hugetlb page could be demoted
> before get_hwpoison_hugetlb_folio() holding hugetlb_lock but after
> page_folio() is fetched. So get_hwpoison_hugetlb_folio() will hold
> unexpected extra refcnt of hugetlb folio while leaving demoted page
> un-refcnted.

Very nice, thank you for finding the issue.

> 
> Fixes: 25182f05ffed ("mm,hwpoison: fix race with hugetlb page allocation")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory-failure.c | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 76d88d27cdbe..066bf57f2d22 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1388,8 +1388,14 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
>  	bool hugetlb = false;
>  
>  	ret = get_hwpoison_hugetlb_folio(folio, &hugetlb, false);
> -	if (hugetlb)
> -		return ret;
> +	if (hugetlb) {
> +		if (folio == page_folio(page))
> +			return ret;

Some short comment about the race against demotion here is helpful.

Anyway, the patch looks good to me.

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

> +		if (ret > 0) {
> +			folio_put(folio);
> +			folio = page_folio(page);
> +		}
> +	}
>  
>  	/*
>  	 * This check prevents from calling folio_try_get() for any
> @@ -1478,8 +1484,12 @@ static int __get_unpoison_page(struct page *page)
>  	bool hugetlb = false;
>  
>  	ret = get_hwpoison_hugetlb_folio(folio, &hugetlb, true);
> -	if (hugetlb)
> -		return ret;
> +	if (hugetlb) {
> +		if (folio == page_folio(page))
> +			return ret;
> +		if (ret > 0)
> +			folio_put(folio);
> +	}
>  
>  	/*
>  	 * PageHWPoisonTakenOff pages are not only marked as PG_hwpoison,
> -- 
> 2.33.0
> 
> 
> 

Re: [PATCH 8/8] mm: memory-failure: fix race window when trying to get hugetlb folio

[Miaohe Lin]

On 2023/7/10 15:58, Naoya Horiguchi wrote:
> On Sat, Jul 08, 2023 at 04:57:44PM +0800, Miaohe Lin wrote:
>> page_folio() is fetched before calling get_hwpoison_hugetlb_folio()
>> without hugetlb_lock being held. So hugetlb page could be demoted
>> before get_hwpoison_hugetlb_folio() holding hugetlb_lock but after
>> page_folio() is fetched. So get_hwpoison_hugetlb_folio() will hold
>> unexpected extra refcnt of hugetlb folio while leaving demoted page
>> un-refcnted.
> 
> Very nice, thank you for finding the issue.
> 
>>
>> Fixes: 25182f05ffed ("mm,hwpoison: fix race with hugetlb page allocation")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/memory-failure.c | 18 ++++++++++++++----
>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 76d88d27cdbe..066bf57f2d22 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1388,8 +1388,14 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
>>  	bool hugetlb = false;
>>  
>>  	ret = get_hwpoison_hugetlb_folio(folio, &hugetlb, false);
>> -	if (hugetlb)
>> -		return ret;
>> +	if (hugetlb) {
>> +		if (folio == page_folio(page))
>> +			return ret;
> 
> Some short comment about the race against demotion here is helpful.

Does the below comment makes sense to you?

"
  Make sure hugetlb demotion did not happen from under us.
"

> 
> Anyway, the patch looks good to me.
> 
> Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

Many thanks for your review and comment, Naoya.

Re: [PATCH 8/8] mm: memory-failure: fix race window when trying to get hugetlb folio

[Naoya Horiguchi]

On Mon, Jul 10, 2023 at 04:32:27PM +0800, Miaohe Lin wrote:
> On 2023/7/10 15:58, Naoya Horiguchi wrote:
> > On Sat, Jul 08, 2023 at 04:57:44PM +0800, Miaohe Lin wrote:
> >> page_folio() is fetched before calling get_hwpoison_hugetlb_folio()
> >> without hugetlb_lock being held. So hugetlb page could be demoted
> >> before get_hwpoison_hugetlb_folio() holding hugetlb_lock but after
> >> page_folio() is fetched. So get_hwpoison_hugetlb_folio() will hold
> >> unexpected extra refcnt of hugetlb folio while leaving demoted page
> >> un-refcnted.
> > 
> > Very nice, thank you for finding the issue.
> > 
> >>
> >> Fixes: 25182f05ffed ("mm,hwpoison: fix race with hugetlb page allocation")
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >>  mm/memory-failure.c | 18 ++++++++++++++----
> >>  1 file changed, 14 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >> index 76d88d27cdbe..066bf57f2d22 100644
> >> --- a/mm/memory-failure.c
> >> +++ b/mm/memory-failure.c
> >> @@ -1388,8 +1388,14 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
> >>  	bool hugetlb = false;
> >>  
> >>  	ret = get_hwpoison_hugetlb_folio(folio, &hugetlb, false);
> >> -	if (hugetlb)
> >> -		return ret;
> >> +	if (hugetlb) {
> >> +		if (folio == page_folio(page))
> >> +			return ret;
> > 
> > Some short comment about the race against demotion here is helpful.
> 
> Does the below comment makes sense to you?
> 
> "
>   Make sure hugetlb demotion did not happen from under us.
> "

Yes, this sounds fine.

Thanks,
Naoya Horiguchi

Re: [PATCH 8/8] mm: memory-failure: fix race window when trying to get hugetlb folio

[Miaohe Lin]

On 2023/7/10 16:39, Naoya Horiguchi wrote:
> On Mon, Jul 10, 2023 at 04:32:27PM +0800, Miaohe Lin wrote:
>> On 2023/7/10 15:58, Naoya Horiguchi wrote:
>>> On Sat, Jul 08, 2023 at 04:57:44PM +0800, Miaohe Lin wrote:
>>>> page_folio() is fetched before calling get_hwpoison_hugetlb_folio()
>>>> without hugetlb_lock being held. So hugetlb page could be demoted
>>>> before get_hwpoison_hugetlb_folio() holding hugetlb_lock but after
>>>> page_folio() is fetched. So get_hwpoison_hugetlb_folio() will hold
>>>> unexpected extra refcnt of hugetlb folio while leaving demoted page
>>>> un-refcnted.
>>>
>>> Very nice, thank you for finding the issue.
>>>
>>>>
>>>> Fixes: 25182f05ffed ("mm,hwpoison: fix race with hugetlb page allocation")
>>>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>>>> ---
>>>>  mm/memory-failure.c | 18 ++++++++++++++----
>>>>  1 file changed, 14 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>> index 76d88d27cdbe..066bf57f2d22 100644
>>>> --- a/mm/memory-failure.c
>>>> +++ b/mm/memory-failure.c
>>>> @@ -1388,8 +1388,14 @@ static int __get_hwpoison_page(struct page *page, unsigned long flags)
>>>>  	bool hugetlb = false;
>>>>  
>>>>  	ret = get_hwpoison_hugetlb_folio(folio, &hugetlb, false);
>>>> -	if (hugetlb)
>>>> -		return ret;
>>>> +	if (hugetlb) {
>>>> +		if (folio == page_folio(page))
>>>> +			return ret;
>>>
>>> Some short comment about the race against demotion here is helpful.
>>
>> Does the below comment makes sense to you?
>>
>> "
>>   Make sure hugetlb demotion did not happen from under us.
>> "
> 
> Yes, this sounds fine.

Will do it in v2. Thanks.

> 
> Thanks,
> Naoya Horiguchi
> 
> .
>