Re: [PATCH 1/2] kexec: fix KEXEC_FILE dependencies
From: Eric DeVolder <hidden>
Date: 2023-10-25 22:19:15
On Wednesday, October 25, 2023 at 04:58:20 AM CDT, Baoquan He [off-list ref] wrote:
On 10/24/23 at 03:17pm, Arnd Bergmann wrote:On Tue, Oct 24, 2023, at 14:44, Baoquan He wrote:quoted
Just add people and mailing list to CC since I didn't find this mail in my box, just drag it via 'b4 am'. On 10/23/23 at 01:01pm, Arnd Bergmann wrote: ......quoted
quoted
diff --git a/kernel/Kconfig.kexec b/kernel/Kconfig.kexec index 7aff28ded2f48..bfc636d64ff2b 100644 --- a/kernel/Kconfig.kexec +++ b/kernel/Kconfig.kexec@@ -36,6 +36,7 @@ config KEXEC config KEXEC_FILE bool "Enable kexec file based system call" depends on ARCH_SUPPORTS_KEXEC_FILE + depends on CRYPTO_SHA256=y || !ARCH_SUPPORTS_KEXEC_PURGATORYI am not sure if the logic is correct. In theory, kexec_file code utilizes purgatory to verify the checksum digested during kernel loading when try to jump to the kernel. That means kexec_file depends on purgatory, but not contrary?The expression I wrote is a bit confusing, but I think this just keeps the existing behavior: - on architectures that select ARCH_SUPPORTS_KEXEC_PURGATORY (powerpc, riscv, s390 and x86), we also require CRYPTO_SHA256 to be built-in. - on architectures that do not have ARCH_SUPPORTS_KEXEC_PURGATORY (arm64 and parisc), CRYPTO_SHA256 is not used and can be disabled or =m. Since ARCH_SUPPORTS_KEXEC_PURGATORY is a 'bool' symbol, it could be written as depends on (ARCH_SUPPORTS_KEXEC_PURGATORY && CRYPTO_SHA256=y) \ || !ARCH_SUPPORTS_KEXEC_PURGATORY
Yes, this seems to be clearer to me. Thanks.
if you find that clearer. I see that the second patch actually gets this wrong, it should actually do select CRYPTO if ARCH_SUPPORTS_KEXEC_PURGATORY select CRYPTO_SHA256 if ARCH_SUPPORTS_KEXEC_PURGATORY
Yeah, makes sense to me. Hi Eric, Do you have comment about these? [eric]: The original goal of the conversion patches was to consolidate, but keep existing behavior.Then I had to break existing behavior a bit by tying CRASH to KEXEC. I was hoping to haveavoided introducing new problems, but looks like there was an escape. At any rate, I thinkthe ideas here are on track and necessary. Unfortunately at the moment I'm not at a placewhere I can test.
quoted hunk ↗ jump to hunk
quoted
With these changes, we can achieve the goal to avoid building issue, whereas the code logic becomes confusing. E.g people could disable CONFIG_KEXEC_FILE, but still get purgatory code built in which is totally useless. Not sure if I think too much over this.I see your point here, and I would suggest changing the CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY symbol to just indicate the availability of the purgatory code for the arch, rather than actually controlling the code itself. I already mentioned this for s390, but riscv would need the same thing on top. I think the change below should address your concern. Arnddiff --git a/arch/riscv/kernel/elf_kexec.c b/arch/riscv/kernel/elf_kexec.c index e60fbd8660c4..3ac341d296db 100644 --- a/arch/riscv/kernel/elf_kexec.c +++ b/arch/riscv/kernel/elf_kexec.c@@ -266,7 +266,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, cmdline = modified_cmdline; } -#ifdef CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY +#ifdef CONFIG_KEXEC_FILE /* Add purgatory to the image */ kbuf.top_down = true; kbuf.mem = KEXEC_BUF_MEM_UNKNOWN;@@ -280,7 +280,7 @@ static void *elf_kexec_load(struct kimage *image, char *kernel_buf, sizeof(kernel_start), 0); if (ret) pr_err("Error update purgatory ret=%d\n", ret); -#endif /* CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY */ +#endif /* CONFIG_KEXEC_FILE */
If so, we don't need the CONFIG_KEXEC_FILE ifdeffery because the file elf_kexec.c relied on CONFIG_KEXEC_FILE enabling to build in. We can just remove the "#ifdef CONFIG_KEXEC_FILE..#endif" as x86 does.
quoted hunk ↗ jump to hunk
/* Add the initrd to the image */ if (initrd != NULL) {diff --git a/arch/riscv/Kbuild b/arch/riscv/Kbuild index d25ad1c19f88..ab181d187c23 100644 --- a/arch/riscv/Kbuild +++ b/arch/riscv/Kbuild@@ -5,7 +5,7 @@ obj-$(CONFIG_BUILTIN_DTB) += boot/dts/ obj-y += errata/ obj-$(CONFIG_KVM) += kvm/ -obj-$(CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY) += purgatory/ +obj-$(CONFIG_KEXEC_FILE) += purgatory/ # for cleaning subdir- += bootdiff --git a/arch/s390/Kbuild b/arch/s390/Kbuild index a5d3503b353c..361aa01dbd49 100644 --- a/arch/s390/Kbuild +++ b/arch/s390/Kbuild@@ -7,7 +7,7 @@ obj-$(CONFIG_S390_HYPFS) += hypfs/ obj-$(CONFIG_APPLDATA_BASE) += appldata/ obj-y += net/ obj-$(CONFIG_PCI) += pci/ -obj-$(CONFIG_ARCH_SUPPORTS_KEXEC_PURGATORY) += purgatory/ +obj-$(CONFIG_KEXEC_FILE) += purgatory/ # for cleaning subdir- += boot tools