mm, hwpoison: fix condition in free hugetlb page path
Subsystems (2)
PATCH 1/1 mm, hwpoison: fix condition in free hugetlb page path
From: Naoya Horiguchi <[email protected]>
When a memory error hits a tail page of a free hugepage, __page_handle_poison() is expected to be called to isolate the error in 4kB unit, but it's not called due to the outdated if-condition in memory_failure_hugetlb(). This loses the chance to isolate the error in the finer unit, so it's not optimal. Drop the condition.
This "(p != head && TestSetPageHWPoison(head)" condition is based on the old semantics of PageHWPoison on hugepage (where PG_hwpoison flag was set on the subpage), so it's not necessray any more. By getting to set PG_hwpoison on head page for hugepages, concurrent error events on different subpages in a single hugepage can be prevented by TestSetPageHWPoison(head) at the beginning of memory_failure_hugetlb(). So dropping the condition should not reopen the race window originally mentioned in commit b985194c8c0a ("hwpoison, hugetlb:
lock_page/unlock_page does not match for handling a free hugepage")
---
I set v5.14+ for stable trees because the base code was greatly changed by commit 0ed950d1f281 ("mm,hwpoison: make get_hwpoison_page() call get_any_page()"), and this patch is not cleanly applicable, although the original issue was introduced more previously.
--- mm/memory-failure.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
mm/memory-failure.c +7 -14
@@ -1521,24 +1521,17 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)if (!(flags &MF_COUNT_INCREASED )) { res =get_hwpoison_page (p, flags);if (!res) { -/* -* Check "filter hit" and "race with other subpage." -*/ lock_page (head); -if (PageHWPoison (head)) { -if ((hwpoison_filter (p) &&TestClearPageHWPoison (p)) - || (p != head &&TestSetPageHWPoison (head))) { +if (hwpoison_filter (p)) { +if (TestClearPageHWPoison (head))num_poisoned_pages_dec (); -unlock_page (head); -return 0 ; - } +unlock_page (head); +return 0 ; }unlock_page (head); - res =MF_FAILED ; -if (__page_handle_poison (p)) { -page_ref_inc (p); - res =MF_RECOVERED ; - } + res =MF_RECOVERED ; +if (!page_handle_poison (p, true, false)) + res =MF_FAILED ;action_result (pfn,MF_MSG_FREE_HUGE , res);return res ==MF_RECOVERED ?0 : -EBUSY ; }else if (res <0 ) {
Signature · 1 line
2.25.1
Quoted 21 lines · Naoya Horiguchi
From: Naoya Horiguchi <[email protected]>
When a memory error hits a tail page of a free hugepage,
__page_handle_poison()is expected to be called to isolate the error in 4kB unit, but it's not called due to the outdated if-condition inmemory_failure_hugetlb(). This loses the chance to isolate the error in the finer unit, so it's not optimal. Drop the condition.This "(p != head && TestSetPageHWPoison(head)" condition is based on the old semantics of PageHWPoison on hugepage (where
PG_hwpoisonflag was set on the subpage), so it's not necessray any more. By getting to setPG_hwpoisonon head page for hugepages, concurrent error events on different subpages in a single hugepage can be prevented by TestSetPageHWPoison(head) at the beginning ofmemory_failure_hugetlb(). So dropping the condition should not reopen the race window originally mentioned in commitb985194c8c0a("hwpoison, hugetlb:
lock_page/unlock_page does not match for handling a free hugepage")Reported-by Fei Luo <[email protected]>Signed-off-by Naoya Horiguchi <[email protected]>Cc <[email protected]> # v5.14+
Quoted 8 lines
---
I set v5.14+ for stable trees because the base code was greatly changed by commit0ed950d1f281("mm,hwpoison: makeget_hwpoison_page()callget_any_page()"), and this patch is not cleanly applicable, although the original issue was introduced more previously.--- mm/memory-failure.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
Thank you Naoya!
In my spare time, I have been testing memory error code for hugetlb pages. I noticed this issue and had created similar patch.
Signature · 1 line
Mike Kravetz
Quoted 53 lines · Naoya Horiguchi
From: Naoya Horiguchi <[email protected]>
When a memory error hits a tail page of a free hugepage,
__page_handle_poison()is expected to be called to isolate the error in 4kB unit, but it's not called due to the outdated if-condition inmemory_failure_hugetlb(). This loses the chance to isolate the error in the finer unit, so it's not optimal. Drop the condition.This "(p != head && TestSetPageHWPoison(head)" condition is based on the old semantics of PageHWPoison on hugepage (where
PG_hwpoisonflag was set on the subpage), so it's not necessray any more. By getting to setPG_hwpoisonon head page for hugepages, concurrent error events on different subpages in a single hugepage can be prevented by TestSetPageHWPoison(head) at the beginning ofmemory_failure_hugetlb(). So dropping the condition should not reopen the race window originally mentioned in commitb985194c8c0a("hwpoison, hugetlb:
lock_page/unlock_page does not match for handling a free hugepage")Reported-by Fei Luo <[email protected]>Signed-off-by Naoya Horiguchi <[email protected]>Cc <[email protected]> # v5.14+---
I set v5.14+ for stable trees because the base code was greatly changed by commit0ed950d1f281("mm,hwpoison: makeget_hwpoison_page()callget_any_page()"), and this patch is not cleanly applicable, although the original issue was introduced more previously.--- mm/memory-failure.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)mm/memory-failure.c +4
-9@@ -1521,24 +1521,17 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)if (!(flags &MF_COUNT_INCREASED )) { res =get_hwpoison_page (p, flags);if (!res) { -/* -* Check "filter hit" and "race with other subpage." -*/ lock_page (head); -if (PageHWPoison (head)) { -if ((hwpoison_filter (p) &&TestClearPageHWPoison (p)) - || (p != head &&TestSetPageHWPoison (head))) { +if (hwpoison_filter (p)) { +if (TestClearPageHWPoison (head))num_poisoned_pages_dec (); -unlock_page (head); -return 0 ; - } +unlock_page (head); +return 0 ;
mm/memory-failure.c static int memory_failure_hugetlb(unsigned long pfn, int flags) · line 1534
Quoted 10 lines
mm/memory-failure.c +3
-5}unlock_page (head); - res =MF_FAILED ; -if (__page_handle_poison (p)) { -page_ref_inc (p); - res =MF_RECOVERED ; - } + res =MF_RECOVERED ; +if (!page_handle_poison (p, true, false)) + res =MF_FAILED ;
Sorry, I just found that this change broke "HardwareCorrupted" counter because page_handle_poison() calls num_poisoned_pages_inc() so the counter was incremented twice in total by a single error event. page_handle_poison() is supposed to be called only in soft-offline context, and __page_handle_poison() should be called in hard-offline context.
Andrew, could you fold the following diff on mm-hwpoison-fix-condition-in-free-hugetlb-page-path.patch in your tree? (Or if desirable, I'll send a updated full patch.)
mm/memory-failure.c +5 -3
@@ -1529,9 +1529,11 @@ static int memory_failure_hugetlb(unsigned long pfn, int flags)return 0 ; }unlock_page (head); - res =MF_RECOVERED ; -if (!page_handle_poison (p, true, false)) - res =MF_FAILED ; + 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 ) {
Thanks,
Naoya Horiguchi