[v4,15/21] sandbox: Fix setjmp/longjmp

Message ID 20180618152315.34233-16-agraf@suse.de
State New
Headers show
Series
  • sandbox: efi_loader support
Related show

Commit Message

Alexander Graf June 18, 2018, 3:23 p.m.
In sandbox, longjmp returns to itself in an endless loop. Cut this through
by calling the real OS function.

Setjmp on the other hand must not return. So here we have to call the OS
setjmp function straight from the code where the setjmp call happens.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/sandbox/cpu/cpu.c            |  5 -----
 arch/sandbox/cpu/os.c             | 20 ++------------------
 arch/sandbox/include/asm/setjmp.h |  4 +++-
 3 files changed, 5 insertions(+), 24 deletions(-)

Comments

Simon Glass June 21, 2018, 2:02 a.m. | #1
Hi Alex,

On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
> In sandbox, longjmp returns to itself in an endless loop. Cut this through
> by calling the real OS function.
>
> Setjmp on the other hand must not return. So here we have to call the OS
> setjmp function straight from the code where the setjmp call happens.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  arch/sandbox/cpu/cpu.c            |  5 -----
>  arch/sandbox/cpu/os.c             | 20 ++------------------
>  arch/sandbox/include/asm/setjmp.h |  4 +++-
>  3 files changed, 5 insertions(+), 24 deletions(-)
>

I think we do need to do something like this. But I cannot find the
man page for _longjmp(). Are you sure this is a valid function on all
systems? Or is it Linux only?

Regards.
Simon
Alexander Graf June 21, 2018, 9:41 a.m. | #2
On 06/21/2018 04:02 AM, Simon Glass wrote:
> Hi Alex,
>
> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>> In sandbox, longjmp returns to itself in an endless loop. Cut this through
>> by calling the real OS function.
>>
>> Setjmp on the other hand must not return. So here we have to call the OS
>> setjmp function straight from the code where the setjmp call happens.
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   arch/sandbox/cpu/cpu.c            |  5 -----
>>   arch/sandbox/cpu/os.c             | 20 ++------------------
>>   arch/sandbox/include/asm/setjmp.h |  4 +++-
>>   3 files changed, 5 insertions(+), 24 deletions(-)
>>
> I think we do need to do something like this. But I cannot find the
> man page for _longjmp(). Are you sure this is a valid function on all
> systems? Or is it Linux only?

I'm afraid it may be Linux (glibc) only.

I guess the alternative to this would be to not use system 
setjmp/longjmp at all and instead use the in-U-Boot versions of them.


Alex
Simon Glass June 21, 2018, 7:45 p.m. | #3
Hi Alex,

On 21 June 2018 at 03:41, Alexander Graf <agraf@suse.de> wrote:
>
> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>
>> Hi Alex,
>>
>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>>>
>>> In sandbox, longjmp returns to itself in an endless loop. Cut this through
>>> by calling the real OS function.
>>>
>>> Setjmp on the other hand must not return. So here we have to call the OS
>>> setjmp function straight from the code where the setjmp call happens.
>>>
>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>> ---
>>>   arch/sandbox/cpu/cpu.c            |  5 -----
>>>   arch/sandbox/cpu/os.c             | 20 ++------------------
>>>   arch/sandbox/include/asm/setjmp.h |  4 +++-
>>>   3 files changed, 5 insertions(+), 24 deletions(-)
>>>
>> I think we do need to do something like this. But I cannot find the
>> man page for _longjmp(). Are you sure this is a valid function on all
>> systems? Or is it Linux only?
>
>
> I'm afraid it may be Linux (glibc) only.
>
> I guess the alternative to this would be to not use system setjmp/longjmp at all and instead use the in-U-Boot versions of them.

So can we update this patch to check for Linux glibc? Failing that,
perhaps setjmp() could return a negative error value? Then (in other
patches) the caller can make a note that longjmp() cannot be used, and
will return errors from things that need to call it.

Regards,
Simon
Alexander Graf June 22, 2018, 11:54 a.m. | #4
On 06/21/2018 09:45 PM, Simon Glass wrote:
> Hi Alex,
>
> On 21 June 2018 at 03:41, Alexander Graf <agraf@suse.de> wrote:
>> On 06/21/2018 04:02 AM, Simon Glass wrote:
>>> Hi Alex,
>>>
>>> On 18 June 2018 at 09:23, Alexander Graf <agraf@suse.de> wrote:
>>>> In sandbox, longjmp returns to itself in an endless loop. Cut this through
>>>> by calling the real OS function.
>>>>
>>>> Setjmp on the other hand must not return. So here we have to call the OS
>>>> setjmp function straight from the code where the setjmp call happens.
>>>>
>>>> Signed-off-by: Alexander Graf <agraf@suse.de>
>>>> ---
>>>>    arch/sandbox/cpu/cpu.c            |  5 -----
>>>>    arch/sandbox/cpu/os.c             | 20 ++------------------
>>>>    arch/sandbox/include/asm/setjmp.h |  4 +++-
>>>>    3 files changed, 5 insertions(+), 24 deletions(-)
>>>>
>>> I think we do need to do something like this. But I cannot find the
>>> man page for _longjmp(). Are you sure this is a valid function on all
>>> systems? Or is it Linux only?
>>
>> I'm afraid it may be Linux (glibc) only.
>>
>> I guess the alternative to this would be to not use system setjmp/longjmp at all and instead use the in-U-Boot versions of them.
> So can we update this patch to check for Linux glibc? Failing that,
> perhaps setjmp() could return a negative error value? Then (in other
> patches) the caller can make a note that longjmp() cannot be used, and
> will return errors from things that need to call it.

I actually think there is a much easier way. We can just directly export 
the symbols for libc's setjmp/longjmp functions in the sandbox setjmp.h 
header. That way we avoid all the nasty problems we've encountered so 
far. I've cooked up a patch and it seems to work fine.


Alex

Patch

diff --git a/arch/sandbox/cpu/cpu.c b/arch/sandbox/cpu/cpu.c
index 29dac8dfda..9087026d71 100644
--- a/arch/sandbox/cpu/cpu.c
+++ b/arch/sandbox/cpu/cpu.c
@@ -167,11 +167,6 @@  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);
diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 5839932b00..9cfdc9f31f 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -630,24 +630,8 @@  void os_localtime(struct rtc_time *rt)
 	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);
+	/* Call the OS longjmp function directly */
+	_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..e103d170e0 100644
--- a/arch/sandbox/include/asm/setjmp.h
+++ b/arch/sandbox/include/asm/setjmp.h
@@ -24,7 +24,9 @@  struct jmp_buf_data {
 
 typedef struct jmp_buf_data jmp_buf[1];
 
-int setjmp(jmp_buf jmp);
+/* Call the OS setjmp function directly, so that we don't introduce returns */
+#define setjmp _setjmp
+int _setjmp(jmp_buf jmp);
 __noreturn void longjmp(jmp_buf jmp, int ret);
 
 #endif /* _SETJMP_H_ */