| Message ID | 20231023110308.1202042-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [1/2] kexec: fix KEXEC_FILE dependencies | expand |
On Mon, Oct 23, 2023 at 01:01:54PM +0200, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > The cleanup for the CONFIG_KEXEC Kconfig logic accidentally changed the > 'depends on CRYPTO=y' dependency to a plain 'depends on CRYPTO', which > causes a link failure when all the crypto support is in a loadable module > and kexec_file support is built-in: > > x86_64-linux-ld: vmlinux.o: in function `__x64_sys_kexec_file_load': > (.text+0x32e30a): undefined reference to `crypto_alloc_shash' > x86_64-linux-ld: (.text+0x32e58e): undefined reference to `crypto_shash_update' > x86_64-linux-ld: (.text+0x32e6ee): undefined reference to `crypto_shash_final' > > Both s390 and x86 have this problem, while ppc64 and riscv have the > correct dependency already. On riscv, the dependency is only used > for the purgatory, not for the kexec_file code itself, which may > be a bit surprising as it means that with CONFIG_CRYPTO=m, it is > possible to enable KEXEC_FILE but then the purgatory code is silently > left out. > > Move this into the common Kconfig.kexec file in a way that is > correct everywhere, using the dependency on CRYPTO_SHA256=y only > when the purgatory code is available. This requires reversing the > dependency between ARCH_SUPPORTS_KEXEC_PURGATORY and KEXEC_FILE, > but the effect remains the same, other than making riscv behave > like the other ones. > > On s390, there is an additional dependency on CRYPTO_SHA256_S390, which > should technically not be required but gives better performance. Remove > this dependency here, noting that it was not present in the initial > Kconfig code but was brought in without an explanation in commit > 71406883fd357 ("s390/kexec_file: Add kexec_file_load system call"). > > Fixes: 6af5138083005 ("x86/kexec: refactor for kernel/Kconfig.kexec") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/powerpc/Kconfig | 4 ++-- > arch/riscv/Kconfig | 4 +--- This doesn't appear to work for rv32. The defconfig build in our patchwork automation complains: /tmp/tmp.Aq21JVRQTx/arch/riscv/purgatory/entry.S:29:2: error: instruction requires the following: RV64I Base Instruction Set > arch/s390/Kconfig | 4 ++-- > arch/x86/Kconfig | 4 ++-- > kernel/Kconfig.kexec | 1 + > 5 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index d5d5388973ac7..4640cee33f123 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -607,10 +607,10 @@ config ARCH_SUPPORTS_KEXEC > def_bool PPC_BOOK3S || PPC_E500 || (44x && !SMP) > > config ARCH_SUPPORTS_KEXEC_FILE > - def_bool PPC64 && CRYPTO=y && CRYPTO_SHA256=y > + def_bool PPC64 > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > + def_bool y > > config ARCH_SELECTS_KEXEC_FILE > def_bool y > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 25474f8c12b79..f571bad2d22d0 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -687,9 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE > select KEXEC_ELF > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > - depends on CRYPTO=y > - depends on CRYPTO_SHA256=y > + def_bool y This being the problem, KEXEC_FILE is 64-bit only. IIRC I commented on this same thing during the original conversion patches. Cheers, Conor. > > config ARCH_SUPPORTS_CRASH_DUMP > def_bool y > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig > index b0d67ac8695f9..ec77106af4137 100644 > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -253,13 +253,13 @@ config ARCH_SUPPORTS_KEXEC > def_bool y > > config ARCH_SUPPORTS_KEXEC_FILE > - def_bool CRYPTO && CRYPTO_SHA256 && CRYPTO_SHA256_S390 > + def_bool y > > config ARCH_SUPPORTS_KEXEC_SIG > def_bool MODULE_SIG_FORMAT > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > + def_bool y > > config ARCH_SUPPORTS_CRASH_DUMP > def_bool y > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 94efde80ebf35..f9975b15ccd57 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2073,7 +2073,7 @@ config ARCH_SUPPORTS_KEXEC > def_bool y > > config ARCH_SUPPORTS_KEXEC_FILE > - def_bool X86_64 && CRYPTO && CRYPTO_SHA256 > + def_bool X86_64 > > config ARCH_SELECTS_KEXEC_FILE > def_bool y > @@ -2081,7 +2081,7 @@ config ARCH_SELECTS_KEXEC_FILE > select HAVE_IMA_KEXEC if IMA > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > + def_bool y > > config ARCH_SUPPORTS_KEXEC_SIG > def_bool y > 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_PURGATORY > select KEXEC_CORE > help > This is new version of kexec system call. This system call is > -- > 2.39.2 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Oct 23, 2023, at 17:37, Conor Dooley wrote: > On Mon, Oct 23, 2023 at 01:01:54PM +0200, Arnd Bergmann wrote: >> index 25474f8c12b79..f571bad2d22d0 100644 >> --- a/arch/riscv/Kconfig >> +++ b/arch/riscv/Kconfig >> @@ -687,9 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE >> select KEXEC_ELF >> >> config ARCH_SUPPORTS_KEXEC_PURGATORY >> - def_bool KEXEC_FILE >> - depends on CRYPTO=y >> - depends on CRYPTO_SHA256=y >> + def_bool y > > This being the problem, KEXEC_FILE is 64-bit only. > > IIRC I commented on this same thing during the original conversion > patches. Does it work with this patch on top? --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -687,7 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE select KEXEC_ELF config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool y + def_bool ARCH_SUPPORTS_KEXEC_FILE config ARCH_SUPPORTS_CRASH_DUMP def_bool y Arnd
On Mon, Oct 23, 2023 at 06:04:06PM +0200, Arnd Bergmann wrote: > On Mon, Oct 23, 2023, at 17:37, Conor Dooley wrote: > > On Mon, Oct 23, 2023 at 01:01:54PM +0200, Arnd Bergmann wrote: > > >> index 25474f8c12b79..f571bad2d22d0 100644 > >> --- a/arch/riscv/Kconfig > >> +++ b/arch/riscv/Kconfig > >> @@ -687,9 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE > >> select KEXEC_ELF > >> > >> config ARCH_SUPPORTS_KEXEC_PURGATORY > >> - def_bool KEXEC_FILE > >> - depends on CRYPTO=y > >> - depends on CRYPTO_SHA256=y > >> + def_bool y > > > > This being the problem, KEXEC_FILE is 64-bit only. > > > > IIRC I commented on this same thing during the original conversion > > patches. > > Does it work with this patch on top? Yah, that modification builds. rv32 being the redheaded stepchild strikes again :! Cheers, Conor. > > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -687,7 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE > select KEXEC_ELF > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool y > + def_bool ARCH_SUPPORTS_KEXEC_FILE > > config ARCH_SUPPORTS_CRASH_DUMP > def_bool y > > > Arnd
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: ...... > --- > arch/powerpc/Kconfig | 4 ++-- > arch/riscv/Kconfig | 4 +--- > arch/s390/Kconfig | 4 ++-- > arch/x86/Kconfig | 4 ++-- > kernel/Kconfig.kexec | 1 + > 5 files changed, 8 insertions(+), 9 deletions(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index d5d5388973ac7..4640cee33f123 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -607,10 +607,10 @@ config ARCH_SUPPORTS_KEXEC > def_bool PPC_BOOK3S || PPC_E500 || (44x && !SMP) > > config ARCH_SUPPORTS_KEXEC_FILE > - def_bool PPC64 && CRYPTO=y && CRYPTO_SHA256=y > + def_bool PPC64 > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > + def_bool y > > config ARCH_SELECTS_KEXEC_FILE > def_bool y > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index 25474f8c12b79..f571bad2d22d0 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -687,9 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE > select KEXEC_ELF > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > - depends on CRYPTO=y > - depends on CRYPTO_SHA256=y > + def_bool y > > config ARCH_SUPPORTS_CRASH_DUMP > def_bool y > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig > index b0d67ac8695f9..ec77106af4137 100644 > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -253,13 +253,13 @@ config ARCH_SUPPORTS_KEXEC > def_bool y > > config ARCH_SUPPORTS_KEXEC_FILE > - def_bool CRYPTO && CRYPTO_SHA256 && CRYPTO_SHA256_S390 > + def_bool y > > config ARCH_SUPPORTS_KEXEC_SIG > def_bool MODULE_SIG_FORMAT > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > + def_bool y > > config ARCH_SUPPORTS_CRASH_DUMP > def_bool y > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index 94efde80ebf35..f9975b15ccd57 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2073,7 +2073,7 @@ config ARCH_SUPPORTS_KEXEC > def_bool y > > config ARCH_SUPPORTS_KEXEC_FILE > - def_bool X86_64 && CRYPTO && CRYPTO_SHA256 > + def_bool X86_64 > > config ARCH_SELECTS_KEXEC_FILE > def_bool y > @@ -2081,7 +2081,7 @@ config ARCH_SELECTS_KEXEC_FILE > select HAVE_IMA_KEXEC if IMA > > config ARCH_SUPPORTS_KEXEC_PURGATORY > - def_bool KEXEC_FILE > + def_bool y > > config ARCH_SUPPORTS_KEXEC_SIG > def_bool y > 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_PURGATORY I 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? 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. > select KEXEC_CORE > help > This is new version of kexec system call. This system call is > -- > 2.39.2
On Tue, Oct 24, 2023, at 14:44, Baoquan He wrote: > 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: > ...... >> 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_PURGATORY > > I 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 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 > 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. Arnd diff --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 */ /* 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- += boot diff --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
On 10/24/23 at 03:17pm, Arnd Bergmann wrote: > On Tue, Oct 24, 2023, at 14:44, Baoquan He wrote: > > 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: > > ...... > > >> 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_PURGATORY > > > > I 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? > > > 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. > > Arnd > > diff --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. > > /* 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- += boot > diff --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 >
Hi Arnd, On 10/24/23 at 03:17pm, Arnd Bergmann wrote: > On Tue, Oct 24, 2023, at 14:44, Baoquan He wrote: > > 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: > > ...... > > >> 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_PURGATORY > > > > I 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 > > 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 > > > 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. Since no new comment, do you mind spinning v2 to wrap all these up? Thanks Baoquan > > Arnd > > diff --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 */ > > /* 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- += boot > diff --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 >
On Thu, 2 Nov 2023 16:03:18 +0800 Baoquan He <bhe@redhat.com> wrote: > > > 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. > > Since no new comment, do you mind spinning v2 to wrap all these up? This patchset remains in mm-hotfixes-unstable from the previous -rc cycle. Eric, do you have any comments? Arnd, do you plan on a v2? If not, should I merge v1? If so, should I now add cc:stable?
On 11/30/23 10:56, Andrew Morton wrote: > On Thu, 2 Nov 2023 16:03:18 +0800 Baoquan He <bhe@redhat.com> wrote: > >>>> 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. >> Since no new comment, do you mind spinning v2 to wrap all these up? > This patchset remains in mm-hotfixes-unstable from the previous -rc > cycle. Eric, do you have any comments? Arnd, do you plan on a v2? If > not, should I merge v1? If so, should I now add cc:stable? My apologies, I lost this. I've looked at these changes, and I am in favor of these changes. Furthermore, I ran the following thru the Kconfig regression script, and did not find anything! I believe the following patch represents the current discussion threads around Kconfig and KEXEC/CRASH. Reviewed-by: Eric DeVolder <eric_devolder@yahoo.com> Tested-by: Eric DeVolder <eric_devolder@yahoo.com> Thanks! eric diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 6f105ee4f3cf..1f11a62809f2 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -608,10 +608,10 @@ config ARCH_SUPPORTS_KEXEC def_bool PPC_BOOK3S || PPC_E500 || (44x && !SMP) config ARCH_SUPPORTS_KEXEC_FILE - def_bool PPC64 && CRYPTO=y && CRYPTO_SHA256=y + def_bool PPC64 config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE + def_bool y config ARCH_SELECTS_KEXEC_FILE def_bool y 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- += boot diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 95a2a06acc6a..98857d76e458 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -702,9 +702,7 @@ config ARCH_SELECTS_KEXEC_FILE select KEXEC_ELF config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE - depends on CRYPTO=y - depends on CRYPTO_SHA256=y + def_bool y config ARCH_SUPPORTS_CRASH_DUMP def_bool y diff --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 */ /* Add the initrd to the image */ if (initrd != NULL) { diff --git a/arch/s390/Kbuild b/arch/s390/Kbuild index a5d3503b353c..f2ce80b65551 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 diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index 3bec98d20283..d5d8f99d1f25 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -254,13 +254,13 @@ config ARCH_SUPPORTS_KEXEC def_bool y config ARCH_SUPPORTS_KEXEC_FILE - def_bool CRYPTO && CRYPTO_SHA256 && CRYPTO_SHA256_S390 + def_bool y config ARCH_SUPPORTS_KEXEC_SIG def_bool MODULE_SIG_FORMAT config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE + def_bool y config ARCH_SUPPORTS_CRASH_DUMP def_bool y diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 3762f41bb092..1566748f16c4 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2072,7 +2072,7 @@ config ARCH_SUPPORTS_KEXEC def_bool y config ARCH_SUPPORTS_KEXEC_FILE - def_bool X86_64 && CRYPTO && CRYPTO_SHA256 + def_bool X86_64 config ARCH_SELECTS_KEXEC_FILE def_bool y @@ -2080,7 +2080,7 @@ config ARCH_SELECTS_KEXEC_FILE select HAVE_IMA_KEXEC if IMA config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE + def_bool y config ARCH_SUPPORTS_KEXEC_SIG def_bool y diff --git a/kernel/Kconfig.kexec b/kernel/Kconfig.kexec index 7aff28ded2f4..92120e396008 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_PURGATORY select KEXEC_CORE help This is new version of kexec system call. This system call is @@ -94,10 +95,8 @@ config KEXEC_JUMP config CRASH_DUMP bool "kernel crash dumps" depends on ARCH_SUPPORTS_CRASH_DUMP - depends on ARCH_SUPPORTS_KEXEC select CRASH_CORE select KEXEC_CORE - select KEXEC help Generate crash dump after being started by kexec. This should be normally only set in special crash dump kernels
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index d5d5388973ac7..4640cee33f123 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig @@ -607,10 +607,10 @@ config ARCH_SUPPORTS_KEXEC def_bool PPC_BOOK3S || PPC_E500 || (44x && !SMP) config ARCH_SUPPORTS_KEXEC_FILE - def_bool PPC64 && CRYPTO=y && CRYPTO_SHA256=y + def_bool PPC64 config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE + def_bool y config ARCH_SELECTS_KEXEC_FILE def_bool y diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index 25474f8c12b79..f571bad2d22d0 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -687,9 +687,7 @@ config ARCH_SELECTS_KEXEC_FILE select KEXEC_ELF config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE - depends on CRYPTO=y - depends on CRYPTO_SHA256=y + def_bool y config ARCH_SUPPORTS_CRASH_DUMP def_bool y diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig index b0d67ac8695f9..ec77106af4137 100644 --- a/arch/s390/Kconfig +++ b/arch/s390/Kconfig @@ -253,13 +253,13 @@ config ARCH_SUPPORTS_KEXEC def_bool y config ARCH_SUPPORTS_KEXEC_FILE - def_bool CRYPTO && CRYPTO_SHA256 && CRYPTO_SHA256_S390 + def_bool y config ARCH_SUPPORTS_KEXEC_SIG def_bool MODULE_SIG_FORMAT config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE + def_bool y config ARCH_SUPPORTS_CRASH_DUMP def_bool y diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 94efde80ebf35..f9975b15ccd57 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2073,7 +2073,7 @@ config ARCH_SUPPORTS_KEXEC def_bool y config ARCH_SUPPORTS_KEXEC_FILE - def_bool X86_64 && CRYPTO && CRYPTO_SHA256 + def_bool X86_64 config ARCH_SELECTS_KEXEC_FILE def_bool y @@ -2081,7 +2081,7 @@ config ARCH_SELECTS_KEXEC_FILE select HAVE_IMA_KEXEC if IMA config ARCH_SUPPORTS_KEXEC_PURGATORY - def_bool KEXEC_FILE + def_bool y config ARCH_SUPPORTS_KEXEC_SIG def_bool y 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_PURGATORY select KEXEC_CORE help This is new version of kexec system call. This system call is