mm/huge_memory: set PG_has_hwpoisoned only after new folio head is established
Subsystems (2)
PATCH 1/1 mm/huge_memory: set PG_has_hwpoisoned only after new folio head is established
__split_folio_to_order() copies the hwpoison state onto each new sub-folio while splitting a folio to a non-zero order. It did so via
if (handle_hwpoison && page_range_has_hwpoisoned(new_head, new_nr_pages)) folio_set_has_hwpoisoned(new_folio);
*before* clear_compound_head(new_head)/prep_compound_page(new_head, ...) turn @new_head from a tail page into a proper folio head.
PG_has_hwpoisoned is a FOLIO_SECOND_PAGE flag, so folio_set_has_hwpoisoned() resolves to folio_flags(folio, 1). With the new compound_info-based page-flags layout, folio_flags() asserts the page is not a tail:
VM_BUG_ON_PGFLAGS(page->compound_info & 1, page); VM_BUG_ON_PGFLAGS(n > 0 && !test_bit(PG_head, &page->flags.f), page);
At the original call site @new_head still has the tail marker (compound_info bit 0 set, PG_head clear), so on CONFIG_DEBUG_VM kernels this hits:
kernel BUG at include/linux/page-flags.h:354 folio_flags+0x82
folio_set_has_hwpoisoned
__split_folio_to_order
__split_unmapped_folio
__folio_split
truncate_inode_partial_folio (shmem hole-punch / MADV_REMOVE)
Reproduced by syzkaller: hwpoison-inject a few subpages of a large shmem folio, then MADV_REMOVE (fallocate punch hole) on the same range, which splits the partial folio to a non-zero order.
Move the folio_set_has_hwpoisoned() call to after clear_compound_head()/prep_compound_page(), where @new_folio is a real order-new_order head folio (handle_hwpoison implies new_order != 0, so a second page always exists). The flag still lands on the same struct page (page[1] of the new folio); only the ordering relative to compound-head setup changes, satisfying the FOLIO_SECOND_PAGE precondition.
--- mm/huge_memory.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
mm/huge_memory.c +12 -4
@@ -3587,10 +3587,6 @@ static void __split_folio_to_order(struct folio *folio, int old_order, (1L << PG_dropbehind) | LRU_GEN_MASK | LRU_REFS_MASK)); -if (handle_hwpoison && -page_range_has_hwpoisoned (new_head, new_nr_pages)) -folio_set_has_hwpoisoned (new_folio); - new_folio->mapping = folio->mapping; new_folio->index = folio->index + i;
@@ -3612,6 +3608,18 @@ static void __split_folio_to_order(struct folio *folio, int old_order,folio_set_large_rmappable (new_folio); } +/* +* PG_has_hwpoisoned is a FOLIO_SECOND_PAGE flag, so it can only +* be set once @new_folio is a real (head) folio. Defer setting +* it until after clear_compound_head()/prep_compound_page() have +* turned @new_head from a tail page into a proper folio head; +* otherwise folio_flags() trips on (page->compound_info & 1). +* handle_hwpoison implies new_order != 0. +*/ +if (handle_hwpoison && +page_range_has_hwpoisoned (new_head, new_nr_pages)) +folio_set_has_hwpoisoned (new_folio); +if (folio_test_young (folio))folio_set_young (new_folio);if (folio_test_idle (folio))
Signature · 1 line
2.53.0-Meta
Quoted 39 lines · Rik van Riel
__split_folio_to_order()copies the hwpoison state onto each new sub-folio while splitting a folio to a non-zero order. It did so viaif (handle_hwpoison && page_range_has_hwpoisoned(new_head, new_nr_pages)) folio_set_has_hwpoisoned(new_folio);*before* clear_compound_head(new_head)/prep_compound_page(new_head, ...) turn @new_head from a tail page into a proper folio head.
PG_has_hwpoisonedis aFOLIO_SECOND_PAGEflag, sofolio_set_has_hwpoisoned()resolves tofolio_flags(folio, 1). With the new compound_info-based page-flags layout,folio_flags()asserts the page is not a tail:VM_BUG_ON_PGFLAGS(page->compound_info & 1, page); VM_BUG_ON_PGFLAGS(n > 0 && !test_bit(PG_head, &page->flags.f), page);At the original call site @new_head still has the tail marker (
compound_infobit 0 set,PG_headclear), so onCONFIG_DEBUG_VMkernels this hits:kernel BUG at
include/linux/page-flags.h:354folio_flags+0x82
folio_set_has_hwpoisoned
__split_folio_to_order
__split_unmapped_folio
__folio_split
truncate_inode_partial_folio(shmem hole-punch /MADV_REMOVE)Reproduced by syzkaller: hwpoison-inject a few subpages of a large shmem folio, then
MADV_REMOVE(fallocate punch hole) on the same range, which splits the partial folio to a non-zero order.Move the
folio_set_has_hwpoisoned()call to after clear_compound_head()/prep_compound_page(), where @new_folio is a real order-new_order head folio (handle_hwpoisonimpliesnew_order!= 0, so a second page always exists). The flag still lands on the same struct page (page[1] of the new folio); only the ordering relative to compound-head setup changes, satisfying theFOLIO_SECOND_PAGEprecondition.
Quoted 7 lines
Signed-off-by Rik van Riel <[email protected]>Assisted-by Claude:claude-opus-4-8Fixes fa5a06170036 ("mm/huge_memory: preserve PG_has_hwpoisoned if a folio is split to >0 order")--- mm/huge_memory.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
Thanks. It makes sense to me.
Please Cc: stable since commit fa5a06170036 is back ported to older kernels.
For the backport to v6.12, the folio_set_has_hwpoisoned() is actually placed after clear_compound_head() and prep_compound_page()[1], so v6.12 does not need this fix. (Do not ask me why I did this way when I back ported the patch, since I do not know either :p )
[1] https://elixir.bootlin.com/linux/v6.12.94/source/mm/huge_memory.c#L3183
Best Regards,
Yan, Zi
Quoted 63 lines · Rik van Riel
__split_folio_to_order()copies the hwpoison state onto each new sub-folio while splitting a folio to a non-zero order. It did so viaif (handle_hwpoison && page_range_has_hwpoisoned(new_head, new_nr_pages)) folio_set_has_hwpoisoned(new_folio);*before* clear_compound_head(new_head)/prep_compound_page(new_head, ...) turn @new_head from a tail page into a proper folio head.
PG_has_hwpoisonedis aFOLIO_SECOND_PAGEflag, sofolio_set_has_hwpoisoned()resolves tofolio_flags(folio, 1). With the new compound_info-based page-flags layout,folio_flags()asserts the page is not a tail:VM_BUG_ON_PGFLAGS(page->compound_info & 1, page); VM_BUG_ON_PGFLAGS(n > 0 && !test_bit(PG_head, &page->flags.f), page);At the original call site @new_head still has the tail marker (
compound_infobit 0 set,PG_headclear), so onCONFIG_DEBUG_VMkernels this hits:kernel BUG at
include/linux/page-flags.h:354folio_flags+0x82
folio_set_has_hwpoisoned
__split_folio_to_order
__split_unmapped_folio
__folio_split
truncate_inode_partial_folio(shmem hole-punch /MADV_REMOVE)Reproduced by syzkaller: hwpoison-inject a few subpages of a large shmem folio, then
MADV_REMOVE(fallocate punch hole) on the same range, which splits the partial folio to a non-zero order.Move the
folio_set_has_hwpoisoned()call to after clear_compound_head()/prep_compound_page(), where @new_folio is a real order-new_order head folio (handle_hwpoisonimpliesnew_order!= 0, so a second page always exists). The flag still lands on the same struct page (page[1] of the new folio); only the ordering relative to compound-head setup changes, satisfying theFOLIO_SECOND_PAGEprecondition.Signed-off-by Rik van Riel <[email protected]>Assisted-by Claude:claude-opus-4-8Fixes fa5a06170036 ("mm/huge_memory: preserve PG_has_hwpoisoned if a folio is split to >0 order")--- mm/huge_memory.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)mm/huge_memory.c +0
-4@@ -3587,10 +3587,6 @@ static void __split_folio_to_order(struct folio *folio, int old_order, (1L << PG_dropbehind) | LRU_GEN_MASK | LRU_REFS_MASK)); -if (handle_hwpoison && -page_range_has_hwpoisoned (new_head, new_nr_pages)) -folio_set_has_hwpoisoned (new_folio); - new_folio->mapping = folio->mapping; new_folio->index = folio->index + i;@@ -3612,6 +3608,18 @@ static void __split_folio_to_order(struct folio *folio, int old_order,folio_set_large_rmappable (new_folio);
mm/huge_memory.c static void __split_folio_to_order(struct folio *folio, int old_order, · line 3618
Quoted 10 lines
mm/huge_memory.c +8
-0} +/* +* PG_has_hwpoisoned is a FOLIO_SECOND_PAGE flag, so it can only +* be set once @new_folio is a real (head) folio. Defer setting +* it until after clear_compound_head()/prep_compound_page() have +* turned @new_head from a tail page into a proper folio head; +* otherwise folio_flags() trips on (page->compound_info & 1). +* handle_hwpoison implies new_order != 0. +*/
This reads like an LLM comment...! A ton of noise and unnecessary detail.
How about:
/* * PG_has_hwpoisoned is on the 2nd page, so set it after * compound head prepped. */
?
Quoted 10 lines
mm/huge_memory.c +4
-1+if (handle_hwpoison && +page_range_has_hwpoisoned (new_head, new_nr_pages)) +folio_set_has_hwpoisoned (new_folio); +if (folio_test_young (folio))folio_set_young (new_folio);if (folio_test_idle (folio)) --2.53.0-Meta
Thanks, Lorenzo
Quoted 24 lines · Lorenzo Stoakes
Quoted 17 lines · Rik van Riel
mm/huge_memory.c +2
-0@@ -3612,6 +3608,18 @@ static void __split_folio_to_order(struct folio *folio, int old_order, folio_set_large_rmappable(new_folio); } +/* +* PG_has_hwpoisoned is a FOLIO_SECOND_PAGE flag, so it can only
(quoted diff) +1
-0+ * be set once @new_folio is a real (head) folio.Defer setting
(quoted diff) +1
-0+ * it until afterclear_compound_head()/prep_compound_page() have
(quoted diff) +1
-0+ * turned @new_head from a tail page into a properfolio head;
(quoted diff) +1
-0+ * otherwise folio_flags() trips on (page-Quoted 1 line
compound_info& 1).mm/huge_memory.c static void __split_folio_to_order(struct folio *folio, int old_order, · line 3618
Quoted 2 lines
mm/huge_memory.c +2
-0+* handle_hwpoison implies new_order != 0. +*/ This reads like an LLM comment...! A ton of noise and unnecessary detail.
Quoted 7 lines
How about:
/* * PG_has_hwpoisoned is on the 2nd page, so set it after * compound head prepped. */
It was an LLM comment, indeed. I harvest these syzkaller fixes from an automated run every once in a while, where the fix is "verified" by checking whether the issue can still reproduce after a tentative fix has been applied.
I've fixed up the comment as suggested, and will CC: [email protected] on v2.
thank you for the review,
Rik
Signature · 1 line
All Rights Reversed.
Quoted 28 lines · Rik van Riel
__split_folio_to_order()copies the hwpoison state onto each new sub-folio while splitting a folio to a non-zero order. It did so viaif (handle_hwpoison && page_range_has_hwpoisoned(new_head, new_nr_pages)) folio_set_has_hwpoisoned(new_folio);*before* clear_compound_head(new_head)/prep_compound_page(new_head, ...) turn @new_head from a tail page into a proper folio head.
PG_has_hwpoisonedis aFOLIO_SECOND_PAGEflag, sofolio_set_has_hwpoisoned()resolves tofolio_flags(folio, 1). With the new compound_info-based page-flags layout,folio_flags()asserts the page is not a tail:VM_BUG_ON_PGFLAGS(page->compound_info & 1, page); VM_BUG_ON_PGFLAGS(n > 0 && !test_bit(PG_head, &page->flags.f), page);At the original call site @new_head still has the tail marker (
compound_infobit 0 set,PG_headclear), so onCONFIG_DEBUG_VMkernels this hits:kernel BUG at
include/linux/page-flags.h:354folio_flags+0x82
folio_set_has_hwpoisoned
__split_folio_to_order
__split_unmapped_folio
__folio_split
truncate_inode_partial_folio(shmem hole-punch /MADV_REMOVE)
Quoted 3 lines
Reproduced by syzkaller: hwpoison-inject a few subpages of a large shmem folio, then
MADV_REMOVE(fallocate punch hole) on the same range, which splits the partial folio to a non-zero order.
Hmm, a bit weird ... for shmem, hwpoison-inject should call try_to_split_thp_page(..., 0), i.e. a uniform split to order-0, no?
So MADV_REMOVE should no longer see a large poisoned folio. Am I missing something, or is there a syzkaller link?
Quoted 11 lines
Move the
folio_set_has_hwpoisoned()call to after clear_compound_head()/prep_compound_page(), where @new_folio is a real order-new_order head folio (handle_hwpoisonimpliesnew_order!= 0, so a second page always exists). The flag still lands on the same struct page (page[1] of the new folio); only the ordering relative to compound-head setup changes, satisfying theFOLIO_SECOND_PAGEprecondition.Signed-off-by Rik van Riel <[email protected]>Assisted-by Claude:claude-opus-4-8Fixes fa5a06170036 ("mm/huge_memory: preserve PG_has_hwpoisoned if a folio is split to >0 order")---
Anyway, I used a local split_huge_pages_pid() hack to create this exact
situation: a large shmem folio with PG_has_hwpoisoned set and a poisoned subpage before the non-uniform split. The BUG is gone with this patch :)
Quoted 8 lines · Rik van Riel
__split_folio_to_order()copies the hwpoison state onto each new sub-folio while splitting a folio to a non-zero order. It did so viaif (handle_hwpoison && page_range_has_hwpoisoned(new_head, new_nr_pages)) folio_set_has_hwpoisoned(new_folio);*before* clear_compound_head(new_head)/prep_compound_page(new_head, ...) turn @new_head from a tail page into a proper folio head.
There is some grammatical issue that makes me wonder whether you are talking about the present or the past.
"copies" ... "did so" "before ... turn".
Should it be "does so" and "before ... turns"
Quoted 9 lines
PG_has_hwpoisonedis aFOLIO_SECOND_PAGEflag, sofolio_set_has_hwpoisoned()resolves tofolio_flags(folio, 1). With the new compound_info-based page-flags layout,folio_flags()asserts the page is not a tail:VM_BUG_ON_PGFLAGS(page->compound_info & 1, page); VM_BUG_ON_PGFLAGS(n > 0 && !test_bit(PG_head, &page->flags.f), page);At the original call site @new_head still has the tail marker
"current" call site?
Quoted 11 lines
(
compound_infobit 0 set,PG_headclear), so onCONFIG_DEBUG_VMkernels this hits:kernel BUG at
include/linux/page-flags.h:354folio_flags+0x82
folio_set_has_hwpoisoned
__split_folio_to_order
__split_unmapped_folio
__folio_split
truncate_inode_partial_folio(shmem hole-punch /MADV_REMOVE)
Quoted 3 lines
Reproduced by syzkaller: hwpoison-inject a few subpages of a large shmem folio, then
MADV_REMOVE(fallocate punch hole) on the same range, which splits the partial folio to a non-zero order.
As Lance says, after we do the TestSetPageHWPoison() in memory_failure(), we call try_to_split_thp_page(). Does that already suffice, even without the MADV_REMOCE.
Quoted 32 lines
Move the
folio_set_has_hwpoisoned()call to after clear_compound_head()/prep_compound_page(), where @new_folio is a real order-new_order head folio (handle_hwpoisonimpliesnew_order!= 0, so a second page always exists). The flag still lands on the same struct page (page[1] of the new folio); only the ordering relative to compound-head setup changes, satisfying theFOLIO_SECOND_PAGEprecondition.Signed-off-by Rik van Riel <[email protected]>Assisted-by Claude:claude-opus-4-8Fixes fa5a06170036 ("mm/huge_memory: preserve PG_has_hwpoisoned if a folio is split to >0 order")--- mm/huge_memory.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)mm/huge_memory.c +0
-4@@ -3587,10 +3587,6 @@ static void __split_folio_to_order(struct folio *folio, int old_order, (1L << PG_dropbehind) | LRU_GEN_MASK | LRU_REFS_MASK)); -if (handle_hwpoison && -page_range_has_hwpoisoned (new_head, new_nr_pages)) -folio_set_has_hwpoisoned (new_folio); - new_folio->mapping = folio->mapping; new_folio->index = folio->index + i;@@ -3612,6 +3608,18 @@ static void __split_folio_to_order(struct folio *folio, int old_order,folio_set_large_rmappable (new_folio);
mm/huge_memory.c static void __split_folio_to_order(struct folio *folio, int old_order, · line 3618
Quoted 10 lines
mm/huge_memory.c +8
-0} +/* +* PG_has_hwpoisoned is a FOLIO_SECOND_PAGE flag, so it can only +* be set once @new_folio is a real (head) folio. Defer setting +* it until after clear_compound_head()/prep_compound_page() have +* turned @new_head from a tail page into a proper folio head; +* otherwise folio_flags() trips on (page->compound_info & 1). +* handle_hwpoison implies new_order != 0. +*/
I prefer the shorter variant from Lorenzo.
mm/huge_memory.c static void __split_folio_to_order(struct folio *folio, int old_order, · line 3625
Quoted 7 lines
mm/huge_memory.c +4
-0+if (handle_hwpoison && +page_range_has_hwpoisoned (new_head, new_nr_pages)) +folio_set_has_hwpoisoned (new_folio); +if (folio_test_young (folio))folio_set_young (new_folio);if (folio_test_idle (folio))
LGTM. The folio is still frozen at that point.
In general
Signature · 3 lines
Cheers, David
Quoted 5 lines · David Hildenbrand (Arm)
There is some grammatical issue that makes me wonder whether you are talking
about the present or the past.Quoted 1 line
Changelog issues fixed for v2. Thank you.
Quoted 11 lines
Quoted 5 lines
Reproduced by syzkaller: hwpoison-inject a few subpages of a large shmem
folio, thenMADV_REMOVE(fallocate punch hole) on the same range, which
splits the partial folio to a non-zero order.As Lance says, after we do the
TestSetPageHWPoison()inmemory_failure(), we
calltry_to_split_thp_page(). Does that already suffice, even without theMADV_REMOCE.
While fa5a06170036 was created to deal with having the folio split to >0 order, in this case it looks like the try_to_split_thp_page() call from memory_failure() failed, presumably because folio_ref_freeze() could not freeze the order 7 folio, because the reproducer had multiple threads poking at that folio simultaneously.
This test case is rather contrived, like most syzkaller tests, but given that the try_to_split_thp_page() call could fail for various reasons, we do need this fallback path.
Signature · 1 line
All Rights Reversed.
Lance Yang
reviewerQuoted 28 lines · Rik van Riel
Quoted 5 lines · David Hildenbrand (Arm)
There is some grammatical issue that makes me wonder whether you are talking
about the present or the past.Quoted 1 line
Changelog issues fixed for v2. Thank you.
Quoted 11 lines
Quoted 5 lines
Reproduced by syzkaller: hwpoison-inject a few subpages of a large shmem
folio, thenMADV_REMOVE(fallocate punch hole) on the same range, which
splits the partial folio to a non-zero order.As Lance says, after we do the
TestSetPageHWPoison()inmemory_failure(), we
calltry_to_split_thp_page(). Does that already suffice, even without theMADV_REMOCE.While
fa5a06170036was created to deal with having the folio split to >0 order, in this case it looks like thetry_to_split_thp_page()call frommemory_failure()failed, presumably becausefolio_ref_freeze()could not freeze the order 7 folio, because the reproducer had multiple threads poking at that folio simultaneously.
Quoted 4 lines
This test case is rather contrived, like most syzkaller tests, but given that the
try_to_split_thp_page()call could fail for various reasons, we do need this fallback path.
Should be rare, but yeah, there it is. Wondered if the hwpoison-time split had failed somehow ... turns out it did :)
Thanks for spelling that out!