From: takahiro.akashi@linaro.org (AKASHI Takahiro) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v30 05/11] arm64: kdump: protect crash dump kernel memory Date: Thu, 26 Jan 2017 20:28:12 +0900 [thread overview] Message-ID: <20170126112811.GG23406@linaro.org> (raw) In-Reply-To: <5888E262.4050208@arm.com> James, I will try to revisit your comments later, but quick replies now On Wed, Jan 25, 2017 at 05:37:38PM +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 > > - move copying kexec's control_code_page to machine_kexec_prepare() > > so that the region will be completely read-only after loading. > > > > Note that the region must reside in linear mapping and have corresponding > > page structures in order to be potentially freed by shrinking it through > > /sys/kernel/kexec_crash_size. > > Nasty! Presumably you have to build the crash region out of individual page > mappings, This might be an alternative, but > so that they can be returned to the slab-allocator one page at a time, > and still be able to set/clear the valid bits on the remaining chunk. > (I don't see how that happens in this patch) As far as shrinking feature is concerned, I believe, crash_shrink_memory(), which eventually calls free_reserved_page(), will take care of all the things to do. I can see increased number of "MemFree" in /proc/meminfo. (Please note that the region is memblock_reserve()'d at boot time.) > debug_pagealloc has to do this too so it can flip the valid bits one page at a > time. You could change the debug_pagealloc_enabled() value passed in at the top > __create_pgd_mapping() level to be a needs_per_page_mapping(addr, size) test > that happens as we build the linear map. (This would save the 3 extra calls to > __create_pgd_mapping() in __map_memblock()) > > I'm glad to see you can't resize the region if a crash kernel is loaded! > > This secretly-unmapped is the sort of thing that breaks hibernate, it blindly > assumes pfn_valid() means it can access the page if it wants to. Setting > PG_Reserved is a quick way to trick it out of doing this, but that would leave > the crash kernel region un-initialised after resume, while kexec_crash_image > still has a value. Ouch, I didn't notice this issue. > I think the best fix for this is to forbid hibernate if kexec_crash_loaded() > arguing these are mutually-exclusive features, and the protect crash-dump > feature exists to prevent things like hibernate corrupting the crash region. This restriction is really painful. Is there any hibernation hook that will be invoked before suspending and after resuming? If so, arch_kexec_unprotect_crashkres()/protect_crashkres() will be able to be called. Or if "read-only (without unmapping)" approach would be acceptable, those two features might be no longer mutually-exclusive. > > > > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > > index bc96c8a7fc79..f7938fecf3ff 100644 > > --- a/arch/arm64/kernel/machine_kexec.c > > +++ b/arch/arm64/kernel/machine_kexec.c > > @@ -159,32 +171,20 @@ void machine_kexec(struct kimage *kimage) > > kimage->control_code_page); > > pr_debug("%s:%d: reboot_code_buffer_phys: %pa\n", __func__, __LINE__, > > &reboot_code_buffer_phys); > > - pr_debug("%s:%d: reboot_code_buffer: %p\n", __func__, __LINE__, > > - reboot_code_buffer); > > pr_debug("%s:%d: relocate_new_kernel: %p\n", __func__, __LINE__, > > arm64_relocate_new_kernel); > > pr_debug("%s:%d: relocate_new_kernel_size: 0x%lx(%lu) bytes\n", > > __func__, __LINE__, arm64_relocate_new_kernel_size, > > arm64_relocate_new_kernel_size); > > > > - /* > > - * Copy arm64_relocate_new_kernel to the reboot_code_buffer for use > > - * after the kernel is shut down. > > - */ > > - memcpy(reboot_code_buffer, arm64_relocate_new_kernel, > > - arm64_relocate_new_kernel_size); > > - > > - /* Flush the reboot_code_buffer in preparation for its execution. */ > > - __flush_dcache_area(reboot_code_buffer, arm64_relocate_new_kernel_size); > > - flush_icache_range((uintptr_t)reboot_code_buffer, > > - arm64_relocate_new_kernel_size); > > > > > - /* Flush the kimage list and its buffers. */ > > - kexec_list_flush(kimage); > > + if (kimage != kexec_crash_image) { > > + /* Flush the kimage list and its buffers. */ > > + kexec_list_flush(kimage); > > > > - /* Flush the new image if already in place. */ > > - if (kimage->head & IND_DONE) > > - kexec_segment_flush(kimage); > > + /* Flush the new image if already in place. */ > > + if (kimage->head & IND_DONE) > > + kexec_segment_flush(kimage); > > + } > > So for kdump we cleaned the kimage->segment[i].mem regions in > arch_kexec_protect_crashkres(), so don't need to do it here. Correct. > What about the kimage->head[i] array of list entries that were cleaned by > kexec_list_flush()? Now we don't clean that for kdump either, but we do pass it > arm64_relocate_new_kernel() at the end of this function: > > cpu_soft_restart(1, reboot_code_buffer_phys, kimage->head, kimage_start, 0); Kimage->head holds a list of memory regions that are overlapped between the primary kernel and the secondary kernel, but in kedump case, the whole memory is isolated and the list should be empty. That is why kexec_list_flush() is skipped here, but yes, "kimage->head" might better be cleaned anyway. (I believe, from the past discussions, that cache coherency is still maintained in kdump case though.) > Can we test the IND_DONE_BIT of kimage->head, so that we know that > arm64_relocate_new_kernel() won't try to walk the unclean list? > Alternatively we could call kexec_list_flush() in arch_kexec_protect_crashkres() > too. So call kexec_list_flush() in machine_kexec() either in kexec or kdump. Thanks, -Takahiro AKASHI > > > Thanks, > > James >
WARNING: multiple messages have this Message-ID (diff)
From: AKASHI Takahiro <takahiro.akashi@linaro.org> To: James Morse <james.morse@arm.com> Cc: mark.rutland@arm.com, geoff@infradead.org, catalin.marinas@arm.com, will.deacon@arm.com, bauerman@linux.vnet.ibm.com, dyoung@redhat.com, kexec@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v30 05/11] arm64: kdump: protect crash dump kernel memory Date: Thu, 26 Jan 2017 20:28:12 +0900 [thread overview] Message-ID: <20170126112811.GG23406@linaro.org> (raw) In-Reply-To: <5888E262.4050208@arm.com> James, I will try to revisit your comments later, but quick replies now On Wed, Jan 25, 2017 at 05:37:38PM +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 > > - move copying kexec's control_code_page to machine_kexec_prepare() > > so that the region will be completely read-only after loading. > > > > Note that the region must reside in linear mapping and have corresponding > > page structures in order to be potentially freed by shrinking it through > > /sys/kernel/kexec_crash_size. > > Nasty! Presumably you have to build the crash region out of individual page > mappings, This might be an alternative, but > so that they can be returned to the slab-allocator one page at a time, > and still be able to set/clear the valid bits on the remaining chunk. > (I don't see how that happens in this patch) As far as shrinking feature is concerned, I believe, crash_shrink_memory(), which eventually calls free_reserved_page(), will take care of all the things to do. I can see increased number of "MemFree" in /proc/meminfo. (Please note that the region is memblock_reserve()'d at boot time.) > debug_pagealloc has to do this too so it can flip the valid bits one page at a > time. You could change the debug_pagealloc_enabled() value passed in at the top > __create_pgd_mapping() level to be a needs_per_page_mapping(addr, size) test > that happens as we build the linear map. (This would save the 3 extra calls to > __create_pgd_mapping() in __map_memblock()) > > I'm glad to see you can't resize the region if a crash kernel is loaded! > > This secretly-unmapped is the sort of thing that breaks hibernate, it blindly > assumes pfn_valid() means it can access the page if it wants to. Setting > PG_Reserved is a quick way to trick it out of doing this, but that would leave > the crash kernel region un-initialised after resume, while kexec_crash_image > still has a value. Ouch, I didn't notice this issue. > I think the best fix for this is to forbid hibernate if kexec_crash_loaded() > arguing these are mutually-exclusive features, and the protect crash-dump > feature exists to prevent things like hibernate corrupting the crash region. This restriction is really painful. Is there any hibernation hook that will be invoked before suspending and after resuming? If so, arch_kexec_unprotect_crashkres()/protect_crashkres() will be able to be called. Or if "read-only (without unmapping)" approach would be acceptable, those two features might be no longer mutually-exclusive. > > > > diff --git a/arch/arm64/kernel/machine_kexec.c b/arch/arm64/kernel/machine_kexec.c > > index bc96c8a7fc79..f7938fecf3ff 100644 > > --- a/arch/arm64/kernel/machine_kexec.c > > +++ b/arch/arm64/kernel/machine_kexec.c > > @@ -159,32 +171,20 @@ void machine_kexec(struct kimage *kimage) > > kimage->control_code_page); > > pr_debug("%s:%d: reboot_code_buffer_phys: %pa\n", __func__, __LINE__, > > &reboot_code_buffer_phys); > > - pr_debug("%s:%d: reboot_code_buffer: %p\n", __func__, __LINE__, > > - reboot_code_buffer); > > pr_debug("%s:%d: relocate_new_kernel: %p\n", __func__, __LINE__, > > arm64_relocate_new_kernel); > > pr_debug("%s:%d: relocate_new_kernel_size: 0x%lx(%lu) bytes\n", > > __func__, __LINE__, arm64_relocate_new_kernel_size, > > arm64_relocate_new_kernel_size); > > > > - /* > > - * Copy arm64_relocate_new_kernel to the reboot_code_buffer for use > > - * after the kernel is shut down. > > - */ > > - memcpy(reboot_code_buffer, arm64_relocate_new_kernel, > > - arm64_relocate_new_kernel_size); > > - > > - /* Flush the reboot_code_buffer in preparation for its execution. */ > > - __flush_dcache_area(reboot_code_buffer, arm64_relocate_new_kernel_size); > > - flush_icache_range((uintptr_t)reboot_code_buffer, > > - arm64_relocate_new_kernel_size); > > > > > - /* Flush the kimage list and its buffers. */ > > - kexec_list_flush(kimage); > > + if (kimage != kexec_crash_image) { > > + /* Flush the kimage list and its buffers. */ > > + kexec_list_flush(kimage); > > > > - /* Flush the new image if already in place. */ > > - if (kimage->head & IND_DONE) > > - kexec_segment_flush(kimage); > > + /* Flush the new image if already in place. */ > > + if (kimage->head & IND_DONE) > > + kexec_segment_flush(kimage); > > + } > > So for kdump we cleaned the kimage->segment[i].mem regions in > arch_kexec_protect_crashkres(), so don't need to do it here. Correct. > What about the kimage->head[i] array of list entries that were cleaned by > kexec_list_flush()? Now we don't clean that for kdump either, but we do pass it > arm64_relocate_new_kernel() at the end of this function: > > cpu_soft_restart(1, reboot_code_buffer_phys, kimage->head, kimage_start, 0); Kimage->head holds a list of memory regions that are overlapped between the primary kernel and the secondary kernel, but in kedump case, the whole memory is isolated and the list should be empty. That is why kexec_list_flush() is skipped here, but yes, "kimage->head" might better be cleaned anyway. (I believe, from the past discussions, that cache coherency is still maintained in kdump case though.) > Can we test the IND_DONE_BIT of kimage->head, so that we know that > arm64_relocate_new_kernel() won't try to walk the unclean list? > Alternatively we could call kexec_list_flush() in arch_kexec_protect_crashkres() > too. So call kexec_list_flush() in machine_kexec() either in kexec or kdump. Thanks, -Takahiro AKASHI > > > Thanks, > > James > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2017-01-26 11:28 UTC|newest] Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-01-24 8:46 [PATCH v30 00/11] arm64: add kdump support AKASHI Takahiro 2017-01-24 8:46 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 01/11] memblock: add memblock_cap_memory_range() AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 02/11] arm64: limit memory regions based on DT property, usable-memory-range AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 03/11] arm64: kdump: reserve memory for crash dump kernel AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 04/11] arm64: mm: allow for unmapping memory region from kernel mapping AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-24 11:32 ` Pratyush Anand 2017-01-24 11:32 ` Pratyush Anand 2017-01-25 6:37 ` AKASHI Takahiro 2017-01-25 6:37 ` AKASHI Takahiro 2017-01-25 15:49 ` James Morse 2017-01-25 15:49 ` James Morse 2017-01-26 8:08 ` AKASHI Takahiro 2017-01-26 8:08 ` AKASHI Takahiro 2017-01-24 8:49 ` [PATCH v30 05/11] arm64: kdump: protect crash dump kernel memory AKASHI Takahiro 2017-01-24 8:49 ` AKASHI Takahiro 2017-01-25 17:37 ` James Morse 2017-01-25 17:37 ` James Morse 2017-01-26 11:28 ` AKASHI Takahiro [this message] 2017-01-26 11:28 ` AKASHI Takahiro 2017-01-27 11:19 ` James Morse 2017-01-27 11:19 ` James Morse 2017-01-27 17:15 ` AKASHI Takahiro 2017-01-27 17:15 ` AKASHI Takahiro 2017-01-27 18:56 ` Mark Rutland 2017-01-27 18:56 ` Mark Rutland 2017-01-30 8:42 ` AKASHI Takahiro 2017-01-30 8:42 ` AKASHI Takahiro 2017-01-30 8:27 ` AKASHI Takahiro 2017-01-30 8:27 ` AKASHI Takahiro 2017-01-27 13:59 ` James Morse 2017-01-27 13:59 ` James Morse 2017-01-27 15:42 ` AKASHI Takahiro 2017-01-27 15:42 ` AKASHI Takahiro 2017-01-27 19:41 ` Mark Rutland 2017-01-27 19:41 ` Mark Rutland 2017-01-24 8:50 ` [PATCH v30 06/11] arm64: kdump: implement machine_crash_shutdown() AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 07/11] arm64: kdump: add VMCOREINFO's for user-space tools AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 08/11] arm64: kdump: provide /proc/vmcore file AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 09/11] arm64: kdump: enable kdump in defconfig AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro 2017-01-24 8:50 ` [PATCH v30 10/11] Documentation: kdump: describe arm64 port AKASHI Takahiro 2017-01-24 8:50 ` AKASHI Takahiro [not found] ` <20170124084638.3770-1-takahiro.akashi-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 2017-01-24 8:53 ` [PATCH v30 11/11] Documentation: dt: chosen properties for arm64 kdump AKASHI Takahiro 2017-01-24 8:53 ` AKASHI Takahiro 2017-01-24 8:53 ` AKASHI Takahiro
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170126112811.GG23406@linaro.org \ --to=takahiro.akashi@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.