Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> writes: > On Fri, 29 Apr, at 11:53:56AM, Ingo Molnar wrote: >> >> * tip-bot for Compostella, Jeremy <tipbot@xxxxxxxxx> wrote: >> >> > Commit-ID: 06f7d4a1618dbb086e738c93cd1ef416ab01027d >> > Gitweb: http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d >> > Author: Compostella, Jeremy <jeremy.compostella@xxxxxxxxx> >> > AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100 >> > Committer: Ingo Molnar <mingo@xxxxxxxxxx> >> > CommitDate: Thu, 28 Apr 2016 11:34:02 +0200 >> > >> > efibc: Add EFI Bootloader Control module >> > >> > This module installs a reboot callback, such that if reboot() is invoked >> > with a string argument NNN, "NNN" is copied to the "LoaderEntryOneShot" >> > EFI variable, to be read by the bootloader. >> >> > drivers/firmware/efi/Kconfig | 15 +++++++ >> > drivers/firmware/efi/Makefile | 1 + >> > drivers/firmware/efi/efibc.c | 101 ++++++++++++++++++++++++++++++++++++++++++ >> > include/linux/efi.h | 4 ++ >> > 4 files changed, 121 insertions(+) >> >> So this bloated things a bit on 32-bit x86 allyesconfig kernels, we now have this >> new warning: >> >> drivers/firmware/efi/efibc.c:53:1: warning: the frame size of 2256 >> bytes is larger than 1024 bytes [-Wframe-larger-than=] >> >> 2K of stack use for a function is quite excessive, can we improve the stack >> footprint of this code? > > I'm waiting to hear from Jeremy on whether we can simply move the > struct efivar_entry (which is the cause of the stack bloat) off the > stack and into the .bss, because it only gets used from the reboot > notifier call chain. > > But upon reading kernel_restart() I'm no longer sure it's guaranteed > to be called only once, or even non-concurrently. It seems that if the > user executes the reboot command and either the sysrq reboot code is > invoked, or an error is encountered dm-verify-target driver or any > other kernel_restart() caller is invoked we could race. > > Perhaps we should guard efi_reboot_notifier_call() with an atomic_t > and exit if we've already invoked it? > > Alternatively, we could just kmalloc() the object ;) I would go for this last proposal because it fixes the issue, it works with the potential race condition you mentioned and it is a simple solution. Yet, it means that if we really run into a race condition, the LoaderEntryOneShot value might be indeterminate. Though, this is a very unlikely situation and if it happens I cannot think of any good reason to prefer to store the first restart target or the second. Do you want me to make this change ? Jérémy -- One Emacs to rule them all -- To unsubscribe from this list: send the line "unsubscribe linux-tip-commits" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
|