[PATCH v30 05/11] arm64: kdump: protect crash dump kernel memory

[takahiro.akashi@linaro.org (AKASHI Takahiro)]

James,

On Fri, Jan 27, 2017 at 01:59:05PM +0000, James Morse wrote:
> Hi Akashi,
> 
> On 24/01/17 08:49, AKASHI Takahiro wrote:
> > To protect the memory reserved for crash dump kernel once after loaded,
> > arch_kexec_protect_crashres/unprotect_crashres() are meant to deal with
> > permissions of the corresponding kernel mappings.
> > 
> > We also have to
> > - put the region in an isolated mapping, and
> 
> > diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> > index 9c7adcce8e4e..2d4a0b68a852 100644
> > --- a/arch/arm64/mm/mmu.c
> > +++ b/arch/arm64/mm/mmu.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/errno.h>
> >  #include <linux/init.h>
> > +#include <linux/kexec.h>
> >  #include <linux/libfdt.h>
> >  #include <linux/mman.h>
> >  #include <linux/nodemask.h>
> > @@ -367,6 +368,39 @@ static void __init __map_memblock(pgd_t *pgd, phys_addr_t start, phys_addr_t end
> >  	unsigned long kernel_start = __pa(_text);
> >  	unsigned long kernel_end = __pa(__init_begin);
> >  
> > +#ifdef CONFIG_KEXEC_CORE
> > +	/*
> > +	 * While crash dump kernel memory is contained in a single memblock
> > +	 * for now, it should appear in an isolated mapping so that we can
> > +	 * independently unmap the region later.
> > +	 */
> > +	if (crashk_res.end && crashk_res.start >= start &&
> > +	    crashk_res.end <= end) {
> > +		if (crashk_res.start != start)
> > +			__create_pgd_mapping(pgd, start, __phys_to_virt(start),
> > +					     crashk_res.start - start,
> > +					     PAGE_KERNEL,
> > +					     early_pgtable_alloc,
> > +					     debug_pagealloc_enabled());
> > +
> > +		/* before kexec_load(), the region can be read-writable. */
> > +		__create_pgd_mapping(pgd, crashk_res.start,
> > +				     __phys_to_virt(crashk_res.start),
> > +				     crashk_res.end - crashk_res.start + 1,
> > +				     PAGE_KERNEL, early_pgtable_alloc,
> > +				     debug_pagealloc_enabled());
> > +
> > +		if (crashk_res.end != end)
> > +			__create_pgd_mapping(pgd, crashk_res.end + 1,
> > +					     __phys_to_virt(crashk_res.end + 1),
> > +					     end - crashk_res.end - 1,
> > +					     PAGE_KERNEL,
> > +					     early_pgtable_alloc,
> > +					     debug_pagealloc_enabled());
> 
> > +		return;
> 
> Doesn't this mean we skip all the 'does this overlap with the kernel text' tests
> that happen further down in this file?

You're right. We should still ckeck the overlap against
[start..crashk_res.start] and [crashk_res.end+1..end].

(Using memblock_isolate_range() could simplify the code.)

> (I think this can be fixed by replacing page_mappings_only with something
>  like needs_per_page_mapping(addr, size) called when we try to place a block
>  mapping. needs_per_page_mapping() can then take kdump and debug_pagealloc into
>  account.)
>
> 
> I see boot failures on v4.10-rc5, with this series (and your fixup diff for
> patch 4). I'm using defconfig with 64K pages and 42bit VA on Juno. I pass
> 'crashkernel=1G':
> [    0.000000] efi: Getting EFI parameters from FDT:
> [    0.000000] efi: EFI v2.50 by ARM Juno EFI Nov 24 2015 12:36:35
> [    0.000000] efi:  ACPI=0xf95b0000  ACPI 2.0=0xf95b0014  PROP=0xfe8db4d8
> [    0.000000] Reserving 1024MB of memory at 2560MB for crashkernel
> [    0.000000] cma: Failed to reserve 512 MiB
> [    0.000000] ------------[ cut here ]------------
> [    0.000000] kernel BUG at ../arch/arm64/mm/mmu.c:118!
> [    0.000000] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
> [    0.000000] Modules linked in:
> [    0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.10.0-rc5-00012-gdd6fe39
> 0c85d #6873
> [    0.000000] Hardware name: ARM Juno development board (r1) (DT)
> [    0.000000] task: fffffc0008e2c900 task.stack: fffffc0008df0000
> [    0.000000] PC is at __create_pgd_mapping+0x2c8/0x2e8
> [    0.000000] LR is at paging_init+0x2b0/0x6fc
> [    0.000000] pc : [<fffffc0008096b20>] lr : [<fffffc0008ce5a60>] pstate: 60000
> 0c5
> [    0.000000] sp : fffffc0008df3e20
> [    0.000000] x29: fffffc0008df3e20 x28: 00e8000000000713
> [    0.000000] x27: 0000000000000001 x26: 00000000e00f0000
> [    0.000000] x25: fffffe00794a0000 x24: fffffe00794a0000
> [    0.000000] x23: fffffe00600f0000 x22: 00e80000e0000711
> [    0.000000] x21: 0000000000000000 x20: fffffdff7e5e8018
> [    0.000000] x19: 000000000000e00f x18: 0000000000000000
> [    0.000000] x17: fffffc0008f61cd0 x16: 0000000000000005
> [    0.000000] x15: 0000000880000000 x14: 00000000001fffff
> [    0.000000] x13: 00f8000000000713 x12: fffffc0008e3d000
> [    0.000000] x11: 0000000000000801 x10: 00e8000000000713
> [    0.000000] x9 : 0000000000000000 x8 : 0000000000001003
> [    0.000000] x7 : 0000000000000000 x6 : 0000000000000000
> [    0.000000] x5 : fffffc0008ce5744 x4 : 00e8000000000713
> [    0.000000] x3 : fffffe007949ffff x2 : fffffe00e00f0000
> [    0.000000] x1 : 00e8000000000713 x0 : 0000000000000001
> [    0.000000]
> [    0.000000] Process swapper (pid: 0, stack limit = 0xfffffc0008df0000)
> [    0.000000] Stack: (0xfffffc0008df3e20 to 0xfffffc0008df4000)
> 
> [    0.000000] Call trace:
> 
> [    0.000000] [<fffffc0008096b20>] __create_pgd_mapping+0x2c8/0x2e8
> [    0.000000] [<fffffc0008ce5a60>] paging_init+0x2b0/0x6fc
> [    0.000000] [<fffffc0008ce27d0>] setup_arch+0x1c0/0x5ac
> [    0.000000] [<fffffc0008ce0838>] start_kernel+0x70/0x394
> [    0.000000] [<fffffc0008ce01e8>] __primary_switched+0x64/0x6c
> [    0.000000] Code: f29fefe1 ea01001f 54ffff00 d4210000 (d4210000)
> [    0.000000] ---[ end trace 0000000000000000 ]---
> [    0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
> [    0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
> 
> 
> Adding some debug shows the crash region was allocated as 0xa0000000:0xdfffffff,
> and the first memblock to hit __map_memblock() was 0x80000000:0xe0000000.
> 
> This causes the end of the crash region to be padded, as 0xdfffffff!=0xe0000000,
> but your 'crashk_res.end + 1' actually mapped 0xe0000000 to 0xe0000000 with 0
> size. This causes __create_pgd_mapping() to choke when it next uses 0xe0000000
> as a start address, as it evidently mapped something when given a 0 size.
> 
> You need to round-up crashk_res.end to a a page boundary if it isn't already
> aligned.

The start address is already enforced to be 2MB aligned and the size of
region (hence start + size) is also page-size aligned. (See patch#3)
Since crashk_res.end is 'inclusive,' the fix would be

> > +		if (crashk_res.end != end)
> > +			__create_pgd_mapping(pgd, crashk_res.end + 1,

To
		if ((crashk_res.end + 1) < end)

Thanks,
-Takahiro AKASHI

> 
> 
> > +	}
> > +#endif
> > +
> >  	/*
> >  	 * Take care not to create a writable alias for the
> >  	 * read-only text and rodata sections of the kernel image.
> > 
> 
> Thanks,
> 
> James