[v5,04/10] sandbox: Fix setjmp/longjmp

Message ID 20180622124418.52892-5-agraf@suse.de
State Accepted
Commit 3fcb7147584f8cf5128e1c5eb6b7e49dd86a5473
Headers show
Series
  • sandbox: efi_loader support
Related show

Commit Message

Alexander Graf June 22, 2018, 12:44 p.m.
In sandbox, longjmp returns to itself in an endless loop because
os_longjmp() calls into longjmp() which is provided by U-Boot which
again calls os_longjmp().

Setjmp on the other hand must not return because otherwise the
return freees up stack elements that we need during longjmp().

The only straight forward fix that doesn't involve nasty hacks I
could find is to directly link against the system setjmp/longjmp
implementations. That means we just provide the compiler with
hints that the symbol will be available and actually fill them
out with versions from libc.

This approach should be reasonably platform agnostic

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v4 -> v5:

  - Use system setjmp/longjmp directly from target code
---
 arch/sandbox/cpu/cpu.c            | 12 ------------
 arch/sandbox/cpu/os.c             | 22 ----------------------
 arch/sandbox/include/asm/setjmp.h |  5 +++++
 include/os.h                      | 21 ---------------------
 4 files changed, 5 insertions(+), 55 deletions(-)

Comments

Simon Glass June 22, 2018, 7:28 p.m. | #1
Hi Alex,

On 22 June 2018 at 06:44, Alexander Graf <agraf@suse.de> wrote:
> In sandbox, longjmp returns to itself in an endless loop because
> os_longjmp() calls into longjmp() which is provided by U-Boot which
> again calls os_longjmp().
>
> Setjmp on the other hand must not return because otherwise the
> return freees up stack elements that we need during longjmp().
>
> The only straight forward fix that doesn't involve nasty hacks I
> could find is to directly link against the system setjmp/longjmp
> implementations. That means we just provide the compiler with
> hints that the symbol will be available and actually fill them
> out with versions from libc.
>
> This approach should be reasonably platform agnostic
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
>
> ---
>
> v4 -> v5:
>
>   - Use system setjmp/longjmp directly from target code
> ---
>  arch/sandbox/cpu/cpu.c            | 12 ------------
>  arch/sandbox/cpu/os.c             | 22 ----------------------
>  arch/sandbox/include/asm/setjmp.h |  5 +++++
>  include/os.h                      | 21 ---------------------
>  4 files changed, 5 insertions(+), 55 deletions(-)

I was wondering if that would work. It seems much better to me.

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

Regards,
Simon
Alexander Graf June 23, 2018, 7:36 a.m. | #2
On 22.06.18 21:28, Simon Glass wrote:
> Hi Alex,
> 
> On 22 June 2018 at 06:44, Alexander Graf <agraf@suse.de> wrote:
>> In sandbox, longjmp returns to itself in an endless loop because
>> os_longjmp() calls into longjmp() which is provided by U-Boot which
>> again calls os_longjmp().
>>
>> Setjmp on the other hand must not return because otherwise the
>> return freees up stack elements that we need during longjmp().
>>
>> The only straight forward fix that doesn't involve nasty hacks I
>> could find is to directly link against the system setjmp/longjmp
>> implementations. That means we just provide the compiler with
>> hints that the symbol will be available and actually fill them
>> out with versions from libc.
>>
>> This approach should be reasonably platform agnostic
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>
>> ---
>>
>> v4 -> v5:
>>
>>   - Use system setjmp/longjmp directly from target code
>> ---
>>  arch/sandbox/cpu/cpu.c            | 12 ------------
>>  arch/sandbox/cpu/os.c             | 22 ----------------------
>>  arch/sandbox/include/asm/setjmp.h |  5 +++++
>>  include/os.h                      | 21 ---------------------
>>  4 files changed, 5 insertions(+), 55 deletions(-)
> 
> I was wondering if that would work. It seems much better to me.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>

Awesome ;).

So the way forward that I would envision is that once the merge window
is open, I'll send out a pull request for efi-next to Tom. Pretty much
all the foundational work should be in upstream by then. The only
remaining parts are x86, distro and sandbox specific.

For these (basically all the patches of this patch set) I think it makes
sense if you send them via your tree then.

Beware that currently travis complains about sandbox failing with this
patch set applied. I assume it's your mao_to_sysmem() patch though:

  https://travis-ci.org/agraf/u-boot/jobs/395446382


Alex
Simon Glass June 25, 2018, 3:05 a.m. | #3
Hi Alex,

On 23 June 2018 at 01:36, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 22.06.18 21:28, Simon Glass wrote:
>> Hi Alex,
>>
>> On 22 June 2018 at 06:44, Alexander Graf <agraf@suse.de> wrote:
>>> In sandbox, longjmp returns to itself in an endless loop because
>>> os_longjmp() calls into longjmp() which is provided by U-Boot which
>>> again calls os_longjmp().
>>>
>>> Setjmp on the other hand must not return because otherwise the
>>> return freees up stack elements that we need during longjmp().
>>>
>>> The only straight forward fix that doesn't involve nasty hacks I
>>> could find is to directly link against the system setjmp/longjmp
>>> implementations. That means we just provide the compiler with
>>> hints that the symbol will be available and actually fill them
>>> out with versions from libc.
>>>
>>> This approach should be reasonably platform agnostic
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>
>>> ---
>>>
>>> v4 -> v5:
>>>
>>>   - Use system setjmp/longjmp directly from target code
>>> ---
>>>  arch/sandbox/cpu/cpu.c            | 12 ------------
>>>  arch/sandbox/cpu/os.c             | 22 ----------------------
>>>  arch/sandbox/include/asm/setjmp.h |  5 +++++
>>>  include/os.h                      | 21 ---------------------
>>>  4 files changed, 5 insertions(+), 55 deletions(-)
>>
>> I was wondering if that would work. It seems much better to me.
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Awesome ;).
>
> So the way forward that I would envision is that once the merge window
> is open, I'll send out a pull request for efi-next to Tom. Pretty much
> all the foundational work should be in upstream by then. The only
> remaining parts are x86, distro and sandbox specific.
>
> For these (basically all the patches of this patch set) I think it makes
> sense if you send them via your tree then.
>
> Beware that currently travis complains about sandbox failing with this
> patch set applied. I assume it's your mao_to_sysmem() patch though:
>
>   https://travis-ci.org/agraf/u-boot/jobs/395446382

Yes that looks like a bug although I don't see it with my series.

Regards,
Simon
Alexander Graf Sept. 15, 2018, 12:17 p.m. | #4
> In sandbox, longjmp returns to itself in an endless loop because
> os_longjmp() calls into longjmp() which is provided by U-Boot which
> again calls os_longjmp().
> 
> Setjmp on the other hand must not return because otherwise the
> return freees up stack elements that we need during longjmp().
> 
> The only straight forward fix that doesn't involve nasty hacks I
> could find is to directly link against the system setjmp/longjmp
> implementations. That means we just provide the compiler with
> hints that the symbol will be available and actually fill them
> out with versions from libc.
> 
> This approach should be reasonably platform agnostic
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Thanks, applied to efi-next

Alex

Patch

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index cde0b055a6..e7d3c272a4 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -165,15 +165,3 @@  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 5839932b00..fc2f9dbc7a 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -629,25 +629,3 @@  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
index 1fe37c91cc..001c7ea322 100644
--- a/arch/sandbox/include/asm/setjmp.h
+++ b/arch/sandbox/include/asm/setjmp.h
@@ -24,6 +24,11 @@  struct jmp_buf_data {
 
 typedef struct jmp_buf_data jmp_buf[1];
 
+/*
+ * We have to directly link with the system versions of
+ * setjmp/longjmp, because setjmp must not return as otherwise
+ * the stack may become invalid.
+ */
 int setjmp(jmp_buf jmp);
 __noreturn void longjmp(jmp_buf jmp, int ret);
 
diff --git a/include/os.h b/include/os.h
index c8e0f52d30..64e89a06c9 100644
--- a/include/os.h
+++ b/include/os.h
@@ -330,25 +330,4 @@  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