Skip to contentSkip to conversation

kho: align kho_scratch to MAX_ORDER_NR_PAGES pages

PATCH Applied Michal Clapinski 3 participants 6 messages Jul 17 – Aug 3

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

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 << 9
MAX_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.

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..175c08a6e41e 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 (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
maintainer
#1 lore raw
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_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 << 9
MAX_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.

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.

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
index 4834a809985a..175c08a6e41e 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 (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
#2 lore raw
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_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 << 9
MAX_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.

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.

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
index 4834a809985a..175c08a6e41e 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 (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

#4 lore raw
Quoted 11 lines · Mike Rapoport
Quoted 1 line · Michal Clapinski

kho: align kho_scratch to MAX_ORDER_NR_PAGES pages

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

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.

maintainer
#5 lore raw
Quoted 10 lines · Michał Cłapiński
Quoted 9 lines · Mike Rapoport
Quoted 1 line · Michal Clapinski

kho: align kho_scratch to MAX_ORDER_NR_PAGES pages

Applied to fixes branch of liveupdate/linux.git tree, thanks!

[1/1] kho: align kho_scratch to MAX_ORDER_NR_PAGES pages

      commit: c553c774c9715e1c92c48b342f30d60601693fb4
Quoted 6 lines
Quoted 2 lines

tree: 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.

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.