gcc complains about a newly added file for the EFI Bootloader Control:
drivers/firmware/efi/efibc.c: In function 'efibc_set_variable':
drivers/firmware/efi/efibc.c:53:1: error: the frame size of 2272 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
The problem is the declaration of a local variable of type
struct efivar_entry, which is by itself larger than the warning
limit of 1024 bytes.
We know that the reboot notifiers are not called from a deep stack,
so this is not an actual bug, but we should still try to rework
the code to avoid the warning. We also know that reboot notifiers
are never run concurrently on multiple CPUs, so there is no problem
in just making the variable 'static'.
Signed-off-by: Arnd Bergmann <[email protected]>
Fixes: 06f7d4a1618d ("efibc: Add EFI Bootloader Control module")
---
drivers/firmware/efi/efibc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/efi/efibc.c b/drivers/firmware/efi/efibc.c
index 2e0c7ccd9d9e..83ac90efa03f 100644
--- a/drivers/firmware/efi/efibc.c
+++ b/drivers/firmware/efi/efibc.c
@@ -31,8 +31,9 @@ static void efibc_str_to_str16(const char *str, efi_char16_t *str16)
static void efibc_set_variable(const char *name, const char *value)
{
int ret;
- efi_guid_t guid = LINUX_EFI_LOADER_ENTRY_GUID;
- struct efivar_entry entry;
+ static struct efivar_entry entry = {
+ .var.VendorGuid = LINUX_EFI_LOADER_ENTRY_GUID,
+ };
size_t size = (strlen(value) + 1) * sizeof(efi_char16_t);
if (size > sizeof(entry.var.Data))
@@ -40,7 +41,6 @@ static void efibc_set_variable(const char *name, const char *value)
efibc_str_to_str16(name, entry.var.VariableName);
efibc_str_to_str16(value, (efi_char16_t *)entry.var.Data);
- memcpy(&entry.var.VendorGuid, &guid, sizeof(guid));
ret = efivar_entry_set(&entry,
EFI_VARIABLE_NON_VOLATILE
--
2.7.0
On Fri, 29 Apr, at 07:48:31PM, Arnd Bergmann wrote:
> gcc complains about a newly added file for the EFI Bootloader Control:
>
> drivers/firmware/efi/efibc.c: In function 'efibc_set_variable':
> drivers/firmware/efi/efibc.c:53:1: error: the frame size of 2272 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> The problem is the declaration of a local variable of type
> struct efivar_entry, which is by itself larger than the warning
> limit of 1024 bytes.
>
> We know that the reboot notifiers are not called from a deep stack,
> so this is not an actual bug, but we should still try to rework
> the code to avoid the warning. We also know that reboot notifiers
> are never run concurrently on multiple CPUs, so there is no problem
> in just making the variable 'static'.
I assumed reboot notifiers were guaranteed to be non-concurrent too
but having dug into the callers of kernel_reboot(), I couldn't find
any kind of mutual exclusion.
How/where is this guaranteed?
On Saturday 30 April 2016 21:14:49 Matt Fleming wrote:
> On Fri, 29 Apr, at 07:48:31PM, Arnd Bergmann wrote:
> > gcc complains about a newly added file for the EFI Bootloader Control:
> >
> > drivers/firmware/efi/efibc.c: In function 'efibc_set_variable':
> > drivers/firmware/efi/efibc.c:53:1: error: the frame size of 2272 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> >
> > The problem is the declaration of a local variable of type
> > struct efivar_entry, which is by itself larger than the warning
> > limit of 1024 bytes.
> >
> > We know that the reboot notifiers are not called from a deep stack,
> > so this is not an actual bug, but we should still try to rework
> > the code to avoid the warning. We also know that reboot notifiers
> > are never run concurrently on multiple CPUs, so there is no problem
> > in just making the variable 'static'.
>
> I assumed reboot notifiers were guaranteed to be non-concurrent too
> but having dug into the callers of kernel_reboot(), I couldn't find
> any kind of mutual exclusion.
>
> How/where is this guaranteed?
The sys_restart() system call takes a mutex before calling kernel_restart()
or kernel_poweroff().
I've had a closer look now and found that there are a few other
callers of kernel_restart, so I guess if you restart using sysctl
at the exact same time as calling /sbin/reboot, things may break.
It's not something we'd have to worry about in practice, but it does
make my patch incorrect. Should we come up with a different way to
do it?
Arnd
On Sun, 01 May, at 12:34:29AM, Arnd Bergmann wrote:
>
> The sys_restart() system call takes a mutex before calling kernel_restart()
> or kernel_poweroff().
>
> I've had a closer look now and found that there are a few other
> callers of kernel_restart, so I guess if you restart using sysctl
> at the exact same time as calling /sbin/reboot, things may break.
Right. Or if the dm-verify-target driver saw an error.
> It's not something we'd have to worry about in practice, but it does
> make my patch incorrect. Should we come up with a different way to
> do it?
Jeremy proposed a patch to dynamically allocate the memory, which I
think is the correct way to go given that our (reasonable) assumptions
about reboot notifier concurrency are not guaranteed,
https://lkml.kernel.org/r/[email protected]
On Saturday 30 April 2016 23:46:41 Matt Fleming wrote:
>
> > It's not something we'd have to worry about in practice, but it does
> > make my patch incorrect. Should we come up with a different way to
> > do it?
>
> Jeremy proposed a patch to dynamically allocate the memory, which I
> think is the correct way to go given that our (reasonable) assumptions
> about reboot notifier concurrency are not guaranteed,
>
> https://lkml.kernel.org/r/[email protected]
Sure, that works. I considered doing it that way but it seemed more
complicated. Please use that patch instead of mine.
Arnd