kho: align kho_scratch to MAX_ORDER_NR_PAGES pages
Subsystems (3)
PATCH v2 1/1 kho: align kho_scratch to MAX_ORDER_NR_PAGES pages
While booting with KHO, the following crash was observed:
BUG: unable to handle page fault for address: ff19164fffff8328 RIP: 0010:__free_one_page+0x1a1/0x6b0 Call Trace: <TASK> [<ffffffff913208bf>] free_one_page+0xaf/0x240 [<ffffffff93973288>] deferred_free_pages+0xa8/0xd0 [<ffffffff93971b4f>] deferred_init_memmap_chunk+0x10f/0x1b0 [<ffffffff9396e265>] padata_mt_helper+0x65/0xa0 [<ffffffff90fac402>] process_scheduled_works+0x202/0x410 [<ffffffff90fae739>] worker_thread+0x1f9/0x2d0 [<ffffffff90fb62fd>] kthread+0x27d/0x2f0 [<ffffffff90fae540>] ? __pfx_worker_thread+0x10/0x10 [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10 [<ffffffff90efdc55>] ret_from_fork+0x145/0x280 [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10 [<ffffffff90e2e46a>] ret_from_fork_asm+0x1a/0x30 </TASK>
deferred_init_memmap_chunk() interleaves initialization of struct pages with freeing them. This works fine without KHO because free regions will never be neighbors. However, with KHO, free memory will be split into (free && scratch) and (free && !scratch), which can be neighbors.
KHO scratch is aligned to CMA_MIN_ALIGNMENT_PAGES pages but buddy looks at the neighborhood of MAX_ORDER_NR_PAGES pages. These values are configurable but CMA_MIN_ALIGNMENT_PAGES is always less or equal to MAX_ORDER_NR_PAGES. In the crashing configuration they were set as follows:CMA_MIN_ALIGNMENT_PAGES = 1 << 9MAX_ORDER_NR_PAGES = 1 << 10
So while freeing one chunk, buddy accessed uninitialized struct pages from another chunk, tried to merge the blocks and crashed.
To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.
---
Unfortunately, this is very hard to catch, so I don't have a good reproducer. But I run the code with the fix through extensive testing and it seems fine.
I think I've never caught it before because enabling HUGETLB is what causes CMA_MIN_ALIGNMENT_PAGES to be 1 << 9, instead of 1 << 10.
If this passes review, please cherry-pick it to 7.2 so we don't release a broken kernel.
--- kernel/liveupdate/kexec_handover.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)
kernel/liveupdate/kexec_handover.c +16 -6
@@ -38,6 +38,16 @@#include "../kexec_internal.h" #include "kexec_handover_internal.h" +/* +* This is the minimal alignment required by deferred struct page init. +* deferred_init_memmap_chunk frees memory to the buddy allocator, which looks +* at the neighboring pages (up to MAX_PAGE_ORDER) to merge them. +* If KHO scratch is not aligned to that value, buddy can access uninitialized +* struct pages, which can cause a crash. +*/ +#define SCRATCH_ALIGNMENT_BYTES (PAGE_SIZE * MAX_ORDER_NR_PAGES) +static_assert (SCRATCH_ALIGNMENT_BYTES >=CMA_MIN_ALIGNMENT_BYTES ); +/* The magic token for preserved pages */ #define KHO_PAGE_MAGIC 0x4b484f50U/* ASCII for 'KHOP' */
@@ -640,8 +650,8 @@ static void __init scratch_size_update(void)* Scratch areas are released as MIGRATE_CMA. Round them up to the right * size. */ - scratch_size_lowmem =round_up (scratch_size_lowmem,CMA_MIN_ALIGNMENT_BYTES ); - scratch_size_global =round_up (scratch_size_global,CMA_MIN_ALIGNMENT_BYTES ); + scratch_size_lowmem =round_up (scratch_size_lowmem,SCRATCH_ALIGNMENT_BYTES ); + scratch_size_global =round_up (scratch_size_global,SCRATCH_ALIGNMENT_BYTES ); }static phys_addr_t __init scratch_size_node (int nid)
@@ -656,7 +666,7 @@ static phys_addr_t __init scratch_size_node(int nid) size = scratch_size_pernode; } -return round_up (size,CMA_MIN_ALIGNMENT_BYTES ); +return round_up (size,SCRATCH_ALIGNMENT_BYTES ); }/**
@@ -692,7 +702,7 @@ static void __init kho_reserve_scratch(void)* next kernel */ size = scratch_size_lowmem; - addr =memblock_phys_alloc_range (size,CMA_MIN_ALIGNMENT_BYTES ,0 , + addr =memblock_phys_alloc_range (size,SCRATCH_ALIGNMENT_BYTES ,0 ,ARCH_LOW_ADDRESS_LIMIT );if (!addr) {pr_err ("Failed to reserve lowmem scratch buffer\n" );
@@ -705,7 +715,7 @@ static void __init kho_reserve_scratch(void)/* reserve large contiguous area for allocations without nid */ size = scratch_size_global; - addr =memblock_phys_alloc (size,CMA_MIN_ALIGNMENT_BYTES ); + addr =memblock_phys_alloc (size,SCRATCH_ALIGNMENT_BYTES );if (!addr) {pr_err ("Failed to reserve global scratch buffer\n" ); goto err_free_scratch_areas;
@@ -721,7 +731,7 @@ static void __init kho_reserve_scratch(void)*/ for_each_node_state (nid,N_MEMORY ) { size =scratch_size_node (nid); - addr =memblock_alloc_range_nid (size,CMA_MIN_ALIGNMENT_BYTES , + addr =memblock_alloc_range_nid (size,SCRATCH_ALIGNMENT_BYTES ,0 ,MEMBLOCK_ALLOC_ACCESSIBLE , nid, true);if (!addr) {
Signature · 1 line
2.55.0.229.g6434b31f56-goog
Pasha Tatashin
maintainerQuoted 47 lines · Michal Clapinski
While booting with KHO, the following crash was observed:
BUG: unable to handle page fault for address: ff19164fffff8328 RIP: 0010:__free_one_page+0x1a1/0x6b0 Call Trace: <TASK> [<ffffffff913208bf>] free_one_page+0xaf/0x240 [<ffffffff93973288>] deferred_free_pages+0xa8/0xd0 [<ffffffff93971b4f>] deferred_init_memmap_chunk+0x10f/0x1b0 [<ffffffff9396e265>] padata_mt_helper+0x65/0xa0 [<ffffffff90fac402>] process_scheduled_works+0x202/0x410 [<ffffffff90fae739>] worker_thread+0x1f9/0x2d0 [<ffffffff90fb62fd>] kthread+0x27d/0x2f0 [<ffffffff90fae540>] ? __pfx_worker_thread+0x10/0x10 [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10 [<ffffffff90efdc55>] ret_from_fork+0x145/0x280 [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10 [<ffffffff90e2e46a>] ret_from_fork_asm+0x1a/0x30 </TASK>
deferred_init_memmap_chunk()interleaves initialization of struct pages with freeing them. This works fine without KHO because free regions will never be neighbors. However, with KHO, free memory will be split into (free && scratch) and (free && !scratch), which can be neighbors.KHO scratch is aligned to
CMA_MIN_ALIGNMENT_PAGESpages but buddy looks at the neighborhood ofMAX_ORDER_NR_PAGESpages. These values are configurable butCMA_MIN_ALIGNMENT_PAGESis always less or equal toMAX_ORDER_NR_PAGES. In the crashing configuration they were set as follows:CMA_MIN_ALIGNMENT_PAGES= 1 << 9MAX_ORDER_NR_PAGES= 1 << 10So while freeing one chunk, buddy accessed uninitialized struct pages from another chunk, tried to merge the blocks and crashed.
To fix this, let's just align KHO scratch to
MAX_ORDER_NR_PAGESpages.Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")Signed-off-by Michal Clapinski <[email protected]>---
Unfortunately, this is very hard to catch, so I don't have a good reproducer. But I run the code with the fix through extensive testing and it seems fine.
I think I've never caught it before because enabling HUGETLB is what causesCMA_MIN_ALIGNMENT_PAGESto be 1 << 9, instead of 1 << 10.
Quoted 2 lines
If this passes review, please cherry-pick it to 7.2 so we don't release a broken kernel.
A changelog would be super helful here.
Pasha
Quoted 75 lines
--- kernel/liveupdate/kexec_handover.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)kernel/liveupdate/kexec_handover.c +16
-6@@ -38,6 +38,16 @@#include "../kexec_internal.h" #include "kexec_handover_internal.h" +/* +* This is the minimal alignment required by deferred struct page init. +* deferred_init_memmap_chunk frees memory to the buddy allocator, which looks +* at the neighboring pages (up to MAX_PAGE_ORDER) to merge them. +* If KHO scratch is not aligned to that value, buddy can access uninitialized +* struct pages, which can cause a crash. +*/ +#define SCRATCH_ALIGNMENT_BYTES (PAGE_SIZE * MAX_ORDER_NR_PAGES) +static_assert (SCRATCH_ALIGNMENT_BYTES >=CMA_MIN_ALIGNMENT_BYTES ); +/* The magic token for preserved pages */ #define KHO_PAGE_MAGIC 0x4b484f50U/* ASCII for 'KHOP' */ @@ -640,8 +650,8 @@ static void __init scratch_size_update(void)* Scratch areas are released as MIGRATE_CMA. Round them up to the right * size. */ - scratch_size_lowmem =round_up (scratch_size_lowmem,CMA_MIN_ALIGNMENT_BYTES ); - scratch_size_global =round_up (scratch_size_global,CMA_MIN_ALIGNMENT_BYTES ); + scratch_size_lowmem =round_up (scratch_size_lowmem,SCRATCH_ALIGNMENT_BYTES ); + scratch_size_global =round_up (scratch_size_global,SCRATCH_ALIGNMENT_BYTES ); }static phys_addr_t __init scratch_size_node (int nid)@@ -656,7 +666,7 @@ static phys_addr_t __init scratch_size_node(int nid) size = scratch_size_pernode; } -return round_up (size,CMA_MIN_ALIGNMENT_BYTES ); +return round_up (size,SCRATCH_ALIGNMENT_BYTES ); }/** @@ -692,7 +702,7 @@ static void __init kho_reserve_scratch(void)* next kernel */ size = scratch_size_lowmem; - addr =memblock_phys_alloc_range (size,CMA_MIN_ALIGNMENT_BYTES ,0 , + addr =memblock_phys_alloc_range (size,SCRATCH_ALIGNMENT_BYTES ,0 ,ARCH_LOW_ADDRESS_LIMIT );if (!addr) {pr_err ("Failed to reserve lowmem scratch buffer\n" );@@ -705,7 +715,7 @@ static void __init kho_reserve_scratch(void)/* reserve large contiguous area for allocations without nid */ size = scratch_size_global; - addr =memblock_phys_alloc (size,CMA_MIN_ALIGNMENT_BYTES ); + addr =memblock_phys_alloc (size,SCRATCH_ALIGNMENT_BYTES );if (!addr) {pr_err ("Failed to reserve global scratch buffer\n" ); goto err_free_scratch_areas;@@ -721,7 +731,7 @@ static void __init kho_reserve_scratch(void)*/ for_each_node_state (nid,N_MEMORY ) { size =scratch_size_node (nid); - addr =memblock_alloc_range_nid (size,CMA_MIN_ALIGNMENT_BYTES , + addr =memblock_alloc_range_nid (size,SCRATCH_ALIGNMENT_BYTES ,0 ,MEMBLOCK_ALLOC_ACCESSIBLE , nid, true);if (!addr) {Signature · 1 line
2.55.0.229.g6434b31f56-goog
Quoted 49 lines · Pasha Tatashin
Quoted 47 lines · Michal Clapinski
While booting with KHO, the following crash was observed:
BUG: unable to handle page fault for address: ff19164fffff8328 RIP: 0010:__free_one_page+0x1a1/0x6b0 Call Trace: <TASK> [<ffffffff913208bf>] free_one_page+0xaf/0x240 [<ffffffff93973288>] deferred_free_pages+0xa8/0xd0 [<ffffffff93971b4f>] deferred_init_memmap_chunk+0x10f/0x1b0 [<ffffffff9396e265>] padata_mt_helper+0x65/0xa0 [<ffffffff90fac402>] process_scheduled_works+0x202/0x410 [<ffffffff90fae739>] worker_thread+0x1f9/0x2d0 [<ffffffff90fb62fd>] kthread+0x27d/0x2f0 [<ffffffff90fae540>] ? __pfx_worker_thread+0x10/0x10 [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10 [<ffffffff90efdc55>] ret_from_fork+0x145/0x280 [<ffffffff90fb6080>] ? __pfx_kthread+0x10/0x10 [<ffffffff90e2e46a>] ret_from_fork_asm+0x1a/0x30 </TASK>
deferred_init_memmap_chunk()interleaves initialization of struct pages with freeing them. This works fine without KHO because free regions will never be neighbors. However, with KHO, free memory will be split into (free && scratch) and (free && !scratch), which can be neighbors.KHO scratch is aligned to
CMA_MIN_ALIGNMENT_PAGESpages but buddy looks at the neighborhood ofMAX_ORDER_NR_PAGESpages. These values are configurable butCMA_MIN_ALIGNMENT_PAGESis always less or equal toMAX_ORDER_NR_PAGES. In the crashing configuration they were set as follows:CMA_MIN_ALIGNMENT_PAGES= 1 << 9MAX_ORDER_NR_PAGES= 1 << 10So while freeing one chunk, buddy accessed uninitialized struct pages from another chunk, tried to merge the blocks and crashed.
To fix this, let's just align KHO scratch to
MAX_ORDER_NR_PAGESpages.Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")Signed-off-by Michal Clapinski <[email protected]>---
Unfortunately, this is very hard to catch, so I don't have a good reproducer. But I run the code with the fix through extensive testing and it seems fine.
I think I've never caught it before because enabling HUGETLB is what causesCMA_MIN_ALIGNMENT_PAGESto be 1 << 9, instead of 1 << 10.
Quoted 4 lines
Quoted 2 lines
If this passes review, please cherry-pick it to 7.2 so we don't release a broken kernel.
A changelog would be super helful here.
Sorry, forgot.
Changelog:
- functionally it's exactly the same
- changed SCRATCH_ALIGNMENT_BYTES to be calculated using multiplication instead of binary shift
- updated the commit msg as Mike asked
- rebased on top of liveupdate/next
Quoted 77 lines
Pasha
Quoted 75 lines
--- kernel/liveupdate/kexec_handover.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)kernel/liveupdate/kexec_handover.c +16
-7@@ -38,6 +38,16 @@#include "../kexec_internal.h" #include "kexec_handover_internal.h" +/* +* This is the minimal alignment required by deferred struct page init. +* deferred_init_memmap_chunk frees memory to the buddy allocator, which looks +* at the neighboring pages (up to MAX_PAGE_ORDER) to merge them. +* If KHO scratch is not aligned to that value, buddy can access uninitialized +* struct pages, which can cause a crash. +*/ +#define SCRATCH_ALIGNMENT_BYTES (PAGE_SIZE * MAX_ORDER_NR_PAGES) +static_assert (SCRATCH_ALIGNMENT_BYTES >=CMA_MIN_ALIGNMENT_BYTES ); +/* The magic token for preserved pages */ #define KHO_PAGE_MAGIC 0x4b484f50U/* ASCII for 'KHOP' */ @@ -640,8 +650,8 @@ static void __init scratch_size_update(void)* Scratch areas are released as MIGRATE_CMA. Round them up to the right * size. */ - scratch_size_lowmem =round_up (scratch_size_lowmem,CMA_MIN_ALIGNMENT_BYTES ); - scratch_size_global =round_up (scratch_size_global,CMA_MIN_ALIGNMENT_BYTES ); + scratch_size_lowmem =round_up (scratch_size_lowmem,SCRATCH_ALIGNMENT_BYTES ); + scratch_size_global =round_up (scratch_size_global,SCRATCH_ALIGNMENT_BYTES ); }static phys_addr_t __init scratch_size_node (int nid)@@ -656,7 +666,7 @@ static phys_addr_t __init scratch_size_node(int nid) size = scratch_size_pernode; } -return round_up (size,CMA_MIN_ALIGNMENT_BYTES ); +return round_up (size,SCRATCH_ALIGNMENT_BYTES ); }/** @@ -692,7 +702,7 @@ static void __init kho_reserve_scratch(void)* next kernel */ size = scratch_size_lowmem; - addr =memblock_phys_alloc_range (size,CMA_MIN_ALIGNMENT_BYTES ,0 , + addr =memblock_phys_alloc_range (size,SCRATCH_ALIGNMENT_BYTES ,0 ,ARCH_LOW_ADDRESS_LIMIT );if (!addr) {pr_err ("Failed to reserve lowmem scratch buffer\n" );@@ -705,7 +715,7 @@ static void __init kho_reserve_scratch(void)/* reserve large contiguous area for allocations without nid */ size = scratch_size_global; - addr =memblock_phys_alloc (size,CMA_MIN_ALIGNMENT_BYTES ); + addr =memblock_phys_alloc (size,SCRATCH_ALIGNMENT_BYTES );if (!addr) {pr_err ("Failed to reserve global scratch buffer\n" ); goto err_free_scratch_areas;@@ -721,7 +731,7 @@ static void __init kho_reserve_scratch(void)*/ for_each_node_state (nid,N_MEMORY ) { size =scratch_size_node (nid); - addr =memblock_alloc_range_nid (size,CMA_MIN_ALIGNMENT_BYTES , + addr =memblock_alloc_range_nid (size,SCRATCH_ALIGNMENT_BYTES ,0 ,MEMBLOCK_ALLOC_ACCESSIBLE , nid, true);if (!addr) { --2.55.0.229.g6434b31f56-goog
Quoted 1 line · Michal Clapinski
kho: align
kho_scratchtoMAX_ORDER_NR_PAGESpages
Applied to fixes branch of liveupdate/linux.git tree, thanks!
[1/1] kho: align kho_scratch to MAX_ORDER_NR_PAGES pages
commit: c553c774c9715e1c92c48b342f30d60601693fb4
tree: https://git.kernel.org/pub/scm/linux/kernel/git/liveupdate/linux
branch: fixes
--
Sincerely yours,
Mike.
Quoted 11 lines · Mike Rapoport
Quoted 1 line · Michal Clapinski
kho: align
kho_scratchtoMAX_ORDER_NR_PAGESpagesApplied to fixes branch of liveupdate/linux.git tree, thanks!
[1/1] kho: align
kho_scratchtoMAX_ORDER_NR_PAGESpagescommit: c553c774c9715e1c92c48b342f30d60601693fb4tree: https://git.kernel.org/pub/scm/linux/kernel/git/liveupdate/linux
branch: fixes
Hey,
I see that this change (merged 2 weeks ago) didn't make it into linux rc6. Do we need to do something manually to get it into the nearest release?
Thanks.
Mike Rapoport
maintainerQuoted 10 lines · Michał Cłapiński
Quoted 9 lines · Mike Rapoport
Quoted 1 line · Michal Clapinski
kho: align
kho_scratchtoMAX_ORDER_NR_PAGESpagesApplied to fixes branch of liveupdate/linux.git tree, thanks!
[1/1] kho: align
kho_scratchtoMAX_ORDER_NR_PAGESpagescommit: c553c774c9715e1c92c48b342f30d60601693fb4
Quoted 6 lines
Quoted 2 lines
tree: https://git.kernel.org/pub/scm/linux/kernel/git/liveupdate/linux
branch: fixesHey,
I see that this change (merged 2 weeks ago) didn't make it into linux rc6.
I had a really busy week last week, sorry for the delay.
Quoted 2 lines
Do we need to do something manually to get it into the nearest release?
I already did :)
https://lore.kernel.org/all/[email protected]/
Quoted 1 line
Thanks.
Signature · 2 lines
Sincerely yours, Mike.