Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753199AbcD2LhV (ORCPT ); Fri, 29 Apr 2016 07:37:21 -0400 Received: from mga14.intel.com ([192.55.52.115]:43964 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752621AbcD2LhT convert rfc822-to-8bit (ORCPT ); Fri, 29 Apr 2016 07:37:19 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,551,1455004800"; d="scan'208";a="694258466" From: jeremy.compostella@intel.com (Compostella, Jeremy) To: Matt Fleming Cc: Ingo Molnar , , , , , , , , Subject: Re: [tip:efi/core] efibc: Add EFI Bootloader Control module References: <1461614832-17633-26-git-send-email-matt@codeblueprint.co.uk> <20160429095356.GA29957@gmail.com> <20160429103011.GE2839@codeblueprint.co.uk> Date: Fri, 29 Apr 2016 13:36:01 +0200 In-Reply-To: <20160429103011.GE2839@codeblueprint.co.uk> (Matt Fleming's message of "Fri, 29 Apr 2016 11:30:11 +0100") Message-ID: <87lh3wejfi.fsf@jcompost-MOBL1.tl.intel.com> User-Agent: Gnus/5.130012 (Ma Gnus v0.12) Emacs/25.0.50 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2649 Lines: 63 Matt Fleming writes: > On Fri, 29 Apr, at 11:53:56AM, Ingo Molnar wrote: >> >> * tip-bot for Compostella, Jeremy wrote: >> >> > Commit-ID: 06f7d4a1618dbb086e738c93cd1ef416ab01027d >> > Gitweb: http://git.kernel.org/tip/06f7d4a1618dbb086e738c93cd1ef416ab01027d >> > Author: Compostella, Jeremy >> > AuthorDate: Mon, 25 Apr 2016 21:06:57 +0100 >> > Committer: Ingo Molnar >> > 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