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

[Matt Fleming]

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 ;)