[PATCH] efibc: avoid stack overflow warning

[PATCH] efibc: avoid stack overflow warning

[Arnd Bergmann]

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 <arnd@arndb.de>
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

Re: [PATCH] efibc: avoid stack overflow warning

[Matt Fleming]

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?

Re: [PATCH] efibc: avoid stack overflow warning

[Arnd Bergmann]

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

Re: [PATCH] efibc: avoid stack overflow warning

[Matt Fleming]

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/87h9eked24.fsf-e2kLcBBGnpIIbNWpFUBdLQzuBQEzu/OPQQ4Iyu8u01E@public.gmane.org

Re: [PATCH] efibc: avoid stack overflow warning

[Arnd Bergmann]

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/87h9eked24.fsf-e2kLcBBGnpIIbNWpFUBdLQzuBQEzu/OPQQ4Iyu8u01E@public.gmane.org

Sure, that works. I considered doing it that way but it seemed more
complicated. Please use that patch instead of mine.

	Arnd

Re: [PATCH] efibc: avoid stack overflow warning

[Matt Fleming]

On Sun, 01 May, at 01:25:12AM, Arnd Bergmann wrote:
> 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/87h9eked24.fsf-e2kLcBBGnpIIbNWpFUBdLQzuBQEzu/OPQQ4Iyu8u01E@public.gmane.org
> 
> Sure, that works. I considered doing it that way but it seemed more
> complicated. Please use that patch instead of mine.

Thanks Ard!