Skip to contentSkip to conversation

kho: align kho_scratch to MAX_ORDER_NR_PAGES pages

PATCH Superseded Michal Clapinski 3 participants 8 messages Jul 14 – Jul 17

Subsystems (3)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 kho: align kho_scratch to MAX_ORDER_NR_PAGES pages

I caught this crash:

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() 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). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages). This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")

---
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
index 4834a809985a..50928aaa3371 100644
@@ -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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))
+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.141.g00534a21ce-goog
maintainer
#1 lore raw

Hi Michal,

Quoted 40 lines

I caught this crash:

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() 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). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages). This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")
kernel/liveupdate/kexec_handover.c +0 -0
index 4834a809985ab..50928aaa33710 100644
@@ -38,6 +38,16 @@
 #include "../kexec_internal.h"
 #include "kexec_handover_internal.h"

kernel/liveupdate/kexec_handover.c line 49

Quoted 10 lines
kernel/liveupdate/kexec_handover.c +9 -0
 
+/*
+ * 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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))
+static_assert(SCRATCH_ALIGNMENT_BYTES >= CMA_MIN_ALIGNMENT_BYTES);

Maybe make it a variable and set it at runtime?
We won't need to worry about different confugurations that override MAX_ORDER and potentail divergence of pageblock size from MAX_ORDER in the future.

Signature · 2 lines
Sincerely yours,
Mike.
#2 lore raw
Quoted 44 lines · Mike Rapoport

Hi Michal,

Quoted 41 lines

I caught this crash:

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() 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). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages). This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")
kernel/liveupdate/kexec_handover.c +0 -0
index 4834a809985ab..50928aaa33710 100644
@@ -38,6 +38,16 @@
 #include "../kexec_internal.h"
 #include "kexec_handover_internal.h"
 
Quoted 14 lines

kernel/liveupdate/kexec_handover.c line 49

Quoted 9 lines
kernel/liveupdate/kexec_handover.c +9 -0
+/*
+ * 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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))
+static_assert(SCRATCH_ALIGNMENT_BYTES >= CMA_MIN_ALIGNMENT_BYTES);

Maybe make it a variable and set it at runtime?
We won't need to worry about different confugurations that override MAX_ORDER and potentail divergence of pageblock size from MAX_ORDER in the future.

Sorry, I don't track. What would I set it to at runtime? MAX_PAGE_ORDER must be >= PAGE_BLOCK_MAX_ORDER and I don't understand what's the problem if some configurations override MAX_PAGE_ORDER. They can't override it to be smaller than PAGE_BLOCK_MAX_ORDER.

Are you saying that in the future PAGE_BLOCK_MAX_ORDER could be bigger than MAX_PAGE_ORDER?

maintainer
#3 lore raw
Quoted 60 lines · Michał Cłapiński
Quoted 44 lines · Mike Rapoport

Hi Michal,

Quoted 41 lines

I caught this crash:

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() 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). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages). This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")
kernel/liveupdate/kexec_handover.c +0 -0
index 4834a809985ab..50928aaa33710 100644
@@ -38,6 +38,16 @@
 #include "../kexec_internal.h"
 #include "kexec_handover_internal.h"
 
Quoted 14 lines

kernel/liveupdate/kexec_handover.c line 49

Quoted 9 lines
kernel/liveupdate/kexec_handover.c +9 -0
+/*
+ * 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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))
+static_assert(SCRATCH_ALIGNMENT_BYTES >= CMA_MIN_ALIGNMENT_BYTES);

Maybe make it a variable and set it at runtime?
We won't need to worry about different confugurations that override MAX_ORDER and potentail divergence of pageblock size from MAX_ORDER in the future.

Quoted 7 lines

Sorry, I don't track. What would I set it to at runtime? MAX_PAGE_ORDER must be >= PAGE_BLOCK_MAX_ORDER and I don't understand what's the problem if some configurations override MAX_PAGE_ORDER. They can't override it to be smaller than PAGE_BLOCK_MAX_ORDER.

Are you saying that in the future PAGE_BLOCK_MAX_ORDER could be bigger than MAX_PAGE_ORDER?

That's very unlikely, indeed.

Signature · 2 lines
Sincerely yours,
Mike.
maintainer
#4 lore raw
In reply to Mike Rapoport
Quoted 43 lines · Mike Rapoport

Hi Michal,

Quoted 41 lines

I caught this crash:

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() 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). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages). This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")
kernel/liveupdate/kexec_handover.c +0 -0
index 4834a809985ab..50928aaa33710 100644
@@ -38,6 +38,16 @@
 #include "../kexec_internal.h"
 #include "kexec_handover_internal.h"
 
Quoted 13 lines

kernel/liveupdate/kexec_handover.c line 49

Quoted 9 lines
kernel/liveupdate/kexec_handover.c +9 -0
+/*
+ * 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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))
+static_assert(SCRATCH_ALIGNMENT_BYTES >= CMA_MIN_ALIGNMENT_BYTES);

Maybe make it a variable and set it at runtime?
We won't need to worry about different confugurations that override MAX_ORDER and potentail divergence of pageblock size from MAX_ORDER in

Do those architectures reduce MAX_ORDER when they override it? I would hope we could set the alignment once and make it large enough to work everywhere, rather than depending on dynamic changes even during boot...

Quoted 6 lines

the future.

Signature · 2 lines
Sincerely yours,
Mike.
#5 lore raw
Quoted 59 lines · Pasha Tatashin
Quoted 43 lines · Mike Rapoport

Hi Michal,

Quoted 41 lines

I caught this crash:

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() 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). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages). This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")
kernel/liveupdate/kexec_handover.c +0 -0
index 4834a809985ab..50928aaa33710 100644
@@ -38,6 +38,16 @@
 #include "../kexec_internal.h"
 #include "kexec_handover_internal.h"
 
Quoted 13 lines

kernel/liveupdate/kexec_handover.c line 49

Quoted 9 lines
kernel/liveupdate/kexec_handover.c +9 -0
+/*
+ * 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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))
+static_assert(SCRATCH_ALIGNMENT_BYTES >= CMA_MIN_ALIGNMENT_BYTES);

Maybe make it a variable and set it at runtime?
We won't need to worry about different confugurations that override MAX_ORDER and potentail divergence of pageblock size from MAX_ORDER in

Quoted 3 lines

Do those architectures reduce MAX_ORDER when they override it? I would hope we could set the alignment once and make it large enough to work everywhere, rather than depending on dynamic changes even during boot...

MAX_PAGE_ORDER can't be modified at runtime. It's either 10 or it's CONFIG_ARCH_FORCE_MAX_ORDER.

maintainer
#6 lore raw
In reply to Pasha Tatashin
Quoted 58 lines · Pasha Tatashin
Quoted 43 lines · Mike Rapoport

Hi Michal,

Quoted 41 lines

I caught this crash:

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() 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). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages). This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")
kernel/liveupdate/kexec_handover.c +0 -0
index 4834a809985ab..50928aaa33710 100644
@@ -38,6 +38,16 @@
 #include "../kexec_internal.h"
 #include "kexec_handover_internal.h"
 
Quoted 13 lines

kernel/liveupdate/kexec_handover.c line 49

Quoted 9 lines
kernel/liveupdate/kexec_handover.c +9 -0
+/*
+ * 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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))
+static_assert(SCRATCH_ALIGNMENT_BYTES >= CMA_MIN_ALIGNMENT_BYTES);

Maybe make it a variable and set it at runtime?
We won't need to worry about different confugurations that override MAX_ORDER and potentail divergence of pageblock size from MAX_ORDER in

Quoted 3 lines

Do those architectures reduce MAX_ORDER when they override it? I would hope we could set the alignment once and make it large enough to work everywhere, rather than depending on dynamic changes even during boot...

They could, e.g. arm64 has this:

config ARCH_FORCE_MAX_ORDER

	int
	default "13" if ARM64_64K_PAGES
	default "11" if ARM64_16K_PAGES
	default "10"
Quoted 6 lines
Quoted 6 lines

the future.

Signature · 2 lines
Sincerely yours,
Mike.
Signature · 2 lines
Sincerely yours,
Mike.
maintainer
#7 lore raw
In reply to Michal Clapinski
Quoted 1 line · Michal Clapinski

I caught this crash:

We are not as strict as x86 folks, but imperative voice is preferred.

Quoted 18 lines
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>
Quoted 2 lines

deferred_init_memmap_chunk() works fine without KHO because free regions will never be neighbors. However, with KHO, free memory

I'd change it to something like

deferred_init_memmap_chunk() interleaves initialization of struct pages
with freeing them. This works fine without KHO ...

Quoted 4 lines

will be split into (free && scratch) and (free && !scratch). KHO scratch is aligned to CMA_MIN_ALIGNMENT_BYTES (which on my setup is equal to 1 << 9 pages) but buddy can look at the neighborhood of MAX_ORDER_NR_PAGES pages (which is equal to 1 << 10 pages).

Please make it clearer that the values correspond to a particular configuration. E.g.

in a configuration where CMA_MIN_ALIGNMENT_PAGES is 512 and MAX_ORDER_NR_PAGES is 1024

Quoted 26 lines

This sometimes crashes when one half of the neighborhood is being initialized but the other part is yet to be initialized.

To fix this, let's just align KHO scratch to MAX_ORDER_NR_PAGES pages.

Fixes c6073743d0c7 ("kho: make preserved pages compatible with deferred struct page init")

---
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 +0 -0
index 4834a809985a..50928aaa3371 100644
@@ -38,6 +38,16 @@
 #include "../kexec_internal.h"

kernel/liveupdate/kexec_handover.c line 48

Quoted 10 lines
kernel/liveupdate/kexec_handover.c +8 -0
 #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 (1 << (PAGE_SHIFT + MAX_PAGE_ORDER))

I believe PAGE_SIZE * MAX_ORDER_NR_PAGES looks better

Quoted 55 lines
kernel/liveupdate/kexec_handover.c +8 -6
+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.141.g00534a21ce-goog
Signature · 2 lines
Sincerely yours,
Mike.