[U-Boot,v4,05/16] sandbox: Add a setjmp() implementation

Message ID 20180516154233.21457-6-sjg@chromium.org
State Accepted
Delegated to: Alexander Graf
Headers show
Series
  • efi: Enable basic sandbox support for EFI loader
Related show

Commit Message

Simon Glass May 16, 2018, 3:42 p.m.
Add an implementation of setjmp() and longjmp() which rely on the
underlying host C library. Since we cannot know how large the jump buffer
needs to be, pick something that should be suitable and check it at
runtime. At present we need access to the underlying struct as well.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

Changes in v4:
- Fix up the sizeof() operations on jmp_buf
- Update SPDX tags

Changes in v3: None
Changes in v2: None

 arch/sandbox/cpu/cpu.c            | 13 +++++++++++++
 arch/sandbox/cpu/os.c             | 23 +++++++++++++++++++++++
 arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++
 include/os.h                      | 21 +++++++++++++++++++++
 4 files changed, 87 insertions(+)
 create mode 100644 arch/sandbox/include/asm/setjmp.h

Comments

Alexander Graf May 24, 2018, 12:44 p.m. | #1
> Add an implementation of setjmp() and longjmp() which rely on the
> underlying host C library. Since we cannot know how large the jump buffer
> needs to be, pick something that should be suitable and check it at
> runtime. At present we need access to the underlying struct as well.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Thanks, applied to efi-next

Alex
Alexander Graf June 15, 2018, 12:01 p.m. | #2
On 16.05.18 17:42, Simon Glass wrote:
> Add an implementation of setjmp() and longjmp() which rely on the
> underlying host C library. Since we cannot know how large the jump buffer
> needs to be, pick something that should be suitable and check it at
> runtime. At present we need access to the underlying struct as well.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v4:
> - Fix up the sizeof() operations on jmp_buf
> - Update SPDX tags
> 
> Changes in v3: None
> Changes in v2: None
> 
>  arch/sandbox/cpu/cpu.c            | 13 +++++++++++++
>  arch/sandbox/cpu/os.c             | 23 +++++++++++++++++++++++
>  arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++
>  include/os.h                      | 21 +++++++++++++++++++++
>  4 files changed, 87 insertions(+)
>  create mode 100644 arch/sandbox/include/asm/setjmp.h
> 
> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
> index d4ad020012e..cde0b055a67 100644
> --- a/arch/sandbox/cpu/cpu.c
> +++ b/arch/sandbox/cpu/cpu.c
> @@ -9,6 +9,7 @@
>  #include <linux/libfdt.h>
>  #include <os.h>
>  #include <asm/io.h>
> +#include <asm/setjmp.h>
>  #include <asm/state.h>
>  #include <dm/root.h>
>  
> @@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
>  
>  	return (count - base_count) / 1000;
>  }
> +
> +int setjmp(jmp_buf jmp)
> +{
> +	return os_setjmp((ulong *)jmp, sizeof(*jmp));

So, this doesn't work. Function returns increase the stack pointer which
means after setjmp() you are not allowed to return until the longjmp
occured. The documentation is quite clear about this:


DESCRIPTION
       setjmp() and longjmp(3) are useful for dealing with errors and
interrupts encountered in a low-level subroutine of a program.  setjmp()
saves the stack context/environment in env for later use by longjmp(3).
The stack context will be invalidated if the function which called
setjmp() returns.


So we need to find a way to call setjmp() directly from the code point
where we want to call it, rather than jump through helper functions, as
these break its functionality.

Also, os_longjmp() is broken. It calls longjmp() which however is not
the system longjmp, but the U-Boot internal one that again calls
os_longjmp. My quick fix was to make it call _longjmp() instead - that
at least makes that part work.


Alex
Simon Glass June 15, 2018, 3:16 p.m. | #3
Hi Alex,

On 15 June 2018 at 06:01, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 16.05.18 17:42, Simon Glass wrote:
>> Add an implementation of setjmp() and longjmp() which rely on the
>> underlying host C library. Since we cannot know how large the jump buffer
>> needs to be, pick something that should be suitable and check it at
>> runtime. At present we need access to the underlying struct as well.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>> Changes in v4:
>> - Fix up the sizeof() operations on jmp_buf
>> - Update SPDX tags
>>
>> Changes in v3: None
>> Changes in v2: None
>>
>>  arch/sandbox/cpu/cpu.c            | 13 +++++++++++++
>>  arch/sandbox/cpu/os.c             | 23 +++++++++++++++++++++++
>>  arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++
>>  include/os.h                      | 21 +++++++++++++++++++++
>>  4 files changed, 87 insertions(+)
>>  create mode 100644 arch/sandbox/include/asm/setjmp.h
>>
>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>> index d4ad020012e..cde0b055a67 100644
>> --- a/arch/sandbox/cpu/cpu.c
>> +++ b/arch/sandbox/cpu/cpu.c
>> @@ -9,6 +9,7 @@
>>  #include <linux/libfdt.h>
>>  #include <os.h>
>>  #include <asm/io.h>
>> +#include <asm/setjmp.h>
>>  #include <asm/state.h>
>>  #include <dm/root.h>
>>
>> @@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
>>
>>       return (count - base_count) / 1000;
>>  }
>> +
>> +int setjmp(jmp_buf jmp)
>> +{
>> +     return os_setjmp((ulong *)jmp, sizeof(*jmp));
>
> So, this doesn't work. Function returns increase the stack pointer which
> means after setjmp() you are not allowed to return until the longjmp
> occured. The documentation is quite clear about this:
>
>
> DESCRIPTION
>        setjmp() and longjmp(3) are useful for dealing with errors and
> interrupts encountered in a low-level subroutine of a program.  setjmp()
> saves the stack context/environment in env for later use by longjmp(3).
> The stack context will be invalidated if the function which called
> setjmp() returns.
>
>
> So we need to find a way to call setjmp() directly from the code point
> where we want to call it, rather than jump through helper functions, as
> these break its functionality.

That sounds hard but perhaps we can do something with inline
functions? It's hard because we want to call the C library in the
sandbox case. Perhaps we should rename the U-Boot version of the
function.

I wonder if in my test I just relied on hello world returning, rather
than calling the exit() function? BTW we need to find a way for
efi_selftest() to exit cleanly too. At the moment it resets.

>
> Also, os_longjmp() is broken. It calls longjmp() which however is not
> the system longjmp, but the U-Boot internal one that again calls
> os_longjmp. My quick fix was to make it call _longjmp() instead - that
> at least makes that part work.

But it hangs after that?

Regards,
Simon
Alexander Graf June 15, 2018, 7:59 p.m. | #4
> Am 15.06.2018 um 17:16 schrieb Simon Glass <sjg@chromium.org>:
> 
> Hi Alex,
> 
>> On 15 June 2018 at 06:01, Alexander Graf <agraf@suse.de> wrote:
>> 
>> 
>>> On 16.05.18 17:42, Simon Glass wrote:
>>> Add an implementation of setjmp() and longjmp() which rely on the
>>> underlying host C library. Since we cannot know how large the jump buffer
>>> needs to be, pick something that should be suitable and check it at
>>> runtime. At present we need access to the underlying struct as well.
>>> 
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>> 
>>> Changes in v4:
>>> - Fix up the sizeof() operations on jmp_buf
>>> - Update SPDX tags
>>> 
>>> Changes in v3: None
>>> Changes in v2: None
>>> 
>>> arch/sandbox/cpu/cpu.c            | 13 +++++++++++++
>>> arch/sandbox/cpu/os.c             | 23 +++++++++++++++++++++++
>>> arch/sandbox/include/asm/setjmp.h | 30 ++++++++++++++++++++++++++++++
>>> include/os.h                      | 21 +++++++++++++++++++++
>>> 4 files changed, 87 insertions(+)
>>> create mode 100644 arch/sandbox/include/asm/setjmp.h
>>> 
>>> diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
>>> index d4ad020012e..cde0b055a67 100644
>>> --- a/arch/sandbox/cpu/cpu.c
>>> +++ b/arch/sandbox/cpu/cpu.c
>>> @@ -9,6 +9,7 @@
>>> #include <linux/libfdt.h>
>>> #include <os.h>
>>> #include <asm/io.h>
>>> +#include <asm/setjmp.h>
>>> #include <asm/state.h>
>>> #include <dm/root.h>
>>> 
>>> @@ -164,3 +165,15 @@ ulong timer_get_boot_us(void)
>>> 
>>>      return (count - base_count) / 1000;
>>> }
>>> +
>>> +int setjmp(jmp_buf jmp)
>>> +{
>>> +     return os_setjmp((ulong *)jmp, sizeof(*jmp));
>> 
>> So, this doesn't work. Function returns increase the stack pointer which
>> means after setjmp() you are not allowed to return until the longjmp
>> occured. The documentation is quite clear about this:
>> 
>> 
>> DESCRIPTION
>>       setjmp() and longjmp(3) are useful for dealing with errors and
>> interrupts encountered in a low-level subroutine of a program.  setjmp()
>> saves the stack context/environment in env for later use by longjmp(3).
>> The stack context will be invalidated if the function which called
>> setjmp() returns.
>> 
>> 
>> So we need to find a way to call setjmp() directly from the code point
>> where we want to call it, rather than jump through helper functions, as
>> these break its functionality.
> 
> That sounds hard but perhaps we can do something with inline
> functions? It's hard because we want to call the C library in the
> sandbox case. Perhaps we should rename the U-Boot version of the
> function.
> 
> I wonder if in my test I just relied on hello world returning, rather
> than calling the exit() function? BTW we need to find a way for
> efi_selftest() to exit cleanly too. At the moment it resets.
> 
>> 
>> Also, os_longjmp() is broken. It calls longjmp() which however is not
>> the system longjmp, but the U-Boot internal one that again calls
>> os_longjmp. My quick fix was to make it call _longjmp() instead - that
>> at least makes that part work.
> 
> But it hangs after that?

It hangs in the return path of setjmp afterwards because of the clobbered stack. See my patch in the latest series for reference. With that patch it works (might be  glibc only though).

Alex

Patch

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index d4ad020012e..cde0b055a67 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -9,6 +9,7 @@ 
 #include <linux/libfdt.h>
 #include <os.h>
 #include <asm/io.h>
+#include <asm/setjmp.h>
 #include <asm/state.h>
 #include <dm/root.h>
 
@@ -164,3 +165,15 @@  ulong timer_get_boot_us(void)
 
 	return (count - base_count) / 1000;
 }
+
+int setjmp(jmp_buf jmp)
+{
+	return os_setjmp((ulong *)jmp, sizeof(*jmp));
+}
+
+void longjmp(jmp_buf jmp, int ret)
+{
+	os_longjmp((ulong *)jmp, ret);
+	while (1)
+		;
+}
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index d76d0211a2d..5839932b005 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -7,6 +7,7 @@ 
 #include <errno.h>
 #include <fcntl.h>
 #include <getopt.h>
+#include <setjmp.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <stdlib.h>
@@ -628,3 +629,25 @@  void os_localtime(struct rtc_time *rt)
 	rt->tm_yday = tm->tm_yday;
 	rt->tm_isdst = tm->tm_isdst;
 }
+
+int os_setjmp(ulong *jmp, int size)
+{
+	jmp_buf dummy;
+
+	/*
+	 * We cannot rely on the struct name that jmp_buf uses, so use a
+	 * local variable here
+	 */
+	if (size < sizeof(dummy)) {
+		printf("setjmp: jmpbuf is too small (%d bytes, need %d)\n",
+		       size, sizeof(jmp_buf));
+		return -ENOSPC;
+	}
+
+	return setjmp((struct __jmp_buf_tag *)jmp);
+}
+
+void os_longjmp(ulong *jmp, int ret)
+{
+	longjmp((struct __jmp_buf_tag *)jmp, ret);
+}
diff --git a/arch/sandbox/include/asm/setjmp.h b/arch/sandbox/include/asm/setjmp.h
new file mode 100644
index 00000000000..0fb1a11f234
--- /dev/null
+++ b/arch/sandbox/include/asm/setjmp.h
@@ -0,0 +1,30 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * (C) 2018 Google, Inc
+ * Written by Simon Glass <sjg@chromium.org>
+ */
+
+#ifndef _SETJMP_H_
+#define _SETJMP_H_
+
+struct jmp_buf_data {
+	/*
+	 * We're not sure how long this should be:
+	 *
+	 *   amd64: 200 bytes
+	 *   arm64: 392 bytes
+	 *   armhf: 392 bytes
+	 *
+	 * So allow space for all of those, plus some extra.
+	 * We don't need to worry about 16-byte alignment, since this does not
+	 * run on Windows.
+	 */
+	ulong data[128];
+};
+
+typedef struct jmp_buf_data jmp_buf[1];
+
+int setjmp(jmp_buf jmp);
+__noreturn void longjmp(jmp_buf jmp, int ret);
+
+#endif /* _SETJMP_H_ */
diff --git a/include/os.h b/include/os.h
index 64e89a06c94..c8e0f52d306 100644
--- a/include/os.h
+++ b/include/os.h
@@ -330,4 +330,25 @@  int os_spl_to_uboot(const char *fname);
  */
 void os_localtime(struct rtc_time *rt);
 
+/**
+ * os_setjmp() - Call setjmp()
+ *
+ * Call the host system's setjmp() function.
+ *
+ * @jmp: Buffer to store current execution state
+ * @size: Size of buffer
+ * @return normal setjmp() value if OK, -ENOSPC if @size is too small
+ */
+int os_setjmp(ulong *jmp, int size);
+
+/**
+ * os_longjmp() - Call longjmp()
+ *
+ * Call the host system's longjmp() function.
+ *
+ * @jmp: Buffer where previous execution state was stored
+ * @ret: Value to pass to longjmp()
+ */
+void os_longjmp(ulong *jmp, int ret);
+
 #endif