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

[Compostella, Jeremy]

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