| Message ID | 20221104113000.487098-5-coxu@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | Support kdump with LUKS encryption by reusing LUKS volume key | expand |
On 11/4/22 04:29, Coiby Xu wrote: > + if (kexec_crash_image->luks_volume_key_addr) { > + start = kexec_crash_image->luks_volume_key_addr; > + end = start + kexec_crash_image->luks_volume_key_sz - 1; > + page = pfn_to_page(start >> PAGE_SHIFT); > + nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1; > + set_memory_np((unsigned long)page_address(page), nr_pages); > + } Why does this go pfn -> page -> vaddr? What good does having the page do? Can you just do phys_to_virt() on the start address? Maybe: start_paddr = kexec_crash_image->luks_volume_key_addr; end_paddr = start_paddr + kexec_crash_image->luks_volume_key_sz - 1; nr_pages = (PAGE_ALIGN(end_paddr) - PAGE_ALIGN_DOWN(start_paddr))/ PAGE_SIZE; set_memory_np((unsigned long)phys_to_virt(start_paddr), nr_pages); Also, if you resend this, please just cc the x86 folks on the series. The other patches and cover letter have desperately needed context around this. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
Hi Dave, Thanks for the quick review! On Fri, Nov 04, 2022 at 07:38:17AM -0700, Dave Hansen wrote: >On 11/4/22 04:29, Coiby Xu wrote: >> + if (kexec_crash_image->luks_volume_key_addr) { >> + start = kexec_crash_image->luks_volume_key_addr; >> + end = start + kexec_crash_image->luks_volume_key_sz - 1; >> + page = pfn_to_page(start >> PAGE_SHIFT); >> + nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1; >> + set_memory_np((unsigned long)page_address(page), nr_pages); >> + } > >Why does this go pfn -> page -> vaddr? What good does having the page >do? Sorry it's an imitation of kexec_mark_crashkres. > Can you just do phys_to_virt() on the start address? Maybe: > > start_paddr = kexec_crash_image->luks_volume_key_addr; > end_paddr = start_paddr + kexec_crash_image->luks_volume_key_sz - 1; > nr_pages = (PAGE_ALIGN(end_paddr) - PAGE_ALIGN_DOWN(start_paddr))/ >PAGE_SIZE; > set_memory_np((unsigned long)phys_to_virt(start_paddr), nr_pages); Thanks for suggesting a smarter implementation! I'll apply it to next version. > >Also, if you resend this, please just cc the x86 folks on the series. >The other patches and cover letter have desperately needed context >around this. Sure, I'll cc the x86 list the complete patch set next time. Sorry you'll have to go to https://lore.kernel.org/lkml/20221104113000.487098-5-coxu@redhat.com/t/ to read related emails for now. >
diff --git a/arch/x86/kernel/machine_kexec_64.c b/arch/x86/kernel/machine_kexec_64.c index 0611fd83858e..f3d51c38a1c9 100644 --- a/arch/x86/kernel/machine_kexec_64.c +++ b/arch/x86/kernel/machine_kexec_64.c @@ -557,9 +557,25 @@ static void kexec_mark_crashkres(bool protect) kexec_mark_range(control, crashk_res.end, protect); } +static void kexec_mark_luks_volume_key_inaccessible(void) +{ + unsigned long start, end; + struct page *page; + unsigned int nr_pages; + + if (kexec_crash_image->luks_volume_key_addr) { + start = kexec_crash_image->luks_volume_key_addr; + end = start + kexec_crash_image->luks_volume_key_sz - 1; + page = pfn_to_page(start >> PAGE_SHIFT); + nr_pages = (end >> PAGE_SHIFT) - (start >> PAGE_SHIFT) + 1; + set_memory_np((unsigned long)page_address(page), nr_pages); + } +} + void arch_kexec_protect_crashkres(void) { kexec_mark_crashkres(true); + kexec_mark_luks_volume_key_inaccessible(); } void arch_kexec_unprotect_crashkres(void)
This adds an addition layer of protection for the saved copy of LUKS volume key. Trying to access the saved copy will cause page fault. Suggested-by: Pingfan Liu <kernelfans@gmail.com> Signed-off-by: Coiby Xu <coxu@redhat.com> --- arch/x86/kernel/machine_kexec_64.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)