Re: [tip:efi/core] efibc: Add EFI Bootloader Control module

Compostella, Jeremy
2016-04-29T11:37:21Z

Matt Fleming <matt@codeblueprint.co.uk> writes:

> On Fri, 29 Apr, at 11:53:56AM, Ingo Molnar wrote:
>>
>> * tip-bot for Compostella, Jeremy <tipbot@zytor.com> wrote:
>>
>> > Commit-ID: 06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> > Gitweb: http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d
>> > Author: Compostella, Jeremy <jeremy.compostella@intel.com>
>> > AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100
>> > Committer: Ingo Molnar <mingo@kernel.org>
>> > 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