Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1391312 > unrolled thread

[PATCH] efibc: avoid stack overflow warning

Back to article view | Back to linux.kernel

Contents

  [PATCH] efibc: avoid stack overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-04-29 19:50 +0200
    Re: [PATCH] efibc: avoid stack overflow warning Matt Fleming <matt@codeblueprint.co.uk> - 2016-04-30 22:20 +0200
      Re: [PATCH] efibc: avoid stack overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-05-01 00:40 +0200
        Re: [PATCH] efibc: avoid stack overflow warning Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-01 00:50 +0200
          Re: [PATCH] efibc: avoid stack overflow warning Arnd Bergmann <arnd@arndb.de> - 2016-05-01 01:30 +0200
            Re: [PATCH] efibc: avoid stack overflow warning Matt Fleming <matt@codeblueprint.co.uk> - 2016-05-01 15:20 +0200

#1391312 — [PATCH] efibc: avoid stack overflow warning

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

[toc] | [next] | [standalone]

#1391673

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?

[toc] | [prev] | [next] | [standalone]

#1391712

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

[toc] | [prev] | [next] | [standalone]

#1391714

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@jcompost-MOBL1.tl.intel.com

[toc] | [prev] | [next] | [standalone]

#1391721

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@jcompost-MOBL1.tl.intel.com

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

	Arnd

[toc] | [prev] | [next] | [standalone]

#1391814

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@jcompost-MOBL1.tl.intel.com
> 
> Sure, that works. I considered doing it that way but it seemed more
> complicated. Please use that patch instead of mine.

Thanks Ard!

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web