Message-ID: <YyrBFvD2ID8sqhEK@kernel.org>
Date:   Wed, 21 Sep 2022 10:45:26 +0300
From:   Mike Rapoport <rppt@...nel.org>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Baoquan He <bhe@...hat.com>, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, catalin.marinas@....com,
        guanghuifeng@...ux.alibaba.com, mark.rutland@....com,
        will@...nel.org, linux-mm@...ck.org, thunder.leizhen@...wei.com,
        wangkefeng.wang@...wei.com, kexec@...ts.infradead.org
Subject: Re: [PATCH 1/2] arm64, kdump: enforce to take 4G as the crashkernel
 low memory end

On Tue, Sep 06, 2022 at 03:05:57PM +0200, Ard Biesheuvel wrote:
> 
> While I appreciate the effort that has gone into solving this problem,
> I don't think there is any consensus that an elaborate fix is required
> to ensure that the crash kernel can be unmapped from the linear map at
> all cost. In fact, I personally think we shouldn't bother, and IIRC,
> Will made a remark along the same lines back when the Huawei engineers
> were still driving this effort.
> 
> So perhaps we could align on that before doing yet another version of this?

I suggest to start with disabling crash kernel protection when its memory
reservation is deferred and then Baoquan and kdump folks can take it from
here.

>From 6430407f784f3571da9b4d79340487f2647a44ab Mon Sep 17 00:00:00 2001
From: Mike Rapoport <rppt@...ux.ibm.com>
Date: Wed, 21 Sep 2022 10:14:46 +0300
Subject: [PATCH] arm64/mm: don't protect crash kernel memory with
 CONFIG_ZONE_DMA/DMA32

Currently, in order to allow protection of crash kernel memory when
CONFIG_ZONE_DMA/DMA32 is enabled, the block mappings in the linear map are
disabled and the entire linear map uses base size pages.

This results in performance degradation because of higher TLB pressure for
kernel memory accesses, so there is a trade off between performance and
ability to protect the crash kernel memory.

Baoquan He said [1]:

	In fact, panic is a small probability event, and accidental
	corruption on kdump kernel data is a much smaller probability
	event.

With this, it makes sense to only protect crash kernel memory only when it
can be reserved before creation of the linear map.

Simplify the logic around crash kernel protection in map_mem() so that it
will use base pages only if crash kernel memory is already reserved and
introduce crashkres_protection_possible variable to ensure that
arch_kexec_protect_crashkres() and arch_kexec_unprotect_crashkres() won't
try to modify page table if crash kernel is not mapped with base pages.

[1] https://lore.kernel.org/all/Yw2C9ahluhX4Mg3G@MiWiFi-R3L-srv

Suggested-by: Will Deacon <will@...nel.org>
Signed-off-by: Mike Rapoport <rppt@...ux.ibm.com>
---
 arch/arm64/include/asm/mmu.h      |  1 +
 arch/arm64/kernel/machine_kexec.c |  6 ++++
 arch/arm64/mm/init.c              | 30 +++++++++-----------
 arch/arm64/mm/mmu.c               | 46 ++++++++-----------------------
 4 files changed, 32 insertions(+), 51 deletions(-)

diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index 48f8466a4be9..975607843548 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -71,6 +71,7 @@ extern void create_pgd_mapping(struct mm_struct *mm, phys_addr_t phys,
 extern void *fixmap_remap_fdt(phys_addr_t dt_phys, int *size, pgprot_t prot);
 extern void mark_linear_text_alias_ro(void);
 extern bool kaslr_requires_kpti(void);
+extern bool crashkres_protection_possible;
 
 #define INIT_MM_CONTEXT(name)	\
 	.pgd = init_pg_dir,
diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c
index 19c2d487cb08..68295403aa40 100644
--- a/arch/arm64/kernel/machine_kexec.c
+++ b/arch/arm64/kernel/machine_kexec.c
@@ -272,6 +272,9 @@ void arch_kexec_protect_crashkres(void)
 {
 	int i;
 
+	if (!crashkres_protection_possible)
+		return;
+
 	for (i = 0; i < kexec_crash_image->nr_segments; i++)
 		set_memory_valid(
 			__phys_to_virt(kexec_crash_image->segment[i].mem),
@@ -282,6 +285,9 @@ void arch_kexec_unprotect_crashkres(void)
 {
 	int i;
 
+	if (!crashkres_protection_possible)
+		return;
+
 	for (i = 0; i < kexec_crash_image->nr_segments; i++)
 		set_memory_valid(
 			__phys_to_virt(kexec_crash_image->segment[i].mem),
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index b9af30be813e..220d45655918 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -62,27 +62,21 @@ EXPORT_SYMBOL(memstart_addr);
  * In such case, ZONE_DMA32 covers the rest of the 32-bit addressable memory,
  * otherwise it is empty.
  *
- * Memory reservation for crash kernel either done early or deferred
- * depending on DMA memory zones configs (ZONE_DMA) --
+ * Memory reservation for crash kernel must know the upper limit of low
+ * memory in order to allow DMA access for devices with kdump kernel. When
+ * ZONE_DMA/DMA32 is enabled, this limit is determined after DT/ACPI is
+ * parsed, and crash kernel reservation happens afterwards. In this case,
+ * the crash kernel memory is reserved after linear map is created, there
+ * is no guarantee that crash kernel memory will be mapped with the base
+ * pages in the linear map, and thus the protection if the crash kernel
+ * memory is disabled.
  *
  * In absence of ZONE_DMA configs arm64_dma_phys_limit initialized
  * here instead of max_zone_phys().  This lets early reservation of
  * crash kernel memory which has a dependency on arm64_dma_phys_limit.
- * Reserving memory early for crash kernel allows linear creation of block
- * mappings (greater than page-granularity) for all the memory bank rangs.
- * In this scheme a comparatively quicker boot is observed.
- *
- * If ZONE_DMA configs are defined, crash kernel memory reservation
- * is delayed until DMA zone memory range size initialization performed in
- * zone_sizes_init().  The defer is necessary to steer clear of DMA zone
- * memory range to avoid overlap allocation.  So crash kernel memory boundaries
- * are not known when mapping all bank memory ranges, which otherwise means
- * not possible to exclude crash kernel range from creating block mappings
- * so page-granularity mappings are created for the entire memory range.
- * Hence a slightly slower boot is observed.
- *
- * Note: Page-granularity mappings are necessary for crash kernel memory
- * range for shrinking its size via /sys/kernel/kexec_crash_size interface.
+ * Reserving crash kernel memory early allows mapping it with base pages in
+ * the linear map so that it can be protected, without preventing usage of
+ * block mappings for creation of the linear map.
  */
 #if IS_ENABLED(CONFIG_ZONE_DMA) || IS_ENABLED(CONFIG_ZONE_DMA32)
 phys_addr_t __ro_after_init arm64_dma_phys_limit;
@@ -90,6 +84,8 @@ phys_addr_t __ro_after_init arm64_dma_phys_limit;
 phys_addr_t __ro_after_init arm64_dma_phys_limit = PHYS_MASK + 1;
 #endif
 
+bool __ro_after_init crashkres_protection_possible;
+
 /* Current arm64 boot protocol requires 2MB alignment */
 #define CRASH_ALIGN			SZ_2M
 
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index c5065abec55a..7b40f38dd3ee 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -502,21 +502,6 @@ void __init mark_linear_text_alias_ro(void)
 			    PAGE_KERNEL_RO);
 }
 
-static bool crash_mem_map __initdata;
-
-static int __init enable_crash_mem_map(char *arg)
-{
-	/*
-	 * Proper parameter parsing is done by reserve_crashkernel(). We only
-	 * need to know if the linear map has to avoid block mappings so that
-	 * the crashkernel reservations can be unmapped later.
-	 */
-	crash_mem_map = true;
-
-	return 0;
-}
-early_param("crashkernel", enable_crash_mem_map);
-
 static void __init map_mem(pgd_t *pgdp)
 {
 	static const u64 direct_map_end = _PAGE_END(VA_BITS_MIN);
@@ -547,13 +532,9 @@ static void __init map_mem(pgd_t *pgdp)
 	memblock_mark_nomap(kernel_start, kernel_end - kernel_start);
 
 #ifdef CONFIG_KEXEC_CORE
-	if (crash_mem_map) {
-		if (defer_reserve_crashkernel())
-			flags |= NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS;
-		else if (crashk_res.end)
-			memblock_mark_nomap(crashk_res.start,
-			    resource_size(&crashk_res));
-	}
+	if (crashk_res.end)
+		memblock_mark_nomap(crashk_res.start,
+				    resource_size(&crashk_res));
 #endif
 
 	/* map all the memory banks */
@@ -584,20 +565,17 @@ static void __init map_mem(pgd_t *pgdp)
 	memblock_clear_nomap(kernel_start, kernel_end - kernel_start);
 
 	/*
-	 * Use page-level mappings here so that we can shrink the region
-	 * in page granularity and put back unused memory to buddy system
-	 * through /sys/kernel/kexec_crash_size interface.
+	 * Use page-level mappings here so that we can protect crash kernel
+	 * memory to allow post-mortem analysis despite memory errors in
+	 * the main kernel.
 	 */
 #ifdef CONFIG_KEXEC_CORE
-	if (crash_mem_map && !defer_reserve_crashkernel()) {
-		if (crashk_res.end) {
-			__map_memblock(pgdp, crashk_res.start,
-				       crashk_res.end + 1,
-				       PAGE_KERNEL,
-				       NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
-			memblock_clear_nomap(crashk_res.start,
-					     resource_size(&crashk_res));
-		}
+	if (crashk_res.end) {
+		__map_memblock(pgdp, crashk_res.start, crashk_res.end + 1,
+			       PAGE_KERNEL, NO_BLOCK_MAPPINGS | NO_CONT_MAPPINGS);
+		memblock_clear_nomap(crashk_res.start,
+				     resource_size(&crashk_res));
+		crashkres_protection_possible = true;
 	}
 #endif
 }
-- 
2.35.3


-- 
Sincerely yours,
Mike.

Powered by blists - more mailing lists